On Wed, Oct 23, 2013 at 04:10:42PM +0100, Marc Zyngier wrote:
> On 2013-10-22 10:08, Christoffer Dall wrote:
> >Support setting the distributor and cpu interface base addresses
> >in the
> >VM physical address space through the KVM_{SET,GET}_DEVICE_ATTR API
> >in addition to the ARM specific API.
> >
> >This has the added benefit of being able to share more code in user
> >space and do things in a uniform maner.
>
> manner?
>
> >Also deprecate the older API at the same time, but backwards
> >compatibility will be maintained.
> >
> >Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >Reviewed-by: Alexander Graf <agraf@xxxxxxx>
> >---
> > Documentation/virtual/kvm/api.txt | 6 +-
> > Documentation/virtual/kvm/devices/arm-vgic.txt | 11 +++
> > arch/arm/include/uapi/asm/kvm.h | 9 +++
> > arch/arm/kvm/arm.c | 2 +-
> > include/kvm/arm_vgic.h | 2 +-
> > virt/kvm/arm/vgic.c | 90
> >++++++++++++++++++++----
> > 6 files changed, 105 insertions(+), 15 deletions(-)
> >
> >diff --git a/Documentation/virtual/kvm/api.txt
> >b/Documentation/virtual/kvm/api.txt
> >index 858aecf..d68b6c2 100644
> >--- a/Documentation/virtual/kvm/api.txt
> >+++ b/Documentation/virtual/kvm/api.txt
> >@@ -2324,7 +2324,7 @@ This ioctl returns the guest registers that are
> >supported for the
> > KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
> >
> >
> >-4.84 KVM_ARM_SET_DEVICE_ADDR
> >+4.84 KVM_ARM_SET_DEVICE_ADDR (deprecated)
> >
> > Capability: KVM_CAP_ARM_SET_DEVICE_ADDR
> > Architectures: arm, arm64
> >@@ -2362,6 +2362,10 @@ must be called after calling
> >KVM_CREATE_IRQCHIP, but before calling
> > KVM_RUN on any of the VCPUs. Calling this ioctl twice for any of
> >the
> > base addresses will return -EEXIST.
> >
> >+Note, this IOCTL is deprecated and the more flexible
> >SET/GET_DEVICE_ATTR API
> >+should be used instead.
> >+
> >+
> > 4.85 KVM_PPC_RTAS_DEFINE_TOKEN
> >
> > Capability: KVM_CAP_PPC_RTAS
> >diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt
> >b/Documentation/virtual/kvm/devices/arm-vgic.txt
> >index 38f27f7..c9febb2 100644
> >--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> >+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> >@@ -8,3 +8,14 @@ Only one VGIC instance may be instantiated through
> >either this API or the
> > legacy KVM_CREATE_IRQCHIP api. The created VGIC will act as the VM
> >interrupt
> > controller, requiring emulated user-space devices to inject
> >interrupts to the
> > VGIC instead of directly to CPUs.
> >+
> >+Groups:
> >+ KVM_DEV_ARM_VGIC_GRP_ADDR
> >+ Attributes:
> >+ KVM_VGIC_V2_ADDR_TYPE_DIST (rw, 64-bit)
> >+ Base address in the guest physical address space of the GIC
> >distributor
> >+ register mappings.
> >+
> >+ 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.
> >diff --git a/arch/arm/include/uapi/asm/kvm.h
> >b/arch/arm/include/uapi/asm/kvm.h
> >index 1c85102..587f1ae 100644
> >--- a/arch/arm/include/uapi/asm/kvm.h
> >+++ b/arch/arm/include/uapi/asm/kvm.h
> >@@ -142,6 +142,15 @@ struct kvm_arch_memory_slot {
> > #define KVM_REG_ARM_VFP_FPINST 0x1009
> > #define KVM_REG_ARM_VFP_FPINST2 0x100A
> >
> >+/* Device Control API: ARM VGIC */
> >+#define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> >+#define KVM_DEV_ARM_VGIC_GRP_DIST_REGS 1
> >+#define KVM_DEV_ARM_VGIC_GRP_CPU_REGS 2
> >+#define KVM_DEV_ARM_VGIC_CPUID_SHIFT 32
> >+#define KVM_DEV_ARM_VGIC_CPUID_MASK (0xffULL <<
> >KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> >+#define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
> >+#define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL <<
> >KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >+
> > /* KVM_IRQ_LINE irq field index values */
> > #define KVM_ARM_IRQ_TYPE_SHIFT 24
> > #define KVM_ARM_IRQ_TYPE_MASK 0xff
> >diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >index ab96af2..3ecee45 100644
> >--- a/arch/arm/kvm/arm.c
> >+++ b/arch/arm/kvm/arm.c
> >@@ -773,7 +773,7 @@ static int kvm_vm_ioctl_set_device_addr(struct
> >kvm *kvm,
> > case KVM_ARM_DEVICE_VGIC_V2:
> > if (!vgic_present)
> > return -ENXIO;
> >- return kvm_vgic_set_addr(kvm, type, dev_addr->addr);
> >+ return kvm_vgic_addr(kvm, type, &dev_addr->addr, true);
> > default:
> > return -ENODEV;
> > }
> >diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >index 7e2d158..be85127 100644
> >--- a/include/kvm/arm_vgic.h
> >+++ b/include/kvm/arm_vgic.h
> >@@ -144,7 +144,7 @@ struct kvm_run;
> > struct kvm_exit_mmio;
> >
> > #ifdef CONFIG_KVM_ARM_VGIC
> >-int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64
> >addr);
> >+int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr,
> >bool write);
> > int kvm_vgic_hyp_init(void);
> > int kvm_vgic_init(struct kvm *kvm);
> > int kvm_vgic_create(struct kvm *kvm);
> >diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> >index 79a8bae..d9c0fc5 100644
> >--- a/virt/kvm/arm/vgic.c
> >+++ b/virt/kvm/arm/vgic.c
> >@@ -1479,6 +1479,12 @@ static int vgic_ioaddr_assign(struct kvm *kvm,
> >phys_addr_t *ioaddr,
> > {
> > int ret;
> >
> >+ if (addr & ~KVM_PHYS_MASK)
> >+ return -E2BIG;
> >+
> >+ if (addr & (SZ_4K - 1))
> >+ return -EINVAL;
> >+
> > if (!IS_VGIC_ADDR_UNDEF(*ioaddr))
> > return -EEXIST;
> > if (addr + size < addr)
> >@@ -1491,26 +1497,41 @@ static int vgic_ioaddr_assign(struct kvm
> >*kvm, phys_addr_t *ioaddr,
> > return ret;
> > }
> >
> >-int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr)
> >+/**
> >+ * kvm_vgic_addr - set or get vgic VM base addresses
> >+ * @kvm: pointer to the vm struct
> >+ * @type: the VGIC addr type, one of KVM_VGIC_V2_ADDR_TYPE_XXX
> >+ * @addr: pointer to address value
> >+ * @write: if true set the address in the VM address space, if false
> >read the
> >+ * address
> >+ *
> >+ * Set or get the vgic base addresses for the distributor and the
> >virtual CPU
> >+ * interface in the VM physical address space. These addresses are
> >properties
> >+ * of the emulated core/SoC and therefore user space initially
> >knows this
> >+ * information.
> >+ */
> >+int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr,
> >bool write)
> > {
> > int r = 0;
> > struct vgic_dist *vgic = &kvm->arch.vgic;
> >
> >- if (addr & ~KVM_PHYS_MASK)
> >- return -E2BIG;
> >-
> >- if (addr & (SZ_4K - 1))
> >- return -EINVAL;
> >-
> > mutex_lock(&kvm->lock);
> > switch (type) {
> > case KVM_VGIC_V2_ADDR_TYPE_DIST:
> >- r = vgic_ioaddr_assign(kvm, &vgic->vgic_dist_base,
> >- addr, KVM_VGIC_V2_DIST_SIZE);
> >+ if (write) {
> >+ r = vgic_ioaddr_assign(kvm, &vgic->vgic_dist_base,
> >+ *addr, KVM_VGIC_V2_DIST_SIZE);
> >+ } else {
> >+ *addr = vgic->vgic_dist_base;
> >+ }
> > break;
> > case KVM_VGIC_V2_ADDR_TYPE_CPU:
> >- r = vgic_ioaddr_assign(kvm, &vgic->vgic_cpu_base,
> >- addr, KVM_VGIC_V2_CPU_SIZE);
> >+ if (write) {
> >+ r = vgic_ioaddr_assign(kvm, &vgic->vgic_cpu_base,
> >+ *addr, KVM_VGIC_V2_CPU_SIZE);
> >+ } else {
> >+ *addr = vgic->vgic_cpu_base;
> >+ }
> > break;
> > default:
> > r = -ENODEV;
> >@@ -1522,16 +1543,61 @@ int kvm_vgic_set_addr(struct kvm *kvm,
> >unsigned long type, u64 addr)
> >
> > static int vgic_set_attr(struct kvm_device *dev, struct
> >kvm_device_attr *attr)
> > {
> >+ int r;
> >+
> >+ switch (attr->group) {
> >+ case KVM_DEV_ARM_VGIC_GRP_ADDR: {
> >+ u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> >+ u64 addr;
> >+ unsigned long type = (unsigned long)attr->attr;
> >+
> >+ if (copy_from_user(&addr, uaddr, sizeof(addr)))
> >+ return -EFAULT;
> >+
> >+ r = kvm_vgic_addr(dev->kvm, type, &addr, true);
> >+ return (r == -ENODEV) ? -ENXIO : r;
> >+ }
> >+ }
> >+
> > return -ENXIO;
> > }
> >
> > static int vgic_get_attr(struct kvm_device *dev, struct
> >kvm_device_attr *attr)
> > {
> >- return -ENXIO;
> >+ int r = ENXIO;
>
> Shouldn't this be a negative number?
>
Yes, it should.
> >+ switch (attr->group) {
> >+ case KVM_DEV_ARM_VGIC_GRP_ADDR: {
> >+ u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> >+ u64 addr;
> >+ unsigned long type = (unsigned long)attr->attr;
> >+
> >+ r = kvm_vgic_addr(dev->kvm, type, &addr, false);
> >+ if (r)
> >+ return (r == -ENODEV) ? -ENXIO : r;
> >+
> >+ r = 0;
>
> Isn't r already zero at this point?
>
yes
> >+ if (copy_to_user(uaddr, &addr, sizeof(addr)))
> >+ return -EFAULT;
> >+ }
> >+ }
> >+
> >+ return r;
> > }
> >
> > static int vgic_has_attr(struct kvm_device *dev, struct
> >kvm_device_attr *attr)
> > {
> >+ phys_addr_t offset;
> >+
> >+ switch (attr->group) {
> >+ case KVM_DEV_ARM_VGIC_GRP_ADDR:
> >+ switch (attr->attr) {
> >+ case KVM_VGIC_V2_ADDR_TYPE_DIST:
> >+ case KVM_VGIC_V2_ADDR_TYPE_CPU:
> >+ return 0;
> >+ }
> >+ break;
> >+ }
> > return -ENXIO;
> > }
>
> Cheers,
>
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
