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

[Peter Maydell <peter.maydell@linaro.org>]

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