Re: [PATCH] KVM: arm64: vgic-its: Grab kvm->lock when reading kvm->devices

[Paolo Bonzini <pbonzini@redhat.com>]

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>