[Qemu-devel] [PATCH 0/4] Create ARM KVM VGIC with device control API

[Qemu-devel] [PATCH 0/4] Create ARM KVM VGIC with device control API

[Christoffer Dall]

This patch series adds generic support for issuing device control
related ioctls and supports creating the ARM KVM-accelerated VGIC using
the device control API while maintaining backwards compatibility for
older kernels.

Christoffer Dall (4):
  kvm: Update headers for device control api
  kvm: Introduce kvm_arch_irqchip_create
  kvm: Common device control API functions
  arm: vgic device control api support

 hw/intc/arm_gic_kvm.c       |   23 ++++++++++++++++--
 include/sysemu/kvm.h        |    9 ++++++++
 kvm-all.c                   |   50 +++++++++++++++++++++++++++++++++++++--
 linux-headers/asm-arm/kvm.h |    8 +++++++
 linux-headers/linux/kvm.h   |    1 +
 target-arm/kvm.c            |   54 +++++++++++++++++++++++++++++++++++++------
 target-arm/kvm_arm.h        |   18 ++++++++++-----
 target-i386/kvm.c           |    5 ++++
 target-ppc/kvm.c            |    5 ++++
 target-s390x/kvm.c          |    5 ++++
 trace-events                |    1 +
 11 files changed, 162 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4] kvm: Update headers for device control api

[Christoffer Dall]

Update the KVM kernel headers to add support for the device control API
on ARM used to create in-kernel devices and set and get attributes on
these.

This is needed for VGIC save/restore with KVM ARM targets.

Headers are included from:
git://git.linaro.org/people/cdall/linux-kvm-arm.git vgic-migrate

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 linux-headers/asm-arm/kvm.h |    8 ++++++++
 linux-headers/linux/kvm.h   |    1 +
 2 files changed, 9 insertions(+)

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index c1ee007..587f1ae 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -142,6 +142,14 @@ 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
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index c614070..7f66a4f 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -839,6 +839,7 @@ struct kvm_device_attr {
 #define KVM_DEV_TYPE_FSL_MPIC_20	1
 #define KVM_DEV_TYPE_FSL_MPIC_42	2
 #define KVM_DEV_TYPE_XICS		3
+#define KVM_DEV_TYPE_ARM_VGIC_V2	4
 
 /*
  * ioctls for VM fds
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/4] kvm: Update headers for device control api

[Peter Maydell]

On 23 August 2013 20:40, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Update the KVM kernel headers to add support for the device control API
> on ARM used to create in-kernel devices and set and get attributes on
> these.
>
> This is needed for VGIC save/restore with KVM ARM targets.
>
> Headers are included from:
> git://git.linaro.org/people/cdall/linux-kvm-arm.git vgic-migrate

NB: this makes this an "RFC" patchset because we can't apply
it until the kernel headers have been accepted upstream.

-- PMM

[Qemu-devel] [PATCH 2/4] kvm: Introduce kvm_arch_irqchip_create

[Christoffer Dall]

Introduce kvm_arch_irqchip_create an arch-specific hook in preparation
for architecture-specific use of the device control API to create IRQ
chips.

Following patches will implement the ARM irqchip create method to prefer
the device control API over the older KVM_CREATE_IRQCHIP API.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 include/sysemu/kvm.h |    4 ++++
 kvm-all.c            |   11 +++++++++--
 target-arm/kvm.c     |    5 +++++
 target-i386/kvm.c    |    5 +++++
 target-ppc/kvm.c     |    5 +++++
 target-s390x/kvm.c   |    5 +++++
 6 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index de74411..1e5847e 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -224,6 +224,10 @@ void kvm_arch_reset_vcpu(CPUState *cpu);
 int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
 int kvm_arch_on_sigbus(int code, void *addr);
 
+/* return negative error on error, 0 if irq chip was not created, and positive
+ * number if it was created */
+int kvm_arch_irqchip_create(KVMState *s);
+
 void kvm_arch_init_irq_routing(KVMState *s);
 
 int kvm_set_irq(KVMState *s, int irq, int level);
diff --git a/kvm-all.c b/kvm-all.c
index 716860f..fe64f3b 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1295,10 +1295,17 @@ static int kvm_irqchip_create(KVMState *s)
         return 0;
     }
 
