[PATCH v2 00/11] Fixes to v7 of the vITS save/restore series

[PATCH v2 00/11] 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-v2

Thanks,
-Christoffer

Changes since v1:
 - We got rid of the requirement to initialize the ITS (it doesn't do
   anything).
 - We fixed a race condition with setting the ITS base address which was
   introduced in v1 of this series.
 - We reworded some commit messages based on Eric's comments
 - We fixed the address check to work with partically unset base addrs
 - We use the vcpu index instead of the vcpu id for allocating redist
   regions.
 - Some renames and code cleanups.

Christoffer Dall (10):
  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: Add kvm_vcpu_get_idx to get vcpu index in kvm->vcpus
  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: Don't call map_resources when restoring ITS tables
  KVM: arm/arm64: vgic-its: Cleanup after failed ITT restore

Marc Zyngier (1):
  KVM: arm/arm64: Get rid of its->initialized field

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |  23 ++--
 include/kvm/arm_vgic.h                             |   2 +-
 include/linux/kvm_host.h                           |  11 ++
 virt/kvm/arm/arm.c                                 |   2 +-
 virt/kvm/arm/vgic/vgic-init.c                      |  25 +++-
 virt/kvm/arm/vgic/vgic-its.c                       |  96 +++++---------
 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                        |  33 +++--
 virt/kvm/arm/vgic/vgic.h                           |   5 +-
 10 files changed, 222 insertions(+), 151 deletions(-)

-- 
2.9.0

[PATCH v2 01/11] 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 Documentation/virtual/kvm/devices/arm-vgic-its.txt | 23 +++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index ba132e9..eb06beb 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -97,8 +97,8 @@ Groups:
 The following ordering must be followed when restoring the GIC and the ITS:
 a) restore all guest memory and create vcpus
 b) restore all redistributors
-c) initialize the ITS and then provide its base address
-   (KVM_DEV_ARM_VGIC_CTRL_INIT, KVM_DEV_ARM_VGIC_GRP_ADDR)
+c) provide the its base address
+   (KVM_DEV_ARM_VGIC_GRP_ADDR)
 d) restore the ITS in the following order:
    1. Restore GITS_CBASER
    2. Restore all other GITS_ registers, except GITS_CTLR!
@@ -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 v2 02/11] 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 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 v2 03/11] KVM: Add kvm_vcpu_get_idx to get vcpu index in kvm->vcpus

[Christoffer Dall]

There are occasional needs to use the index of vcpu in the kvm->vcpus
array to map something related to a VCPU.  For example, unlike the
vcpu->vcpu_id, the vcpu index is guaranteed to not be sparse across all
vcpus which is useful when allocating a memory area for each vcpu.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 include/linux/kvm_host.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

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

[PATCH v2 03/11] KVM: Add kvm_vcpu_get_idx to get vcpu index in kvm->vcpus

[Auger Eric]

Hi Christoffer,

On 09/05/2017 10:56, Christoffer Dall wrote:
> There are occasional needs to use the index of vcpu in the kvm->vcpus
> array to map something related to a VCPU.  For example, unlike the
> vcpu->vcpu_id, the vcpu index is guaranteed to not be sparse across all
> vcpus which is useful when allocating a memory area for each vcpu.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  include/linux/kvm_host.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> 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;\
> 

[PATCH v2 04/11] 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..168269b 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 + kvm_vcpu_get_idx(vcpu) * 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 v2 04/11] KVM: arm/arm64: Refactor vgic_register_redist_iodevs

[Auger Eric]

Hi,

On 09/05/2017 10:56, 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  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..168269b 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 + kvm_vcpu_get_idx(vcpu) * 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);
> 

[PATCH v2 05/11] 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 when setting base addresses as early as
possible.  When setting a base address, we can check that there's
address space enough for its scope and when the last of the two
base addresses (dist and redist) get set, we can also check if the
regions overlap at that time.

This allows us to provide error messages to the user at time when trying
to set the base address, as opposed to later when trying to run the VM.

To do this,  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 | 19 +++++++++++++++----
 virt/kvm/arm/vgic/vgic.h    |  1 +
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 12e52a0..2d53d7a 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -329,19 +329,30 @@ 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;
 
+	/* Both base addresses must be set to check if they overlap */
+	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 v2 05/11] KVM: arm/arm64: Make vgic_v3_check_base more broadly usable

