On 2013-10-22 10:08, Christoffer Dall wrote:
> Add infrastructure to handle distributor and cpu interface register
> accesses through the KVM_{GET/SET}_DEVICE_ATTR interface by adding
> the
> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS
> groups
> and defining the semantics of the attr field to be the MMIO offset as
> specified in the GICv2 specs.
>
> Missing register accesses or other changes in individual register
> access
> functions to support save/restore of the VGIC state is added in
> subsequent patches.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> Reviewed-by: Alexander Graf <agraf@xxxxxxx>
>
> ---
> Changelog[v2]:
> - Added implementation specific format for the GICC_APRn registers.
> ---
> Documentation/virtual/kvm/devices/arm-vgic.txt | 50 +++++++++
> virt/kvm/arm/vgic.c | 143
> ++++++++++++++++++++++++
> 2 files changed, 193 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt
> b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index c9febb2..e6416f8e 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -19,3 +19,53 @@ Groups:
> KVM_VGIC_V2_ADDR_TYPE_CPU (rw, 64-bit)
> Base address in the guest physical address space of the GIC
> virtual cpu
> interface register mappings.
> +
> + KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> + Attributes:
> + The attr field of kvm_device_attr encodes two values:
> + bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
> + values: | reserved | cpu id | offset |
> +
> + All distributor regs are (rw, 32-bit)
> +
> + The offset is relative to the "Distributor base address" as
> defined in the
> + GICv2 specs. Getting or setting such a register has the same
> effect as
> + reading or writing the register on the actual hardware from the
> cpu
> + specified with cpu id field. Note that most distributor fields
> are not
> + banked, but return the same value regardless of the cpu id used
> to access
> + the register.
> + Limitations:
> + - Priorities are not implemented, and registers are RAZ/WI
> + Errors:
> + - ENODEV: Getting or setting this register is not yet supported
-ENODEV?
> + KVM_DEV_ARM_VGIC_GRP_CPU_REGS
> + Attributes:
> + The attr field of kvm_device_attr encodes two values:
> + bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
> + values: | reserved | cpu id | offset |
> +
> + All CPU regs are (rw, 32-bit)
Nit: CPU interface registers
> + The offset specifies the offset from the "CPU interface base
> address" as
> + defined in the GICv2 specs. Getting or setting such a register
> has the
> + same effect as reading or writing the register on the actual
> hardware.
> +
> + The Active Priorities Registers APRn are implementation defined,
> so we set a
> + fixed format for our implementation that fits with the model of
> a "GICv2
> + impementation without the security extensions" which we present
> to the
implementation
> + guest. This interface always exposes four register APR[0-3]
> describing the
> + maximum possible 128 preemption levels. The semantics of the
> register
> + indicate if any interrupts in a given preemption level are in
> the active
> + state by setting the corresponding bit.
> +
> + Thus, preemption level X has one or more active interrupts if
> and only if:
> +
> + APRn[X mod 32] == 0b1, where n = X / 32
> +
> + Bits for undefined preemption levels are RAZ/WI.
> +
> + Limitations:
> + - Priorities are not implemented, and registers are RAZ/WI
> + Errors:
> + - ENODEV: Getting or setting this register is not yet supported
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 1148a2e..f2dc72a 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -589,11 +589,29 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu
> *vcpu,
> return false;
> }
>
> +static bool handle_mmio_sgi_clear(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset)
> +{
> + return false;
> +}
> +
> +static bool handle_mmio_sgi_set(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset)
> +{
> + return false;
> +}
> +
> /*
> * I would have liked to use the kvm_bus_io_*() API instead, but it
> * cannot cope with banked registers (only the VM pointer is passed
> * around, and we need the vcpu). One of these days, someone please
> * fix it!
> + *
> + * Note that the handle_mmio implementations should not use the
> phys_addr
> + * field from the kvm_exit_mmio struct as this will not have any
> sane values
> + * when used to save/restore state from user space.
This is quite ugly... I don't think we'd ever use that field directly,
but reusing a well known structure for that purpose is very messy. I
believe we'd be better off creating our own structure instead of
re-purposing am existing one.
The other possibility would be to properly fill-in the phys_addr field.
How difficult would that be?
> */
> struct mmio_range {
> phys_addr_t base;
> @@ -663,6 +681,16 @@ static const struct mmio_range
> vgic_dist_ranges[] = {
> .len = 4,
> .handle_mmio = handle_mmio_sgi_reg,
> },
> + {
> + .base = GIC_DIST_SGI_CLEAR,
> + .len = VGIC_NR_SGIS,
> + .handle_mmio = handle_mmio_sgi_clear,
> + },
> + {
> + .base = GIC_DIST_SGI_SET,
> + .len = VGIC_NR_SGIS,
> + .handle_mmio = handle_mmio_sgi_set,
> + },
> {}
> };
>
> @@ -1541,6 +1569,80 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned
> long type, u64 *addr, bool write)
> return r;
> }
>
> +static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio, phys_addr_t offset)
> +{
> + return true;
> +}
> +
> +static const struct mmio_range vgic_cpu_ranges[] = {
> + {
> + .base = GIC_CPU_CTRL,
> + .len = 12,
> + .handle_mmio = handle_cpu_mmio_misc,
> + },
> + {
> + .base = GIC_CPU_ALIAS_BINPOINT,
> + .len = 4,
> + .handle_mmio = handle_cpu_mmio_misc,
> + },
> + {
> + .base = GIC_CPU_ACTIVEPRIO,
> + .len = 16,
> + .handle_mmio = handle_cpu_mmio_misc,
> + },
> + {
> + .base = GIC_CPU_IDENT,
> + .len = 4,
> + .handle_mmio = handle_cpu_mmio_misc,
> + },
> +};
> +
> +static struct kvm_exit_mmio dev_attr_mmio = { .len = 4 };
I'm not very fond of a half-initialized structure here. How about
moving this "4" to the location where it is used?
Actually, what if we have several users of this through
vgic_has_attr_regs at the same time? It feels incredibly racy. I suggest
you nuke it and move it to live on the stack in vgic_has_attr_regs.
> +static int vgic_attr_regs_access(struct kvm_device *dev,
> + struct kvm_device_attr *attr,
> + u32 *reg, bool is_write)
> +{
> + const struct mmio_range *r = NULL;
> + phys_addr_t offset;
> + int cpuid;
> + struct kvm_vcpu *vcpu;
> + struct kvm_exit_mmio mmio;
> +
> + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> + KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +
> + if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
> + return -EINVAL;
> +
> + vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> +
> + mmio.len = 4;
> + mmio.is_write = is_write;
> + if (is_write)
> + mmio_data_write(&mmio, ~0, *reg);
> +
> + if (attr->group == KVM_DEV_ARM_VGIC_GRP_DIST_REGS)
> + r = find_matching_range(vgic_dist_ranges, &mmio, offset);
> + else if (attr->group == KVM_DEV_ARM_VGIC_GRP_CPU_REGS)
> + r = find_matching_range(vgic_cpu_ranges, &mmio, offset);
How about having a switch statement instead?
> + if (unlikely(!r || !r->handle_mmio))
> + return -ENXIO;
> +
> + spin_lock(&vcpu->kvm->arch.vgic.lock);
> + offset -= r->base;
> + r->handle_mmio(vcpu, &mmio, offset);
> + spin_unlock(&vcpu->kvm->arch.vgic.lock);
> +
> + if (!is_write)
> + *reg = mmio_data_read(&mmio, ~0);
> +
> + return 0;
> +}
> +
> static int vgic_set_attr(struct kvm_device *dev, struct
> kvm_device_attr *attr)
> {
> int r;
> @@ -1557,6 +1659,18 @@ static int vgic_set_attr(struct kvm_device
> *dev, struct kvm_device_attr *attr)
> r = kvm_vgic_addr(dev->kvm, type, &addr, true);
> return (r == -ENODEV) ? -ENXIO : r;
> }
> +
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + u32 reg;
> +
> + if (get_user(reg, uaddr))
> + return -EFAULT;
> +
> + return vgic_attr_regs_access(dev, attr, ®, true);
> + }
> +
> }
>
> return -ENXIO;
> @@ -1579,12 +1693,35 @@ static int vgic_get_attr(struct kvm_device
> *dev, struct kvm_device_attr *attr)
> r = 0;
> if (copy_to_user(uaddr, &addr, sizeof(addr)))
> return -EFAULT;
> + break;
> }
> +
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + u32 reg = 0;
> +
> + r = vgic_attr_regs_access(dev, attr, ®, false);
> + if (r)
> + return r;
> + r = put_user(reg, uaddr);
> + break;
> + }
> +
> }
>
> return r;
> }
>
> +static int vgic_has_attr_regs(const struct mmio_range *ranges,
> + phys_addr_t offset)
> +{
> + if (find_matching_range(ranges, &dev_attr_mmio, offset))
> + return 0;
> + else
> + return -ENXIO;
> +}
> +
> static int vgic_has_attr(struct kvm_device *dev, struct
> kvm_device_attr *attr)
> {
> phys_addr_t offset;
> @@ -1597,6 +1734,12 @@ static int vgic_has_attr(struct kvm_device
> *dev, struct kvm_device_attr *attr)
> return 0;
> }
> break;
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> + return vgic_has_attr_regs(vgic_dist_ranges, offset);
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> + return vgic_has_attr_regs(vgic_cpu_ranges, offset);
> }
> return -ENXIO;
> }
--
Who you jivin' with that Cosmik Debris?
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
