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 <[email protected]> >> >> 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 <[email protected]> >> Signed-off-by: Andre Przywara <[email protected]> >> --- >> 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 <linux/byteorder.h> >> #include <linux/kvm.h> >> >> -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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
