[PATCH v2] KVM: arm64: ITS: move ITS registration into first VCPU run

[PATCH v2] KVM: arm64: ITS: move ITS registration into first VCPU run

[Andre Przywara]

Currently we register an ITS device upon userland issuing the CTLR_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 map each
ITS that we find.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Changelog v1 .. v2:
- remove not yet upstreamed locking
- remove pointless de-registration on rollback

 virt/kvm/arm/vgic/vgic-its.c | 43 +++++++++++++++++++++++++++++++++----------
 virt/kvm/arm/vgic/vgic-v3.c  |  8 ++++++++
 virt/kvm/arm/vgic/vgic.h     |  6 ++++++
 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 07411cf..7180994 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,30 @@ 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;
+
+	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)
+			return ret;
+		/*
+		 * 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.
+		 */
+	}
+
+	return ret;
+}
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 0506543..9f0dae3 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -289,6 +289,14 @@ int vgic_v3_map_resources(struct kvm *kvm)
 		goto out;
 	}
 
+	if (vgic_has_its(kvm)) {
+		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

[PATCH v2] KVM: arm64: ITS: move ITS registration into first VCPU run

[Andre Przywara]

Currently we register an ITS device upon userland issuing the CTLR_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 map each
ITS that we find.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Changelog v1 .. v2:
- remove not yet upstreamed locking
- remove pointless de-registration on rollback

 virt/kvm/arm/vgic/vgic-its.c | 43 +++++++++++++++++++++++++++++++++----------
 virt/kvm/arm/vgic/vgic-v3.c  |  8 ++++++++
 virt/kvm/arm/vgic/vgic.h     |  6 ++++++
 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 07411cf..7180994 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,30 @@ 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;
+
+	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)
+			return ret;
+		/*
+		 * 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.
+		 */
+	}
+
+	return ret;
+}
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 0506543..9f0dae3 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -289,6 +289,14 @@ int vgic_v3_map_resources(struct kvm *kvm)
 		goto out;
 	}
 
+	if (vgic_has_its(kvm)) {
+		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

Re: [PATCH v2] KVM: arm64: ITS: move ITS registration into first VCPU run

[Auger Eric]

Hi Andre,
On 08/08/2016 17:45, Andre Przywara wrote:
> Currently we register an ITS device upon userland issuing the CTLR_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 map each
> ITS that we find.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Changelog v1 .. v2:
> - remove not yet upstreamed locking
> - remove pointless de-registration on rollback
> 
>  virt/kvm/arm/vgic/vgic-its.c | 43 +++++++++++++++++++++++++++++++++----------
>  virt/kvm/arm/vgic/vgic-v3.c  |  8 ++++++++
>  virt/kvm/arm/vgic/vgic.h     |  6 ++++++
>  3 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 07411cf..7180994 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,30 @@ 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;
> +
> +	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)
> +			return ret;
> +		/*
> +		 * 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.
> +		 */
> +	}
> +
> +	return ret;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 0506543..9f0dae3 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -289,6 +289,14 @@ int vgic_v3_map_resources(struct kvm *kvm)
>  		goto out;
>  	}
>  
> +	if (vgic_has_its(kvm)) {
> +		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;
> 

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Tested on Cavium with QEMU (with accordingly modified init sequence)

Cheers

Eric

[PATCH v2] KVM: arm64: ITS: move ITS registration into first VCPU run

[Auger Eric]

Hi Andre,
On 08/08/2016 17:45, Andre Przywara wrote:
> Currently we register an ITS device upon userland issuing the CTLR_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 map each
> ITS that we find.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Changelog v1 .. v2:
> - remove not yet upstreamed locking
> - remove pointless de-registration on rollback
> 
>  virt/kvm/arm/vgic/vgic-its.c | 43 +++++++++++++++++++++++++++++++++----------
>  virt/kvm/arm/vgic/vgic-v3.c  |  8 ++++++++
>  virt/kvm/arm/vgic/vgic.h     |  6 ++++++
>  3 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 07411cf..7180994 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,30 @@ 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;
> +
> +	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)
> +			return ret;
> +		/*
> +		 * 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.
> +		 */
> +	}
> +
> +	return ret;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 0506543..9f0dae3 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -289,6 +289,14 @@ int vgic_v3_map_resources(struct kvm *kvm)
>  		goto out;
>  	}
>  
> +	if (vgic_has_its(kvm)) {
> +		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;
> 

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Tested on Cavium with QEMU (with accordingly modified init sequence)

Cheers

Eric

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

[Christoffer Dall]

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

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

[Christoffer Dall]

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

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

[Paolo Bonzini]

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>

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

[Paolo Bonzini]

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>

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

[Christoffer Dall]

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

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

[Christoffer Dall]

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