[Auger Eric]

Hi Christoffer,

On 09/05/2017 10:56, Christoffer Dall wrote:
> As we are about to fiddle with the IO device registration mechanism,
> let's be a little more careful when setting base addresses as early as
> possible.  When setting a base address, we can check that there's
> address space enough for its scope and when the last of the two
> base addresses (dist and redist) get set, we can also check if the
> regions overlap at that time.
> 
> This allows us to provide error messages to the user at time when trying
> to set the base address, as opposed to later when trying to run the VM.
> 
> To do this,  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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 19 +++++++++++++++----
>  virt/kvm/arm/vgic/vgic.h    |  1 +
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 12e52a0..2d53d7a 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -329,19 +329,30 @@ 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;
>  
> +	/* Both base addresses must be set to check if they overlap */
> +	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);
> 

[PATCH v2 06/11] 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 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 v2 07/11] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Christoffer Dall]

Instead of waiting with registering KVM iodevs until the first VCPU is
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 creating the VCPUs.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 include/kvm/arm_vgic.h              |  1 +
 virt/kvm/arm/arm.c                  |  2 +-
 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, 72 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..9f6f522a4b 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -335,7 +335,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 
 	kvm_arm_reset_debug_ptr(vcpu);
 
-	return 0;
+	return kvm_vgic_vcpu_init(vcpu);
 }
 
 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 168269b..99da1a2 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 + kvm_vcpu_get_idx(vcpu) * 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 2d53d7a..bb35078 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -397,12 +397,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 v2 07/11] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Jean-Philippe Brucker]

Hi,

On 09/05/17 09:56, Christoffer Dall wrote:
> Instead of waiting with registering KVM iodevs until the first VCPU is
> 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 creating the VCPUs.

This triggers a BUG(), when the order is VGIC init, then VCPU init (which
is what kvmtool does).

Issuing KVM_CREATE_VCPU after VGIC intialization produces the following calls:

kvm_vm_ioctl_create_vcpu
 kvm_arch_vcpu_create
  kvm_vcpu_init
   kvm_arch_vcpu_init
    kvm_vgic_vcpu_init
     vgic_register_redist_iodev
      kvm_vcpu_get_idx
       ... no VCPU registered yet in kvm->vcpus :(
       BUG();

 ... would later register vcpu:
 kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu

My quick fix is to move kvm_vgic_vcpu_init into kvm_arch_vcpu_postcreate,
but it discards the return value of kvm_vgic_vcpu_init, so I don't know
how to do it properly.

Thanks,
Jean

> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/kvm/arm_vgic.h              |  1 +
>  virt/kvm/arm/arm.c                  |  2 +-
>  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, 72 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..9f6f522a4b 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -335,7 +335,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  
>  	kvm_arm_reset_debug_ptr(vcpu);
>  
> -	return 0;
> +	return kvm_vgic_vcpu_init(vcpu);
>  }
>  
>  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 168269b..99da1a2 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 + kvm_vcpu_get_idx(vcpu) * 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 2d53d7a..bb35078 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -397,12 +397,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 v2 07/11] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Auger Eric]

Hi Jean, Christoffer,

