From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37183) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHwBn-0005KA-Ao for qemu-devel@nongnu.org; Fri, 06 Sep 2013 09:35:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VHwBi-00046x-Ab for qemu-devel@nongnu.org; Fri, 06 Sep 2013 09:35:07 -0400 Received: from mail-lb0-f169.google.com ([209.85.217.169]:46085) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHwBh-00042W-UG for qemu-devel@nongnu.org; Fri, 06 Sep 2013 09:35:02 -0400 Received: by mail-lb0-f169.google.com with SMTP id z5so2909772lbh.0 for ; Fri, 06 Sep 2013 06:35:00 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1377286862-5879-5-git-send-email-christoffer.dall@linaro.org> References: <1377286862-5879-1-git-send-email-christoffer.dall@linaro.org> <1377286862-5879-5-git-send-email-christoffer.dall@linaro.org> From: Peter Maydell Date: Fri, 6 Sep 2013 14:34:40 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 4/4] arm: vgic device control api support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoffer Dall Cc: "linaro-kernel@lists.linaro.org" , QEMU Developers , Patch Tracking , "kvmarm@lists.cs.columbia.edu" On 23 August 2013 20:41, Christoffer Dall 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 > --- > 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