-    ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
+    /* First probe and see if there's a arch-specific hook to create the
+     * in-kernel irqchip for us */
+    ret = kvm_arch_irqchip_create(s);
     if (ret < 0) {
-        fprintf(stderr, "Create kernel irqchip failed\n");
         return ret;
+    } else if (ret == 0) {
+        ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
+        if (ret < 0) {
+            fprintf(stderr, "Create kernel irqchip failed\n");
+            return ret;
+        }
     }
 
     kvm_kernel_irqchip = true;
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index b92e00d..2484d90 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -647,3 +647,8 @@ void kvm_arch_remove_all_hw_breakpoints(void)
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
+
+int kvm_arch_irqchip_create(KVMState *s)
+{
+    return 0;
+}
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 376fc70..8a62161 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2188,6 +2188,11 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
            ((env->segs[R_CS].selector  & 3) != 3);
 }
 
+int kvm_arch_irqchip_create(KVMState *s)
+{
+    return 0;
+}
+
 void kvm_arch_init_irq_routing(KVMState *s)
 {
     if (!kvm_check_extension(s, KVM_CAP_IRQ_ROUTING)) {
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 30a870e..15c92e8 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1875,3 +1875,8 @@ int kvm_arch_on_sigbus(int code, void *addr)
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
+
+int kvm_arch_irqchip_create(KVMState *s)
+{
+    return 0;
+}
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 26d18e3..7f4dae9 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -893,6 +893,11 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 
+int kvm_arch_irqchip_create(KVMState *s)
+{
+    return 0;
+}
+
 int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign)
 {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/4] kvm: Introduce kvm_arch_irqchip_create

[Peter Maydell]

On 23 August 2013 20:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Introduce kvm_arch_irqchip_create an arch-specific hook in preparation
> for architecture-specific use of the device control API to create IRQ
> chips.
>
> Following patches will implement the ARM irqchip create method to prefer
> the device control API over the older KVM_CREATE_IRQCHIP API.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/sysemu/kvm.h |    4 ++++
>  kvm-all.c            |   11 +++++++++--
>  target-arm/kvm.c     |    5 +++++
>  target-i386/kvm.c    |    5 +++++
>  target-ppc/kvm.c     |    5 +++++
>  target-s390x/kvm.c   |    5 +++++
>  6 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index de74411..1e5847e 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -224,6 +224,10 @@ void kvm_arch_reset_vcpu(CPUState *cpu);
>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>  int kvm_arch_on_sigbus(int code, void *addr);
>
> +/* return negative error on error, 0 if irq chip was not created, and positive
> + * number if it was created */
> +int kvm_arch_irqchip_create(KVMState *s);

Could you follow the doc-comment coding style for this,
please? (The extract/deposit ops in include/qemu/bitops.h
are the template I usually crib from.)

> +
>  void kvm_arch_init_irq_routing(KVMState *s);
>
>  int kvm_set_irq(KVMState *s, int irq, int level);
> diff --git a/kvm-all.c b/kvm-all.c
> index 716860f..fe64f3b 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1295,10 +1295,17 @@ static int kvm_irqchip_create(KVMState *s)
>          return 0;
>      }
>
> -    ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
> +    /* First probe and see if there's a arch-specific hook to create the
> +     * in-kernel irqchip for us */
> +    ret = kvm_arch_irqchip_create(s);
>      if (ret < 0) {
> -        fprintf(stderr, "Create kernel irqchip failed\n");
>          return ret;
> +    } else if (ret == 0) {
> +        ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
> +        if (ret < 0) {
> +            fprintf(stderr, "Create kernel irqchip failed\n");
> +            return ret;
> +        }
>      }
>
>      kvm_kernel_irqchip = true;
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index b92e00d..2484d90 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -647,3 +647,8 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>  void kvm_arch_init_irq_routing(KVMState *s)
>  {
>  }
> +
> +int kvm_arch_irqchip_create(KVMState *s)
> +{
> +    return 0;
> +}