On 16/05/2017 13:23, Jean-Philippe Brucker wrote:
> Hi,
> 
> On 09/05/17 09:56, Christoffer Dall wrote:
>> Instead of waiting with registering KVM iodevs until the first VCPU is
>> 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 creating the VCPUs.
> 
> This triggers a BUG(), when the order is VGIC init, then VCPU init (which
> is what kvmtool does).
> 
> Issuing KVM_CREATE_VCPU after VGIC intialization produces the following calls:
> 
> kvm_vm_ioctl_create_vcpu
>  kvm_arch_vcpu_create
>   kvm_vcpu_init
>    kvm_arch_vcpu_init
>     kvm_vgic_vcpu_init
>      vgic_register_redist_iodev
>       kvm_vcpu_get_idx
>        ... no VCPU registered yet in kvm->vcpus :(
>        BUG();

in QEMU use case, kvm_vgic_vcpu_init/vgic_register_redist_iodev does
nothing since KVM_VGIC_V3_ADDR_TYPE_REDIST was not called yet and
vgic->vgic_redist_base is undefined. Thus we postpone the redist_iodev
registration until the redist base address is set.

In QEMU case KVM_VGIC_V3_ADDR_TYPE_REDIST is called when all the CPUs
are initialized and we were lucky.
> 
>  ... would later register vcpu:
>  kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu
> 
> My quick fix is to move kvm_vgic_vcpu_init into kvm_arch_vcpu_postcreate,
> but it discards the return value of kvm_vgic_vcpu_init, so I don't know
> how to do it properly.

changing the proto of kvm_arch_vcpu_postcreate and moving the
kvm_vgic_vcpu_init there could be an alternative.
By the way kvm_arch_vcpu_postcreate calls kvm_vgic_early_init() which
looks strange now that we do something clever before.

Thoughts?

Thanks

Eric
> 
> Thanks,
> Jean
> 
>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  include/kvm/arm_vgic.h              |  1 +
>>  virt/kvm/arm/arm.c                  |  2 +-
>>  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, 72 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..9f6f522a4b 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -335,7 +335,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>>  
>>  	kvm_arm_reset_debug_ptr(vcpu);
>>  
>> -	return 0;
>> +	return kvm_vgic_vcpu_init(vcpu);
>>  }
>>  
>>  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 168269b..99da1a2 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 + kvm_vcpu_get_idx(vcpu) * 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 2d53d7a..bb35078 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -397,12 +397,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);
>>
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

[PATCH v2 07/11] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Christoffer Dall]

On Tue, May 16, 2017 at 02:39:18PM +0200, Auger Eric wrote:
> Hi Jean, Christoffer,
> 
> On 16/05/2017 13:23, Jean-Philippe Brucker wrote:
> > Hi,
> > 
> > On 09/05/17 09:56, Christoffer Dall wrote:
> >> Instead of waiting with registering KVM iodevs until the first VCPU is
> >> 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 creating the VCPUs.
> > 
> > This triggers a BUG(), when the order is VGIC init, then VCPU init (which
> > is what kvmtool does).
> > 
> > Issuing KVM_CREATE_VCPU after VGIC intialization produces the following calls:
> > 
> > kvm_vm_ioctl_create_vcpu
> >  kvm_arch_vcpu_create
> >   kvm_vcpu_init
> >    kvm_arch_vcpu_init
> >     kvm_vgic_vcpu_init
> >      vgic_register_redist_iodev
> >       kvm_vcpu_get_idx
> >        ... no VCPU registered yet in kvm->vcpus :(
> >        BUG();
> 
> in QEMU use case, kvm_vgic_vcpu_init/vgic_register_redist_iodev does
> nothing since KVM_VGIC_V3_ADDR_TYPE_REDIST was not called yet and
> vgic->vgic_redist_base is undefined. Thus we postpone the redist_iodev
> registration until the redist base address is set.
> 
> In QEMU case KVM_VGIC_V3_ADDR_TYPE_REDIST is called when all the CPUs
> are initialized and we were lucky.
> > 
> >  ... would later register vcpu:
> >  kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu
> > 
> > My quick fix is to move kvm_vgic_vcpu_init into kvm_arch_vcpu_postcreate,
> > but it discards the return value of kvm_vgic_vcpu_init, so I don't know
> > how to do it properly.
> 
> changing the proto of kvm_arch_vcpu_postcreate and moving the
> kvm_vgic_vcpu_init there could be an alternative.

I think the whole point of postcreate is a hook that can be called where
it doesn't produce an error (rolling back the create at that point is
pretty horrid).

I'll have a closer look in the morning at what we can do - perhaps the
idx thing is just a ridiculous requirement and we can do something more
clever.

Thanks for the heads up, and sorry about breaking stuff.

-Christoffer

[PATCH v2 07/11] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Christoffer Dall]

Hi Jean,

