[PATCH 0/8] Fixes to v7 of the vITS save/restore series

[PATCH 0/8] Fixes to v7 of the vITS save/restore series

[Christoffer Dall]

We spotted a number of isses on the v7 ITS save/restore patch series,
but because most of that patch series is in really good shape, and
because the work discussed is somewhat orthogonal (the register iodevs
in particular), I decided to send out a set of fixes to apply after the
main ITS save/restore series, and we can apply the v7 series plus these
fixes.

The whole series with the fixes applied on top of queue can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git its-save-restore-queue-fixes

Thanks,
-Christoffer

Christoffer Dall (8):
  KVM: arm/arm64: Clarification and relaxation to ITS save/restore ABI
  KVM: arm/arm64: vgic: Rename kvm_vgic_vcpu_init to
    kvm_vgic_vcpu_enable
  KVM: arm/arm64: Refactor vgic_register_redist_iodevs
  KVM: arm/arm64: Make vgic_v3_check_base more broadly usable
  KVM: arm/arm64: Slightly rework kvm_vgic_addr
  KVM: arm/arm64: Register iodevs when setting redist base and creating
    VCPUs
  KVM: arm/arm64: Register ITS iodev when setting base address
  KVM: arm/arm64: vgic-its: Cleanup after failed ITT restore

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |  19 +--
 include/kvm/arm_vgic.h                             |   1 +
 virt/kvm/arm/arm.c                                 |   6 +-
 virt/kvm/arm/vgic/vgic-init.c                      |  25 +++-
 virt/kvm/arm/vgic/vgic-its.c                       |  64 ++++-----
 virt/kvm/arm/vgic/vgic-kvm-device.c                |  29 ++--
 virt/kvm/arm/vgic/vgic-mmio-v3.c                   | 147 +++++++++++++++------
 virt/kvm/arm/vgic/vgic-v3.c                        |  32 ++---
 virt/kvm/arm/vgic/vgic.h                           |   5 +-
 9 files changed, 201 insertions(+), 127 deletions(-)

-- 
2.9.0

[PATCH 1/8] KVM: arm/arm64: Clarification and relaxation to ITS save/restore ABI

[Christoffer Dall]

Clarify what is meant by the save/restore ABI only supporting virtual
physical interrupts.

Relax the requirement of the order that the collection entries are
written in and be clear that there is no particular ordering enforced.

Some cosmetic changes in the capitalization of ID names to align with
the GICv3 manual and remove the empty line in the bottom of the patch.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 Documentation/virtual/kvm/devices/arm-vgic-its.txt | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index ba132e9..d405242 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -110,12 +110,14 @@ Then vcpus can be started.
  ITS Table ABI REV0:
  -------------------
 
- Revision 0 of the ABI only supports physical LPIs.
+ Revision 0 of the ABI only supports the features of a virtual GICv3, and does
+ not support a virtual GICv4 with support for direct injection of virtual
+ interrupts for nested hypervisors.
 
- The device table and ITT are indexed by the deviceid and eventid,
- respectively. The collection table is not indexed by collectionid:
- CTEs are written in the table in the order of collection creation. All
- entries are 8 bytes.
+ The device table and ITT are indexed by the DeviceID and EventID,
+ respectively. The collection table is not indexed by CollectionID, and the
+ entries in the collection are listed in no particular order.
+ All entries are 8 bytes.
 
  Device Table Entry (DTE):
 
@@ -126,10 +128,10 @@ Then vcpus can be started.
  - V indicates whether the entry is valid. If not, other fields
    are not meaningful.
  - next: equals to 0 if this entry is the last one; otherwise it
-   corresponds to the deviceid offset to the next DTE, capped by
+   corresponds to the DeviceID offset to the next DTE, capped by
    2^14 -1.
  - ITT_addr matches bits [51:8] of the ITT address (256 Byte aligned).
- - Size specifies the supported number of bits for the eventid,
+ - Size specifies the supported number of bits for the EventID,
    minus one
 
  Collection Table Entry (CTE):
@@ -151,8 +153,7 @@ Then vcpus can be started.
 
  where:
  - next: equals to 0 if this entry is the last one; otherwise it corresponds
-   to the eventid offset to the next ITE capped by 2^16 -1.
+   to the EventID offset to the next ITE capped by 2^16 -1.
  - pINTID is the physical LPI ID; if zero, it means the entry is not valid
    and other fields are not meaningful.
  - ICID is the collection ID
-
-- 
2.9.0

[PATCH 1/8] KVM: arm/arm64: Clarification and relaxation to ITS save/restore ABI

[Auger Eric]

Hi Christoffer,

On 08/05/2017 13:54, Christoffer Dall wrote:
> Clarify what is meant by the save/restore ABI only supporting virtual
> physical interrupts.
> 
> Relax the requirement of the order that the collection entries are
> written in and be clear that there is no particular ordering enforced.
> 
> Some cosmetic changes in the capitalization of ID names to align with
> the GICv3 manual and remove the empty line in the bottom of the patch.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index ba132e9..d405242 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -110,12 +110,14 @@ Then vcpus can be started.
>   ITS Table ABI REV0:
>   -------------------
>  
> - Revision 0 of the ABI only supports physical LPIs.
> + Revision 0 of the ABI only supports the features of a virtual GICv3, and does
> + not support a virtual GICv4 with support for direct injection of virtual
> + interrupts for nested hypervisors.
>  
> - The device table and ITT are indexed by the deviceid and eventid,
> - respectively. The collection table is not indexed by collectionid:
> - CTEs are written in the table in the order of collection creation. All
> - entries are 8 bytes.
> + The device table and ITT are indexed by the DeviceID and EventID,
> + respectively. The collection table is not indexed by CollectionID, and the
> + entries in the collection are listed in no particular order.
> + All entries are 8 bytes.
>  
>   Device Table Entry (DTE):
>  
> @@ -126,10 +128,10 @@ Then vcpus can be started.
>   - V indicates whether the entry is valid. If not, other fields
>     are not meaningful.
>   - next: equals to 0 if this entry is the last one; otherwise it
> -   corresponds to the deviceid offset to the next DTE, capped by
> +   corresponds to the DeviceID offset to the next DTE, capped by
>     2^14 -1.
>   - ITT_addr matches bits [51:8] of the ITT address (256 Byte aligned).
> - - Size specifies the supported number of bits for the eventid,
> + - Size specifies the supported number of bits for the EventID,
>     minus one
>  
>   Collection Table Entry (CTE):
> @@ -151,8 +153,7 @@ Then vcpus can be started.
>  
>   where:
>   - next: equals to 0 if this entry is the last one; otherwise it corresponds
> -   to the eventid offset to the next ITE capped by 2^16 -1.
> +   to the EventID offset to the next ITE capped by 2^16 -1.
>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>     and other fields are not meaningful.
>   - ICID is the collection ID
> -
> 

[PATCH 2/8] KVM: arm/arm64: vgic: Rename kvm_vgic_vcpu_init to kvm_vgic_vcpu_enable

[Christoffer Dall]

This function really doesn't init anything, it enables the CPU
interface, so name it as such, which gives us the name to use for actual
init work later on.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-init.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 87de048..0ea64a1 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -226,11 +226,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
 	return 0;
 }
 
-/**
- * kvm_vgic_vcpu_init() - Enable the VCPU interface
- * @vcpu: the VCPU which's VGIC should be enabled
- */
-static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
+static void kvm_vgic_vcpu_enable(struct kvm_vcpu *vcpu)
 {
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_enable(vcpu);
@@ -269,7 +265,7 @@ int vgic_init(struct kvm *kvm)
 		dist->msis_require_devid = true;
 
 	kvm_for_each_vcpu(i, vcpu, kvm)
-		kvm_vgic_vcpu_init(vcpu);
+		kvm_vgic_vcpu_enable(vcpu);
 
 	ret = kvm_vgic_setup_default_irq_routing(kvm);
 	if (ret)
-- 
2.9.0

[PATCH 2/8] KVM: arm/arm64: vgic: Rename kvm_vgic_vcpu_init to kvm_vgic_vcpu_enable

[Auger Eric]

Hi,

On 08/05/2017 13:54, Christoffer Dall wrote:
> This function really doesn't init anything, it enables the CPU
> interface, so name it as such, which gives us the name to use for actual
> init work later on.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  virt/kvm/arm/vgic/vgic-init.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 87de048..0ea64a1 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -226,11 +226,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
>  	return 0;
>  }
>  
> -/**
> - * kvm_vgic_vcpu_init() - Enable the VCPU interface
> - * @vcpu: the VCPU which's VGIC should be enabled
> - */
> -static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> +static void kvm_vgic_vcpu_enable(struct kvm_vcpu *vcpu)
>  {
>  	if (kvm_vgic_global_state.type == VGIC_V2)
>  		vgic_v2_enable(vcpu);
> @@ -269,7 +265,7 @@ int vgic_init(struct kvm *kvm)
>  		dist->msis_require_devid = true;
>  
>  	kvm_for_each_vcpu(i, vcpu, kvm)
> -		kvm_vgic_vcpu_init(vcpu);
> +		kvm_vgic_vcpu_enable(vcpu);
>  
>  	ret = kvm_vgic_setup_default_irq_routing(kvm);
>  	if (ret)
> 

[PATCH 3/8] KVM: arm/arm64: Refactor vgic_register_redist_iodevs

[Christoffer Dall]

Split out the function to register all the redistributor iodevs into a
function that handles a single redistributor at a time in preparation
for being able to call this per VCPU as these get created.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 108 ++++++++++++++++++++++++---------------
 virt/kvm/arm/vgic/vgic-v3.c      |   2 +-
 virt/kvm/arm/vgic/vgic.h         |   2 +-
 3 files changed, 68 insertions(+), 44 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 6afb3b4..1828ac7 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -556,61 +556,85 @@ unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev)
 	return SZ_64K;
 }
 
