| Message ID | 20160803145745.7556-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: > Currently we register ITS devices upon userland issuing the CTRL_INIT > ioctl to mark initialization of the ITS as done. > This deviates from the initialization sequence of the existing GIC > devices and does not play well with the way QEMU handles things. > To be more in line with what we are used to, register the ITS(es) just > before the first VCPU is about to run, so in the map_resources() call. > This involves iterating through the list of KVM devices and handle each > ITS that we find. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > this is based upon next-20160728 plus Christoffer's kvm_device locking > fix from today. Please let me know what tree I should base upon and I > will rebase. > Eric, Christoffer: does that do what you expect? Can QEMU live with that? > > Cheers, > Andre. > > virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- > virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ > virt/kvm/arm/vgic/vgic.h | 6 +++++ > 3 files changed, 58 insertions(+), 10 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 07411cf..e677a60 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) > its_sync_lpi_pending_table(vcpu); > } > > -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) > { > struct vgic_io_device *iodev = &its->iodev; > int ret; > > - if (its->initialized) > - return 0; > + if (!its->initialized) > + return -EBUSY; > > if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > return -ENXIO; > @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > mutex_unlock(&kvm->slots_lock); > > - if (!ret) > - its->initialized = true; > - > return ret; > } > > @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, > if (type != KVM_VGIC_ITS_ADDR_TYPE) > return -ENODEV; > > - if (its->initialized) > - return -EBUSY; > - > if (copy_from_user(&addr, uaddr, sizeof(addr))) > return -EFAULT; > > @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, > case KVM_DEV_ARM_VGIC_GRP_CTRL: > switch (attr->attr) { > case KVM_DEV_ARM_VGIC_CTRL_INIT: > - return vgic_its_init_its(dev->kvm, its); > + its->initialized = true; > + > + return 0; > } > break; > } > @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) > return kvm_register_device_ops(&kvm_arm_vgic_its_ops, > KVM_DEV_TYPE_ARM_VGIC_ITS); > } > + > +/* > + * Registers all ITSes with the kvm_io_bus framework. > + * To follow the existing VGIC initialization sequence, this has to be > + * done as late as possible, just before the first VCPU runs. > + */ > +int vgic_register_its_iodevs(struct kvm *kvm) > +{ > + struct kvm_device *dev; > + int ret = 0; > + > + mutex_lock(&kvm->devices_lock); > + > + list_for_each_entry(dev, &kvm->devices, vm_node) { > + if (dev->ops != &kvm_arm_vgic_its_ops) > + continue; > + > + ret = vgic_register_its_iodev(kvm, dev->private); > + if (ret) > + break; > + } > + > + if (ret) { > + /* Iterate backwards to roll back previous registrations. */ > + for (dev = list_prev_entry(dev, vm_node); > + &dev->vm_node != &kvm->devices; > + dev = list_prev_entry(dev, vm_node)) { > + struct vgic_its *its = dev->private; > + > + if (dev->ops != &kvm_arm_vgic_its_ops) > + continue; > + > + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, > + &its->iodev.dev); > + } > + } is the unregister really necessary? > + > + mutex_unlock(&kvm->devices_lock); > + return ret; > +} > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index 0506543..f0d9b2b 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) > goto out; > } > > + ret = vgic_register_its_iodevs(kvm); > + if (ret) { > + kvm_err("Unable to register VGIC ITS MMIO regions\n"); > + goto out; > + } > + > dist->ready = true; > > out: > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index 1d8e21d..6c4625c 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); > int vgic_v3_probe(const struct gic_kvm_info *info); > int vgic_v3_map_resources(struct kvm *kvm); > int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); > +int vgic_register_its_iodevs(struct kvm *kvm); > bool vgic_has_its(struct kvm *kvm); > int kvm_vgic_register_its_device(void); > void vgic_enable_lpis(struct kvm_vcpu *vcpu); > @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, > return -ENODEV; > } > > +static inline int vgic_register_its_iodevs(struct kvm *kvm) > +{ > + return -ENODEV; > +} > + > static inline bool vgic_has_its(struct kvm *kvm) > { > return false; > -- > 2.9.0 > Otherwise this looks good to me. Can someone provide a tested-by ? Thanks, -Christoffer -- 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 Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: > Currently we register ITS devices upon userland issuing the CTRL_INIT > ioctl to mark initialization of the ITS as done. > This deviates from the initialization sequence of the existing GIC > devices and does not play well with the way QEMU handles things. > To be more in line with what we are used to, register the ITS(es) just > before the first VCPU is about to run, so in the map_resources() call. > This involves iterating through the list of KVM devices and handle each > ITS that we find. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > this is based upon next-20160728 plus Christoffer's kvm_device locking Since my fix was horribly broken, I suggest you just base this on next and let it be similarly broken to the vfio-device that we already have in the kernel. That way, we get the userspace ABI right and we fix KVM in general for -rc2. Paolo/Radim, are you ok with this? Thanks, -Christoffer -- 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
Hi, On 03/08/16 18:11, Christoffer Dall wrote: > On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: >> Currently we register ITS devices upon userland issuing the CTRL_INIT >> ioctl to mark initialization of the ITS as done. >> This deviates from the initialization sequence of the existing GIC >> devices and does not play well with the way QEMU handles things. >> To be more in line with what we are used to, register the ITS(es) just >> before the first VCPU is about to run, so in the map_resources() call. >> This involves iterating through the list of KVM devices and handle each >> ITS that we find. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> Hi, >> >> this is based upon next-20160728 plus Christoffer's kvm_device locking >> fix from today. Please let me know what tree I should base upon and I >> will rebase. >> Eric, Christoffer: does that do what you expect? Can QEMU live with that? >> >> Cheers, >> Andre. >> >> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- >> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ >> virt/kvm/arm/vgic/vgic.h | 6 +++++ >> 3 files changed, 58 insertions(+), 10 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 07411cf..e677a60 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) >> its_sync_lpi_pending_table(vcpu); >> } >> >> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) >> { >> struct vgic_io_device *iodev = &its->iodev; >> int ret; >> >> - if (its->initialized) >> - return 0; >> + if (!its->initialized) >> + return -EBUSY; >> >> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) >> return -ENXIO; >> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); >> mutex_unlock(&kvm->slots_lock); >> >> - if (!ret) >> - its->initialized = true; >> - >> return ret; >> } >> >> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, >> if (type != KVM_VGIC_ITS_ADDR_TYPE) >> return -ENODEV; >> >> - if (its->initialized) >> - return -EBUSY; >> - >> if (copy_from_user(&addr, uaddr, sizeof(addr))) >> return -EFAULT; >> >> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, >> case KVM_DEV_ARM_VGIC_GRP_CTRL: >> switch (attr->attr) { >> case KVM_DEV_ARM_VGIC_CTRL_INIT: >> - return vgic_its_init_its(dev->kvm, its); >> + its->initialized = true; >> + >> + return 0; >> } >> break; >> } >> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) >> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, >> KVM_DEV_TYPE_ARM_VGIC_ITS); >> } >> + >> +/* >> + * Registers all ITSes with the kvm_io_bus framework. >> + * To follow the existing VGIC initialization sequence, this has to be >> + * done as late as possible, just before the first VCPU runs. >> + */ >> +int vgic_register_its_iodevs(struct kvm *kvm) >> +{ >> + struct kvm_device *dev; >> + int ret = 0; >> + >> + mutex_lock(&kvm->devices_lock); >> + >> + list_for_each_entry(dev, &kvm->devices, vm_node) { >> + if (dev->ops != &kvm_arm_vgic_its_ops) >> + continue; >> + >> + ret = vgic_register_its_iodev(kvm, dev->private); >> + if (ret) >> + break; >> + } >> + >> + if (ret) { >> + /* Iterate backwards to roll back previous registrations. */ >> + for (dev = list_prev_entry(dev, vm_node); >> + &dev->vm_node != &kvm->devices; >> + dev = list_prev_entry(dev, vm_node)) { >> + struct vgic_its *its = dev->private; >> + >> + if (dev->ops != &kvm_arm_vgic_its_ops) >> + continue; >> + >> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, >> + &its->iodev.dev); >> + } >> + } > > is the unregister really necessary? I was wondering the same, but we do it for the GICv3 redistributors as well (though that was introduced by the same stupid author). That being said I would be too happy to remove both these dodgy routines if we agree that a failure will ultimately lead to a VM teardown and is thus not needed. Cheers, Andre. > >> + >> + mutex_unlock(&kvm->devices_lock); >> + return ret; >> +} >> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c >> index 0506543..f0d9b2b 100644 >> --- a/virt/kvm/arm/vgic/vgic-v3.c >> +++ b/virt/kvm/arm/vgic/vgic-v3.c >> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) >> goto out; >> } >> >> + ret = vgic_register_its_iodevs(kvm); >> + if (ret) { >> + kvm_err("Unable to register VGIC ITS MMIO regions\n"); >> + goto out; >> + } >> + >> dist->ready = true; >> >> out: >> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >> index 1d8e21d..6c4625c 100644 >> --- a/virt/kvm/arm/vgic/vgic.h >> +++ b/virt/kvm/arm/vgic/vgic.h >> @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); >> int vgic_v3_probe(const struct gic_kvm_info *info); >> int vgic_v3_map_resources(struct kvm *kvm); >> int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); >> +int vgic_register_its_iodevs(struct kvm *kvm); >> bool vgic_has_its(struct kvm *kvm); >> int kvm_vgic_register_its_device(void); >> void vgic_enable_lpis(struct kvm_vcpu *vcpu); >> @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, >> return -ENODEV; >> } >> >> +static inline int vgic_register_its_iodevs(struct kvm *kvm) >> +{ >> + return -ENODEV; >> +} >> + >> static inline bool vgic_has_its(struct kvm *kvm) >> { >> return false; >> -- >> 2.9.0 >> > > Otherwise this looks good to me. > > Can someone provide a tested-by ? > > Thanks, > -Christoffer > -- 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
Hi Andre, Christoffer, On 03/08/2016 19:18, Andre Przywara wrote: > Hi, > > On 03/08/16 18:11, Christoffer Dall wrote: >> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: >>> Currently we register ITS devices upon userland issuing the CTRL_INIT >>> ioctl to mark initialization of the ITS as done. >>> This deviates from the initialization sequence of the existing GIC >>> devices and does not play well with the way QEMU handles things. >>> To be more in line with what we are used to, register the ITS(es) just >>> before the first VCPU is about to run, so in the map_resources() call. >>> This involves iterating through the list of KVM devices and handle each >>> ITS that we find. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> Hi, >>> >>> this is based upon next-20160728 plus Christoffer's kvm_device locking >>> fix from today. Please let me know what tree I should base upon and I >>> will rebase. >>> Eric, Christoffer: does that do what you expect? Can QEMU live with that? >>> >>> Cheers, >>> Andre. >>> >>> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- >>> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ >>> virt/kvm/arm/vgic/vgic.h | 6 +++++ >>> 3 files changed, 58 insertions(+), 10 deletions(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>> index 07411cf..e677a60 100644 >>> --- a/virt/kvm/arm/vgic/vgic-its.c >>> +++ b/virt/kvm/arm/vgic/vgic-its.c >>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) >>> its_sync_lpi_pending_table(vcpu); >>> } >>> >>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) >>> { >>> struct vgic_io_device *iodev = &its->iodev; >>> int ret; >>> >>> - if (its->initialized) >>> - return 0; >>> + if (!its->initialized) >>> + return -EBUSY; >>> >>> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) >>> return -ENXIO; >>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); >>> mutex_unlock(&kvm->slots_lock); >>> >>> - if (!ret) >>> - its->initialized = true; >>> - >>> return ret; >>> } >>> >>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>> if (type != KVM_VGIC_ITS_ADDR_TYPE) >>> return -ENODEV; >>> >>> - if (its->initialized) >>> - return -EBUSY; >>> - >>> if (copy_from_user(&addr, uaddr, sizeof(addr))) >>> return -EFAULT; >>> >>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>> case KVM_DEV_ARM_VGIC_GRP_CTRL: >>> switch (attr->attr) { >>> case KVM_DEV_ARM_VGIC_CTRL_INIT: >>> - return vgic_its_init_its(dev->kvm, its); >>> + its->initialized = true; >>> + >>> + return 0; >>> } >>> break; >>> } >>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) >>> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, >>> KVM_DEV_TYPE_ARM_VGIC_ITS); >>> } >>> + >>> +/* >>> + * Registers all ITSes with the kvm_io_bus framework. >>> + * To follow the existing VGIC initialization sequence, this has to be >>> + * done as late as possible, just before the first VCPU runs. >>> + */ >>> +int vgic_register_its_iodevs(struct kvm *kvm) >>> +{ >>> + struct kvm_device *dev; >>> + int ret = 0; >>> + >>> + mutex_lock(&kvm->devices_lock); >>> + >>> + list_for_each_entry(dev, &kvm->devices, vm_node) { >>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>> + continue; >>> + >>> + ret = vgic_register_its_iodev(kvm, dev->private); >>> + if (ret) >>> + break; >>> + } >>> + >>> + if (ret) { >>> + /* Iterate backwards to roll back previous registrations. */ >>> + for (dev = list_prev_entry(dev, vm_node); >>> + &dev->vm_node != &kvm->devices; >>> + dev = list_prev_entry(dev, vm_node)) { >>> + struct vgic_its *its = dev->private; >>> + >>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>> + continue; >>> + >>> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, >>> + &its->iodev.dev); >>> + } >>> + } >> >> is the unregister really necessary? > > I was wondering the same, but we do it for the GICv3 redistributors as > well (though that was introduced by the same stupid author). > That being said I would be too happy to remove both these dodgy routines > if we agree that a failure will ultimately lead to a VM teardown and is > thus not needed. Well to me this will lead to a kvm_vcpu_ioctl/KVM_RUN failure. Then the unregistration only happens in kvm_destroy_vm/kvm_io_bus_destroy and this calls iodevice destructor ops which is not implemented for our device so I don't think this can be removed right now. We shall implement this destructor ops first. In the past I attempted to use the destructor (early ages of new vgic) but this crashed. I think this crash should not happen anymore after: e6e3b5a64e5f15ebd569118a9af16bd4165cbd1a I tested the new version with a qemu integration looking similar to other vgic init: Tested-by: Eric Auger <eric.auger@redhat.com> Cheers Eric > > Cheers, > Andre. > >> >>> + >>> + mutex_unlock(&kvm->devices_lock); >>> + return ret; >>> +} >>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c >>> index 0506543..f0d9b2b 100644 >>> --- a/virt/kvm/arm/vgic/vgic-v3.c >>> +++ b/virt/kvm/arm/vgic/vgic-v3.c >>> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) >>> goto out; >>> } >>> >>> + ret = vgic_register_its_iodevs(kvm); >>> + if (ret) { >>> + kvm_err("Unable to register VGIC ITS MMIO regions\n"); >>> + goto out; >>> + } >>> + >>> dist->ready = true; >>> >>> out: >>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >>> index 1d8e21d..6c4625c 100644 >>> --- a/virt/kvm/arm/vgic/vgic.h >>> +++ b/virt/kvm/arm/vgic/vgic.h >>> @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); >>> int vgic_v3_probe(const struct gic_kvm_info *info); >>> int vgic_v3_map_resources(struct kvm *kvm); >>> int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); >>> +int vgic_register_its_iodevs(struct kvm *kvm); >>> bool vgic_has_its(struct kvm *kvm); >>> int kvm_vgic_register_its_device(void); >>> void vgic_enable_lpis(struct kvm_vcpu *vcpu); >>> @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, >>> return -ENODEV; >>> } >>> >>> +static inline int vgic_register_its_iodevs(struct kvm *kvm) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> static inline bool vgic_has_its(struct kvm *kvm) >>> { >>> return false; >>> -- >>> 2.9.0 >>> >> >> Otherwise this looks good to me. >> >> Can someone provide a tested-by ? >> >> Thanks, >> -Christoffer >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- 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 Wed, Aug 03, 2016 at 06:18:48PM +0100, Andre Przywara wrote: > Hi, > > On 03/08/16 18:11, Christoffer Dall wrote: > > On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: > >> Currently we register ITS devices upon userland issuing the CTRL_INIT > >> ioctl to mark initialization of the ITS as done. > >> This deviates from the initialization sequence of the existing GIC > >> devices and does not play well with the way QEMU handles things. > >> To be more in line with what we are used to, register the ITS(es) just > >> before the first VCPU is about to run, so in the map_resources() call. > >> This involves iterating through the list of KVM devices and handle each > >> ITS that we find. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >> --- > >> Hi, > >> > >> this is based upon next-20160728 plus Christoffer's kvm_device locking > >> fix from today. Please let me know what tree I should base upon and I > >> will rebase. > >> Eric, Christoffer: does that do what you expect? Can QEMU live with that? > >> > >> Cheers, > >> Andre. > >> > >> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- > >> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ > >> virt/kvm/arm/vgic/vgic.h | 6 +++++ > >> 3 files changed, 58 insertions(+), 10 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > >> index 07411cf..e677a60 100644 > >> --- a/virt/kvm/arm/vgic/vgic-its.c > >> +++ b/virt/kvm/arm/vgic/vgic-its.c > >> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) > >> its_sync_lpi_pending_table(vcpu); > >> } > >> > >> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > >> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) > >> { > >> struct vgic_io_device *iodev = &its->iodev; > >> int ret; > >> > >> - if (its->initialized) > >> - return 0; > >> + if (!its->initialized) > >> + return -EBUSY; > >> > >> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > >> return -ENXIO; > >> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > >> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > >> mutex_unlock(&kvm->slots_lock); > >> > >> - if (!ret) > >> - its->initialized = true; > >> - > >> return ret; > >> } > >> > >> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, > >> if (type != KVM_VGIC_ITS_ADDR_TYPE) > >> return -ENODEV; > >> > >> - if (its->initialized) > >> - return -EBUSY; > >> - > >> if (copy_from_user(&addr, uaddr, sizeof(addr))) > >> return -EFAULT; > >> > >> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, > >> case KVM_DEV_ARM_VGIC_GRP_CTRL: > >> switch (attr->attr) { > >> case KVM_DEV_ARM_VGIC_CTRL_INIT: > >> - return vgic_its_init_its(dev->kvm, its); > >> + its->initialized = true; > >> + > >> + return 0; > >> } > >> break; > >> } > >> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) > >> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, > >> KVM_DEV_TYPE_ARM_VGIC_ITS); > >> } > >> + > >> +/* > >> + * Registers all ITSes with the kvm_io_bus framework. > >> + * To follow the existing VGIC initialization sequence, this has to be > >> + * done as late as possible, just before the first VCPU runs. > >> + */ > >> +int vgic_register_its_iodevs(struct kvm *kvm) > >> +{ > >> + struct kvm_device *dev; > >> + int ret = 0; > >> + > >> + mutex_lock(&kvm->devices_lock); > >> + > >> + list_for_each_entry(dev, &kvm->devices, vm_node) { > >> + if (dev->ops != &kvm_arm_vgic_its_ops) > >> + continue; > >> + > >> + ret = vgic_register_its_iodev(kvm, dev->private); > >> + if (ret) > >> + break; > >> + } > >> + > >> + if (ret) { > >> + /* Iterate backwards to roll back previous registrations. */ > >> + for (dev = list_prev_entry(dev, vm_node); > >> + &dev->vm_node != &kvm->devices; > >> + dev = list_prev_entry(dev, vm_node)) { > >> + struct vgic_its *its = dev->private; > >> + > >> + if (dev->ops != &kvm_arm_vgic_its_ops) > >> + continue; > >> + > >> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, > >> + &its->iodev.dev); > >> + } > >> + } > > > > is the unregister really necessary? > > I was wondering the same, but we do it for the GICv3 redistributors as > well (though that was introduced by the same stupid author). > That being said I would be too happy to remove both these dodgy routines > if we agree that a failure will ultimately lead to a VM teardown and is > thus not needed. I think that the other architectures and places in KVM don't do this type of cleanup (I argued for this the last time around too), and this code here is particularly painful (not your fault, just the way it is), so I prefer getting rid of it. Can you argue for why we'd need it? Stuff failed, so even if userspace tries to do more stuff with a broken VM, what do we care if a few devices linger around registered on the IO bus? That shouldn't be able to break anything. In fact, is it not similar to initializing a few ITSes with the current implementation and getting a failure on a later one? Thanks, -Christoffer -- 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
Hi On 03/08/2016 19:48, Auger Eric wrote: > Hi Andre, Christoffer, > > On 03/08/2016 19:18, Andre Przywara wrote: >> Hi, >> >> On 03/08/16 18:11, Christoffer Dall wrote: >>> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: >>>> Currently we register ITS devices upon userland issuing the CTRL_INIT >>>> ioctl to mark initialization of the ITS as done. >>>> This deviates from the initialization sequence of the existing GIC >>>> devices and does not play well with the way QEMU handles things. >>>> To be more in line with what we are used to, register the ITS(es) just >>>> before the first VCPU is about to run, so in the map_resources() call. >>>> This involves iterating through the list of KVM devices and handle each >>>> ITS that we find. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> Hi, >>>> >>>> this is based upon next-20160728 plus Christoffer's kvm_device locking >>>> fix from today. Please let me know what tree I should base upon and I >>>> will rebase. >>>> Eric, Christoffer: does that do what you expect? Can QEMU live with that? >>>> >>>> Cheers, >>>> Andre. >>>> >>>> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- >>>> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ >>>> virt/kvm/arm/vgic/vgic.h | 6 +++++ >>>> 3 files changed, 58 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>>> index 07411cf..e677a60 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-its.c >>>> +++ b/virt/kvm/arm/vgic/vgic-its.c >>>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) >>>> its_sync_lpi_pending_table(vcpu); >>>> } >>>> >>>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) >>>> { >>>> struct vgic_io_device *iodev = &its->iodev; >>>> int ret; >>>> >>>> - if (its->initialized) >>>> - return 0; >>>> + if (!its->initialized) >>>> + return -EBUSY; >>>> >>>> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) >>>> return -ENXIO; >>>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>>> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); >>>> mutex_unlock(&kvm->slots_lock); >>>> >>>> - if (!ret) >>>> - its->initialized = true; >>>> - >>>> return ret; >>>> } >>>> >>>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>>> if (type != KVM_VGIC_ITS_ADDR_TYPE) >>>> return -ENODEV; >>>> >>>> - if (its->initialized) >>>> - return -EBUSY; >>>> - >>>> if (copy_from_user(&addr, uaddr, sizeof(addr))) >>>> return -EFAULT; >>>> >>>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>>> case KVM_DEV_ARM_VGIC_GRP_CTRL: >>>> switch (attr->attr) { >>>> case KVM_DEV_ARM_VGIC_CTRL_INIT: >>>> - return vgic_its_init_its(dev->kvm, its); >>>> + its->initialized = true; >>>> + >>>> + return 0; >>>> } >>>> break; >>>> } >>>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) >>>> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, >>>> KVM_DEV_TYPE_ARM_VGIC_ITS); >>>> } >>>> + >>>> +/* >>>> + * Registers all ITSes with the kvm_io_bus framework. >>>> + * To follow the existing VGIC initialization sequence, this has to be >>>> + * done as late as possible, just before the first VCPU runs. >>>> + */ >>>> +int vgic_register_its_iodevs(struct kvm *kvm) >>>> +{ >>>> + struct kvm_device *dev; >>>> + int ret = 0; >>>> + >>>> + mutex_lock(&kvm->devices_lock); >>>> + >>>> + list_for_each_entry(dev, &kvm->devices, vm_node) { >>>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>>> + continue; >>>> + >>>> + ret = vgic_register_its_iodev(kvm, dev->private); >>>> + if (ret) >>>> + break; >>>> + } >>>> + >>>> + if (ret) { >>>> + /* Iterate backwards to roll back previous registrations. */ >>>> + for (dev = list_prev_entry(dev, vm_node); >>>> + &dev->vm_node != &kvm->devices; >>>> + dev = list_prev_entry(dev, vm_node)) { >>>> + struct vgic_its *its = dev->private; >>>> + >>>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>>> + continue; >>>> + >>>> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, >>>> + &its->iodev.dev); >>>> + } >>>> + } >>> >>> is the unregister really necessary? >> >> I was wondering the same, but we do it for the GICv3 redistributors as >> well (though that was introduced by the same stupid author). >> That being said I would be too happy to remove both these dodgy routines >> if we agree that a failure will ultimately lead to a VM teardown and is >> thus not needed. > > Well to me this will lead to a kvm_vcpu_ioctl/KVM_RUN failure. Then the > unregistration only happens in kvm_destroy_vm/kvm_io_bus_destroy and > this calls iodevice destructor ops which is not implemented for our > device so I don't think this can be removed right now. hum no I mixed things I think: The destructor is needed if we want to remove our io-dev explicit deallocation in vgic and vits. Anyway this could be more elegant than what we have now. I can take this in charge if confirmed. sorry for the noise Eric > > We shall implement this destructor ops first. In the past I attempted to > use the destructor (early ages of new vgic) but this crashed. I think > this crash should not happen anymore after: > e6e3b5a64e5f15ebd569118a9af16bd4165cbd1a > > I tested the new version with a qemu integration looking similar to > other vgic init: > > Tested-by: Eric Auger <eric.auger@redhat.com> > > Cheers > > Eric > > >> >> Cheers, >> Andre. >> >>> >>>> + >>>> + mutex_unlock(&kvm->devices_lock); >>>> + return ret; >>>> +} >>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c >>>> index 0506543..f0d9b2b 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-v3.c >>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c >>>> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) >>>> goto out; >>>> } >>>> >>>> + ret = vgic_register_its_iodevs(kvm); >>>> + if (ret) { >>>> + kvm_err("Unable to register VGIC ITS MMIO regions\n"); >>>> + goto out; >>>> + } >>>> + >>>> dist->ready = true; >>>> >>>> out: >>>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >>>> index 1d8e21d..6c4625c 100644 >>>> --- a/virt/kvm/arm/vgic/vgic.h >>>> +++ b/virt/kvm/arm/vgic/vgic.h >>>> @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); >>>> int vgic_v3_probe(const struct gic_kvm_info *info); >>>> int vgic_v3_map_resources(struct kvm *kvm); >>>> int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); >>>> +int vgic_register_its_iodevs(struct kvm *kvm); >>>> bool vgic_has_its(struct kvm *kvm); >>>> int kvm_vgic_register_its_device(void); >>>> void vgic_enable_lpis(struct kvm_vcpu *vcpu); >>>> @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, >>>> return -ENODEV; >>>> } >>>> >>>> +static inline int vgic_register_its_iodevs(struct kvm *kvm) >>>> +{ >>>> + return -ENODEV; >>>> +} >>>> + >>>> static inline bool vgic_has_its(struct kvm *kvm) >>>> { >>>> return false; >>>> -- >>>> 2.9.0 >>>> >>> >>> Otherwise this looks good to me. >>> >>> Can someone provide a tested-by ? >>> >>> Thanks, >>> -Christoffer >>> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> -- 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 Wed, Aug 03, 2016 at 07:48:15PM +0200, Auger Eric wrote: > Hi Andre, Christoffer, > > On 03/08/2016 19:18, Andre Przywara wrote: > > Hi, > > > > On 03/08/16 18:11, Christoffer Dall wrote: > >> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: > >>> Currently we register ITS devices upon userland issuing the CTRL_INIT > >>> ioctl to mark initialization of the ITS as done. > >>> This deviates from the initialization sequence of the existing GIC > >>> devices and does not play well with the way QEMU handles things. > >>> To be more in line with what we are used to, register the ITS(es) just > >>> before the first VCPU is about to run, so in the map_resources() call. > >>> This involves iterating through the list of KVM devices and handle each > >>> ITS that we find. > >>> > >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >>> --- > >>> Hi, > >>> > >>> this is based upon next-20160728 plus Christoffer's kvm_device locking > >>> fix from today. Please let me know what tree I should base upon and I > >>> will rebase. > >>> Eric, Christoffer: does that do what you expect? Can QEMU live with that? > >>> > >>> Cheers, > >>> Andre. > >>> > >>> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- > >>> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ > >>> virt/kvm/arm/vgic/vgic.h | 6 +++++ > >>> 3 files changed, 58 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > >>> index 07411cf..e677a60 100644 > >>> --- a/virt/kvm/arm/vgic/vgic-its.c > >>> +++ b/virt/kvm/arm/vgic/vgic-its.c > >>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) > >>> its_sync_lpi_pending_table(vcpu); > >>> } > >>> > >>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > >>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) > >>> { > >>> struct vgic_io_device *iodev = &its->iodev; > >>> int ret; > >>> > >>> - if (its->initialized) > >>> - return 0; > >>> + if (!its->initialized) > >>> + return -EBUSY; > >>> > >>> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > >>> return -ENXIO; > >>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > >>> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > >>> mutex_unlock(&kvm->slots_lock); > >>> > >>> - if (!ret) > >>> - its->initialized = true; > >>> - > >>> return ret; > >>> } > >>> > >>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, > >>> if (type != KVM_VGIC_ITS_ADDR_TYPE) > >>> return -ENODEV; > >>> > >>> - if (its->initialized) > >>> - return -EBUSY; > >>> - > >>> if (copy_from_user(&addr, uaddr, sizeof(addr))) > >>> return -EFAULT; > >>> > >>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, > >>> case KVM_DEV_ARM_VGIC_GRP_CTRL: > >>> switch (attr->attr) { > >>> case KVM_DEV_ARM_VGIC_CTRL_INIT: > >>> - return vgic_its_init_its(dev->kvm, its); > >>> + its->initialized = true; > >>> + > >>> + return 0; > >>> } > >>> break; > >>> } > >>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) > >>> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, > >>> KVM_DEV_TYPE_ARM_VGIC_ITS); > >>> } > >>> + > >>> +/* > >>> + * Registers all ITSes with the kvm_io_bus framework. > >>> + * To follow the existing VGIC initialization sequence, this has to be > >>> + * done as late as possible, just before the first VCPU runs. > >>> + */ > >>> +int vgic_register_its_iodevs(struct kvm *kvm) > >>> +{ > >>> + struct kvm_device *dev; > >>> + int ret = 0; > >>> + > >>> + mutex_lock(&kvm->devices_lock); > >>> + > >>> + list_for_each_entry(dev, &kvm->devices, vm_node) { > >>> + if (dev->ops != &kvm_arm_vgic_its_ops) > >>> + continue; > >>> + > >>> + ret = vgic_register_its_iodev(kvm, dev->private); > >>> + if (ret) > >>> + break; > >>> + } > >>> + > >>> + if (ret) { > >>> + /* Iterate backwards to roll back previous registrations. */ > >>> + for (dev = list_prev_entry(dev, vm_node); > >>> + &dev->vm_node != &kvm->devices; > >>> + dev = list_prev_entry(dev, vm_node)) { > >>> + struct vgic_its *its = dev->private; > >>> + > >>> + if (dev->ops != &kvm_arm_vgic_its_ops) > >>> + continue; > >>> + > >>> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, > >>> + &its->iodev.dev); > >>> + } > >>> + } > >> > >> is the unregister really necessary? > > > > I was wondering the same, but we do it for the GICv3 redistributors as > > well (though that was introduced by the same stupid author). > > That being said I would be too happy to remove both these dodgy routines > > if we agree that a failure will ultimately lead to a VM teardown and is > > thus not needed. > > Well to me this will lead to a kvm_vcpu_ioctl/KVM_RUN failure. Then the > unregistration only happens in kvm_destroy_vm/kvm_io_bus_destroy and > this calls iodevice destructor ops which is not implemented for our > device so I don't think this can be removed right now. Then how are these things removed when you just shut down your VM? To be clear: I don't care if the VM is functional or not after having returned an error to userspace. I just care that we're not leaking resources or are (host kernel) vulnerable to attacks. Thanks, -Christoffer -- 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 03/08/2016 19:56, Auger Eric wrote: > Hi > > On 03/08/2016 19:48, Auger Eric wrote: >> Hi Andre, Christoffer, >> >> On 03/08/2016 19:18, Andre Przywara wrote: >>> Hi, >>> >>> On 03/08/16 18:11, Christoffer Dall wrote: >>>> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote: >>>>> Currently we register ITS devices upon userland issuing the CTRL_INIT >>>>> ioctl to mark initialization of the ITS as done. >>>>> This deviates from the initialization sequence of the existing GIC >>>>> devices and does not play well with the way QEMU handles things. >>>>> To be more in line with what we are used to, register the ITS(es) just >>>>> before the first VCPU is about to run, so in the map_resources() call. >>>>> This involves iterating through the list of KVM devices and handle each >>>>> ITS that we find. >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>>> --- >>>>> Hi, >>>>> >>>>> this is based upon next-20160728 plus Christoffer's kvm_device locking >>>>> fix from today. Please let me know what tree I should base upon and I >>>>> will rebase. >>>>> Eric, Christoffer: does that do what you expect? Can QEMU live with that? >>>>> >>>>> Cheers, >>>>> Andre. >>>>> >>>>> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- >>>>> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ >>>>> virt/kvm/arm/vgic/vgic.h | 6 +++++ >>>>> 3 files changed, 58 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>>>> index 07411cf..e677a60 100644 >>>>> --- a/virt/kvm/arm/vgic/vgic-its.c >>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c >>>>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) >>>>> its_sync_lpi_pending_table(vcpu); >>>>> } >>>>> >>>>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>>>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) >>>>> { >>>>> struct vgic_io_device *iodev = &its->iodev; >>>>> int ret; >>>>> >>>>> - if (its->initialized) >>>>> - return 0; >>>>> + if (!its->initialized) >>>>> + return -EBUSY; >>>>> >>>>> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) >>>>> return -ENXIO; >>>>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>>>> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); >>>>> mutex_unlock(&kvm->slots_lock); >>>>> >>>>> - if (!ret) >>>>> - its->initialized = true; >>>>> - >>>>> return ret; >>>>> } >>>>> >>>>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>>>> if (type != KVM_VGIC_ITS_ADDR_TYPE) >>>>> return -ENODEV; >>>>> >>>>> - if (its->initialized) >>>>> - return -EBUSY; >>>>> - >>>>> if (copy_from_user(&addr, uaddr, sizeof(addr))) >>>>> return -EFAULT; >>>>> >>>>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>>>> case KVM_DEV_ARM_VGIC_GRP_CTRL: >>>>> switch (attr->attr) { >>>>> case KVM_DEV_ARM_VGIC_CTRL_INIT: >>>>> - return vgic_its_init_its(dev->kvm, its); >>>>> + its->initialized = true; >>>>> + >>>>> + return 0; >>>>> } >>>>> break; >>>>> } >>>>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) >>>>> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, >>>>> KVM_DEV_TYPE_ARM_VGIC_ITS); >>>>> } >>>>> + >>>>> +/* >>>>> + * Registers all ITSes with the kvm_io_bus framework. >>>>> + * To follow the existing VGIC initialization sequence, this has to be >>>>> + * done as late as possible, just before the first VCPU runs. >>>>> + */ >>>>> +int vgic_register_its_iodevs(struct kvm *kvm) >>>>> +{ >>>>> + struct kvm_device *dev; >>>>> + int ret = 0; >>>>> + >>>>> + mutex_lock(&kvm->devices_lock); >>>>> + >>>>> + list_for_each_entry(dev, &kvm->devices, vm_node) { >>>>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>>>> + continue; >>>>> + >>>>> + ret = vgic_register_its_iodev(kvm, dev->private); >>>>> + if (ret) >>>>> + break; >>>>> + } >>>>> + >>>>> + if (ret) { >>>>> + /* Iterate backwards to roll back previous registrations. */ >>>>> + for (dev = list_prev_entry(dev, vm_node); >>>>> + &dev->vm_node != &kvm->devices; >>>>> + dev = list_prev_entry(dev, vm_node)) { >>>>> + struct vgic_its *its = dev->private; >>>>> + >>>>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>>>> + continue; >>>>> + >>>>> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, >>>>> + &its->iodev.dev); >>>>> + } >>>>> + } >>>> >>>> is the unregister really necessary? >>> >>> I was wondering the same, but we do it for the GICv3 redistributors as >>> well (though that was introduced by the same stupid author). >>> That being said I would be too happy to remove both these dodgy routines >>> if we agree that a failure will ultimately lead to a VM teardown and is >>> thus not needed. >> >> Well to me this will lead to a kvm_vcpu_ioctl/KVM_RUN failure. Then the >> unregistration only happens in kvm_destroy_vm/kvm_io_bus_destroy and >> this calls iodevice destructor ops which is not implemented for our >> device so I don't think this can be removed right now. > hum no I mixed things I think: The destructor is needed if we want to > remove our io-dev explicit deallocation in vgic and vits. Anyway this > could be more elegant than what we have now. I can take this in charge > if confirmed. > > sorry for the noise another trial :-) so I understand the bus is freed in kvm_io_bus_destroy and our kvmio_device are freed on vgic/its destruction (and not through the ops) so I think we can omit the unregistration which, as far as I understand removes the elt and reallocates a bus with a decremented size. Stopping here for today before saying other potential wrong things ... Thanks Eric > > Eric >> >> We shall implement this destructor ops first. In the past I attempted to >> use the destructor (early ages of new vgic) but this crashed. I think >> this crash should not happen anymore after: >> e6e3b5a64e5f15ebd569118a9af16bd4165cbd1a >> >> I tested the new version with a qemu integration looking similar to >> other vgic init: >> >> Tested-by: Eric Auger <eric.auger@redhat.com> >> >> Cheers >> >> Eric >> >> >>> >>> Cheers, >>> Andre. >>> >>>> >>>>> + >>>>> + mutex_unlock(&kvm->devices_lock); >>>>> + return ret; >>>>> +} >>>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c >>>>> index 0506543..f0d9b2b 100644 >>>>> --- a/virt/kvm/arm/vgic/vgic-v3.c >>>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c >>>>> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) >>>>> goto out; >>>>> } >>>>> >>>>> + ret = vgic_register_its_iodevs(kvm); >>>>> + if (ret) { >>>>> + kvm_err("Unable to register VGIC ITS MMIO regions\n"); >>>>> + goto out; >>>>> + } >>>>> + >>>>> dist->ready = true; >>>>> >>>>> out: >>>>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >>>>> index 1d8e21d..6c4625c 100644 >>>>> --- a/virt/kvm/arm/vgic/vgic.h >>>>> +++ b/virt/kvm/arm/vgic/vgic.h >>>>> @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); >>>>> int vgic_v3_probe(const struct gic_kvm_info *info); >>>>> int vgic_v3_map_resources(struct kvm *kvm); >>>>> int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); >>>>> +int vgic_register_its_iodevs(struct kvm *kvm); >>>>> bool vgic_has_its(struct kvm *kvm); >>>>> int kvm_vgic_register_its_device(void); >>>>> void vgic_enable_lpis(struct kvm_vcpu *vcpu); >>>>> @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, >>>>> return -ENODEV; >>>>> } >>>>> >>>>> +static inline int vgic_register_its_iodevs(struct kvm *kvm) >>>>> +{ >>>>> + return -ENODEV; >>>>> +} >>>>> + >>>>> static inline bool vgic_has_its(struct kvm *kvm) >>>>> { >>>>> return false; >>>>> -- >>>>> 2.9.0 >>>>> >>>> >>>> Otherwise this looks good to me. >>>> >>>> Can someone provide a tested-by ? >>>> >>>> Thanks, >>>> -Christoffer >>>> >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> -- 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
Hi Andre,
[auto build test ERROR on kvmarm/next]
[also build test ERROR on next-20160803]
[cannot apply to v4.7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Andre-Przywara/KVM-arm64-ITS-move-ITS-registration-into-first-VCPU-run/20160803-235253
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c: In function 'vgic_register_its_iodevs':
>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c:1508:17: error: 'struct kvm' has no member named 'devices_lock'
mutex_lock(&kvm->devices_lock);
^
arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c:1534:19: error: 'struct kvm' has no member named 'devices_lock'
mutex_unlock(&kvm->devices_lock);
^
vim +1508 arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c
1502 */
1503 int vgic_register_its_iodevs(struct kvm *kvm)
1504 {
1505 struct kvm_device *dev;
1506 int ret = 0;
1507
> 1508 mutex_lock(&kvm->devices_lock);
1509
1510 list_for_each_entry(dev, &kvm->devices, vm_node) {
1511 if (dev->ops != &kvm_arm_vgic_its_ops)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Andre, On 03/08/2016 16:57, Andre Przywara wrote: > Currently we register ITS devices upon userland issuing the CTRL_INIT > ioctl to mark initialization of the ITS as done. > This deviates from the initialization sequence of the existing GIC > devices and does not play well with the way QEMU handles things. > To be more in line with what we are used to, register the ITS(es) just > before the first VCPU is about to run, so in the map_resources() call. > This involves iterating through the list of KVM devices and handle each > ITS that we find. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > this is based upon next-20160728 plus Christoffer's kvm_device locking > fix from today. Please let me know what tree I should base upon and I > will rebase. > Eric, Christoffer: does that do what you expect? Can QEMU live with that? > > Cheers, > Andre. > > virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- > virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ > virt/kvm/arm/vgic/vgic.h | 6 +++++ > 3 files changed, 58 insertions(+), 10 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 07411cf..e677a60 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) > its_sync_lpi_pending_table(vcpu); > } > > -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) > { > struct vgic_io_device *iodev = &its->iodev; > int ret; > > - if (its->initialized) > - return 0; > + if (!its->initialized) > + return -EBUSY; > > if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > return -ENXIO; > @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) > KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > mutex_unlock(&kvm->slots_lock); > > - if (!ret) > - its->initialized = true; > - > return ret; > } > > @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, > if (type != KVM_VGIC_ITS_ADDR_TYPE) > return -ENODEV; > > - if (its->initialized) > - return -EBUSY; > - > if (copy_from_user(&addr, uaddr, sizeof(addr))) > return -EFAULT; > > @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, > case KVM_DEV_ARM_VGIC_GRP_CTRL: > switch (attr->attr) { > case KVM_DEV_ARM_VGIC_CTRL_INIT: > - return vgic_its_init_its(dev->kvm, its); > + its->initialized = true; > + > + return 0; > } > break; > } > @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) > return kvm_register_device_ops(&kvm_arm_vgic_its_ops, > KVM_DEV_TYPE_ARM_VGIC_ITS); > } > + > +/* > + * Registers all ITSes with the kvm_io_bus framework. > + * To follow the existing VGIC initialization sequence, this has to be > + * done as late as possible, just before the first VCPU runs. > + */ > +int vgic_register_its_iodevs(struct kvm *kvm) > +{ > + struct kvm_device *dev; > + int ret = 0; > + > + mutex_lock(&kvm->devices_lock); > + > + list_for_each_entry(dev, &kvm->devices, vm_node) { > + if (dev->ops != &kvm_arm_vgic_its_ops) > + continue; > + > + ret = vgic_register_its_iodev(kvm, dev->private); > + if (ret) > + break; > + } > + > + if (ret) { > + /* Iterate backwards to roll back previous registrations. */ > + for (dev = list_prev_entry(dev, vm_node); > + &dev->vm_node != &kvm->devices; > + dev = list_prev_entry(dev, vm_node)) { > + struct vgic_its *its = dev->private; > + > + if (dev->ops != &kvm_arm_vgic_its_ops) > + continue; > + > + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, > + &its->iodev.dev); > + } > + } > + > + mutex_unlock(&kvm->devices_lock); > + return ret; > +} > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index 0506543..f0d9b2b 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) > goto out; > } > don't you need to check vgic_has_its() first Thanks Eric > + ret = vgic_register_its_iodevs(kvm); > + if (ret) { > + kvm_err("Unable to register VGIC ITS MMIO regions\n"); > + goto out; > + } > + > dist->ready = true; > > out: > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index 1d8e21d..6c4625c 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); > int vgic_v3_probe(const struct gic_kvm_info *info); > int vgic_v3_map_resources(struct kvm *kvm); > int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); > +int vgic_register_its_iodevs(struct kvm *kvm); > bool vgic_has_its(struct kvm *kvm); > int kvm_vgic_register_its_device(void); > void vgic_enable_lpis(struct kvm_vcpu *vcpu); > @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, > return -ENODEV; > } > > +static inline int vgic_register_its_iodevs(struct kvm *kvm) > +{ > + return -ENODEV; > +} > + > static inline bool vgic_has_its(struct kvm *kvm) > { > return false; > -- 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
Hi Eric, On 04/08/16 11:11, Auger Eric wrote: > Hi Andre, > > On 03/08/2016 16:57, Andre Przywara wrote: >> Currently we register ITS devices upon userland issuing the CTRL_INIT >> ioctl to mark initialization of the ITS as done. >> This deviates from the initialization sequence of the existing GIC >> devices and does not play well with the way QEMU handles things. >> To be more in line with what we are used to, register the ITS(es) just >> before the first VCPU is about to run, so in the map_resources() call. >> This involves iterating through the list of KVM devices and handle each >> ITS that we find. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> Hi, >> >> this is based upon next-20160728 plus Christoffer's kvm_device locking >> fix from today. Please let me know what tree I should base upon and I >> will rebase. >> Eric, Christoffer: does that do what you expect? Can QEMU live with that? >> >> Cheers, >> Andre. >> >> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- >> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ >> virt/kvm/arm/vgic/vgic.h | 6 +++++ >> 3 files changed, 58 insertions(+), 10 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 07411cf..e677a60 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) >> its_sync_lpi_pending_table(vcpu); >> } >> >> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) >> { >> struct vgic_io_device *iodev = &its->iodev; >> int ret; >> >> - if (its->initialized) >> - return 0; >> + if (!its->initialized) >> + return -EBUSY; >> >> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) >> return -ENXIO; >> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); >> mutex_unlock(&kvm->slots_lock); >> >> - if (!ret) >> - its->initialized = true; >> - >> return ret; >> } >> >> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, >> if (type != KVM_VGIC_ITS_ADDR_TYPE) >> return -ENODEV; >> >> - if (its->initialized) >> - return -EBUSY; >> - >> if (copy_from_user(&addr, uaddr, sizeof(addr))) >> return -EFAULT; >> >> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, >> case KVM_DEV_ARM_VGIC_GRP_CTRL: >> switch (attr->attr) { >> case KVM_DEV_ARM_VGIC_CTRL_INIT: >> - return vgic_its_init_its(dev->kvm, its); >> + its->initialized = true; >> + >> + return 0; >> } >> break; >> } >> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) >> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, >> KVM_DEV_TYPE_ARM_VGIC_ITS); >> } >> + >> +/* >> + * Registers all ITSes with the kvm_io_bus framework. >> + * To follow the existing VGIC initialization sequence, this has to be >> + * done as late as possible, just before the first VCPU runs. >> + */ >> +int vgic_register_its_iodevs(struct kvm *kvm) >> +{ >> + struct kvm_device *dev; >> + int ret = 0; >> + >> + mutex_lock(&kvm->devices_lock); >> + >> + list_for_each_entry(dev, &kvm->devices, vm_node) { >> + if (dev->ops != &kvm_arm_vgic_its_ops) >> + continue; >> + >> + ret = vgic_register_its_iodev(kvm, dev->private); >> + if (ret) >> + break; >> + } >> + >> + if (ret) { >> + /* Iterate backwards to roll back previous registrations. */ >> + for (dev = list_prev_entry(dev, vm_node); >> + &dev->vm_node != &kvm->devices; >> + dev = list_prev_entry(dev, vm_node)) { >> + struct vgic_its *its = dev->private; >> + >> + if (dev->ops != &kvm_arm_vgic_its_ops) >> + continue; >> + >> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, >> + &its->iodev.dev); >> + } >> + } >> + >> + mutex_unlock(&kvm->devices_lock); >> + return ret; >> +} >> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c >> index 0506543..f0d9b2b 100644 >> --- a/virt/kvm/arm/vgic/vgic-v3.c >> +++ b/virt/kvm/arm/vgic/vgic-v3.c >> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) >> goto out; >> } >> > don't you need to check vgic_has_its() first I guess it's nice to do so, though technically it's not necessary, since we won't find any KVM devices which use the ITS ops if no ITS has been created. But since I respin this one anyway... Cheers, Andre. > Eric >> + ret = vgic_register_its_iodevs(kvm); >> + if (ret) { >> + kvm_err("Unable to register VGIC ITS MMIO regions\n"); >> + goto out; >> + } >> + >> dist->ready = true; >> >> out: >> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >> index 1d8e21d..6c4625c 100644 >> --- a/virt/kvm/arm/vgic/vgic.h >> +++ b/virt/kvm/arm/vgic/vgic.h >> @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); >> int vgic_v3_probe(const struct gic_kvm_info *info); >> int vgic_v3_map_resources(struct kvm *kvm); >> int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); >> +int vgic_register_its_iodevs(struct kvm *kvm); >> bool vgic_has_its(struct kvm *kvm); >> int kvm_vgic_register_its_device(void); >> void vgic_enable_lpis(struct kvm_vcpu *vcpu); >> @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, >> return -ENODEV; >> } >> >> +static inline int vgic_register_its_iodevs(struct kvm *kvm) >> +{ >> + return -ENODEV; >> +} >> + >> static inline bool vgic_has_its(struct kvm *kvm) >> { >> return false; >> > -- 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
Hi Andre, On 04/08/2016 12:17, Andre Przywara wrote: > Hi Eric, > > On 04/08/16 11:11, Auger Eric wrote: >> Hi Andre, >> >> On 03/08/2016 16:57, Andre Przywara wrote: >>> Currently we register ITS devices upon userland issuing the CTRL_INIT >>> ioctl to mark initialization of the ITS as done. >>> This deviates from the initialization sequence of the existing GIC >>> devices and does not play well with the way QEMU handles things. >>> To be more in line with what we are used to, register the ITS(es) just >>> before the first VCPU is about to run, so in the map_resources() call. >>> This involves iterating through the list of KVM devices and handle each >>> ITS that we find. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> Hi, >>> >>> this is based upon next-20160728 plus Christoffer's kvm_device locking >>> fix from today. Please let me know what tree I should base upon and I >>> will rebase. >>> Eric, Christoffer: does that do what you expect? Can QEMU live with that? >>> >>> Cheers, >>> Andre. >>> >>> virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- >>> virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ >>> virt/kvm/arm/vgic/vgic.h | 6 +++++ >>> 3 files changed, 58 insertions(+), 10 deletions(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>> index 07411cf..e677a60 100644 >>> --- a/virt/kvm/arm/vgic/vgic-its.c >>> +++ b/virt/kvm/arm/vgic/vgic-its.c >>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) >>> its_sync_lpi_pending_table(vcpu); >>> } >>> >>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) >>> { >>> struct vgic_io_device *iodev = &its->iodev; >>> int ret; >>> >>> - if (its->initialized) >>> - return 0; >>> + if (!its->initialized) >>> + return -EBUSY; >>> >>> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) >>> return -ENXIO; >>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) >>> KVM_VGIC_V3_ITS_SIZE, &iodev->dev); >>> mutex_unlock(&kvm->slots_lock); >>> >>> - if (!ret) >>> - its->initialized = true; >>> - >>> return ret; >>> } >>> >>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>> if (type != KVM_VGIC_ITS_ADDR_TYPE) >>> return -ENODEV; >>> >>> - if (its->initialized) >>> - return -EBUSY; >>> - >>> if (copy_from_user(&addr, uaddr, sizeof(addr))) >>> return -EFAULT; >>> >>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, >>> case KVM_DEV_ARM_VGIC_GRP_CTRL: >>> switch (attr->attr) { >>> case KVM_DEV_ARM_VGIC_CTRL_INIT: >>> - return vgic_its_init_its(dev->kvm, its); >>> + its->initialized = true; >>> + >>> + return 0; >>> } >>> break; >>> } >>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) >>> return kvm_register_device_ops(&kvm_arm_vgic_its_ops, >>> KVM_DEV_TYPE_ARM_VGIC_ITS); >>> } >>> + >>> +/* >>> + * Registers all ITSes with the kvm_io_bus framework. >>> + * To follow the existing VGIC initialization sequence, this has to be >>> + * done as late as possible, just before the first VCPU runs. >>> + */ >>> +int vgic_register_its_iodevs(struct kvm *kvm) >>> +{ >>> + struct kvm_device *dev; >>> + int ret = 0; >>> + >>> + mutex_lock(&kvm->devices_lock); >>> + >>> + list_for_each_entry(dev, &kvm->devices, vm_node) { >>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>> + continue; >>> + >>> + ret = vgic_register_its_iodev(kvm, dev->private); >>> + if (ret) >>> + break; >>> + } >>> + >>> + if (ret) { >>> + /* Iterate backwards to roll back previous registrations. */ >>> + for (dev = list_prev_entry(dev, vm_node); >>> + &dev->vm_node != &kvm->devices; >>> + dev = list_prev_entry(dev, vm_node)) { >>> + struct vgic_its *its = dev->private; >>> + >>> + if (dev->ops != &kvm_arm_vgic_its_ops) >>> + continue; >>> + >>> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, >>> + &its->iodev.dev); >>> + } >>> + } >>> + >>> + mutex_unlock(&kvm->devices_lock); >>> + return ret; >>> +} >>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c >>> index 0506543..f0d9b2b 100644 >>> --- a/virt/kvm/arm/vgic/vgic-v3.c >>> +++ b/virt/kvm/arm/vgic/vgic-v3.c >>> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) >>> goto out; >>> } >>> >> don't you need to check vgic_has_its() first > > I guess it's nice to do so, though technically it's not necessary, since > we won't find any KVM devices which use the ITS ops if no ITS has been > created. > But since I respin this one anyway... agreed. that's not critical. Cheers Eric > > Cheers, > Andre. > >> Eric >>> + ret = vgic_register_its_iodevs(kvm); >>> + if (ret) { >>> + kvm_err("Unable to register VGIC ITS MMIO regions\n"); >>> + goto out; >>> + } >>> + >>> dist->ready = true; >>> >>> out: >>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >>> index 1d8e21d..6c4625c 100644 >>> --- a/virt/kvm/arm/vgic/vgic.h >>> +++ b/virt/kvm/arm/vgic/vgic.h >>> @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); >>> int vgic_v3_probe(const struct gic_kvm_info *info); >>> int vgic_v3_map_resources(struct kvm *kvm); >>> int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); >>> +int vgic_register_its_iodevs(struct kvm *kvm); >>> bool vgic_has_its(struct kvm *kvm); >>> int kvm_vgic_register_its_device(void); >>> void vgic_enable_lpis(struct kvm_vcpu *vcpu); >>> @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, >>> return -ENODEV; >>> } >>> >>> +static inline int vgic_register_its_iodevs(struct kvm *kvm) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> static inline bool vgic_has_its(struct kvm *kvm) >>> { >>> return false; >>> >> > -- > 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 > -- 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/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 07411cf..e677a60 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) its_sync_lpi_pending_table(vcpu); } -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its) { struct vgic_io_device *iodev = &its->iodev; int ret; - if (its->initialized) - return 0; + if (!its->initialized) + return -EBUSY; if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) return -ENXIO; @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its) KVM_VGIC_V3_ITS_SIZE, &iodev->dev); mutex_unlock(&kvm->slots_lock); - if (!ret) - its->initialized = true; - return ret; } @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev, if (type != KVM_VGIC_ITS_ADDR_TYPE) return -ENODEV; - if (its->initialized) - return -EBUSY; - if (copy_from_user(&addr, uaddr, sizeof(addr))) return -EFAULT; @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev, case KVM_DEV_ARM_VGIC_GRP_CTRL: switch (attr->attr) { case KVM_DEV_ARM_VGIC_CTRL_INIT: - return vgic_its_init_its(dev->kvm, its); + its->initialized = true; + + return 0; } break; } @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void) return kvm_register_device_ops(&kvm_arm_vgic_its_ops, KVM_DEV_TYPE_ARM_VGIC_ITS); } + +/* + * Registers all ITSes with the kvm_io_bus framework. + * To follow the existing VGIC initialization sequence, this has to be + * done as late as possible, just before the first VCPU runs. + */ +int vgic_register_its_iodevs(struct kvm *kvm) +{ + struct kvm_device *dev; + int ret = 0; + + mutex_lock(&kvm->devices_lock); + + list_for_each_entry(dev, &kvm->devices, vm_node) { + if (dev->ops != &kvm_arm_vgic_its_ops) + continue; + + ret = vgic_register_its_iodev(kvm, dev->private); + if (ret) + break; + } + + if (ret) { + /* Iterate backwards to roll back previous registrations. */ + for (dev = list_prev_entry(dev, vm_node); + &dev->vm_node != &kvm->devices; + dev = list_prev_entry(dev, vm_node)) { + struct vgic_its *its = dev->private; + + if (dev->ops != &kvm_arm_vgic_its_ops) + continue; + + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, + &its->iodev.dev); + } + } + + mutex_unlock(&kvm->devices_lock); + return ret; +} diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index 0506543..f0d9b2b 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm) goto out; } + ret = vgic_register_its_iodevs(kvm); + if (ret) { + kvm_err("Unable to register VGIC ITS MMIO regions\n"); + goto out; + } + dist->ready = true; out: diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index 1d8e21d..6c4625c 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu); int vgic_v3_probe(const struct gic_kvm_info *info); int vgic_v3_map_resources(struct kvm *kvm); int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address); +int vgic_register_its_iodevs(struct kvm *kvm); bool vgic_has_its(struct kvm *kvm); int kvm_vgic_register_its_device(void); void vgic_enable_lpis(struct kvm_vcpu *vcpu); @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm, return -ENODEV; } +static inline int vgic_register_its_iodevs(struct kvm *kvm) +{ + return -ENODEV; +} + static inline bool vgic_has_its(struct kvm *kvm) { return false;
Currently we register ITS devices upon userland issuing the CTRL_INIT ioctl to mark initialization of the ITS as done. This deviates from the initialization sequence of the existing GIC devices and does not play well with the way QEMU handles things. To be more in line with what we are used to, register the ITS(es) just before the first VCPU is about to run, so in the map_resources() call. This involves iterating through the list of KVM devices and handle each ITS that we find. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- Hi, this is based upon next-20160728 plus Christoffer's kvm_device locking fix from today. Please let me know what tree I should base upon and I will rebase. Eric, Christoffer: does that do what you expect? Can QEMU live with that? Cheers, Andre. virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++-------- virt/kvm/arm/vgic/vgic-v3.c | 6 +++++ virt/kvm/arm/vgic/vgic.h | 6 +++++ 3 files changed, 58 insertions(+), 10 deletions(-)