On Tue, May 16, 2017 at 12:23:52PM +0100, Jean-Philippe Brucker wrote:
> Hi,
> 
> On 09/05/17 09:56, Christoffer Dall wrote:
> > Instead of waiting with registering KVM iodevs until the first VCPU is
> > 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 creating the VCPUs.
> 
> This triggers a BUG(), when the order is VGIC init, then VCPU init (which
> is what kvmtool does).
> 
> Issuing KVM_CREATE_VCPU after VGIC intialization produces the following calls:
> 
> kvm_vm_ioctl_create_vcpu
>  kvm_arch_vcpu_create
>   kvm_vcpu_init
>    kvm_arch_vcpu_init
>     kvm_vgic_vcpu_init
>      vgic_register_redist_iodev
>       kvm_vcpu_get_idx
>        ... no VCPU registered yet in kvm->vcpus :(
>        BUG();
> 
>  ... would later register vcpu:
>  kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu
> 
> My quick fix is to move kvm_vgic_vcpu_init into kvm_arch_vcpu_postcreate,
> but it discards the return value of kvm_vgic_vcpu_init, so I don't know
> how to do it properly.
> 

Would you mind giving this patch a go with your setup?

commit 7370dc8eefc9004923c2454c2f01c49850c8d94b (HEAD -> vcpu_idx_redist_bugfix)
Author: Christoffer Dall <cdall@linaro.org>
Date:   Wed May 17 13:12:51 2017 +0200

    KVM: arm/arm64: Fix bug when registering redist iodevs
    
    If userspace creates the VCPUs after initializing the VGIC, then we end
    up in a situation where we trigger a bug in kvm_vcpu_get_idx(), because
    it is called prior to adding the VCPU into the vcpus array on the VM.
    
    There is no tight coupling between the VCPU index and the area of the
    redistributor region used for the VCPU, so we can simply ensure that all
    creations of redistributors are serialized per VM, and increment an
    offset when we successfully add a redistributor.
    
    The vgic_register_redist_iodev() function can be called from two paths:
    vgic_redister_all_redist_iodev() which is called via the kvm_vgic_addr()
    device attribute handler.  This patch already holds the kvm->lock mutex.
    
    The other path is via kvm_vgic_vcpu_init, which is called through a
    longer chain from kvm_vm_ioctl_create_vcpu(), which releases the
    kvm->lock mutex just before calling kvm_arch_vcpu_create(), so we can
    simply take this mutex again later for our purposes.
    
    Signed-off-by: Christoffer Dall <cdall@linaro.org>

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 97b8d37..2304aeb 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -195,9 +195,13 @@ struct vgic_dist {
 		/* either a GICv2 CPU interface */
 		gpa_t			vgic_cpu_base;
 		/* or a number of GICv3 redistributor regions */
-		gpa_t			vgic_redist_base;
+		struct {
+			gpa_t		vgic_redist_base;
+			gpa_t		vgic_redist_free_offset;
+		};
 	};
 
+
 	/* distributor enabled */
 	bool			enabled;
 
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index dc68e2e..3a0b899 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -242,8 +242,11 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 	 * 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)
+	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+		mutex_lock(&vcpu->kvm->lock);
 		ret = vgic_register_redist_iodev(vcpu);
+		mutex_unlock(&vcpu->kvm->lock);
+	}
 	return ret;
 }
 
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 99da1a2..9b0f681 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 + kvm_vcpu_get_idx(vcpu) * SZ_64K * 2;
+	rd_base = vgic->vgic_redist_base + vgic->vgic_redist_free_offset;
 	sgi_base = rd_base + SZ_64K;
 
 	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
@@ -615,11 +615,14 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
 				      SZ_64K, &sgi_dev->dev);
 	mutex_unlock(&kvm->slots_lock);
-	if (ret)
+	if (ret) {
 		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
 					  &rd_dev->dev);
+		return ret;
+	}
 
-	return ret;
+	vgic->vgic_redist_free_offset += 2 * SZ_64K;
+	return 0;
 }
 
 static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)


Thanks,
-Christoffer

[PATCH v2 07/11] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Jean-Philippe Brucker]