-int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
+/**
+ * vgic_register_redist_iodev - register a single redist iodev
+ * @vcpu:    The VCPU to which the redistributor belongs
+ *
+ * Register a KVM iodev for this VCPU's redistributor using the address
+ * provided.
+ *
+ * Return 0 on success, -ERRNO otherwise.
+ */
+static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+	struct vgic_dist *vgic = &kvm->arch.vgic;
+	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
+	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
+	gpa_t rd_base, sgi_base;
+	int ret;
+
+	rd_base = vgic->vgic_redist_base + vcpu->vcpu_id * SZ_64K * 2;
+	sgi_base = rd_base + SZ_64K;
+
+	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
+	rd_dev->base_addr = rd_base;
+	rd_dev->iodev_type = IODEV_REDIST;
+	rd_dev->regions = vgic_v3_rdbase_registers;
+	rd_dev->nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
+	rd_dev->redist_vcpu = vcpu;
+
+	mutex_lock(&kvm->slots_lock);
+	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, rd_base,
+				      SZ_64K, &rd_dev->dev);
+	mutex_unlock(&kvm->slots_lock);
+
+	if (ret)
+		return ret;
+
+	kvm_iodevice_init(&sgi_dev->dev, &kvm_io_gic_ops);
+	sgi_dev->base_addr = sgi_base;
+	sgi_dev->iodev_type = IODEV_REDIST;
+	sgi_dev->regions = vgic_v3_sgibase_registers;
+	sgi_dev->nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers);
+	sgi_dev->redist_vcpu = vcpu;
+
+	mutex_lock(&kvm->slots_lock);
+	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
+				      SZ_64K, &sgi_dev->dev);
+	mutex_unlock(&kvm->slots_lock);
+	if (ret)
+		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
+					  &rd_dev->dev);
+
+	return ret;
+}
+
+static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
+{
+	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
+	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
+
+	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &rd_dev->dev);
+	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &sgi_dev->dev);
+}
+
+int vgic_register_redist_iodevs(struct kvm *kvm)
 {
 	struct kvm_vcpu *vcpu;
 	int c, ret = 0;
 
 	kvm_for_each_vcpu(c, vcpu, kvm) {
-		gpa_t rd_base = redist_base_address + c * SZ_64K * 2;
-		gpa_t sgi_base = rd_base + SZ_64K;
-		struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
-		struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
-
-		kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
-		rd_dev->base_addr = rd_base;
-		rd_dev->iodev_type = IODEV_REDIST;
-		rd_dev->regions = vgic_v3_rdbase_registers;
-		rd_dev->nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
-		rd_dev->redist_vcpu = vcpu;
-
-		mutex_lock(&kvm->slots_lock);
-		ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, rd_base,
-					      SZ_64K, &rd_dev->dev);
-		mutex_unlock(&kvm->slots_lock);
-
+		ret = vgic_register_redist_iodev(vcpu);
 		if (ret)
 			break;
-
-		kvm_iodevice_init(&sgi_dev->dev, &kvm_io_gic_ops);
-		sgi_dev->base_addr = sgi_base;
-		sgi_dev->iodev_type = IODEV_REDIST;
-		sgi_dev->regions = vgic_v3_sgibase_registers;
-		sgi_dev->nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers);
-		sgi_dev->redist_vcpu = vcpu;
-
-		mutex_lock(&kvm->slots_lock);
-		ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
-					      SZ_64K, &sgi_dev->dev);
-		mutex_unlock(&kvm->slots_lock);
-		if (ret) {
-			kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
-						  &rd_dev->dev);
-			break;
-		}
 	}
 
 	if (ret) {
 		/* The current c failed, so we start with the previous one. */
 		for (c--; c >= 0; c--) {
-			struct vgic_cpu *vgic_cpu;
-
 			vcpu = kvm_get_vcpu(kvm, c);
-			vgic_cpu = &vcpu->arch.vgic_cpu;
-			kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
-						  &vgic_cpu->rd_iodev.dev);
-			kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
-						  &vgic_cpu->sgi_iodev.dev);
+			vgic_unregister_redist_iodev(vcpu);
 		}
 	}
 
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 54dee72..12e52a0 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -386,7 +386,7 @@ int vgic_v3_map_resources(struct kvm *kvm)
 		goto out;
 	}
 
-	ret = vgic_register_redist_iodevs(kvm, dist->vgic_redist_base);
+	ret = vgic_register_redist_iodevs(kvm);
 	if (ret) {
 		kvm_err("Unable to register VGICv3 redist MMIO regions\n");
 		goto out;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 8259f0a..a2aeaa8 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -174,7 +174,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info);
 int vgic_v3_map_resources(struct kvm *kvm);
 int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
 int vgic_v3_save_pending_tables(struct kvm *kvm);
-int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address);
+int vgic_register_redist_iodevs(struct kvm *kvm);
 
 void vgic_v3_load(struct kvm_vcpu *vcpu);
 void vgic_v3_put(struct kvm_vcpu *vcpu);
-- 
2.9.0

[PATCH 3/8] KVM: arm/arm64: Refactor vgic_register_redist_iodevs

[Auger Eric]

Hi Christoffer,

On 08/05/2017 13:54, Christoffer Dall wrote:
> Split out the function to register all the redistributor iodevs into a
> function that handles a single redistributor at a time in preparation
> for being able to call this per VCPU as these get created.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 108 ++++++++++++++++++++++++---------------
>  virt/kvm/arm/vgic/vgic-v3.c      |   2 +-
>  virt/kvm/arm/vgic/vgic.h         |   2 +-
>  3 files changed, 68 insertions(+), 44 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 6afb3b4..1828ac7 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -556,61 +556,85 @@ unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev)
>  	return SZ_64K;
>  }
>  
> -int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
> +/**
> + * vgic_register_redist_iodev - register a single redist iodev
> + * @vcpu:    The VCPU to which the redistributor belongs
> + *
> + * Register a KVM iodev for this VCPU's redistributor using the address
> + * provided.
> + *
> + * Return 0 on success, -ERRNO otherwise.
> + */
> +static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	struct vgic_dist *vgic = &kvm->arch.vgic;
> +	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
> +	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
> +	gpa_t rd_base, sgi_base;
> +	int ret;
> +
> +	rd_base = vgic->vgic_redist_base + vcpu->vcpu_id * SZ_64K * 2;
Previously we had
gpa_t rd_base = redist_base_address + c * SZ_64K * 2; where c is the
index in the vcpu array. Now we use the vpcu_id instead of c. Aren't
they potentially different?

Thanks

Eric
> +	sgi_base = rd_base + SZ_64K;
> +
> +	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
> +	rd_dev->base_addr = rd_base;
> +	rd_dev->iodev_type = IODEV_REDIST;
> +	rd_dev->regions = vgic_v3_rdbase_registers;
> +	rd_dev->nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
> +	rd_dev->redist_vcpu = vcpu;
> +
> +	mutex_lock(&kvm->slots_lock);
> +	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, rd_base,
> +				      SZ_64K, &rd_dev->dev);
> +	mutex_unlock(&kvm->slots_lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	kvm_iodevice_init(&sgi_dev->dev, &kvm_io_gic_ops);
> +	sgi_dev->base_addr = sgi_base;
> +	sgi_dev->iodev_type = IODEV_REDIST;
> +	sgi_dev->regions = vgic_v3_sgibase_registers;
> +	sgi_dev->nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers);
> +	sgi_dev->redist_vcpu = vcpu;
> +
> +	mutex_lock(&kvm->slots_lock);
> +	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
> +				      SZ_64K, &sgi_dev->dev);
> +	mutex_unlock(&kvm->slots_lock);
> +	if (ret)
> +		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> +					  &rd_dev->dev);
> +
> +	return ret;
> +}
> +
> +static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
> +{
> +	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
> +	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
> +
> +	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &rd_dev->dev);
> +	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &sgi_dev->dev);
> +}
> +
> +int vgic_register_redist_iodevs(struct kvm *kvm)
>  {
>  	struct kvm_vcpu *vcpu;
>  	int c, ret = 0;
>  
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
> -		gpa_t rd_base = redist_base_address + c * SZ_64K * 2;
> -		gpa_t sgi_base = rd_base + SZ_64K;
> -		struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
> -		struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
> -
> -		kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
> -		rd_dev->base_addr = rd_base;
> -		rd_dev->iodev_type = IODEV_REDIST;
> -		rd_dev->regions = vgic_v3_rdbase_registers;
> -		rd_dev->nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
> -		rd_dev->redist_vcpu = vcpu;
> -
> -		mutex_lock(&kvm->slots_lock);
> -		ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, rd_base,
> -					      SZ_64K, &rd_dev->dev);
> -		mutex_unlock(&kvm->slots_lock);
> -
> +		ret = vgic_register_redist_iodev(vcpu);
>  		if (ret)
>  			break;
> -
> -		kvm_iodevice_init(&sgi_dev->dev, &kvm_io_gic_ops);
> -		sgi_dev->base_addr = sgi_base;
> -		sgi_dev->iodev_type = IODEV_REDIST;
> -		sgi_dev->regions = vgic_v3_sgibase_registers;
> -		sgi_dev->nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers);
> -		sgi_dev->redist_vcpu = vcpu;
> -
> -		mutex_lock(&kvm->slots_lock);
> -		ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
> -					      SZ_64K, &sgi_dev->dev);
> -		mutex_unlock(&kvm->slots_lock);
> -		if (ret) {
> -			kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> -						  &rd_dev->dev);
> -			break;
> -		}
>  	}
>  
>  	if (ret) {
>  		/* The current c failed, so we start with the previous one. */
>  		for (c--; c >= 0; c--) {
> -			struct vgic_cpu *vgic_cpu;
> -
>  			vcpu = kvm_get_vcpu(kvm, c);
> -			vgic_cpu = &vcpu->arch.vgic_cpu;
> -			kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> -						  &vgic_cpu->rd_iodev.dev);
> -			kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> -						  &vgic_cpu->sgi_iodev.dev);
> +			vgic_unregister_redist_iodev(vcpu);
>  		}
>  	}
>  
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 54dee72..12e52a0 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -386,7 +386,7 @@ int vgic_v3_map_resources(struct kvm *kvm)
>  		goto out;
>  	}
>  
> -	ret = vgic_register_redist_iodevs(kvm, dist->vgic_redist_base);
> +	ret = vgic_register_redist_iodevs(kvm);
>  	if (ret) {
>  		kvm_err("Unable to register VGICv3 redist MMIO regions\n");
>  		goto out;
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 8259f0a..a2aeaa8 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -174,7 +174,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info);
>  int vgic_v3_map_resources(struct kvm *kvm);
>  int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
>  int vgic_v3_save_pending_tables(struct kvm *kvm);
> -int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address);
> +int vgic_register_redist_iodevs(struct kvm *kvm);
>  
>  void vgic_v3_load(struct kvm_vcpu *vcpu);
>  void vgic_v3_put(struct kvm_vcpu *vcpu);
> 

[PATCH 3/8] KVM: arm/arm64: Refactor vgic_register_redist_iodevs

[Christoffer Dall]