[ditto in s390/ppc/i386]

We can avoid all these identical stub versions of
this function by using our stubs mechanism:
create a file stubs/kvm.c, put the dummy function
definition in it, and add a line to stubs/Makefile.obj
saying "stub-obj-$(CONFIG_KVM) += kvm.o"

Then only the targets which actually need a real
implementation of this function have to provide one;
everybody else gets the stub version automatically.

In fact you could have the stub version's implementation
be to call kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP), which
would then simplify the kvm-all.c code a little.

(There are probably a few other kvm target functions
we could provide stubs for, but that's a later cleanup.)

-- PMM

[Qemu-devel] [PATCH 3/4] kvm: Common device control API functions

[Christoffer Dall]

Introduces two simple functions:
    int kvm_device_ioctl(int fd, int type, ...);
    int kvm_create_device(KVMState *s, uint64_t type, bool test);

These functions wrap the basic ioctl-based interactions with KVM in a
way similar to other KVM ioctl wrappers.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 include/sysemu/kvm.h |    5 +++++
 kvm-all.c            |   39 +++++++++++++++++++++++++++++++++++++++
 trace-events         |    1 +
 3 files changed, 45 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 1e5847e..84ca5ef 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -190,6 +190,11 @@ int kvm_vm_ioctl(KVMState *s, int type, ...);
 
 int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
 
+int kvm_device_ioctl(int fd, int type, ...);
+
+int kvm_create_device(KVMState *s, uint64_t type, bool test);
+
+
 /* Arch specific hooks */
 
 extern const KVMCapabilityInfo kvm_arch_required_capabilities[];
diff --git a/kvm-all.c b/kvm-all.c
index fe64f3b..957b961 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1770,6 +1770,24 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
     return ret;
 }
 
+int kvm_device_ioctl(int fd, int type, ...)
+{
+    int ret;
+    void *arg;
+    va_list ap;
+
+    va_start(ap, type);
+    arg = va_arg(ap, void *);
+    va_end(ap);
+
+    trace_kvm_device_ioctl(fd, type, arg);
+    ret = ioctl(fd, type, arg);
+    if (ret == -1) {
+        ret = -errno;
+    }
+    return ret;
+}
+
 int kvm_has_sync_mmu(void)
 {
     return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
@@ -2064,3 +2082,24 @@ int kvm_on_sigbus(int code, void *addr)
 {
     return kvm_arch_on_sigbus(code, addr);
 }
+
+int kvm_create_device(KVMState *s, uint64_t type, bool test)
+{
+    int ret;
+    struct kvm_create_device create_dev;
+
+    create_dev.type = type;
+    create_dev.fd = -1;
+    create_dev.flags = (test) ? KVM_CREATE_DEVICE_TEST : 0;
+
+    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
+        return -1;
+    }
+
+    ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &create_dev);
+    if (ret) {
+        return ret;
+    }
+
+    return (test) ? 0 : create_dev.fd;
+}
diff --git a/trace-events b/trace-events
index 3856b5c..5372c6e 100644
--- a/trace-events
+++ b/trace-events
@@ -1163,6 +1163,7 @@ migrate_set_state(int new_state) "new state %d"
 kvm_ioctl(int type, void *arg) "type %d, arg %p"
 kvm_vm_ioctl(int type, void *arg) "type %d, arg %p"
 kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type %d, arg %p"
+kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type %d, arg %p"
 kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
 
 # memory.c
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/4] kvm: Common device control API functions

[Peter Maydell]

On 23 August 2013 20:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Introduces two simple functions:
>     int kvm_device_ioctl(int fd, int type, ...);
>     int kvm_create_device(KVMState *s, uint64_t type, bool test);
>
> These functions wrap the basic ioctl-based interactions with KVM in a
> way similar to other KVM ioctl wrappers.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/sysemu/kvm.h |    5 +++++
>  kvm-all.c            |   39 +++++++++++++++++++++++++++++++++++++++
>  trace-events         |    1 +
>  3 files changed, 45 insertions(+)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 1e5847e..84ca5ef 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -190,6 +190,11 @@ int kvm_vm_ioctl(KVMState *s, int type, ...);
>
>  int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
>
> +int kvm_device_ioctl(int fd, int type, ...);
> +
> +int kvm_create_device(KVMState *s, uint64_t type, bool test);