On 17/05/17 12:18, Christoffer Dall wrote:
> Hi Jean,
> 
> On Tue, May 16, 2017 at 12:23:52PM +0100, Jean-Philippe Brucker wrote:
>> Hi,
>>
>> On 09/05/17 09:56, Christoffer Dall wrote:
>>> Instead of waiting with registering KVM iodevs until the first VCPU is
>>> 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 creating the VCPUs.
>>
>> This triggers a BUG(), when the order is VGIC init, then VCPU init (which
>> is what kvmtool does).
>>
>> Issuing KVM_CREATE_VCPU after VGIC intialization produces the following calls:
>>
>> kvm_vm_ioctl_create_vcpu
>>  kvm_arch_vcpu_create
>>   kvm_vcpu_init
>>    kvm_arch_vcpu_init
>>     kvm_vgic_vcpu_init
>>      vgic_register_redist_iodev
>>       kvm_vcpu_get_idx
>>        ... no VCPU registered yet in kvm->vcpus :(
>>        BUG();
>>
>>  ... would later register vcpu:
>>  kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu
>>
>> My quick fix is to move kvm_vgic_vcpu_init into kvm_arch_vcpu_postcreate,
>> but it discards the return value of kvm_vgic_vcpu_init, so I don't know
>> how to do it properly.
>>
> 
> Would you mind giving this patch a go with your setup?

Yes, this patch works with kvmtool. If it helps:

Tested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>

> commit 7370dc8eefc9004923c2454c2f01c49850c8d94b (HEAD -> vcpu_idx_redist_bugfix)
> Author: Christoffer Dall <cdall@linaro.org>
> Date:   Wed May 17 13:12:51 2017 +0200
> 
>     KVM: arm/arm64: Fix bug when registering redist iodevs
>     
>     If userspace creates the VCPUs after initializing the VGIC, then we end
>     up in a situation where we trigger a bug in kvm_vcpu_get_idx(), because
>     it is called prior to adding the VCPU into the vcpus array on the VM.
>     
>     There is no tight coupling between the VCPU index and the area of the
>     redistributor region used for the VCPU, so we can simply ensure that all
>     creations of redistributors are serialized per VM, and increment an
>     offset when we successfully add a redistributor.
>     
>     The vgic_register_redist_iodev() function can be called from two paths:
>     vgic_redister_all_redist_iodev() which is called via the kvm_vgic_addr()
>     device attribute handler.  This patch already holds the kvm->lock mutex.
>     
>     The other path is via kvm_vgic_vcpu_init, which is called through a
>     longer chain from kvm_vm_ioctl_create_vcpu(), which releases the
>     kvm->lock mutex just before calling kvm_arch_vcpu_create(), so we can
>     simply take this mutex again later for our purposes.
>     
>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 97b8d37..2304aeb 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -195,9 +195,13 @@ struct vgic_dist {
>  		/* either a GICv2 CPU interface */
>  		gpa_t			vgic_cpu_base;
>  		/* or a number of GICv3 redistributor regions */
> -		gpa_t			vgic_redist_base;
> +		struct {
> +			gpa_t		vgic_redist_base;
> +			gpa_t		vgic_redist_free_offset;
> +		};
>  	};
>  
> +

(whitespace change)

Thanks,
Jean

>  	/* distributor enabled */
>  	bool			enabled;
>  
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index dc68e2e..3a0b899 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -242,8 +242,11 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  	 * 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)
> +	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> +		mutex_lock(&vcpu->kvm->lock);
>  		ret = vgic_register_redist_iodev(vcpu);
> +		mutex_unlock(&vcpu->kvm->lock);
> +	}
>  	return ret;
>  }
>  
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 99da1a2..9b0f681 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 + kvm_vcpu_get_idx(vcpu) * SZ_64K * 2;
> +	rd_base = vgic->vgic_redist_base + vgic->vgic_redist_free_offset;
>  	sgi_base = rd_base + SZ_64K;
>  
>  	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
> @@ -615,11 +615,14 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
>  				      SZ_64K, &sgi_dev->dev);
>  	mutex_unlock(&kvm->slots_lock);
> -	if (ret)
> +	if (ret) {
>  		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
>  					  &rd_dev->dev);
> +		return ret;
> +	}
>  
> -	return ret;
> +	vgic->vgic_redist_free_offset += 2 * SZ_64K;
> +	return 0;
>  }
>  
>  static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
> 
> 
> Thanks,
> -Christoffer
> 

[PATCH v2 07/11] KVM: arm/arm64: Register iodevs when setting redist base and creating VCPUs

[Auger Eric]