On Mon, May 08, 2017 at 06:03:14PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 08/05/2017 13:54, Christoffer Dall wrote:
> > Split out the function to register all the redistributor iodevs into a
> > function that handles a single redistributor at a time in preparation
> > for being able to call this per VCPU as these get created.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic/vgic-mmio-v3.c | 108 ++++++++++++++++++++++++---------------
> >  virt/kvm/arm/vgic/vgic-v3.c      |   2 +-
> >  virt/kvm/arm/vgic/vgic.h         |   2 +-
> >  3 files changed, 68 insertions(+), 44 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 6afb3b4..1828ac7 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -556,61 +556,85 @@ unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev)
> >  	return SZ_64K;
> >  }
> >  
> > -int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
> > +/**
> > + * vgic_register_redist_iodev - register a single redist iodev
> > + * @vcpu:    The VCPU to which the redistributor belongs
> > + *
> > + * Register a KVM iodev for this VCPU's redistributor using the address
> > + * provided.
> > + *
> > + * Return 0 on success, -ERRNO otherwise.
> > + */
> > +static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm *kvm = vcpu->kvm;
> > +	struct vgic_dist *vgic = &kvm->arch.vgic;
> > +	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
> > +	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
> > +	gpa_t rd_base, sgi_base;
> > +	int ret;
> > +
> > +	rd_base = vgic->vgic_redist_base + vcpu->vcpu_id * SZ_64K * 2;
> Previously we had
> gpa_t rd_base = redist_base_address + c * SZ_64K * 2; where c is the
> index in the vcpu array. Now we use the vpcu_id instead of c. Aren't
> they potentially different?
> 

Nicely spotted!

They might be, theoretically.  I never realized this, but we have occurences
of this assumption already, see kvm_pmu_update_state(), for example.
Even worse, the ITS PE numbering is based on the vcpu_id, and not the
index of the vcpu id (see vgic_mmio_read_v3r_typer).

So there are two issues, one is that we should change most occurences of
kvm_get_vcpu() to kvm_get_vcpu_by_id() in our code (I'll write a patch
for this).

The second issue is that vcpu_id is not enforced to be contiguous so we
may end up with a sparse redist frame, which obviously doesn't work, so
I think I'll need to add the following:


diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2c14ad9..12eb26d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -490,6 +490,17 @@ static inline struct kvm_vcpu *kvm_get_vcpu_by_id(struct kvm *kvm, int id)
 	return NULL;
 }
 
+static inline int kvm_vcpu_get_idx(struct kvm_vcpu *vcpu)
+{
+	struct kvm_vcpu *tmp;
+	int idx;
+
+	kvm_for_each_vcpu(idx, tmp, vcpu->kvm)
+		if (tmp == vcpu)
+			return idx;
+	BUG();
+}
+
 #define kvm_for_each_memslot(memslot, slots)	\
 	for (memslot = &slots->memslots[0];	\
 	      memslot < slots->memslots + KVM_MEM_SLOTS_NUM && memslot->npages;\
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 297557b..99da1a2 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -586,7 +586,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	if (!vgic_v3_check_base(kvm))
 		return -EINVAL;
 
-	rd_base = vgic->vgic_redist_base + vcpu->vcpu_id * SZ_64K * 2;
+	rd_base = vgic->vgic_redist_base + kvm_vcpu_get_idx(vcpu) * SZ_64K * 2;
 	sgi_base = rd_base + SZ_64K;
 
 	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);

Thanks,
-Christoffer

[PATCH 4/8] KVM: arm/arm64: Make vgic_v3_check_base more broadly usable

[Christoffer Dall]

As we are about to fiddle with the io device registration mechanism
let's be a little more careful in verifying the addresses we can ealier
on to provide error messages to the user at time related to him/her
setting overlapping addresses.  We still want to check a consistent
system before actually running the VM for the first time, so we make
vgic_v3_check_base available in the core vgic-v3 code as well as in the
other parts of the GICv3 code, namely the MMIO config code.

We also return true for undefined base addresses so that the function
can be used before all base addresses are set; all callers already check
for uninitialized addresses before calling this function.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-v3.c | 18 ++++++++++++++----
 virt/kvm/arm/vgic/vgic.h    |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 12e52a0..b934e78 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -329,19 +329,29 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 	return 0;
 }
 
-/* check for overlapping regions and for regions crossing the end of memory */
-static bool vgic_v3_check_base(struct kvm *kvm)
+/*
+ * Check for overlapping regions and for regions crossing the end of memory
+ * for base addresses which have already been set.
+ */
+bool vgic_v3_check_base(struct kvm *kvm)
 {
 	struct vgic_dist *d = &kvm->arch.vgic;
 	gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
 
 	redist_size *= atomic_read(&kvm->online_vcpus);
 
-	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
+	if (!IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
+	    d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
 		return false;
-	if (d->vgic_redist_base + redist_size < d->vgic_redist_base)
+
+	if (!IS_VGIC_ADDR_UNDEF(d->vgic_redist_base) &&
+	    d->vgic_redist_base + redist_size < d->vgic_redist_base)
 		return false;
 
+	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
+	    IS_VGIC_ADDR_UNDEF(d->vgic_redist_base))
+		return true;
+
 	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= d->vgic_redist_base)
 		return true;
 	if (d->vgic_redist_base + redist_size <= d->vgic_dist_base)
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index a2aeaa8..89eb935 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -175,6 +175,7 @@ int vgic_v3_map_resources(struct kvm *kvm);
 int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
 int vgic_v3_save_pending_tables(struct kvm *kvm);
 int vgic_register_redist_iodevs(struct kvm *kvm);
+bool vgic_v3_check_base(struct kvm *kvm);
 
 void vgic_v3_load(struct kvm_vcpu *vcpu);
 void vgic_v3_put(struct kvm_vcpu *vcpu);
-- 
2.9.0

[PATCH 4/8] KVM: arm/arm64: Make vgic_v3_check_base more broadly usable

[Auger Eric]

Hi Christoffer,

On 08/05/2017 13:54, Christoffer Dall wrote:
> As we are about to fiddle with the io device registration mechanism
> let's be a little more careful in verifying the addresses we can ealier
> on to provide error messages to the user at time related to him/her
> setting overlapping addresses. 
Above sentence would need some rewording.
 We still want to check a consistent
> system before actually running the VM for the first time, so we make
> vgic_v3_check_base available in the core vgic-v3 code as well as in the
> other parts of the GICv3 code, namely the MMIO config code.
> 
> We also return true for undefined base addresses so that the function
> can be used before all base addresses are set; all callers already check
> for uninitialized addresses before calling this function.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 18 ++++++++++++++----
>  virt/kvm/arm/vgic/vgic.h    |  1 +
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 12e52a0..b934e78 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -329,19 +329,29 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  	return 0;
>  }
>  
> -/* check for overlapping regions and for regions crossing the end of memory */
> -static bool vgic_v3_check_base(struct kvm *kvm)
> +/*
> + * Check for overlapping regions and for regions crossing the end of memory
> + * for base addresses which have already been set.
> + */
> +bool vgic_v3_check_base(struct kvm *kvm)
>  {
>  	struct vgic_dist *d = &kvm->arch.vgic;
>  	gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
>  
>  	redist_size *= atomic_read(&kvm->online_vcpus);
>  
> -	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
> +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
> +	    d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
>  		return false;
> -	if (d->vgic_redist_base + redist_size < d->vgic_redist_base)
> +
> +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_redist_base) &&
> +	    d->vgic_redist_base + redist_size < d->vgic_redist_base)
>  		return false;
>  
> +	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
> +	    IS_VGIC_ADDR_UNDEF(d->vgic_redist_base))
> +		return true;
> +
>  	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= d->vgic_redist_base)
>  		return true;
It is unclear to me if the dunction can be called if either of the
address is unset?

Thanks

Eric
>  	if (d->vgic_redist_base + redist_size <= d->vgic_dist_base)
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index a2aeaa8..89eb935 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -175,6 +175,7 @@ int vgic_v3_map_resources(struct kvm *kvm);
>  int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
>  int vgic_v3_save_pending_tables(struct kvm *kvm);
>  int vgic_register_redist_iodevs(struct kvm *kvm);
> +bool vgic_v3_check_base(struct kvm *kvm);
>  
>  void vgic_v3_load(struct kvm_vcpu *vcpu);
>  void vgic_v3_put(struct kvm_vcpu *vcpu);
> 

[PATCH 4/8] KVM: arm/arm64: Make vgic_v3_check_base more broadly usable

[Christoffer Dall]

On Mon, May 08, 2017 at 06:13:01PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 08/05/2017 13:54, Christoffer Dall wrote:
> > As we are about to fiddle with the io device registration mechanism
> > let's be a little more careful in verifying the addresses we can ealier
> > on to provide error messages to the user at time related to him/her
> > setting overlapping addresses. 
> Above sentence would need some rewording.
>  We still want to check a consistent

indeed :)

> > system before actually running the VM for the first time, so we make
> > vgic_v3_check_base available in the core vgic-v3 code as well as in the
> > other parts of the GICv3 code, namely the MMIO config code.
> > 
> > We also return true for undefined base addresses so that the function
> > can be used before all base addresses are set; all callers already check
> > for uninitialized addresses before calling this function.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic/vgic-v3.c | 18 ++++++++++++++----
> >  virt/kvm/arm/vgic/vgic.h    |  1 +
> >  2 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> > index 12e52a0..b934e78 100644
> > --- a/virt/kvm/arm/vgic/vgic-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-v3.c
> > @@ -329,19 +329,29 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
> >  	return 0;
> >  }
> >  
> > -/* check for overlapping regions and for regions crossing the end of memory */
> > -static bool vgic_v3_check_base(struct kvm *kvm)
> > +/*
> > + * Check for overlapping regions and for regions crossing the end of memory
> > + * for base addresses which have already been set.
> > + */
> > +bool vgic_v3_check_base(struct kvm *kvm)
> >  {
> >  	struct vgic_dist *d = &kvm->arch.vgic;
> >  	gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
> >  
> >  	redist_size *= atomic_read(&kvm->online_vcpus);
> >  
> > -	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
> > +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
> > +	    d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
> >  		return false;
> > -	if (d->vgic_redist_base + redist_size < d->vgic_redist_base)
> > +
> > +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_redist_base) &&
> > +	    d->vgic_redist_base + redist_size < d->vgic_redist_base)
> >  		return false;
> >  
> > +	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
> > +	    IS_VGIC_ADDR_UNDEF(d->vgic_redist_base))
> > +		return true;
> > +
> >  	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= d->vgic_redist_base)
> >  		return true;
> It is unclear to me if the dunction can be called if either of the
> address is unset?

