[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] arm: vgic device control api support
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] arm: vgic device control api support |
Date: |
Fri, 6 Sep 2013 14:34:40 +0100 |
On 23 August 2013 20:41, Christoffer Dall <address@hidden> 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 <address@hidden>
> ---
> 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,
Peter Maydell <=