Hi Christoffer,
On 17/05/2017 13:18, Christoffer Dall wrote:
> Hi Jean,
> 
> On Tue, May 16, 2017 at 12:23:52PM +0100, Jean-Philippe Brucker wrote:
>> Hi,
>>
>> On 09/05/17 09:56, Christoffer Dall wrote:
>>> Instead of waiting with registering KVM iodevs until the first VCPU is
>>> 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 creating the VCPUs.
>>
>> This triggers a BUG(), when the order is VGIC init, then VCPU init (which
>> is what kvmtool does).
>>
>> Issuing KVM_CREATE_VCPU after VGIC intialization produces the following calls:
>>
>> kvm_vm_ioctl_create_vcpu
>>  kvm_arch_vcpu_create
>>   kvm_vcpu_init
>>    kvm_arch_vcpu_init
>>     kvm_vgic_vcpu_init
>>      vgic_register_redist_iodev
>>       kvm_vcpu_get_idx
>>        ... no VCPU registered yet in kvm->vcpus :(
>>        BUG();
>>
>>  ... would later register vcpu:
>>  kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu
>>
>> My quick fix is to move kvm_vgic_vcpu_init into kvm_arch_vcpu_postcreate,
>> but it discards the return value of kvm_vgic_vcpu_init, so I don't know
>> how to do it properly.
>>
> 
> Would you mind giving this patch a go with your setup?
> 
> commit 7370dc8eefc9004923c2454c2f01c49850c8d94b (HEAD -> vcpu_idx_redist_bugfix)
> Author: Christoffer Dall <cdall@linaro.org>
> Date:   Wed May 17 13:12:51 2017 +0200
> 
>     KVM: arm/arm64: Fix bug when registering redist iodevs
>     
>     If userspace creates the VCPUs after initializing the VGIC, then we end
>     up in a situation where we trigger a bug in kvm_vcpu_get_idx(), because
>     it is called prior to adding the VCPU into the vcpus array on the VM.
>     
>     There is no tight coupling between the VCPU index and the area of the
>     redistributor region used for the VCPU, so we can simply ensure that all
>     creations of redistributors are serialized per VM, and increment an
>     offset when we successfully add a redistributor.
>     
>     The vgic_register_redist_iodev() function can be called from two paths:
>     vgic_redister_all_redist_iodev() which is called via the kvm_vgic_addr()
>     device attribute handler.  This patch already holds the kvm->lock mutex.
>     
>     The other path is via kvm_vgic_vcpu_init, which is called through a
>     longer chain from kvm_vm_ioctl_create_vcpu(), which releases the
>     kvm->lock mutex just before calling kvm_arch_vcpu_create(), so we can
>     simply take this mutex again later for our purposes.
>     
>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 97b8d37..2304aeb 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -195,9 +195,13 @@ struct vgic_dist {
>  		/* either a GICv2 CPU interface */
>  		gpa_t			vgic_cpu_base;
>  		/* or a number of GICv3 redistributor regions */
> -		gpa_t			vgic_redist_base;
> +		struct {
> +			gpa_t		vgic_redist_base;
> +			gpa_t		vgic_redist_free_offset;
> +		};
>  	};
>  
> +
>  	/* distributor enabled */
>  	bool			enabled;
>  
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index dc68e2e..3a0b899 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -242,8 +242,11 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  	 * 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)
> +	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> +		mutex_lock(&vcpu->kvm->lock);
>  		ret = vgic_register_redist_iodev(vcpu);
> +		mutex_unlock(&vcpu->kvm->lock);
> +	}
>  	return ret;
>  }
>  
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 99da1a2..9b0f681 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 + kvm_vcpu_get_idx(vcpu) * SZ_64K * 2;
> +	rd_base = vgic->vgic_redist_base + vgic->vgic_redist_free_offset;
>  	sgi_base = rd_base + SZ_64K;
>  
>  	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
> @@ -615,11 +615,14 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
>  				      SZ_64K, &sgi_dev->dev);
>  	mutex_unlock(&kvm->slots_lock);
> -	if (ret)
> +	if (ret) {
>  		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
>  					  &rd_dev->dev);
Not related to this patch but I noticed kvm_io_bus_unregister_dev must
also be called with slots_lock held.

Also true in vgic_unregister_redist_iodev().

Then on this very patch file, looks good to me.

Thanks

Eric

> +		return ret;
> +	}
>  
> -	return ret;
> +	vgic->vgic_redist_free_offset += 2 * SZ_64K;
> +	return 0;
>  }
>  
>  static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
> 
> 
> Thanks,
> -Christoffer
> 

