[4/4] arm: vgic device control api support

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(-)

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

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