Could we have doc comments for these, please?

> +
> +
>  /* Arch specific hooks */
>
>  extern const KVMCapabilityInfo kvm_arch_required_capabilities[];
> diff --git a/kvm-all.c b/kvm-all.c
> index fe64f3b..957b961 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1770,6 +1770,24 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
>      return ret;
>  }
>
> +int kvm_device_ioctl(int fd, int type, ...)
> +{
> +    int ret;
> +    void *arg;
> +    va_list ap;
> +
> +    va_start(ap, type);
> +    arg = va_arg(ap, void *);
> +    va_end(ap);
> +
> +    trace_kvm_device_ioctl(fd, type, arg);
> +    ret = ioctl(fd, type, arg);
> +    if (ret == -1) {
> +        ret = -errno;
> +    }
> +    return ret;
> +}
> +
>  int kvm_has_sync_mmu(void)
>  {
>      return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> @@ -2064,3 +2082,24 @@ int kvm_on_sigbus(int code, void *addr)
>  {
>      return kvm_arch_on_sigbus(code, addr);
>  }
> +
> +int kvm_create_device(KVMState *s, uint64_t type, bool test)
> +{
> +    int ret;
> +    struct kvm_create_device create_dev;
> +
> +    create_dev.type = type;
> +    create_dev.fd = -1;
> +    create_dev.flags = (test) ? KVM_CREATE_DEVICE_TEST : 0;

Why the brackets round 'test' ?

> +
> +    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
> +        return -1;

We should probably return -$some_errno here, since we
pass through a -errno return from kvm_vm_ioctl below.

> +    }
> +
> +    ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &create_dev);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    return (test) ? 0 : create_dev.fd;
> +}

thanks
-- PMM

[Qemu-devel] [PATCH 4/4] arm: vgic device control api support

[Christoffer Dall]

Support creating the ARM vgic device through the device control API and
setting the base address for the distributor and cpu interfaces in KVM
VMs using this API.

Because the older KVM_CREATE_IRQCHIP interface needs the irq chip to be
created prior to creating the VCPUs, we first test if if can use the
device control API in kvm_arch_irqchip_create (using the test flag from
the device control API).  If we cannot, it means we have to fall back to
KVM_CREATE_IRQCHIP and use the older ioctl at this point in time.  If
however, we can use the device control API, we don't do anything and
wait until the arm_gic_kvm driver initializes and let that use the
device control API.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/intc/arm_gic_kvm.c |   23 +++++++++++++++++++++--
 target-arm/kvm.c      |   49 ++++++++++++++++++++++++++++++++++++++++++-------
 target-arm/kvm_arm.h  |   18 ++++++++++++------
 3 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index f713975..9f0a852 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -35,6 +35,7 @@ typedef struct KVMARMGICClass {
     ARMGICCommonClass parent_class;
     DeviceRealize parent_realize;
     void (*parent_reset)(DeviceState *dev);
+    int dev_fd;
 } KVMARMGICClass;
 
 static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