Yes, it can be called if both addreses are unset, in which case you'll
get a positive result.  If a single address is set, we cannot check
interaction between the two addresses, but we can check the requirements
for the single address, and the interaction must be checked later.

Thanks,
-Christoffer

> >  	if (d->vgic_redist_base + redist_size <= d->vgic_dist_base)
> > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> > index a2aeaa8..89eb935 100644
> > --- a/virt/kvm/arm/vgic/vgic.h
> > +++ b/virt/kvm/arm/vgic/vgic.h
> > @@ -175,6 +175,7 @@ int vgic_v3_map_resources(struct kvm *kvm);
> >  int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
> >  int vgic_v3_save_pending_tables(struct kvm *kvm);
> >  int vgic_register_redist_iodevs(struct kvm *kvm);
> > +bool vgic_v3_check_base(struct kvm *kvm);
> >  
> >  void vgic_v3_load(struct kvm_vcpu *vcpu);
> >  void vgic_v3_put(struct kvm_vcpu *vcpu);
> > 

[PATCH 4/8] KVM: arm/arm64: Make vgic_v3_check_base more broadly usable

[Auger Eric]

Hi,

On 08/05/2017 19:18, Christoffer Dall wrote:
> On Mon, May 08, 2017 at 06:13:01PM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 08/05/2017 13:54, Christoffer Dall wrote:
>>> As we are about to fiddle with the io device registration mechanism
>>> let's be a little more careful in verifying the addresses we can ealier
>>> on to provide error messages to the user at time related to him/her
>>> setting overlapping addresses. 
>> Above sentence would need some rewording.
>>  We still want to check a consistent
> 
> indeed :)
> 
>>> system before actually running the VM for the first time, so we make
>>> vgic_v3_check_base available in the core vgic-v3 code as well as in the
>>> other parts of the GICv3 code, namely the MMIO config code.
>>>
>>> We also return true for undefined base addresses so that the function
>>> can be used before all base addresses are set; all callers already check
>>> for uninitialized addresses before calling this function.
>>>
>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-v3.c | 18 ++++++++++++++----
>>>  virt/kvm/arm/vgic/vgic.h    |  1 +
>>>  2 files changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>>> index 12e52a0..b934e78 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -329,19 +329,29 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>>>  	return 0;
>>>  }
>>>  
>>> -/* check for overlapping regions and for regions crossing the end of memory */
>>> -static bool vgic_v3_check_base(struct kvm *kvm)
>>> +/*
>>> + * Check for overlapping regions and for regions crossing the end of memory
>>> + * for base addresses which have already been set.
>>> + */
>>> +bool vgic_v3_check_base(struct kvm *kvm)
>>>  {
>>>  	struct vgic_dist *d = &kvm->arch.vgic;
>>>  	gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
>>>  
>>>  	redist_size *= atomic_read(&kvm->online_vcpus);
>>>  
>>> -	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
>>> +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
>>> +	    d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
>>>  		return false;
>>> -	if (d->vgic_redist_base + redist_size < d->vgic_redist_base)
>>> +
>>> +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_redist_base) &&
>>> +	    d->vgic_redist_base + redist_size < d->vgic_redist_base)
>>>  		return false;
>>>  
>>> +	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
>>> +	    IS_VGIC_ADDR_UNDEF(d->vgic_redist_base))
>>> +		return true;
>>> +
>>>  	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= d->vgic_redist_base)
>>>  		return true;
>> It is unclear to me if the dunction can be called if either of the
>> address is unset?
> 
> Yes, it can be called if both addreses are unset, in which case you'll
> get a positive result.  If a single address is set, we cannot check
> interaction between the two addresses, but we can check the requirements
> for the single address, and the interaction must be checked later.
Although unlikely can't you have the redist_base set at 0x0 and
dist_base unset. Wouldn't this return false?

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
>>>  	if (d->vgic_redist_base + redist_size <= d->vgic_dist_base)
>>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>>> index a2aeaa8..89eb935 100644
>>> --- a/virt/kvm/arm/vgic/vgic.h
>>> +++ b/virt/kvm/arm/vgic/vgic.h
>>> @@ -175,6 +175,7 @@ int vgic_v3_map_resources(struct kvm *kvm);
>>>  int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
>>>  int vgic_v3_save_pending_tables(struct kvm *kvm);
>>>  int vgic_register_redist_iodevs(struct kvm *kvm);
>>> +bool vgic_v3_check_base(struct kvm *kvm);
>>>  
>>>  void vgic_v3_load(struct kvm_vcpu *vcpu);
>>>  void vgic_v3_put(struct kvm_vcpu *vcpu);
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 4/8] KVM: arm/arm64: Make vgic_v3_check_base more broadly usable

[Christoffer Dall]

On Mon, May 08, 2017 at 07:39:24PM +0200, Auger Eric wrote:
> Hi,
> 
> On 08/05/2017 19:18, Christoffer Dall wrote:
> > On Mon, May 08, 2017 at 06:13:01PM +0200, Auger Eric wrote:
> >> Hi Christoffer,
> >>
> >> On 08/05/2017 13:54, Christoffer Dall wrote:
> >>> As we are about to fiddle with the io device registration mechanism
> >>> let's be a little more careful in verifying the addresses we can ealier
> >>> on to provide error messages to the user at time related to him/her
> >>> setting overlapping addresses. 
> >> Above sentence would need some rewording.
> >>  We still want to check a consistent
> > 
> > indeed :)
> > 
> >>> system before actually running the VM for the first time, so we make
> >>> vgic_v3_check_base available in the core vgic-v3 code as well as in the
> >>> other parts of the GICv3 code, namely the MMIO config code.
> >>>
> >>> We also return true for undefined base addresses so that the function
> >>> can be used before all base addresses are set; all callers already check
> >>> for uninitialized addresses before calling this function.
> >>>
> >>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> >>> ---
> >>>  virt/kvm/arm/vgic/vgic-v3.c | 18 ++++++++++++++----
> >>>  virt/kvm/arm/vgic/vgic.h    |  1 +
> >>>  2 files changed, 15 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> >>> index 12e52a0..b934e78 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-v3.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> >>> @@ -329,19 +329,29 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> -/* check for overlapping regions and for regions crossing the end of memory */
> >>> -static bool vgic_v3_check_base(struct kvm *kvm)
> >>> +/*
> >>> + * Check for overlapping regions and for regions crossing the end of memory
> >>> + * for base addresses which have already been set.
> >>> + */
> >>> +bool vgic_v3_check_base(struct kvm *kvm)
> >>>  {
> >>>  	struct vgic_dist *d = &kvm->arch.vgic;
> >>>  	gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
> >>>  
> >>>  	redist_size *= atomic_read(&kvm->online_vcpus);
> >>>  
> >>> -	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
> >>> +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
> >>> +	    d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
> >>>  		return false;
> >>> -	if (d->vgic_redist_base + redist_size < d->vgic_redist_base)
> >>> +
> >>> +	if (!IS_VGIC_ADDR_UNDEF(d->vgic_redist_base) &&
> >>> +	    d->vgic_redist_base + redist_size < d->vgic_redist_base)
> >>>  		return false;
> >>>  
> >>> +	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
> >>> +	    IS_VGIC_ADDR_UNDEF(d->vgic_redist_base))
> >>> +		return true;
> >>> +
> >>>  	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= d->vgic_redist_base)
> >>>  		return true;
> >> It is unclear to me if the dunction can be called if either of the
> >> address is unset?
> > 
> > Yes, it can be called if both addreses are unset, in which case you'll
> > get a positive result.  If a single address is set, we cannot check
> > interaction between the two addresses, but we can check the requirements
> > for the single address, and the interaction must be checked later.
> Although unlikely can't you have the redist_base set at 0x0 and
> dist_base unset. Wouldn't this return false?

In the case od fist_base == VGIC_ADDR_UNDEF and rd_base == 0, we'll get:

Ah, duh, my && should be a ||.  I'll fix this.

Thanks,
-Christoffer

[PATCH 5/8] KVM: arm/arm64: Slightly rework kvm_vgic_addr

[Christoffer Dall]

As we are about to handle setting the address for the redistributor base
region separately from some of the other base addresses, let's rework
this function to leave a little more room for being flexible in what
each type of base address does.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-kvm-device.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index d48743c..69ccfd5 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -37,6 +37,14 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
 	return 0;
 }
 
