From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH 03/11] kvmtool: AArch{32,64}: use KVM_CREATE_DEVICE & co to instanciate the GIC Date: Mon, 26 Jan 2015 12:06:01 +0000 Message-ID: <54C62DA9.7010809@arm.com> References: <1422030910-17881-1-git-send-email-andre.przywara@arm.com> <1422030910-17881-4-git-send-email-andre.przywara@arm.com> <20150126112603.GE15598@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: "penberg@kernel.org" , "kvm@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" , Marc Zyngier To: Will Deacon Return-path: Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]:55000 "EHLO cam-admin0.cambridge.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752202AbbAZMGH (ORCPT ); Mon, 26 Jan 2015 07:06:07 -0500 In-Reply-To: <20150126112603.GE15598@arm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 26/01/15 11:26, Will Deacon wrote: > On Fri, Jan 23, 2015 at 04:35:02PM +0000, Andre Przywara wrote: >> From: Marc Zyngier >> >> As of 3.14, KVM/arm supports the creation/configuration of the GIC through >> a more generic device API, which is now the preferred way to do so. >> >> Plumb the new API in, and allow the old code to be used as a fallback. >> >> [Andre: Rename some functions on the way to differentiate between >> creation and initialisation more clearly.] >> >> Signed-off-by: Marc Zyngier >> Signed-off-by: Andre Przywara >> --- >> tools/kvm/arm/gic.c | 60 ++++++++++++++++++++++++++++---- >> tools/kvm/arm/include/arm-common/gic.h | 2 +- >> tools/kvm/arm/kvm.c | 6 ++-- >> 3 files changed, 57 insertions(+), 11 deletions(-) >> >> diff --git a/tools/kvm/arm/gic.c b/tools/kvm/arm/gic.c >> index 5d8cbe6..ce5f7fa 100644 >> --- a/tools/kvm/arm/gic.c >> +++ b/tools/kvm/arm/gic.c >> @@ -7,7 +7,41 @@ >> #include >> #include >> >> -int gic__init_irqchip(struct kvm *kvm) >> +static int gic_fd = -1; >> + >> +static int gic__create_device(struct kvm *kvm) >> +{ >> + int err; >> + u64 cpu_if_addr = ARM_GIC_CPUI_BASE; >> + u64 dist_addr = ARM_GIC_DIST_BASE; >> + struct kvm_create_device gic_device = { >> + .type = KVM_DEV_TYPE_ARM_VGIC_V2, >> + }; >> + struct kvm_device_attr cpu_if_attr = { >> + .group = KVM_DEV_ARM_VGIC_GRP_ADDR, >> + .attr = KVM_VGIC_V2_ADDR_TYPE_CPU, >> + .addr = (u64)(unsigned long)&cpu_if_addr, >> + }; >> + struct kvm_device_attr dist_attr = { >> + .group = KVM_DEV_ARM_VGIC_GRP_ADDR, >> + .attr = KVM_VGIC_V2_ADDR_TYPE_DIST, >> + .addr = (u64)(unsigned long)&dist_addr, >> + }; >> + >> + err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device); >> + if (err) >> + return err; >> + >> + gic_fd = gic_device.fd; >> + >> + err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr); >> + if (err) >> + return err; >> + >> + return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr); >> +} >> + >> +static int gic__create_irqchip(struct kvm *kvm) >> { >> int err; >> struct kvm_arm_device_addr gic_addr[] = { >> @@ -23,12 +57,6 @@ int gic__init_irqchip(struct kvm *kvm) >> } >> }; >> >> - if (kvm->nrcpus > GIC_MAX_CPUS) { >> - pr_warning("%d CPUS greater than maximum of %d -- truncating\n", >> - kvm->nrcpus, GIC_MAX_CPUS); >> - kvm->nrcpus = GIC_MAX_CPUS; >> - } >> - >> err = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP); >> if (err) >> return err; >> @@ -41,6 +69,24 @@ int gic__init_irqchip(struct kvm *kvm) >> return err; >> } >> >> +int gic__create(struct kvm *kvm) >> +{ >> + int err; >> + >> + if (kvm->nrcpus > GIC_MAX_CPUS) { >> + pr_warning("%d CPUS greater than maximum of %d -- truncating\n", >> + kvm->nrcpus, GIC_MAX_CPUS); >> + kvm->nrcpus = GIC_MAX_CPUS; >> + } >> + >> + /* Try the new way first, and fallback on legacy method otherwise */ >> + err = gic__create_device(kvm); >> + if (err) >> + err = gic__create_irqchip(kvm); > > This fallback doesn't look safe to me: > > - gic_fd might remain initialised > - What does the kernel vgic driver do if you've already done > a successful KVM_CREATE_DEVICE and then try to use the legacy method? Good point. I think we need to cleanup the device by closing the fd (and resetting the variable to -1) in case any of the subsequent ioctls return with an error (e.g. due to unaligned addresses). I have to check what happens in the kernel in that case, though. Cheers, Andre.