[PATCH v2 08/11] KVM: arm/arm64: Get rid of its->initialized field

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

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>
Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 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 4ff65ef..bfde6fb 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 9f7105c..18318c6 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;
 
@@ -2397,8 +2393,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.9.0

[PATCH v2 08/11] KVM: arm/arm64: Get rid of its->initialized field

[Auger Eric]

Hi,

On 09/05/2017 10:56, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> 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>
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  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 4ff65ef..bfde6fb 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 9f7105c..18318c6 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;
>  
> @@ -2397,8 +2393,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);
> 

[PATCH v2 09/11] 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 the VCPUs have 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.

  [ Code to fix concurrency issues when setting the ITS base address and
    to fix the undef base address check written by Marc Zyngier ]

Signed-off-by: Christoffer Dall <cdall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 44 ++++++++++----------------------------------
 virt/kvm/arm/vgic/vgic-v3.c  |  8 --------
 virt/kvm/arm/vgic/vgic.h     |  1 -
 3 files changed, 10 insertions(+), 43 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 18318c6..89acaef 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;
@@ -2384,9 +2389,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 		if (ret)
 			return ret;
 
-		its->vgic_its_base = addr;
-
-		return 0;
+		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);
@@ -2462,30 +2465,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 bb35078..8fa737e 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -397,14 +397,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 v2 09/11] KVM: arm/arm64: Register ITS iodev when setting base address

[Auger Eric]

Hi,

On 09/05/2017 10:56, 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 the VCPUs have 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.
> 
>   [ Code to fix concurrency issues when setting the ITS base address and
>     to fix the undef base address check written by Marc Zyngier ]
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

thanks

Eric
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 44 ++++++++++----------------------------------
>  virt/kvm/arm/vgic/vgic-v3.c  |  8 --------
>  virt/kvm/arm/vgic/vgic.h     |  1 -
>  3 files changed, 10 insertions(+), 43 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 18318c6..89acaef 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;
> @@ -2384,9 +2389,7 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>  		if (ret)
>  			return ret;
>  
> -		its->vgic_its_base = addr;
> -
> -		return 0;
> +		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);
> @@ -2462,30 +2465,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 bb35078..8fa737e 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -397,14 +397,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 v2 10/11] 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 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 89acaef..9aeaff0 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2308,20 +2308,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 v2 11/11] 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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 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 9aeaff0..2dff288 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1613,13 +1613,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_device(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;
 
 	/*
@@ -1630,19 +1637,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_device(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);
 
@@ -2012,8 +2019,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_device(its->dev->kvm, dev);
 		return ret;
+	}
 
 	return offset;
 }
-- 
2.9.0

[PATCH v2 00/11] Fixes to v7 of the vITS save/restore series

[Auger Eric]

Hi Christoffer,

On 09/05/2017 10:56, Christoffer Dall wrote:
> 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-v2
I tested ITS migration series v7 + this version on Cavium Thunderx and
it works fine for me.

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

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
> Changes since v1:
>  - We got rid of the requirement to initialize the ITS (it doesn't do
>    anything).
>  - We fixed a race condition with setting the ITS base address which was
>    introduced in v1 of this series.
>  - We reworded some commit messages based on Eric's comments
>  - We fixed the address check to work with partically unset base addrs
>  - We use the vcpu index instead of the vcpu id for allocating redist
>    regions.
>  - Some renames and code cleanups.
> 
> Christoffer Dall (10):
>   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: Add kvm_vcpu_get_idx to get vcpu index in kvm->vcpus
>   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: Don't call map_resources when restoring ITS tables
>   KVM: arm/arm64: vgic-its: Cleanup after failed ITT restore
> 
> Marc Zyngier (1):
>   KVM: arm/arm64: Get rid of its->initialized field
> 
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt |  23 ++--
>  include/kvm/arm_vgic.h                             |   2 +-
>  include/linux/kvm_host.h                           |  11 ++
>  virt/kvm/arm/arm.c                                 |   2 +-
>  virt/kvm/arm/vgic/vgic-init.c                      |  25 +++-
>  virt/kvm/arm/vgic/vgic-its.c                       |  96 +++++---------
>  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                        |  33 +++--
>  virt/kvm/arm/vgic/vgic.h                           |   5 +-
>  10 files changed, 222 insertions(+), 151 deletions(-)
>