diff mbox

KVM: arm64: vgic-its: Grab kvm->lock when reading kvm->devices

Message ID 20160810103914.30322-1-christoffer.dall@linaro.org
State New
Headers show

Commit Message

Christoffer Dall Aug. 10, 2016, 10:39 a.m. UTC 
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(-)

-- 
2.9.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Christoffer Dall Aug. 10, 2016, 2:34 p.m. UTC | #1 
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

Patch

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;
 }