| Message ID | 20160810103914.30322-1-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/08/2016 12:39, Christoffer Dall wrote: > Since we are about to synchronize all accesses to kvm->devices using the > kvm->lock mutex, we should hold this mutex while iterating over the list > of devices in the ITS code. > > Also move the vgic_register_its_iodev function to where it's called and > rename it to register_its_iodev to avoid having two almost identially > named functions. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > virt/kvm/arm/vgic/vgic-its.c | 64 +++++++++++++++++++++++--------------------- > 1 file changed, 34 insertions(+), 30 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 1cf9f59..4e76877 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1319,32 +1319,6 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) > its_sync_lpi_pending_table(vcpu); > } > > -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 -EBUSY; > - > - if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > - return -ENXIO; > - > - iodev->regions = its_registers; > - iodev->nr_regions = ARRAY_SIZE(its_registers); > - kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops); > - > - iodev->base_addr = its->vgic_its_base; > - iodev->iodev_type = IODEV_ITS; > - iodev->its = its; > - mutex_lock(&kvm->slots_lock); > - ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr, > - KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > - mutex_unlock(&kvm->slots_lock); > - > - return ret; > -} > - > #define INITIAL_BASER_VALUE \ > (GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWb) | \ > GIC_BASER_CACHEABILITY(GITS_BASER, OUTER, SameAsInner) | \ > @@ -1526,6 +1500,32 @@ int kvm_vgic_register_its_device(void) > KVM_DEV_TYPE_ARM_VGIC_ITS); > } > > +static int register_its_iodev(struct kvm *kvm, struct vgic_its *its) > +{ > + struct vgic_io_device *iodev = &its->iodev; > + int ret; > + > + if (!its->initialized) > + return -EBUSY; > + > + if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > + return -ENXIO; > + > + iodev->regions = its_registers; > + iodev->nr_regions = ARRAY_SIZE(its_registers); > + kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops); > + > + iodev->base_addr = its->vgic_its_base; > + iodev->iodev_type = IODEV_ITS; > + iodev->its = its; > + mutex_lock(&kvm->slots_lock); > + ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr, > + KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > + mutex_unlock(&kvm->slots_lock); > + > + return ret; > +} > + > /* > * Registers all ITSes with the kvm_io_bus framework. > * To follow the existing VGIC initialization sequence, this has to be > @@ -1536,19 +1536,23 @@ int vgic_register_its_iodevs(struct kvm *kvm) > struct kvm_device *dev; > int ret = 0; > > + mutex_lock(&kvm->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); > + ret = register_its_iodev(kvm, dev->private); > if (ret) > - return ret; > + goto out; > + > /* > * We don't need to care about tearing down previously > - * registered ITSes, as the kvm_io_bus framework removes > - * them for us if the VM gets destroyed. > + * registered ITSes on error, as the kvm_io_bus framework > + * removes them for us if the VM gets destroyed. > */ > } > > +out: > + mutex_unlock(&kvm->lock); > return ret; > } > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> -- 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 10, 2016 at 03:10:51PM +0200, Paolo Bonzini wrote: > > > On 10/08/2016 12:39, Christoffer Dall wrote: > > Since we are about to synchronize all accesses to kvm->devices using the > > kvm->lock mutex, we should hold this mutex while iterating over the list > > of devices in the ITS code. > > > > Also move the vgic_register_its_iodev function to where it's called and > > rename it to register_its_iodev to avoid having two almost identially > > named functions. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > virt/kvm/arm/vgic/vgic-its.c | 64 +++++++++++++++++++++++--------------------- > > 1 file changed, 34 insertions(+), 30 deletions(-) > > > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > > index 1cf9f59..4e76877 100644 > > --- a/virt/kvm/arm/vgic/vgic-its.c > > +++ b/virt/kvm/arm/vgic/vgic-its.c > > @@ -1319,32 +1319,6 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) > > its_sync_lpi_pending_table(vcpu); > > } > > > > -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 -EBUSY; > > - > > - if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > > - return -ENXIO; > > - > > - iodev->regions = its_registers; > > - iodev->nr_regions = ARRAY_SIZE(its_registers); > > - kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops); > > - > > - iodev->base_addr = its->vgic_its_base; > > - iodev->iodev_type = IODEV_ITS; > > - iodev->its = its; > > - mutex_lock(&kvm->slots_lock); > > - ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr, > > - KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > > - mutex_unlock(&kvm->slots_lock); > > - > > - return ret; > > -} > > - > > #define INITIAL_BASER_VALUE \ > > (GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWb) | \ > > GIC_BASER_CACHEABILITY(GITS_BASER, OUTER, SameAsInner) | \ > > @@ -1526,6 +1500,32 @@ int kvm_vgic_register_its_device(void) > > KVM_DEV_TYPE_ARM_VGIC_ITS); > > } > > > > +static int register_its_iodev(struct kvm *kvm, struct vgic_its *its) > > +{ > > + struct vgic_io_device *iodev = &its->iodev; > > + int ret; > > + > > + if (!its->initialized) > > + return -EBUSY; > > + > > + if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) > > + return -ENXIO; > > + > > + iodev->regions = its_registers; > > + iodev->nr_regions = ARRAY_SIZE(its_registers); > > + kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops); > > + > > + iodev->base_addr = its->vgic_its_base; > > + iodev->iodev_type = IODEV_ITS; > > + iodev->its = its; > > + mutex_lock(&kvm->slots_lock); > > + ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr, > > + KVM_VGIC_V3_ITS_SIZE, &iodev->dev); > > + mutex_unlock(&kvm->slots_lock); > > + > > + return ret; > > +} > > + > > /* > > * Registers all ITSes with the kvm_io_bus framework. > > * To follow the existing VGIC initialization sequence, this has to be > > @@ -1536,19 +1536,23 @@ int vgic_register_its_iodevs(struct kvm *kvm) > > struct kvm_device *dev; > > int ret = 0; > > > > + mutex_lock(&kvm->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); > > + ret = register_its_iodev(kvm, dev->private); > > if (ret) > > - return ret; > > + goto out; > > + > > /* > > * We don't need to care about tearing down previously > > - * registered ITSes, as the kvm_io_bus framework removes > > - * them for us if the VM gets destroyed. > > + * registered ITSes on error, as the kvm_io_bus framework > > + * removes them for us if the VM gets destroyed. > > */ > > } > > > > +out: > > + mutex_unlock(&kvm->lock); > > return ret; > > } > > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks Paolo! -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
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 1cf9f59..4e76877 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1319,32 +1319,6 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu) its_sync_lpi_pending_table(vcpu); } -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 -EBUSY; - - if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) - return -ENXIO; - - iodev->regions = its_registers; - iodev->nr_regions = ARRAY_SIZE(its_registers); - kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops); - - iodev->base_addr = its->vgic_its_base; - iodev->iodev_type = IODEV_ITS; - iodev->its = its; - mutex_lock(&kvm->slots_lock); - ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr, - KVM_VGIC_V3_ITS_SIZE, &iodev->dev); - mutex_unlock(&kvm->slots_lock); - - return ret; -} - #define INITIAL_BASER_VALUE \ (GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWb) | \ GIC_BASER_CACHEABILITY(GITS_BASER, OUTER, SameAsInner) | \ @@ -1526,6 +1500,32 @@ int kvm_vgic_register_its_device(void) KVM_DEV_TYPE_ARM_VGIC_ITS); } +static int register_its_iodev(struct kvm *kvm, struct vgic_its *its) +{ + struct vgic_io_device *iodev = &its->iodev; + int ret; + + if (!its->initialized) + return -EBUSY; + + if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) + return -ENXIO; + + iodev->regions = its_registers; + iodev->nr_regions = ARRAY_SIZE(its_registers); + kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops); + + iodev->base_addr = its->vgic_its_base; + iodev->iodev_type = IODEV_ITS; + iodev->its = its; + mutex_lock(&kvm->slots_lock); + ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr, + KVM_VGIC_V3_ITS_SIZE, &iodev->dev); + mutex_unlock(&kvm->slots_lock); + + return ret; +} + /* * Registers all ITSes with the kvm_io_bus framework. * To follow the existing VGIC initialization sequence, this has to be @@ -1536,19 +1536,23 @@ int vgic_register_its_iodevs(struct kvm *kvm) struct kvm_device *dev; int ret = 0; + mutex_lock(&kvm->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); + ret = register_its_iodev(kvm, dev->private); if (ret) - return ret; + goto out; + /* * We don't need to care about tearing down previously - * registered ITSes, as the kvm_io_bus framework removes - * them for us if the VM gets destroyed. + * registered ITSes on error, as the kvm_io_bus framework + * removes them for us if the VM gets destroyed. */ } +out: + mutex_unlock(&kvm->lock); return ret; }
Since we are about to synchronize all accesses to kvm->devices using the kvm->lock mutex, we should hold this mutex while iterating over the list of devices in the ITS code. Also move the vgic_register_its_iodev function to where it's called and rename it to register_its_iodev to avoid having two almost identially named functions. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- virt/kvm/arm/vgic/vgic-its.c | 64 +++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 30 deletions(-)