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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
Thanks Paolo!
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