@@ -97,6 +98,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
     GICState *s = KVM_ARM_GIC(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
+    int ret;
 
     kgc->parent_realize(dev, errp);
     if (error_is_set(errp)) {
@@ -119,13 +121,27 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < s->num_cpu; i++) {
         sysbus_init_irq(sbd, &s->parent_irq[i]);
     }
+
+    /* Try to create the device via the device control API */
+    kgc->dev_fd = -1;
+    ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V2, false);
+    if (ret >= 0) {
+        kgc->dev_fd = ret;
+    } else if (ret != -ENODEV) {
+        fprintf(stderr, "Error creating in-kernel VGIC: %d\n", ret);
+        abort();
+    }
+
     /* Distributor */
     memory_region_init_reservation(&s->iomem, OBJECT(s),
                                    "kvm-gic_dist", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
     kvm_arm_register_device(&s->iomem,
                             (KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT)
-                            | KVM_VGIC_V2_ADDR_TYPE_DIST);
+                            | KVM_VGIC_V2_ADDR_TYPE_DIST,
+                            KVM_DEV_ARM_VGIC_GRP_ADDR,
+                            KVM_VGIC_V2_ADDR_TYPE_DIST,
+                            kgc->dev_fd);
     /* CPU interface for current core. Unlike arm_gic, we don't
      * provide the "interface for core #N" memory regions, because
      * cores with a VGIC don't have those.
@@ -135,7 +151,10 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->cpuiomem[0]);
     kvm_arm_register_device(&s->cpuiomem[0],
                             (KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT)
-                            | KVM_VGIC_V2_ADDR_TYPE_CPU);
+                            | KVM_VGIC_V2_ADDR_TYPE_CPU,
+                            KVM_DEV_ARM_VGIC_GRP_ADDR,
+                            KVM_VGIC_V2_ADDR_TYPE_CPU,
+                            kgc->dev_fd);
 }
 
 static void kvm_arm_gic_class_init(ObjectClass *klass, void *data)
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 2484d90..aed3d86 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -184,8 +184,10 @@ out:
  */
 typedef struct KVMDevice {
     struct kvm_arm_device_addr kda;
+    struct kvm_device_attr kdattr;
     MemoryRegion *mr;
     QSLIST_ENTRY(KVMDevice) entries;
+    int dev_fd;
 } KVMDevice;
 
 static QSLIST_HEAD(kvm_devices_head, KVMDevice) kvm_devices_head;
@@ -219,6 +221,28 @@ static MemoryListener devlistener = {
     .region_del = kvm_arm_devlistener_del,
 };
 
+static void kvm_arm_set_device_addr(KVMDevice *kd)
+{
+    struct kvm_device_attr *attr = &kd->kdattr;
+    int ret;
+
+    /* If the device control API is available and we have a device fd on the
+     * KVMDevice struct, let's use the newer API */
+    if (kd->dev_fd >= 0) {
+        uint64_t addr = kd->kda.addr;
+        attr->addr = (uint64_t)(long)&addr;
+        ret = kvm_device_ioctl(kd->dev_fd, KVM_SET_DEVICE_ATTR, attr);
+    } else {
+        ret = kvm_vm_ioctl(kvm_state, KVM_ARM_SET_DEVICE_ADDR, &kd->kda);
+    }
+
+    if (ret < 0) {
+            fprintf(stderr, "Failed to set device address: %s\n",
+                    strerror(errno));
+            abort();
+    }
+}
+
 static void kvm_arm_machine_init_done(Notifier *notifier, void *data)
 {
     KVMDevice *kd, *tkd;
@@ -226,12 +250,7 @@ static void kvm_arm_machine_init_done(Notifier *notifier, void *data)
     memory_listener_unregister(&devlistener);
     QSLIST_FOREACH_SAFE(kd, &kvm_devices_head, entries, tkd) {
         if (kd->kda.addr != -1) {
-            if (kvm_vm_ioctl(kvm_state, KVM_ARM_SET_DEVICE_ADDR,
-                             &kd->kda) < 0) {
-                fprintf(stderr, "KVM_ARM_SET_DEVICE_ADDRESS failed: %s\n",
-                        strerror(errno));
-                abort();
-            }
+            kvm_arm_set_device_addr(kd);
         }
         memory_region_unref(kd->mr);
         g_free(kd);
@@ -242,7 +261,8 @@ static Notifier notify = {
     .notify = kvm_arm_machine_init_done,
 };
 
-void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid)
+void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
+                             uint64_t attr, int dev_fd)
 {
     KVMDevice *kd;
 
@@ -258,6 +278,10 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid)
     kd->mr = mr;
     kd->kda.id = devid;
     kd->kda.addr = -1;
+    kd->kdattr.flags = 0;
+    kd->kdattr.group = group;
+    kd->kdattr.attr = attr;
+    kd->dev_fd = dev_fd;
     QSLIST_INSERT_HEAD(&kvm_devices_head, kd, entries);
     memory_region_ref(kd->mr);
 }