+static int vgic_check_type(struct kvm *kvm, int type_needed)
+{
+	if (kvm->arch.vgic.vgic_model != type_needed)
+		return -ENODEV;
+	else
+		return 0;
+}
+
 /**
  * kvm_vgic_addr - set or get vgic VM base addresses
  * @kvm:   pointer to the vm struct
@@ -57,40 +65,36 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 {
 	int r = 0;
 	struct vgic_dist *vgic = &kvm->arch.vgic;
-	int type_needed;
 	phys_addr_t *addr_ptr, alignment;
 
 	mutex_lock(&kvm->lock);
 	switch (type) {
 	case KVM_VGIC_V2_ADDR_TYPE_DIST:
-		type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
+		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
 		addr_ptr = &vgic->vgic_dist_base;
 		alignment = SZ_4K;
 		break;
 	case KVM_VGIC_V2_ADDR_TYPE_CPU:
-		type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
+		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
 		addr_ptr = &vgic->vgic_cpu_base;
 		alignment = SZ_4K;
 		break;
 	case KVM_VGIC_V3_ADDR_TYPE_DIST:
-		type_needed = KVM_DEV_TYPE_ARM_VGIC_V3;
+		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
 		addr_ptr = &vgic->vgic_dist_base;
 		alignment = SZ_64K;
 		break;
 	case KVM_VGIC_V3_ADDR_TYPE_REDIST:
-		type_needed = KVM_DEV_TYPE_ARM_VGIC_V3;
+		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
 		addr_ptr = &vgic->vgic_redist_base;
 		alignment = SZ_64K;
 		break;
 	default:
 		r = -ENODEV;
-		goto out;
 	}
 
-	if (vgic->vgic_model != type_needed) {
-		r = -ENODEV;
+	if (r)
 		goto out;
-	}
 
 	if (write) {
 		r = vgic_check_ioaddr(kvm, addr_ptr, *addr, alignment);
-- 
2.9.0

[PATCH 5/8] KVM: arm/arm64: Slightly rework kvm_vgic_addr

[Auger Eric]

Hi Christoffer,

On 08/05/2017 13:54, Christoffer Dall wrote:
> As we are about to handle setting the address for the redistributor base
> region separately from some of the other base addresses, let's rework
> this function to leave a little more room for being flexible in what
> each type of base address does.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index d48743c..69ccfd5 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -37,6 +37,14 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
>  	return 0;
>  }
>  
> +static int vgic_check_type(struct kvm *kvm, int type_needed)
> +{
> +	if (kvm->arch.vgic.vgic_model != type_needed)
> +		return -ENODEV;
> +	else
> +		return 0;
> +}
> +
>  /**
>   * kvm_vgic_addr - set or get vgic VM base addresses
>   * @kvm:   pointer to the vm struct
> @@ -57,40 +65,36 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>  {
>  	int r = 0;
>  	struct vgic_dist *vgic = &kvm->arch.vgic;
> -	int type_needed;
>  	phys_addr_t *addr_ptr, alignment;
>  
>  	mutex_lock(&kvm->lock);
>  	switch (type) {
>  	case KVM_VGIC_V2_ADDR_TYPE_DIST:
> -		type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
> +		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
>  		addr_ptr = &vgic->vgic_dist_base;
>  		alignment = SZ_4K;
>  		break;
>  	case KVM_VGIC_V2_ADDR_TYPE_CPU:
> -		type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
> +		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
>  		addr_ptr = &vgic->vgic_cpu_base;
>  		alignment = SZ_4K;
>  		break;
>  	case KVM_VGIC_V3_ADDR_TYPE_DIST:
> -		type_needed = KVM_DEV_TYPE_ARM_VGIC_V3;
> +		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
>  		addr_ptr = &vgic->vgic_dist_base;
>  		alignment = SZ_64K;
>  		break;
>  	case KVM_VGIC_V3_ADDR_TYPE_REDIST:
> -		type_needed = KVM_DEV_TYPE_ARM_VGIC_V3;
> +		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
>  		addr_ptr = &vgic->vgic_redist_base;
>  		alignment = SZ_64K;
>  		break;
>  	default:
>  		r = -ENODEV;
> -		goto out;
>  	}
>  
> -	if (vgic->vgic_model != type_needed) {
> -		r = -ENODEV;
> +	if (r)
>  		goto out;
> -	}
>  
>  	if (write) {
>  		r = vgic_check_ioaddr(kvm, addr_ptr, *addr, alignment);
> 

[PATCH 6/8] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Christoffer Dall]

Instead of waiting with registering KVM iodevs until the very last VCPU
is created, we can actually create the iodevs when the redist base
address is set.  The only downside is that we must now also check if we
need to do this for VCPUs which are created after creating the VGIC,
because there is no enforced ordering between creating the VGIC and the
VCPUs.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 include/kvm/arm_vgic.h              |  1 +
 virt/kvm/arm/arm.c                  |  6 +++++-
 virt/kvm/arm/vgic/vgic-init.c       | 21 ++++++++++++++++++
 virt/kvm/arm/vgic/vgic-kvm-device.c |  7 +++++-
 virt/kvm/arm/vgic/vgic-mmio-v3.c    | 43 +++++++++++++++++++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-v3.c         |  6 ------
 virt/kvm/arm/vgic/vgic.h            |  3 ++-
 7 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index fabcc64..4ff65ef 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -286,6 +286,7 @@ extern struct static_key_false vgic_v2_cpuif_trap;
 
 int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
 void kvm_vgic_early_init(struct kvm *kvm);
+int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
 int kvm_vgic_create(struct kvm *kvm, u32 type);
 void kvm_vgic_destroy(struct kvm *kvm);
 void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 7941699..dd74d39 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -326,6 +326,8 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
 
 int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 {
+	int ret = 0;
+
 	/* Force users to call KVM_ARM_VCPU_INIT */
 	vcpu->arch.target = -1;
 	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
@@ -335,7 +337,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 
 	kvm_arm_reset_debug_ptr(vcpu);
 
-	return 0;
+	ret = kvm_vgic_vcpu_init(vcpu);
+
+	return ret;
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 0ea64a1..962bb57 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -226,6 +226,27 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
 	return 0;
 }
 
+/**
+ * kvm_vgic_vcpu_init() - Register VCPU-specific KVM iodevs
+ * @vcpu: pointer to the VCPU being created and initialized
+ */
+int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
+{
+	int ret = 0;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	if (!irqchip_in_kernel(vcpu->kvm))
+		return 0;
+
+	/*
+	 * If we are creating a VCPU with a GICv3 we must also register the
+	 * KVM io device for the redistributor that belongs to this VCPU.
+	 */
+	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+		ret = vgic_register_redist_iodev(vcpu);
+	return ret;
+}
+
 static void kvm_vgic_vcpu_enable(struct kvm_vcpu *vcpu)
 {
 	if (kvm_vgic_global_state.type == VGIC_V2)
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 69ccfd5..10ae6f3 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -86,8 +86,13 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 		break;
 	case KVM_VGIC_V3_ADDR_TYPE_REDIST:
 		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
+		if (r)
+			break;
+		if (write) {
+			r = vgic_v3_set_redist_base(kvm, *addr);
+			goto out;
+		}
 		addr_ptr = &vgic->vgic_redist_base;
-		alignment = SZ_64K;
 		break;
 	default:
 		r = -ENODEV;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 1828ac7..297557b 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -565,7 +565,7 @@ unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev)
  *
  * Return 0 on success, -ERRNO otherwise.
  */
-static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
+int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = vcpu->kvm;
 	struct vgic_dist *vgic = &kvm->arch.vgic;
@@ -574,6 +574,18 @@ static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	gpa_t rd_base, sgi_base;
 	int ret;
 
+	/*
+	 * We may be creating VCPUs before having set the base address for the
+	 * redistributor region, in which case we will come back to this
+	 * function for all VCPUs when the base address is set.  Just return
+	 * without doing any work for now.
+	 */
+	if (IS_VGIC_ADDR_UNDEF(vgic->vgic_redist_base))
+		return 0;
+
+	if (!vgic_v3_check_base(kvm))
+		return -EINVAL;
+
 	rd_base = vgic->vgic_redist_base + vcpu->vcpu_id * SZ_64K * 2;
 	sgi_base = rd_base + SZ_64K;
 
@@ -619,7 +631,7 @@ static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
 	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &sgi_dev->dev);
 }
 
-int vgic_register_redist_iodevs(struct kvm *kvm)
+static int vgic_register_all_redist_iodevs(struct kvm *kvm)
 {
 	struct kvm_vcpu *vcpu;
 	int c, ret = 0;
@@ -641,6 +653,33 @@ int vgic_register_redist_iodevs(struct kvm *kvm)
 	return ret;
 }
 
