| Message ID | 1422030910-17881-4-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 23, 2015 at 04:35:02PM +0000, Andre Przywara wrote: > From: Marc Zyngier <marc.zyngier@arm.com> > > 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 <marc.zyngier@arm.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > 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? Will -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <marc.zyngier@arm.com> >> >> 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 <marc.zyngier@arm.com> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + + return err; +} + void gic__generate_fdt_nodes(void *fdt, u32 phandle) { u64 reg_prop[] = { diff --git a/tools/kvm/arm/include/arm-common/gic.h b/tools/kvm/arm/include/arm-common/gic.h index 5487125..9bbe81e 100644 --- a/tools/kvm/arm/include/arm-common/gic.h +++ b/tools/kvm/arm/include/arm-common/gic.h @@ -29,7 +29,7 @@ struct kvm; int gic__alloc_irqnum(void); -int gic__init_irqchip(struct kvm *kvm); +int gic__create(struct kvm *kvm); void gic__generate_fdt_nodes(void *fdt, u32 phandle); #endif /* ARM_COMMON__GIC_H */ diff --git a/tools/kvm/arm/kvm.c b/tools/kvm/arm/kvm.c index 58ad9fa..bcd2533 100644 --- a/tools/kvm/arm/kvm.c +++ b/tools/kvm/arm/kvm.c @@ -81,7 +81,7 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size) madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size, MADV_MERGEABLE | MADV_HUGEPAGE); - /* Initialise the virtual GIC. */ - if (gic__init_irqchip(kvm)) - die("Failed to initialise virtual GIC"); + /* Create the virtual GIC. */ + if (gic__create(kvm)) + die("Failed to create virtual GIC"); }