@@ -650,5 +674,16 @@ void kvm_arch_init_irq_routing(KVMState *s)
 
 int kvm_arch_irqchip_create(KVMState *s)
 {
+    int ret;
+
+    /* If we can create the VGIC using the newer device control API, we
+     * let the device do this when it initializes itself, otherwise we
+     * fall back to the old API */
+
+    ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V2, true);
+    if (ret == 0) {
+        return 1;
+    }
+
     return 0;
 }
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index 5d14887..ea1805a 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -18,16 +18,22 @@
  * kvm_arm_register_device:
  * @mr: memory region for this device
  * @devid: the KVM device ID
+ * @type: device control API device type
+ * @group: device control API group for setting addresses
+ * @attr: device control API address type
+ * @dev_fd: device control device file descriptor (or -1 if not supported)
  *
  * Remember the memory region @mr, and when it is mapped by the
  * machine model, tell the kernel that base address using the
- * KVM_SET_DEVICE_ADDRESS ioctl. @devid should be the ID of
- * the device as defined by KVM_SET_DEVICE_ADDRESS.
- * The machine model may map and unmap the device multiple times;
- * the kernel will only be told the final address at the point
- * where machine init is complete.
+ * KVM_ARM_SET_DEVICE_ADDRESS ioctl or the newer device control API.  @devid
+ * should be the ID of the device as defined by KVM_ARM_SET_DEVICE_ADDRESS or
+ * the arm-vgic device in the device control API.
+ * The machine model may map
+ * and unmap the device multiple times; the kernel will only be told the final
+ * address at the point where machine init is complete.
  */