+int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
+{
+	struct vgic_dist *vgic = &kvm->arch.vgic;
+	int ret;
+
+	/* vgic_check_ioaddr makes sure we don't do this twice */
+	ret = vgic_check_ioaddr(kvm, &vgic->vgic_redist_base, addr, SZ_64K);
+	if (ret)
+		return ret;
+
+	vgic->vgic_redist_base = addr;
+	if (!vgic_v3_check_base(kvm)) {
+		vgic->vgic_redist_base = VGIC_ADDR_UNDEF;
+		return -EINVAL;
+	}
+
+	/*
+	 * Register iodevs for each existing VCPU.  Adding more VCPUs
+	 * afterwards will register the iodevs when needed.
+	 */
+	ret = vgic_register_all_redist_iodevs(kvm);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
 {
 	const struct vgic_register_region *region;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index b934e78..4d51d1f 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -396,12 +396,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
 		goto out;
 	}
 
-	ret = vgic_register_redist_iodevs(kvm);
-	if (ret) {
-		kvm_err("Unable to register VGICv3 redist MMIO regions\n");
-		goto out;
-	}
-
 	if (vgic_has_its(kvm)) {
 		ret = vgic_register_its_iodevs(kvm);
 		if (ret) {
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 89eb935..5f17eac 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -174,7 +174,8 @@ int vgic_v3_probe(const struct gic_kvm_info *info);
 int vgic_v3_map_resources(struct kvm *kvm);
 int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
 int vgic_v3_save_pending_tables(struct kvm *kvm);
-int vgic_register_redist_iodevs(struct kvm *kvm);
+int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr);
+int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
 bool vgic_v3_check_base(struct kvm *kvm);
 
 void vgic_v3_load(struct kvm_vcpu *vcpu);
-- 
2.9.0

[PATCH 6/8] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Auger Eric]

Hi Christoffer,

On 08/05/2017 13:54, Christoffer Dall wrote:
> Instead of waiting with registering KVM iodevs until the very last VCPU
> is created,
aren't they registered on the 1st VCPU run?
 we can actually create the iodevs when the redist base
> address is set.  The only downside is that we must now also check if we
> need to do this for VCPUs which are created after creating the VGIC,
> because there is no enforced ordering between creating the VGIC
(and setting its base addresses)
 and the
> VCPUs.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  include/kvm/arm_vgic.h              |  1 +
>  virt/kvm/arm/arm.c                  |  6 +++++-
>  virt/kvm/arm/vgic/vgic-init.c       | 21 ++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-kvm-device.c |  7 +++++-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c    | 43 +++++++++++++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-v3.c         |  6 ------
>  virt/kvm/arm/vgic/vgic.h            |  3 ++-
>  7 files changed, 76 insertions(+), 11 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index fabcc64..4ff65ef 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -286,6 +286,7 @@ extern struct static_key_false vgic_v2_cpuif_trap;
>  
>  int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
>  void kvm_vgic_early_init(struct kvm *kvm);
> +int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
>  int kvm_vgic_create(struct kvm *kvm, u32 type);
>  void kvm_vgic_destroy(struct kvm *kvm);
>  void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 7941699..dd74d39 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -326,6 +326,8 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
>  
>  int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  {
> +	int ret = 0;
> +
>  	/* Force users to call KVM_ARM_VCPU_INIT */
>  	vcpu->arch.target = -1;
>  	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> @@ -335,7 +337,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  
>  	kvm_arm_reset_debug_ptr(vcpu);
>  
> -	return 0;
> +	ret = kvm_vgic_vcpu_init(vcpu);
> +
> +	return ret;
nit: ret can be removed
>  }
>  
>  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 0ea64a1..962bb57 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -226,6 +226,27 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
>  	return 0;
>  }
>  
> +/**
> + * kvm_vgic_vcpu_init() - Register VCPU-specific KVM iodevs
> + * @vcpu: pointer to the VCPU being created and initialized
> + */
> +int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> +{
> +	int ret = 0;
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	if (!irqchip_in_kernel(vcpu->kvm))
> +		return 0;
> +
> +	/*
> +	 * If we are creating a VCPU with a GICv3 we must also register the
> +	 * KVM io device for the redistributor that belongs to this VCPU.
> +	 */
> +	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> +		ret = vgic_register_redist_iodev(vcpu);
> +	return ret;
> +}
> +
>  static void kvm_vgic_vcpu_enable(struct kvm_vcpu *vcpu)
>  {
>  	if (kvm_vgic_global_state.type == VGIC_V2)
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 69ccfd5..10ae6f3 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -86,8 +86,13 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>  		break;
>  	case KVM_VGIC_V3_ADDR_TYPE_REDIST:
>  		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
> +		if (r)
> +			break;
> +		if (write) {
> +			r = vgic_v3_set_redist_base(kvm, *addr);
> +			goto out;
> +		}
>  		addr_ptr = &vgic->vgic_redist_base;
> -		alignment = SZ_64K;
>  		break;
>  	default:
>  		r = -ENODEV;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 1828ac7..297557b 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -565,7 +565,7 @@ unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev)
>   *
>   * Return 0 on success, -ERRNO otherwise.
>   */
> -static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> +int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm *kvm = vcpu->kvm;
>  	struct vgic_dist *vgic = &kvm->arch.vgic;
> @@ -574,6 +574,18 @@ static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  	gpa_t rd_base, sgi_base;
>  	int ret;
>  
> +	/*
> +	 * We may be creating VCPUs before having set the base address for the
> +	 * redistributor region, in which case we will come back to this
> +	 * function for all VCPUs when the base address is set.  Just return
> +	 * without doing any work for now.
> +	 */
> +	if (IS_VGIC_ADDR_UNDEF(vgic->vgic_redist_base))
> +		return 0;
> +
> +	if (!vgic_v3_check_base(kvm))
> +		return -EINVAL;
> +
>  	rd_base = vgic->vgic_redist_base + vcpu->vcpu_id * SZ_64K * 2;
>  	sgi_base = rd_base + SZ_64K;
>  
> @@ -619,7 +631,7 @@ static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
>  	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &sgi_dev->dev);
>  }
>  
> -int vgic_register_redist_iodevs(struct kvm *kvm)
> +static int vgic_register_all_redist_iodevs(struct kvm *kvm)
>  {
>  	struct kvm_vcpu *vcpu;
>  	int c, ret = 0;
> @@ -641,6 +653,33 @@ int vgic_register_redist_iodevs(struct kvm *kvm)
>  	return ret;
>  }
>  
> +int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
> +{
> +	struct vgic_dist *vgic = &kvm->arch.vgic;
> +	int ret;
> +
> +	/* vgic_check_ioaddr makes sure we don't do this twice */
> +	ret = vgic_check_ioaddr(kvm, &vgic->vgic_redist_base, addr, SZ_64K);
> +	if (ret)
> +		return ret;
> +
> +	vgic->vgic_redist_base = addr;
> +	if (!vgic_v3_check_base(kvm)) {
So we must make sure this check takes into account the case dist base
address is not set yet.

Otherwise looks good to me.

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

Thanks


Eric


> +		vgic->vgic_redist_base = VGIC_ADDR_UNDEF;
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Register iodevs for each existing VCPU.  Adding more VCPUs
> +	 * afterwards will register the iodevs when needed.
> +	 */
> +	ret = vgic_register_all_redist_iodevs(kvm);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
>  {
>  	const struct vgic_register_region *region;
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index b934e78..4d51d1f 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -396,12 +396,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
>  		goto out;
>  	}
>  
> -	ret = vgic_register_redist_iodevs(kvm);
> -	if (ret) {
> -		kvm_err("Unable to register VGICv3 redist MMIO regions\n");
> -		goto out;
> -	}
> -
>  	if (vgic_has_its(kvm)) {
>  		ret = vgic_register_its_iodevs(kvm);
>  		if (ret) {
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 89eb935..5f17eac 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -174,7 +174,8 @@ int vgic_v3_probe(const struct gic_kvm_info *info);
>  int vgic_v3_map_resources(struct kvm *kvm);
>  int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
>  int vgic_v3_save_pending_tables(struct kvm *kvm);
> -int vgic_register_redist_iodevs(struct kvm *kvm);
> +int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr);
> +int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
>  bool vgic_v3_check_base(struct kvm *kvm);
>  
>  void vgic_v3_load(struct kvm_vcpu *vcpu);
> 

[PATCH 6/8] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Christoffer Dall]

On Mon, May 08, 2017 at 07:09:07PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 08/05/2017 13:54, Christoffer Dall wrote:
> > Instead of waiting with registering KVM iodevs until the very last VCPU
> > is created,
> aren't they registered on the 1st VCPU run?

yeah, that's what i meant

>>  we can actually create the iodevs when the redist base
> > address is set.  The only downside is that we must now also check if we
> > need to do this for VCPUs which are created after creating the VGIC,
> > because there is no enforced ordering between creating the VGIC
> (and setting its base addresses)

ok


>>  and the
> > VCPUs.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  include/kvm/arm_vgic.h              |  1 +
> >  virt/kvm/arm/arm.c                  |  6 +++++-
> >  virt/kvm/arm/vgic/vgic-init.c       | 21 ++++++++++++++++++
> >  virt/kvm/arm/vgic/vgic-kvm-device.c |  7 +++++-
> >  virt/kvm/arm/vgic/vgic-mmio-v3.c    | 43 +++++++++++++++++++++++++++++++++++--
> >  virt/kvm/arm/vgic/vgic-v3.c         |  6 ------
> >  virt/kvm/arm/vgic/vgic.h            |  3 ++-
> >  7 files changed, 76 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index fabcc64..4ff65ef 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -286,6 +286,7 @@ extern struct static_key_false vgic_v2_cpuif_trap;
> >  
> >  int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
> >  void kvm_vgic_early_init(struct kvm *kvm);
> > +int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
> >  int kvm_vgic_create(struct kvm *kvm, u32 type);
> >  void kvm_vgic_destroy(struct kvm *kvm);
> >  void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu);
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 7941699..dd74d39 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -326,6 +326,8 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
> >  
> >  int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> >  {
> > +	int ret = 0;
> > +
> >  	/* Force users to call KVM_ARM_VCPU_INIT */
> >  	vcpu->arch.target = -1;
> >  	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> > @@ -335,7 +337,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> >  
> >  	kvm_arm_reset_debug_ptr(vcpu);
> >  
> > -	return 0;
> > +	ret = kvm_vgic_vcpu_init(vcpu);
> > +
> > +	return ret;
> nit: ret can be removed
> >  }
> >  
> >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> > index 0ea64a1..962bb57 100644
> > --- a/virt/kvm/arm/vgic/vgic-init.c
> > +++ b/virt/kvm/arm/vgic/vgic-init.c
> > @@ -226,6 +226,27 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * kvm_vgic_vcpu_init() - Register VCPU-specific KVM iodevs
> > + * @vcpu: pointer to the VCPU being created and initialized
> > + */
> > +int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> > +{
> > +	int ret = 0;
> > +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> > +
> > +	if (!irqchip_in_kernel(vcpu->kvm))
> > +		return 0;
> > +
> > +	/*
> > +	 * If we are creating a VCPU with a GICv3 we must also register the
> > +	 * KVM io device for the redistributor that belongs to this VCPU.
> > +	 */
> > +	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> > +		ret = vgic_register_redist_iodev(vcpu);
> > +	return ret;
> > +}
> > +
> >  static void kvm_vgic_vcpu_enable(struct kvm_vcpu *vcpu)
> >  {
> >  	if (kvm_vgic_global_state.type == VGIC_V2)
> > diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> > index 69ccfd5..10ae6f3 100644
> > --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> > +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> > @@ -86,8 +86,13 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
> >  		break;
> >  	case KVM_VGIC_V3_ADDR_TYPE_REDIST:
> >  		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
> > +		if (r)
> > +			break;
> > +		if (write) {
> > +			r = vgic_v3_set_redist_base(kvm, *addr);
> > +			goto out;
> > +		}
> >  		addr_ptr = &vgic->vgic_redist_base;
> > -		alignment = SZ_64K;
> >  		break;
> >  	default:
> >  		r = -ENODEV;
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 1828ac7..297557b 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -565,7 +565,7 @@ unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev)
> >   *
> >   * Return 0 on success, -ERRNO otherwise.
> >   */
> > -static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> > +int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm *kvm = vcpu->kvm;
> >  	struct vgic_dist *vgic = &kvm->arch.vgic;
> > @@ -574,6 +574,18 @@ static int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> >  	gpa_t rd_base, sgi_base;
> >  	int ret;
> >  
> > +	/*
> > +	 * We may be creating VCPUs before having set the base address for the
> > +	 * redistributor region, in which case we will come back to this
> > +	 * function for all VCPUs when the base address is set.  Just return
> > +	 * without doing any work for now.
> > +	 */
> > +	if (IS_VGIC_ADDR_UNDEF(vgic->vgic_redist_base))
> > +		return 0;
> > +
> > +	if (!vgic_v3_check_base(kvm))
> > +		return -EINVAL;
> > +
> >  	rd_base = vgic->vgic_redist_base + vcpu->vcpu_id * SZ_64K * 2;
> >  	sgi_base = rd_base + SZ_64K;
> >  
> > @@ -619,7 +631,7 @@ static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
> >  	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &sgi_dev->dev);
> >  }
> >  
> > -int vgic_register_redist_iodevs(struct kvm *kvm)
> > +static int vgic_register_all_redist_iodevs(struct kvm *kvm)
> >  {
> >  	struct kvm_vcpu *vcpu;
> >  	int c, ret = 0;
> > @@ -641,6 +653,33 @@ int vgic_register_redist_iodevs(struct kvm *kvm)
> >  	return ret;
> >  }
> >  
> > +int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
> > +{
> > +	struct vgic_dist *vgic = &kvm->arch.vgic;
> > +	int ret;
> > +
> > +	/* vgic_check_ioaddr makes sure we don't do this twice */
> > +	ret = vgic_check_ioaddr(kvm, &vgic->vgic_redist_base, addr, SZ_64K);
> > +	if (ret)
> > +		return ret;
> > +
> > +	vgic->vgic_redist_base = addr;
> > +	if (!vgic_v3_check_base(kvm)) {
> So we must make sure this check takes into account the case dist base
> address is not set yet.

It should already.


> 
> Otherwise looks good to me.
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 

Thanks,
-Christoffer

> > +		vgic->vgic_redist_base = VGIC_ADDR_UNDEF;
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * Register iodevs for each existing VCPU.  Adding more VCPUs
> > +	 * afterwards will register the iodevs when needed.
> > +	 */
> > +	ret = vgic_register_all_redist_iodevs(kvm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> >  int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> >  {
> >  	const struct vgic_register_region *region;
> > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> > index b934e78..4d51d1f 100644
> > --- a/virt/kvm/arm/vgic/vgic-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-v3.c
> > @@ -396,12 +396,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
> >  		goto out;
> >  	}
> >  
> > -	ret = vgic_register_redist_iodevs(kvm);
> > -	if (ret) {
> > -		kvm_err("Unable to register VGICv3 redist MMIO regions\n");
> > -		goto out;
> > -	}
> > -
> >  	if (vgic_has_its(kvm)) {
> >  		ret = vgic_register_its_iodevs(kvm);
> >  		if (ret) {
> > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> > index 89eb935..5f17eac 100644
> > --- a/virt/kvm/arm/vgic/vgic.h
> > +++ b/virt/kvm/arm/vgic/vgic.h
> > @@ -174,7 +174,8 @@ int vgic_v3_probe(const struct gic_kvm_info *info);
> >  int vgic_v3_map_resources(struct kvm *kvm);
> >  int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
> >  int vgic_v3_save_pending_tables(struct kvm *kvm);
> > -int vgic_register_redist_iodevs(struct kvm *kvm);
> > +int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr);
> > +int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
> >  bool vgic_v3_check_base(struct kvm *kvm);
> >  
> >  void vgic_v3_load(struct kvm_vcpu *vcpu);
> > 

[PATCH 7/8] KVM: arm/arm64: Register ITS iodev when setting base address

[Christoffer Dall]

We have to register the ITS iodevice before running the VM, because in
migration scenarios, we may be restoring a live device that wishes to
inject MSIs before we get a chance to run the VM.

All we need to register the ITS io device is the base address of the
ITS, so we can simply register that when the base address of the ITS is
set.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-its.c | 29 +----------------------------
 virt/kvm/arm/vgic/vgic-v3.c  |  8 --------
 virt/kvm/arm/vgic/vgic.h     |  1 -
 3 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9f7105c..375c503 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2390,7 +2390,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 
 		its->vgic_its_base = addr;
 
-		return 0;
+		return vgic_register_its_iodev(dev->kvm, its);
 	}
 	case KVM_DEV_ARM_VGIC_GRP_CTRL: {
 		const struct vgic_its_abi *abi = vgic_its_get_abi(its);
@@ -2467,30 +2467,3 @@ 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 4d51d1f..d77fcb3 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -396,14 +396,6 @@ 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 5f17eac..8ac7397 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -181,7 +181,6 @@ bool vgic_v3_check_base(struct kvm *kvm);
 void vgic_v3_load(struct kvm_vcpu *vcpu);
 void vgic_v3_put(struct kvm_vcpu *vcpu);
 
-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);
-- 
2.9.0

[PATCH 7/8] KVM: arm/arm64: Register ITS iodev when setting base address

[Auger Eric]

Hi,

On 08/05/2017 13:54, Christoffer Dall wrote:
> We have to register the ITS iodevice before running the VM, because in
> migration scenarios, we may be restoring a live device that wishes to
> inject MSIs before we get a chance to run the VM.

actually the VM is in running state but the vcpus has not yet started.
> 
> All we need to register the ITS io device is the base address of the
> ITS, so we can simply register that when the base address of the ITS is
> set.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 29 +----------------------------
>  virt/kvm/arm/vgic/vgic-v3.c  |  8 --------
>  virt/kvm/arm/vgic/vgic.h     |  1 -
>  3 files changed, 1 insertion(+), 37 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 9f7105c..375c503 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2390,7 +2390,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>  
>  		its->vgic_its_base = addr;
>  
> -		return 0;
> +		return vgic_register_its_iodev(dev->kvm, its);
>  	}
>  	case KVM_DEV_ARM_VGIC_GRP_CTRL: {
>  		const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> @@ -2467,30 +2467,3 @@ 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 4d51d1f..d77fcb3 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -396,14 +396,6 @@ 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 5f17eac..8ac7397 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -181,7 +181,6 @@ bool vgic_v3_check_base(struct kvm *kvm);
>  void vgic_v3_load(struct kvm_vcpu *vcpu);
>  void vgic_v3_put(struct kvm_vcpu *vcpu);
>  
> -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);
> 

[PATCH 7/8] KVM: arm/arm64: Register ITS iodev when setting base address

[Marc Zyngier]

Hi Christoffer,

On 08/05/17 12:54, Christoffer Dall wrote:
> We have to register the ITS iodevice before running the VM, because in
> migration scenarios, we may be restoring a live device that wishes to
> inject MSIs before we get a chance to run the VM.
> 
> All we need to register the ITS io device is the base address of the
> ITS, so we can simply register that when the base address of the ITS is
> set.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 29 +----------------------------
>  virt/kvm/arm/vgic/vgic-v3.c  |  8 --------
>  virt/kvm/arm/vgic/vgic.h     |  1 -
>  3 files changed, 1 insertion(+), 37 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 9f7105c..375c503 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2390,7 +2390,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>  
>  		its->vgic_its_base = addr;
>  
> -		return 0;
> +		return vgic_register_its_iodev(dev->kvm, its);
>  	}
>  	case KVM_DEV_ARM_VGIC_GRP_CTRL: {
>  		const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> @@ -2467,30 +2467,3 @@ int kvm_vgic_register_its_device(void)
>  	return kvm_register_device_ops(&kvm_arm_vgic_its_ops,
>  				       KVM_DEV_TYPE_ARM_VGIC_ITS);
>  }

This patch has the unfortunate side effect of breaking kvmtool 
(although the ITS support patches are not merged yet). The code 
sequence now requires that KVM_DEV_ARM_VGIC_CTRL_INIT has been issued 
on the ITS *before* we can set the ITS base address.

I find it a bit odd (I see KVM_DEV_ARM_VGIC_CTRL_INIT as a way to say 
"I'm done, do what you must and make it work"), but that's obviously 
what QEMU must be doing (the save/restore sequence hints at that, but I 
only just realized the issue). Of course, kvmtool does the opposite.

But now that we've coupled setting the address and mapping the iodev, 
there is not much that's left for KVM_DEV_ARM_VGIC_CTRL_INIT, and I 
wonder why we're keeping it (other than for backward compatibility)?

We're now unable to set the address more than once (fair enough). What 
is also interesting is that this makes it plain obvious that we have a 
race between setting the address and registering the device (no lock is 
taken here, and two threads could try and play a dirty game on us).

I've come up with the two following patches, that seem to make it work.

Thanks,

	M.

>From 25e5367b7b7c3bcbeaff3a1b74605aed41e996cf Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 8 May 2017 18:15:40 +0100
Subject: [PATCH 1/2] KVM: arm/arm64: Get rid of its->initialized field

The its->initialized doesn't bring much to the table, and creates
unnecessary ordering between setting the address and initializing it
(which amounts to exactly nothing).

Let's kill it altogether, making KVM_DEV_ARM_VGIC_CTRL_INIT the no-op
it deserves to be.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h       | 1 -
 virt/kvm/arm/vgic/vgic-its.c | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index a5c5c4eda6f4..97b8d3728b31 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -148,7 +148,6 @@ struct vgic_its {
 	gpa_t			vgic_its_base;
 
 	bool			enabled;
-	bool			initialized;
 	struct vgic_io_device	iodev;
 	struct kvm_device	*dev;
 
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9b67621df08d..c6237cd6595c 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1545,9 +1545,6 @@ 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;
 
@@ -1597,7 +1594,6 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
 	INIT_LIST_HEAD(&its->collection_list);
 
 	dev->kvm->arch.vgic.has_its = true;
-	its->initialized = false;
 	its->enabled = false;
 	its->dev = dev;
 
@@ -2398,8 +2394,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 
 		switch (attr->attr) {
 		case KVM_DEV_ARM_VGIC_CTRL_INIT:
-			its->initialized = true;
-
+			/* Nothing to do */
 			return 0;
 		case KVM_DEV_ARM_ITS_SAVE_TABLES:
 			return abi->save_tables(its);
-- 
2.11.0


>From fdcc35008da30248054979f102ef4064594b2001 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 8 May 2017 18:34:25 +0100
Subject: [PATCH 2/2] KVM: arm/arm64: Plug race between ITS base address
 setting and iodev registration

We set the ITS base address (and other fields) outside of the critical
section, leaving the door open for two thread to race on changing the
fields.

Change vgic_register_its_iodev() to take an address parameter, and
move all the initialization inside the mutex-protected section.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index c6237cd6595c..0a97ab49b44f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1540,14 +1540,19 @@ 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)
+static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its,
+				   u64 addr)
 {
 	struct vgic_io_device *iodev = &its->iodev;
 	int ret;
 
-	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
-		return -ENXIO;
+	mutex_lock(&kvm->slots_lock);
+	if (!IS_VGIC_ADDR_UNDEF(its->vgic_its_base)) {
+		ret = -EBUSY;
+		goto out;
+	}
 
+	its->vgic_its_base = addr;
 	iodev->regions = its_registers;
 	iodev->nr_regions = ARRAY_SIZE(its_registers);
 	kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops);
@@ -1555,9 +1560,9 @@ static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
 	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);
