On Wed, Oct 23, 2013 at 04:46:56PM +0100, Marc Zyngier wrote:
> 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?
>
indeed
> >+ 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.
Hmmm, I don't think this is about re-purposing an existing structure, it
is about generally using a structure in a file which happens to contain
a superflous field, which should never have to be used in this file anyway.
Now we will actually use this structure where this unnecessary field (in
this context) does not contain a sane value and we clearly document that
in the comment. Further, introducing another type adds another memcpy
or makes the whole io_mem_abort() - vgic_handle_mmio() messy. I
actually had a go at it that I can pass your way if you are set on this
approach...
>
> The other possibility would be to properly fill-in the phys_addr
> field. How difficult would that be?
>
Not really difficult at all, let me do that for a v3.
> > */
> > 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.
>
fair enough, since this is just another way of passing the constant four
to a match function and that constant is only ever read, I don't think
there's any race here, but ok, it's completely fine to just allocate it on
the stack.
> >+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?
>
sure.
> >+ 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;
> > }
>
Thanks!
--
Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