-void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid);
+void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
+                             uint64_t attr, int dev_fd);
 
 /**
  * write_list_to_kvmstate:
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/4] arm: vgic device control api support

[Peter Maydell]

On 23 August 2013 20:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Support creating the ARM vgic device through the device control API and
> setting the base address for the distributor and cpu interfaces in KVM
> VMs using this API.
>
> Because the older KVM_CREATE_IRQCHIP interface needs the irq chip to be
> created prior to creating the VCPUs, we first test if if can use the

"if we"

> device control API in kvm_arch_irqchip_create (using the test flag from
> the device control API).  If we cannot, it means we have to fall back to
> KVM_CREATE_IRQCHIP and use the older ioctl at this point in time.  If
> however, we can use the device control API, we don't do anything and
> wait until the arm_gic_kvm driver initializes and let that use the
> device control API.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/intc/arm_gic_kvm.c |   23 +++++++++++++++++++++--
>  target-arm/kvm.c      |   49 ++++++++++++++++++++++++++++++++++++++++++-------
>  target-arm/kvm_arm.h  |   18 ++++++++++++------
>  3 files changed, 75 insertions(+), 15 deletions(-)
>
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index f713975..9f0a852 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -35,6 +35,7 @@ typedef struct KVMARMGICClass {
>      ARMGICCommonClass parent_class;
>      DeviceRealize parent_realize;
>      void (*parent_reset)(DeviceState *dev);
> +    int dev_fd;
>  } KVMARMGICClass;

This looks wrong -- surely we should have a dev_fd per
instance of KVMARMGIC, not just one in the class struct?

>  static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
> @@ -97,6 +98,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>      GICState *s = KVM_ARM_GIC(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
> +    int ret;
>
>      kgc->parent_realize(dev, errp);
>      if (error_is_set(errp)) {
> @@ -119,13 +121,27 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>      for (i = 0; i < s->num_cpu; i++) {
>          sysbus_init_irq(sbd, &s->parent_irq[i]);
>      }
> +
> +    /* Try to create the device via the device control API */
> +    kgc->dev_fd = -1;
> +    ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V2, false);
> +    if (ret >= 0) {
> +        kgc->dev_fd = ret;
> +    } else if (ret != -ENODEV) {
> +        fprintf(stderr, "Error creating in-kernel VGIC: %d\n", ret);
> +        abort();

We shouldn't abort here, we can just report our failure
back to the caller via the Error** it passed us:

    error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
    return;

(there's also an error_setg() if there's no errno involved;
both versions use a printf-style format string and can take
extra args accordingly.)

> +    }
> +
>      /* Distributor */
>      memory_region_init_reservation(&s->iomem, OBJECT(s),
>                                     "kvm-gic_dist", 0x1000);
>      sysbus_init_mmio(sbd, &s->iomem);
>      kvm_arm_register_device(&s->iomem,
>                              (KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT)
> -                            | KVM_VGIC_V2_ADDR_TYPE_DIST);
> +                            | KVM_VGIC_V2_ADDR_TYPE_DIST,
> +                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                            KVM_VGIC_V2_ADDR_TYPE_DIST,
> +                            kgc->dev_fd);
>      /* CPU interface for current core. Unlike arm_gic, we don't
>       * provide the "interface for core #N" memory regions, because
>       * cores with a VGIC don't have those.
> @@ -135,7 +151,10 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(sbd, &s->cpuiomem[0]);
>      kvm_arm_register_device(&s->cpuiomem[0],
>                              (KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT)
> -                            | KVM_VGIC_V2_ADDR_TYPE_CPU);
> +                            | KVM_VGIC_V2_ADDR_TYPE_CPU,
> +                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                            KVM_VGIC_V2_ADDR_TYPE_CPU,
> +                            kgc->dev_fd);
>  }
>
>  static void kvm_arm_gic_class_init(ObjectClass *klass, void *data)
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 2484d90..aed3d86 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -184,8 +184,10 @@ out:
>   */
>  typedef struct KVMDevice {
>      struct kvm_arm_device_addr kda;
> +    struct kvm_device_attr kdattr;
>      MemoryRegion *mr;
>      QSLIST_ENTRY(KVMDevice) entries;
> +    int dev_fd;
>  } KVMDevice;
>
>  static QSLIST_HEAD(kvm_devices_head, KVMDevice) kvm_devices_head;
> @@ -219,6 +221,28 @@ static MemoryListener devlistener = {
>      .region_del = kvm_arm_devlistener_del,
>  };
>
> +static void kvm_arm_set_device_addr(KVMDevice *kd)
> +{
> +    struct kvm_device_attr *attr = &kd->kdattr;
> +    int ret;
> +
> +    /* If the device control API is available and we have a device fd on the
> +     * KVMDevice struct, let's use the newer API */

putting the closing */ on a line of its own fits the style
in the rest of this file (and looks nicer imho ;-))


> +    if (kd->dev_fd >= 0) {
> +        uint64_t addr = kd->kda.addr;
> +        attr->addr = (uint64_t)(long)&addr;

why (uint64_t)(long)? Other places (like the get/put register
code) where we need to fill in an address into a uint64_t
field in a kernel struct we do with a simple
   attr->addr = (uintptr_t)&addr;

> +        ret = kvm_device_ioctl(kd->dev_fd, KVM_SET_DEVICE_ATTR, attr);
> +    } else {
> +        ret = kvm_vm_ioctl(kvm_state, KVM_ARM_SET_DEVICE_ADDR, &kd->kda);
> +    }
> +
> +    if (ret < 0) {
> +            fprintf(stderr, "Failed to set device address: %s\n",
> +                    strerror(errno));

Your error code is in -ret here, not in errno.

> +            abort();
> +    }
> +}

Otherwise looks OK.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/4] arm: vgic device control api support

[Christoffer Dall]

On Fri, Sep 06, 2013 at 02:34:40PM +0100, Peter Maydell wrote:
> On 23 August 2013 20:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Support creating the ARM vgic device through the device control API and
> > setting the base address for the distributor and cpu interfaces in KVM
> > VMs using this API.
> >
> > Because the older KVM_CREATE_IRQCHIP interface needs the irq chip to be
> > created prior to creating the VCPUs, we first test if if can use the
> 
> "if we"
> 
Peter,

Thanks for your review comments and pointers on how to fix.  Much
appreciated.  I've addressed your comments and will get a new series out
asap.

-Christoffer