+out:
 	mutex_unlock(&kvm->slots_lock);
 
 	return ret;
@@ -2385,9 +2390,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 		if (ret)
 			return ret;
 
-		its->vgic_its_base = addr;
-
-		return vgic_register_its_iodev(dev->kvm, its);
+		return vgic_register_its_iodev(dev->kvm, its, addr);
 	}
 	case KVM_DEV_ARM_VGIC_GRP_CTRL: {
 		const struct vgic_its_abi *abi = vgic_its_get_abi(its);
-- 
2.11.0


-- 
Jazz is not dead. It just smells funny...

[PATCH 7/8] KVM: arm/arm64: Register ITS iodev when setting base address

[Christoffer Dall]

On Mon, May 08, 2017 at 06:41:36PM +0100, Marc Zyngier wrote:
> Hi Christoffer,
> 
> On 08/05/17 12:54, Christoffer Dall wrote:
> > We have to register the ITS iodevice before running the VM, because in
> > migration scenarios, we may be restoring a live device that wishes to
> > inject MSIs before we get a chance to run the VM.
> > 
> > All we need to register the ITS io device is the base address of the
> > ITS, so we can simply register that when the base address of the ITS is
> > set.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic/vgic-its.c | 29 +----------------------------
> >  virt/kvm/arm/vgic/vgic-v3.c  |  8 --------
> >  virt/kvm/arm/vgic/vgic.h     |  1 -
> >  3 files changed, 1 insertion(+), 37 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> > index 9f7105c..375c503 100644
> > --- a/virt/kvm/arm/vgic/vgic-its.c
> > +++ b/virt/kvm/arm/vgic/vgic-its.c
> > @@ -2390,7 +2390,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
> >  
> >  		its->vgic_its_base = addr;
> >  
> > -		return 0;
> > +		return vgic_register_its_iodev(dev->kvm, its);
> >  	}
> >  	case KVM_DEV_ARM_VGIC_GRP_CTRL: {
> >  		const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > @@ -2467,30 +2467,3 @@ int kvm_vgic_register_its_device(void)
> >  	return kvm_register_device_ops(&kvm_arm_vgic_its_ops,
> >  				       KVM_DEV_TYPE_ARM_VGIC_ITS);
> >  }
> 
> This patch has the unfortunate side effect of breaking kvmtool 
> (although the ITS support patches are not merged yet). The code 
> sequence now requires that KVM_DEV_ARM_VGIC_CTRL_INIT has been issued 
> on the ITS *before* we can set the ITS base address.
> 
> I find it a bit odd (I see KVM_DEV_ARM_VGIC_CTRL_INIT as a way to say 
> "I'm done, do what you must and make it work"), but that's obviously 
> what QEMU must be doing (the save/restore sequence hints at that, but I 
> only just realized the issue). Of course, kvmtool does the opposite.
> 
> But now that we've coupled setting the address and mapping the iodev, 
> there is not much that's left for KVM_DEV_ARM_VGIC_CTRL_INIT, and I 
> wonder why we're keeping it (other than for backward compatibility)?
> 
> We're now unable to set the address more than once (fair enough). What 
> is also interesting is that this makes it plain obvious that we have a 
> race between setting the address and registering the device (no lock is 
> taken here, and two threads could try and play a dirty game on us).
> 
> I've come up with the two following patches, that seem to make it work.

Both patches look good, thanks.

-Christoffer

[PATCH 8/8] KVM: arm/arm64: vgic-its: Cleanup after failed ITT restore

[Christoffer Dall]

When failing to restore the ITT for a DTE, we should remove the failed
device entry from the list and free the object.

We slightly refactor vgic_its_destroy to be able to reuse the now
separate vgic_its_free_dte() function.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-its.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 375c503..00f2990 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1612,13 +1612,20 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
 	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
 }
 
+static void vgic_its_free_dte(struct kvm *kvm, struct its_device *dev)
+{
+	struct its_ite *ite, *tmp;
+
+	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
+		its_free_ite(kvm, ite);
+	list_del(&dev->dev_list);
+	kfree(dev);
+}
+
 static void vgic_its_destroy(struct kvm_device *kvm_dev)
 {
 	struct kvm *kvm = kvm_dev->kvm;
 	struct vgic_its *its = kvm_dev->private;
-	struct its_device *dev;
-	struct its_ite *ite;
-	struct list_head *dev_cur, *dev_temp;
 	struct list_head *cur, *temp;
 
 	/*
@@ -1629,19 +1636,19 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
 		return;
 
 	mutex_lock(&its->its_lock);
-	list_for_each_safe(dev_cur, dev_temp, &its->device_list) {
-		dev = container_of(dev_cur, struct its_device, dev_list);
-		list_for_each_safe(cur, temp, &dev->itt_head) {
-			ite = (container_of(cur, struct its_ite, ite_list));
-			its_free_ite(kvm, ite);
-		}
-		list_del(dev_cur);
-		kfree(dev);
+	list_for_each_safe(cur, temp, &its->device_list) {
+		struct its_device *dev;
+
+		dev = list_entry(cur, struct its_device, dev_list);
+		vgic_its_free_dte(kvm, dev);
 	}
 
 	list_for_each_safe(cur, temp, &its->collection_list) {
+		struct its_collection *coll;
+
+		coll = list_entry(cur, struct its_collection, coll_list);
 		list_del(cur);
-		kfree(container_of(cur, struct its_collection, coll_list));
+		kfree(coll);
 	}
 	mutex_unlock(&its->its_lock);
 
@@ -2011,8 +2018,10 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
 		return PTR_ERR(dev);
 
 	ret = vgic_its_restore_itt(its, dev);
-	if (ret)
+	if (ret) {
+		vgic_its_free_dte(its->dev->kvm, dev);
 		return ret;
+	}
 
 	return offset;
 }
-- 
2.9.0

[PATCH 8/8] KVM: arm/arm64: vgic-its: Cleanup after failed ITT restore

[Auger Eric]

Hi Christoffer,
On 08/05/2017 13:54, Christoffer Dall wrote:
> When failing to restore the ITT for a DTE, we should remove the failed
> device entry from the list and free the object.
> 
> We slightly refactor vgic_its_destroy to be able to reuse the now
> separate vgic_its_free_dte() function.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 375c503..00f2990 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1612,13 +1612,20 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
>  	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
>  }
>  
> +static void vgic_its_free_dte(struct kvm *kvm, struct its_device *dev)
nit: I would rename this into vgic_its_free_device as this is the
counterpart of vgic_its_alloc_device

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

Thanks

Eric
> +{
> +	struct its_ite *ite, *tmp;
> +
> +	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
> +		its_free_ite(kvm, ite);
> +	list_del(&dev->dev_list);
> +	kfree(dev);
> +}
> +
>  static void vgic_its_destroy(struct kvm_device *kvm_dev)
>  {
>  	struct kvm *kvm = kvm_dev->kvm;
>  	struct vgic_its *its = kvm_dev->private;
> -	struct its_device *dev;
> -	struct its_ite *ite;
> -	struct list_head *dev_cur, *dev_temp;
>  	struct list_head *cur, *temp;
>  
>  	/*
> @@ -1629,19 +1636,19 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
>  		return;
>  
>  	mutex_lock(&its->its_lock);
> -	list_for_each_safe(dev_cur, dev_temp, &its->device_list) {
> -		dev = container_of(dev_cur, struct its_device, dev_list);
> -		list_for_each_safe(cur, temp, &dev->itt_head) {
> -			ite = (container_of(cur, struct its_ite, ite_list));
> -			its_free_ite(kvm, ite);
> -		}
> -		list_del(dev_cur);
> -		kfree(dev);
> +	list_for_each_safe(cur, temp, &its->device_list) {
> +		struct its_device *dev;
> +
> +		dev = list_entry(cur, struct its_device, dev_list);
> +		vgic_its_free_dte(kvm, dev);
>  	}
>  
>  	list_for_each_safe(cur, temp, &its->collection_list) {
> +		struct its_collection *coll;
> +
> +		coll = list_entry(cur, struct its_collection, coll_list);
>  		list_del(cur);
> -		kfree(container_of(cur, struct its_collection, coll_list));
> +		kfree(coll);
>  	}
>  	mutex_unlock(&its->its_lock);
>  
> @@ -2011,8 +2018,10 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>  		return PTR_ERR(dev);
>  
>  	ret = vgic_its_restore_itt(its, dev);
> -	if (ret)
> +	if (ret) {
> +		vgic_its_free_dte(its->dev->kvm, dev);
>  		return ret;
> +	}
>  
>  	return offset;
>  }
> 

[PATCH 9/8] KVM: arm/arm64: Don't call map_resources when restoring ITS tables

[Christoffer Dall]

The only reason we called kvm_vgic_map_resources() when restoring the
ITS tables was because we wanted to have the KVM iodevs registered in
the KVM IO bus framework at the time when the ITS was restored such that
a restored and active device can inject MSIs prior to otherwise calling
kvm_vgic_map_resources() from the first run of a VCPU.

Since we now register the KVM iodevs for the redestributors and ITS as
soon as possible (when setting the base addresses), we no longer need
this call and kvm_vgic_map_resources() is again called only when first
running a VCPU.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
Forgot to include this when posting the series, which was the whole
point of the iodev rework in the first place.  Apologies about the
confusing 9/8 subject thing.

 virt/kvm/arm/vgic/vgic-its.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 00f2990..9b67621 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2316,20 +2316,12 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 		goto out;
 
 	ret = vgic_its_restore_device_tables(its);
-
 out:
 	unlock_all_vcpus(kvm);
 	mutex_unlock(&its->its_lock);
 	mutex_unlock(&kvm->lock);
 
-	if (ret)
-		return ret;
-
-	/*
-	 * On restore path, MSI injections can happen before the
-	 * first VCPU run so let's complete the GIC init here.
-	 */
-	return kvm_vgic_map_resources(its->dev->kvm);
+	return ret;
 }
 
 static int vgic_its_commit_v0(struct vgic_its *its)
-- 
2.9.0

[PATCH 9/8] KVM: arm/arm64: Don't call map_resources when restoring ITS tables

[Auger Eric]

Hi Christoffer,
On 08/05/2017 14:49, Christoffer Dall wrote:
> The only reason we called kvm_vgic_map_resources() when restoring the
> ITS tables was because we wanted to have the KVM iodevs registered in
> the KVM IO bus framework at the time when the ITS was restored such that
> a restored and active device can inject MSIs prior to otherwise calling
> kvm_vgic_map_resources() from the first run of a VCPU.
> 
> Since we now register the KVM iodevs for the redestributors and ITS as
> soon as possible (when setting the base addresses), we no longer need
> this call and kvm_vgic_map_resources() is again called only when first
> running a VCPU.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
> Forgot to include this when posting the series, which was the whole
> point of the iodev rework in the first place.  Apologies about the
> confusing 9/8 subject thing.
> 
>  virt/kvm/arm/vgic/vgic-its.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 00f2990..9b67621 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2316,20 +2316,12 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  		goto out;
>  
>  	ret = vgic_its_restore_device_tables(its);
> -
>  out:
>  	unlock_all_vcpus(kvm);
>  	mutex_unlock(&its->its_lock);
>  	mutex_unlock(&kvm->lock);
>  
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * On restore path, MSI injections can happen before the
> -	 * first VCPU run so let's complete the GIC init here.
> -	 */
> -	return kvm_vgic_map_resources(its->dev->kvm);
> +	return ret;
>  }
>  
>  static int vgic_its_commit_v0(struct vgic_its *its)
>