[PATCH v6 0/9] KVM/ARM: Some vgic fixes and init sequence KVM selftests

[PATCH v6 0/9] KVM/ARM: Some vgic fixes and init sequence KVM selftests

[Eric Auger]

While writting vgic v3 init sequence KVM selftests I noticed some
relatively minor issues. This was also the opportunity to try to
fix the issue laterly reported by Zenghui, related to the RDIST_TYPER
last bit emulation. The final patch is a first batch of VGIC init
sequence selftests. Of course they can be augmented with a lot more
register access tests, but let's try to move forward incrementally ...

Best Regards

Eric

This series can be found at:
https://github.com/eauger/linux/tree/vgic_kvmselftests_v6

History:

v5 -> v6:
- Fix the note in "KVM: arm64: vgic-v3: Expose
  GICR_TYPER.Last for userspace", confirming the rdist
  region list is sorted by index and not by base address.
- Properly send 9 patches  :-/

v4 -> v5:
- rewrite the last bit detection according to Marc's
  interpretation of the spec and modify the kvm selftests
  accordingly
v3 -> v4:
- take into account Drew's comment on the kvm selftests. No
  change to the KVM related patches compared to v3
v2 ->v3:
- reworked last bit read accessor to handle contiguous redist
  regions and rdist not registered in ascending order
- removed [PATCH 5/9] KVM: arm: move has_run_once after the
  map_resources
v1 -> v2:
- Took into account all comments from Marc and Alexandru's except
  the has_run_once still after the map_resources (this would oblige
  me to revisit in depth the selftests)


Eric Auger (9):
  KVM: arm64: vgic-v3: Fix some error codes when setting RDIST base
  KVM: arm64: Fix KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION read
  KVM: arm64: vgic-v3: Fix error handling in vgic_v3_set_redist_base()
  KVM: arm/arm64: vgic: Reset base address on kvm_vgic_dist_destroy()
  docs: kvm: devices/arm-vgic-v3: enhance KVM_DEV_ARM_VGIC_CTRL_INIT doc
  KVM: arm64: Simplify argument passing to vgic_uaccess_[read|write]
  kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
  KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
  KVM: selftests: aarch64/vgic-v3 init sequence tests

 .../virt/kvm/devices/arm-vgic-v3.rst          |   2 +-
 arch/arm64/kvm/vgic/vgic-init.c               |  12 +-
 arch/arm64/kvm/vgic/vgic-kvm-device.c         |   3 +
 arch/arm64/kvm/vgic/vgic-mmio-v3.c            |  93 +--
 arch/arm64/kvm/vgic/vgic-mmio.c               |  10 +-
 arch/arm64/kvm/vgic/vgic.h                    |   1 +
 include/kvm/arm_vgic.h                        |   1 +
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/aarch64/vgic_init.c | 585 ++++++++++++++++++
 .../testing/selftests/kvm/include/kvm_util.h  |   9 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  77 +++
 12 files changed, 746 insertions(+), 49 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c

-- 
2.26.3

[PATCH v6 1/9] KVM: arm64: vgic-v3: Fix some error codes when setting RDIST base

[Eric Auger]

KVM_DEV_ARM_VGIC_GRP_ADDR group doc says we should return
-EEXIST in case the base address of the redist is already set.
We currently return -EINVAL.

However we need to return -EINVAL in case a legacy REDIST address
is attempted to be set while REDIST_REGIONS were set. This case
is discriminated by looking at the count field.

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

---
v3 -> v4:
- rewrite the test checking that we do not mix rdist region types

v1 -> v2:
- simplify the check sequence
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 15a6c98ee92f..ef9baf0d01b5 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -791,10 +791,6 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
 	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
 	int ret;
 
-	/* single rdist region already set ?*/
-	if (!count && !list_empty(rd_regions))
-		return -EINVAL;
-
 	/* cross the end of memory ? */
 	if (base + size < base)
 		return -EINVAL;
@@ -805,11 +801,15 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
 	} else {
 		rdreg = list_last_entry(rd_regions,
 					struct vgic_redist_region, list);
-		if (index != rdreg->index + 1)
+
+		/* Don't mix single region and discrete redist regions */
+		if (!count && rdreg->count)
 			return -EINVAL;
 
-		/* Cannot add an explicitly sized regions after legacy region */
-		if (!rdreg->count)
+		if (!count)
+			return -EEXIST;
+
+		if (index != rdreg->index + 1)
 			return -EINVAL;
 	}
 
-- 
2.26.3

[PATCH v6 2/9] KVM: arm64: Fix KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION read

[Eric Auger]

The doc says:
"The characteristics of a specific redistributor region can
 be read by presetting the index field in the attr data.
 Only valid for KVM_DEV_TYPE_ARM_VGIC_V3"

Unfortunately the existing code fails to read the input attr data.

Fixes: 04c110932225 ("KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION")
Cc: stable@vger.kernel.org#v4.17+
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

---

v1 -> v2:
- in the commit message, remove the statement that the index always is 0
- add Alexandru's R-b
---
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index 44419679f91a..2f66cf247282 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -226,6 +226,9 @@ static int vgic_get_common_attr(struct kvm_device *dev,
 		u64 addr;
 		unsigned long type = (unsigned long)attr->attr;
 
+		if (copy_from_user(&addr, uaddr, sizeof(addr)))
+			return -EFAULT;
+
 		r = kvm_vgic_addr(dev->kvm, type, &addr, false);
 		if (r)
 			return (r == -ENODEV) ? -ENXIO : r;
-- 
2.26.3

[PATCH v6 3/9] KVM: arm64: vgic-v3: Fix error handling in vgic_v3_set_redist_base()

[Eric Auger]

vgic_v3_insert_redist_region() may succeed while
vgic_register_all_redist_iodevs fails. For example this happens
while adding a redistributor region overlapping a dist region. The
failure only is detected on vgic_register_all_redist_iodevs when
vgic_v3_check_base() gets called in vgic_register_redist_iodev().

In such a case, remove the newly added redistributor region and free
it.

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

---

v1 -> v2:
- fix the commit message and split declaration/assignment of rdreg
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index ef9baf0d01b5..fec0555529c0 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -861,8 +861,14 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count)
 	 * afterwards will register the iodevs when needed.
 	 */
 	ret = vgic_register_all_redist_iodevs(kvm);
-	if (ret)
+	if (ret) {
+		struct vgic_redist_region *rdreg;
+
+		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
+		list_del(&rdreg->list);
+		kfree(rdreg);
 		return ret;
+	}
 
 	return 0;
 }
-- 
2.26.3

[PATCH v6 4/9] KVM: arm/arm64: vgic: Reset base address on kvm_vgic_dist_destroy()

[Eric Auger]

On vgic_dist_destroy(), the addresses are not reset. However for
kvm selftest purpose this would allow to continue the test execution
even after a failure when running KVM_RUN. So let's reset the
base addresses.

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

---

v1 -> v2:
- use dist-> in the else and add braces
---
 arch/arm64/kvm/vgic/vgic-init.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 052917deb149..cf6faa0aeddb 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -335,13 +335,16 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 	kfree(dist->spis);
 	dist->spis = NULL;
 	dist->nr_spis = 0;
+	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
 
-	if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
 		list_for_each_entry_safe(rdreg, next, &dist->rd_regions, list) {
 			list_del(&rdreg->list);
 			kfree(rdreg);
 		}
 		INIT_LIST_HEAD(&dist->rd_regions);
+	} else {
+		dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
 	}
 
 	if (vgic_has_its(kvm))
@@ -362,6 +365,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 	vgic_flush_pending_lpis(vcpu);
 
 	INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
+	vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
 }
 
 /* To be called with kvm->lock held */
-- 
2.26.3

[PATCH v6 5/9] docs: kvm: devices/arm-vgic-v3: enhance KVM_DEV_ARM_VGIC_CTRL_INIT doc

[Eric Auger]

kvm_arch_vcpu_precreate() returns -EBUSY if the vgic is
already initialized. So let's document that KVM_DEV_ARM_VGIC_CTRL_INIT
must be called after all vcpu creations.

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

---

v1 -> v2:
- Must be called after all vcpu creations ->
  Must be called after all VCPUs have been created as per
  Alexandru's suggestion
---
 Documentation/virt/kvm/devices/arm-vgic-v3.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/devices/arm-vgic-v3.rst b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
index 5dd3bff51978..51e5e5762571 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-v3.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
@@ -228,7 +228,7 @@ Groups:
 
     KVM_DEV_ARM_VGIC_CTRL_INIT
       request the initialization of the VGIC, no additional parameter in
-      kvm_device_attr.addr.
+      kvm_device_attr.addr. Must be called after all VCPUs have been created.
     KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES
       save all LPI pending bits into guest RAM pending tables.
 
-- 
2.26.3

[PATCH v6 6/9] KVM: arm64: Simplify argument passing to vgic_uaccess_[read|write]

[Eric Auger]

vgic_uaccess() takes a struct vgic_io_device argument, converts it
to a struct kvm_io_device and passes it to the read/write accessor
functions, which convert it back to a struct vgic_io_device.
Avoid the indirection by passing the struct vgic_io_device argument
directly to vgic_uaccess_{read,write}.

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

---

v1 -> v2:
- reworded the commit message as suggested by Alexandru
---
 arch/arm64/kvm/vgic/vgic-mmio.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index b2d73fc0d1ef..48c6067fc5ec 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -938,10 +938,9 @@ vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev,
 	return region;
 }
 
-static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev,
 			     gpa_t addr, u32 *val)
 {
-	struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
 	const struct vgic_register_region *region;
 	struct kvm_vcpu *r_vcpu;
 
@@ -960,10 +959,9 @@ static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 	return 0;
 }
 
-static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev,
 			      gpa_t addr, const u32 *val)
 {
-	struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
 	const struct vgic_register_region *region;
 	struct kvm_vcpu *r_vcpu;
 
@@ -986,9 +984,9 @@ int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
 		 bool is_write, int offset, u32 *val)
 {
 	if (is_write)
-		return vgic_uaccess_write(vcpu, &dev->dev, offset, val);
+		return vgic_uaccess_write(vcpu, dev, offset, val);
 	else
-		return vgic_uaccess_read(vcpu, &dev->dev, offset, val);
+		return vgic_uaccess_read(vcpu, dev, offset, val);
 }
 
 static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
-- 
2.26.3

[PATCH v6 7/9] kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()

[Eric Auger]

To improve the readability, we introduce the new
vgic_v3_free_redist_region helper and also rename
vgic_v3_insert_redist_region into vgic_v3_alloc_redist_region

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arch/arm64/kvm/vgic/vgic-init.c    |  6 ++----
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 17 +++++++++++------
 arch/arm64/kvm/vgic/vgic.h         |  1 +
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index cf6faa0aeddb..58cbda00e56d 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -338,10 +338,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
 
 	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
-		list_for_each_entry_safe(rdreg, next, &dist->rd_regions, list) {
-			list_del(&rdreg->list);
-			kfree(rdreg);
-		}
+		list_for_each_entry_safe(rdreg, next, &dist->rd_regions, list)
+			vgic_v3_free_redist_region(rdreg);
 		INIT_LIST_HEAD(&dist->rd_regions);
 	} else {
 		dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index fec0555529c0..e1ed0c5a8eaa 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -768,7 +768,7 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm)
 }
 
 /**
- * vgic_v3_insert_redist_region - Insert a new redistributor region
+ * vgic_v3_alloc_redist_region - Allocate a new redistributor region
  *
  * Performs various checks before inserting the rdist region in the list.
  * Those tests depend on whether the size of the rdist region is known
@@ -782,8 +782,8 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm)
  *
  * Return 0 on success, < 0 otherwise
  */
-static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
-					gpa_t base, uint32_t count)
+static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
+				       gpa_t base, uint32_t count)
 {
 	struct vgic_dist *d = &kvm->arch.vgic;
 	struct vgic_redist_region *rdreg;
@@ -848,11 +848,17 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
 	return ret;
 }
 
+void vgic_v3_free_redist_region(struct vgic_redist_region *rdreg)
+{
+	list_del(&rdreg->list);
+	kfree(rdreg);
+}
+
 int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count)
 {
 	int ret;
 
-	ret = vgic_v3_insert_redist_region(kvm, index, addr, count);
+	ret = vgic_v3_alloc_redist_region(kvm, index, addr, count);
 	if (ret)
 		return ret;
 
@@ -865,8 +871,7 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count)
 		struct vgic_redist_region *rdreg;
 
 		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
-		list_del(&rdreg->list);
-		kfree(rdreg);
+		vgic_v3_free_redist_region(rdreg);
 		return ret;
 	}
 
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 64fcd7511110..bc418c2c1214 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -293,6 +293,7 @@ vgic_v3_rd_region_size(struct kvm *kvm, struct vgic_redist_region *rdreg)
 
 struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
 							   u32 index);
+void vgic_v3_free_redist_region(struct vgic_redist_region *rdreg);
 
 bool vgic_v3_rdist_overlap(struct kvm *kvm, gpa_t base, size_t size);
 
-- 
2.26.3

[PATCH v6 8/9] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace

[Eric Auger]

Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the
reporting of GICR_TYPER.Last for userspace") temporarily fixed
a bug identified when attempting to access the GICR_TYPER
register before the redistributor region setting, but dropped
the support of the LAST bit.

Emulating the GICR_TYPER.Last bit still makes sense for
architecture compliance though. This patch restores its support
(if the redistributor region was set) while keeping the code safe.

We introduce a new helper, vgic_mmio_vcpu_rdist_is_last() which
computes whether a redistributor is the highest one of a series
of redistributor contributor pages.

With this new implementation we do not need to have a uaccess
read accessor anymore.

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

---

v4 -> v5:
- change the implementation according to Marc's understanding of
  the spec
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 58 +++++++++++++++++-------------
 include/kvm/arm_vgic.h             |  1 +
 2 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index e1ed0c5a8eaa..03a253785700 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -251,45 +251,52 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		vgic_enable_lpis(vcpu);
 }
 
+static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu)
+{
+	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	struct vgic_redist_region *iter, *rdreg = vgic_cpu->rdreg;
+
+	if (!rdreg)
+		return false;
+
+	if (vgic_cpu->rdreg_index < rdreg->free_index - 1) {
+		return false;
+	} else if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) {
+		struct list_head *rd_regions = &vgic->rd_regions;
+		gpa_t end = rdreg->base + rdreg->count * KVM_VGIC_V3_REDIST_SIZE;
+
+		/*
+		 * the rdist is the last one of the redist region,
+		 * check whether there is no other contiguous rdist region
+		 */
+		list_for_each_entry(iter, rd_regions, list) {
+			if (iter->base == end && iter->free_index > 0)
+				return false;
+		}
+	}
+	return true;
+}
+
 static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
 					      gpa_t addr, unsigned int len)
 {
 	unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
-	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
 	int target_vcpu_id = vcpu->vcpu_id;
-	gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
-			(rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
 	u64 value;
 
 	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
 	value |= ((target_vcpu_id & 0xffff) << 8);
 
-	if (addr == last_rdist_typer)
+	if (vgic_has_its(vcpu->kvm))
+		value |= GICR_TYPER_PLPIS;
+
+	if (vgic_mmio_vcpu_rdist_is_last(vcpu))
 		value |= GICR_TYPER_LAST;
-	if (vgic_has_its(vcpu->kvm))
-		value |= GICR_TYPER_PLPIS;
 
 	return extract_bytes(value, addr & 7, len);
 }
 
-static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
-						 gpa_t addr, unsigned int len)
-{
-	unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
-	int target_vcpu_id = vcpu->vcpu_id;
-	u64 value;
-
-	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
-	value |= ((target_vcpu_id & 0xffff) << 8);
-
-	if (vgic_has_its(vcpu->kvm))
-		value |= GICR_TYPER_PLPIS;
-
-	/* reporting of the Last bit is not supported for userspace */
-	return extract_bytes(value, addr & 7, len);
-}
-
 static unsigned long vgic_mmio_read_v3r_iidr(struct kvm_vcpu *vcpu,
 					     gpa_t addr, unsigned int len)
 {
@@ -612,7 +619,7 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_TYPER,
 		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi,
-		vgic_uaccess_read_v3r_typer, vgic_mmio_uaccess_write_wi, 8,
+		NULL, vgic_mmio_uaccess_write_wi, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
 		vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
@@ -714,6 +721,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 		return -EINVAL;
 
 	vgic_cpu->rdreg = rdreg;
+	vgic_cpu->rdreg_index = rdreg->free_index;
 
 	rd_base = rdreg->base + rdreg->free_index * KVM_VGIC_V3_REDIST_SIZE;
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 3d74f1060bd1..ec621180ef09 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -322,6 +322,7 @@ struct vgic_cpu {
 	 */
 	struct vgic_io_device	rd_iodev;
 	struct vgic_redist_region *rdreg;
+	u32 rdreg_index;
 
 	/* Contains the attributes and gpa of the LPI pending tables. */
 	u64 pendbaser;
-- 
2.26.3

[PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests

[Eric Auger]

The tests exercise the VGIC_V3 device creation including the
associated KVM_DEV_ARM_VGIC_GRP_ADDR group attributes:

- KVM_VGIC_V3_ADDR_TYPE_DIST/REDIST
- KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

Some other tests dedicate to KVM_DEV_ARM_VGIC_GRP_REDIST_REGS group
and especially the GICR_TYPER read. The goal was to test the case
recently fixed by commit 23bde34771f1
("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace").

The API under test can be found at
Documentation/virt/kvm/devices/arm-vgic-v3.rst

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

---

v4 -> v5:
- simplify the last bit tests given the simpler interpretation
  of the spec

v3 -> v4:
- update .gitignore
- More vgic-mmio-v3.c change into the previous patch
- rename fuzz_dist_rdist into test_dist_rdist
- cleanup in run_vcpu and guest_code
- max_ipa_bits is global
- s/fuzz/subtest
- added test_kvm_device,
- moved ucall_init() just before the cpu run
- use vm_create_default_with_vcpus
- use vm_gic struct, vm_gic_create, vm_gic_destroy
- revwrite util.c helpers to comply with the usual style
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/aarch64/vgic_init.c | 585 ++++++++++++++++++
 .../testing/selftests/kvm/include/kvm_util.h  |   9 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  77 +++
 5 files changed, 673 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 7bd7e776c266..bb862f91f640 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 /aarch64/get-reg-list
 /aarch64/get-reg-list-sve
+/aarch64/vgic_init
 /s390x/memop
 /s390x/resets
 /s390x/sync_regs_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 67eebb53235f..2fd4801de9ca 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -78,6 +78,7 @@ TEST_GEN_PROGS_x86_64 += steal_time
 
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
+TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += demand_paging_test
 TEST_GEN_PROGS_aarch64 += dirty_log_test
 TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
new file mode 100644
index 000000000000..be1a7c0d0527
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -0,0 +1,585 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * vgic init sequence tests
+ *
+ * Copyright (C) 2020, Red Hat, Inc.
+ */
+#define _GNU_SOURCE
+#include <linux/kernel.h>
+#include <sys/syscall.h>
+#include <asm/kvm.h>
+#include <asm/kvm_para.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+
+#define NR_VCPUS		4
+
+#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) (((uint64_t)(count) << 52) | \
+	((uint64_t)((base) >> 16) << 16) | ((uint64_t)(flags) << 12) | index)
+#define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
+
+#define GICR_TYPER 0x8
+
+struct vm_gic {
+	struct kvm_vm *vm;
+	int gic_fd;
+};
+
+int max_ipa_bits;
+
+/* helper to access a redistributor register */
+static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
+			     uint32_t *val, bool write)
+{
+	uint64_t attr = REG_OFFSET(vcpu, offset);
+
+	return _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS,
+				  attr, val, write);
+}
+
+/* dummy guest code */
+static void guest_code(void)
+{
+	GUEST_SYNC(0);
+	GUEST_SYNC(1);
+	GUEST_SYNC(2);
+	GUEST_DONE();
+}
+
+/* we don't want to assert on run execution, hence that helper */
+static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
+{
+	int ret;
+
+	vcpu_args_set(vm, vcpuid, 1);
+	ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
+	get_ucall(vm, vcpuid, NULL);
+
+	if (ret)
+		return -errno;
+	return 0;
+}
+
+static struct vm_gic vm_gic_create(void)
+{
+	struct vm_gic v;
+
+	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
+	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
+
+	return v;
+}
+
+static void vm_gic_destroy(struct vm_gic *v)
+{
+	close(v->gic_fd);
+	kvm_vm_free(v->vm);
+}
+
+/**
+ * Helper routine that performs KVM device tests in general and
+ * especially ARM_VGIC_V3 ones. Eventually the ARM_VGIC_V3
+ * device gets created, a legacy RDIST region is set at @0x0
+ * and a DIST region is set @0x60000
+ */
+static void subtest_dist_rdist(struct vm_gic *v)
+{
+	int ret;
+	uint64_t addr;
+
+	/* Check existing group/attributes */
+	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				    KVM_VGIC_V3_ADDR_TYPE_DIST);
+	TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_DIST supported");
+
+	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				    KVM_VGIC_V3_ADDR_TYPE_REDIST);
+	TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST supported");
+
+	/* check non existing attribute */
+	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
+	TEST_ASSERT(ret == -ENXIO, "attribute not supported");
+
+	/* misaligned DIST and REDIST address settings */
+	addr = 0x1000;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "GICv3 dist base not 64kB aligned");
+
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "GICv3 redist base not 64kB aligned");
+
+	/* out of range address */
+	if (max_ipa_bits) {
+		addr = 1ULL << max_ipa_bits;
+		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+					 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
+		TEST_ASSERT(ret == -E2BIG, "dist address beyond IPA limit");
+
+		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+					 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+		TEST_ASSERT(ret == -E2BIG, "redist address beyond IPA limit");
+	}
+
+	/* set REDIST base address @0x0*/
+	addr = 0x00000;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+	TEST_ASSERT(!ret, "GICv3 redist base set");
+
+	/* Attempt to create a second legacy redistributor region */
+	addr = 0xE0000;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+	TEST_ASSERT(ret == -EEXIST, "GICv3 redist base set again");
+
+	/* Attempt to mix legacy and new redistributor regions */
+	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "attempt to mix GICv3 REDIST and REDIST_REGION");
+
+	/*
+	 * Set overlapping DIST / REDIST, cannot be detected here. Will be detected
+	 * on first vcpu run instead.
+	 */
+	addr = 3 * 2 * 0x10000;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_DIST,
+				 &addr, true);
+	TEST_ASSERT(!ret, "dist overlapping rdist");
+}
+
+/* Test the new REDIST region API */
+static void subtest_redist_regions(struct vm_gic *v)
+{
+	uint64_t addr, expected_addr;
+	int ret;
+
+	ret = kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				     KVM_VGIC_V3_ADDR_TYPE_REDIST);
+	TEST_ASSERT(!ret, "Multiple redist regions advertised");
+
+	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 2, 0);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "redist region attr value with flags != 0");
+
+	addr = REDIST_REGION_ATTR_ADDR(0, 0x100000, 0, 0);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "redist region attr value with count== 0");
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 1);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "attempt to register the first rdist region with index != 0");
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x201000, 0, 1);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "rdist region with misaligned address");
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "First valid redist region with 2 rdist @ 0x200000, index 0");
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 1);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "register an rdist region with already used index");
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 0x210000, 0, 2);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "register an rdist region overlapping with another one");
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 2);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "register redist region with index not +1");
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 1);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "register valid redist region with 1 rdist @ 0x220000, index 1");
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 1ULL << max_ipa_bits, 0, 2);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -E2BIG, "register redist region with base address beyond IPA range");
+
+	addr = 0x260000;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "Mix KVM_VGIC_V3_ADDR_TYPE_REDIST and REDIST_REGION");
+
+	/*
+	 * Now there are 2 redist regions:
+	 * region 0 @ 0x200000 2 redists
+	 * region 1 @ 0x240000 1 redist
+	 * Attempt to read their characteristics
+	 */
+
+	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 0);
+	expected_addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
+	TEST_ASSERT(!ret && addr == expected_addr, "read characteristics of region #0");
+
+	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 1);
+	expected_addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 1);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
+	TEST_ASSERT(!ret && addr == expected_addr, "read characteristics of region #1");
+
+	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 2);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
+	TEST_ASSERT(ret == -ENOENT, "read characteristics of non existing region");
+
+	addr = 0x260000;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
+	TEST_ASSERT(!ret, "set dist region");
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 0x260000, 0, 2);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "register redist region colliding with dist");
+}
+
+/*
+ * VGIC KVM device is created and initialized before the secondary CPUs
+ * get created
+ */
+static void test_vgic_then_vcpus(void)
+{
+	struct vm_gic v;
+	int ret, i;
+
+	v.vm = vm_create_default(0, 0, guest_code);
+	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
+
+	subtest_dist_rdist(&v);
+
+	/* Add the rest of the VCPUs */
+	for (i = 1; i < NR_VCPUS; ++i)
+		vm_vcpu_add_default(v.vm, i, guest_code);
+
+	ucall_init(v.vm, NULL);
+	ret = run_vcpu(v.vm, 3);
+	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
+
+	vm_gic_destroy(&v);
+}
+
+/* All the VCPUs are created before the VGIC KVM device gets initialized */
+static void test_vcpus_then_vgic(void)
+{
+	struct vm_gic v;
+	int ret;
+
+	v = vm_gic_create();
+
+	subtest_dist_rdist(&v);
+
+	ucall_init(v.vm, NULL);
+	ret = run_vcpu(v.vm, 3);
+	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
+
+	vm_gic_destroy(&v);
+}
+
+static void test_new_redist_regions(void)
+{
+	void *dummy = NULL;
+	struct vm_gic v;
+	uint64_t addr;
+	int ret;
+
+	v = vm_gic_create();
+	subtest_redist_regions(&v);
+	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+	TEST_ASSERT(!ret, "init the vgic");
+
+	ucall_init(v.vm, NULL);
+	ret = run_vcpu(v.vm, 3);
+	TEST_ASSERT(ret == -ENXIO, "running without sufficient number of rdists");
+	vm_gic_destroy(&v);
+
+	/* step2 */
+
+	v = vm_gic_create();
+	subtest_redist_regions(&v);
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
+	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "register a third region allowing to cover the 4 vcpus");
+
+	ucall_init(v.vm, NULL);
+	ret = run_vcpu(v.vm, 3);
+	TEST_ASSERT(ret == -EBUSY, "running without vgic explicit init");
+
+	vm_gic_destroy(&v);
+
+	/* step 3 */
+
+	v = vm_gic_create();
+	subtest_redist_regions(&v);
+
+	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, dummy, true);
+	TEST_ASSERT(ret == -EFAULT, "register a third region allowing to cover the 4 vcpus");
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
+	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "register a third region allowing to cover the 4 vcpus");
+
+	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+	TEST_ASSERT(!ret, "init the vgic");
+
+	ucall_init(v.vm, NULL);
+	ret = run_vcpu(v.vm, 3);
+	TEST_ASSERT(!ret, "vcpu run");
+
+	vm_gic_destroy(&v);
+}
+
+static void test_typer_accesses(void)
+{
+	int ret, i, gicv3_fd = -1;
+	uint64_t addr;
+	struct kvm_vm *vm;
+	uint32_t val;
+
+	vm = vm_create_default(0, 0, guest_code);
+
+	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
+
+	vm_vcpu_add_default(vm, 3, guest_code);
+
+	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
+	TEST_ASSERT(ret == -EINVAL, "attempting to read GICR_TYPER of non created vcpu");
+
+	vm_vcpu_add_default(vm, 1, guest_code);
+
+	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
+	TEST_ASSERT(ret == -EBUSY, "read GICR_TYPER before GIC initialized");
+
+	vm_vcpu_add_default(vm, 2, guest_code);
+
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
+
+	for (i = 0; i < NR_VCPUS ; i++) {
+		ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
+		TEST_ASSERT(!ret && !val, "read GICR_TYPER before rdist region setting");
+	}
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "first rdist region with a capacity of 2 rdists");
+
+	/* The 2 first rdists should be put there (vcpu 0 and 3) */
+	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && !val, "read typer of rdist #0");
+
+	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #1");
+
+	addr = REDIST_REGION_ATTR_ADDR(10, 0x100000, 0, 1);
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret == -EINVAL, "collision with previous rdist region");
+
+	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x100,
+		    "no redist region attached to vcpu #1 yet, last cannot be returned");
+
+	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x200,
+		    "no redist region attached to vcpu #2, last cannot be returned");
+
+	addr = REDIST_REGION_ATTR_ADDR(10, 0x20000, 0, 1);
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "second rdist region");
+
+	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
+
+	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x210,
+		    "read typer of rdist #1, last properly returned");
+
+	close(gicv3_fd);
+	kvm_vm_free(vm);
+}
+
+/**
+ * Test GICR_TYPER last bit with new redist regions
+ * rdist regions #1 and #2 are contiguous
+ * rdist region #0 @0x100000 2 rdist capacity
+ *     rdists: 0, 3 (Last)
+ * rdist region #1 @0x240000 2 rdist capacity
+ *     rdists:  5, 4 (Last)
+ * rdist region #2 @0x200000 2 rdist capacity
+ *     rdists: 1, 2
+ */
+static void test_last_bit_redist_regions(void)
+{
+	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
+	int ret, gicv3_fd;
+	uint64_t addr;
+	struct kvm_vm *vm;
+	uint32_t val;
+
+	vm = vm_create_default_with_vcpus(6, 0, 0, guest_code, vcpuids);
+
+	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
+
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x100000, 0, 0);
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "rdist region #0 (2 rdist)");
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x240000, 0, 1);
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "rdist region #1 (1 rdist) contiguous with #2");
+
+	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 2);
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(!ret, "rdist region #2 with a capacity of 2 rdists");
+
+	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
+
+	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
+
+	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x200, "read typer of rdist #2");
+
+	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #3");
+
+	ret = access_redist_reg(gicv3_fd, 5, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #5");
+
+	ret = access_redist_reg(gicv3_fd, 4, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x410, "read typer of rdist #4");
+
+	close(gicv3_fd);
+	kvm_vm_free(vm);
+}
+
+/* Test last bit with legacy region */
+static void test_last_bit_single_rdist(void)
+{
+	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
+	int ret, gicv3_fd;
+	uint64_t addr;
+	struct kvm_vm *vm;
+	uint32_t val;
+
+	vm = vm_create_default_with_vcpus(6, 0, 0, guest_code, vcpuids);
+
+	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
+
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
+
+	addr = 0x10000;
+	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+
+	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
+
+	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x300, "read typer of rdist #1");
+
+	ret = access_redist_reg(gicv3_fd, 5, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #2");
+
+	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #3");
+
+	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
+	TEST_ASSERT(!ret && val == 0x210, "read typer of rdist #3");
+
+	close(gicv3_fd);
+	kvm_vm_free(vm);
+}
+
+void test_kvm_device(void)
+{
+	struct vm_gic v;
+	int ret;
+
+	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
+
+	/* try to create a non existing KVM device */
+	ret = _kvm_create_device(v.vm, 0, true);
+	TEST_ASSERT(ret == -ENODEV, "unsupported device");
+
+	/* trial mode with VGIC_V3 device */
+	ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
+	if (ret) {
+		print_skip("GICv3 not supported");
+		exit(KSFT_SKIP);
+	}
+	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(v.gic_fd, "create the GICv3 device");
+
+	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(ret == -EEXIST, "create GICv3 device twice");
+
+	ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
+	TEST_ASSERT(!ret, "create GICv3 in test mode while the same already is created");
+
+	if (!_kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, true)) {
+		ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, false);
+		TEST_ASSERT(ret == -EINVAL, "create GICv2 while v3 exists");
+	}
+
+	vm_gic_destroy(&v);
+}
+
+int main(int ac, char **av)
+{
+	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
+
+	test_kvm_device();
+	test_vcpus_then_vgic();
+	test_vgic_then_vcpus();
+	test_new_redist_regions();
+	test_typer_accesses();
+	test_last_bit_redist_regions();
+	test_last_bit_single_rdist();
+
+	return 0;
+}
+
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 0f4258eaa629..2b4b325cde01 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -225,6 +225,15 @@ int vcpu_nested_state_set(struct kvm_vm *vm, uint32_t vcpuid,
 #endif
 void *vcpu_map_dirty_ring(struct kvm_vm *vm, uint32_t vcpuid);
 
+int _kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr);
+int kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr);
+int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test);
+int kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test);
+int _kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
+		       void *val, bool write);
+int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
+		      void *val, bool write);
+
 const char *exit_reason_str(unsigned int exit_reason);
 
 void virt_pgd_alloc(struct kvm_vm *vm, uint32_t pgd_memslot);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index b8849a1aca79..db2a252be917 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1733,6 +1733,83 @@ int _kvm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
 	return ioctl(vm->kvm_fd, cmd, arg);
 }
 
+/*
+ * Device Ioctl
+ */
+
+int _kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr)
+{
+	struct kvm_device_attr attribute = {
+		.group = group,
+		.attr = attr,
+		.flags = 0,
+	};
+	int ret = ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attribute);
+
+	if (ret == -1)
+		return -errno;
+	return 0;
+}
+
+int kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr)
+{
+	int ret = _kvm_device_check_attr(dev_fd, group, attr);
+
+	TEST_ASSERT(ret >= 0, "KVM_HAS_DEVICE_ATTR failed, errno: %i", errno);
+	return ret;
+}
+
+int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
+{
+	struct kvm_create_device create_dev;
+	int ret;
+
+	create_dev.type = type;
+	create_dev.fd = -1;
+	create_dev.flags = test ? KVM_CREATE_DEVICE_TEST : 0;
+	ret = ioctl(vm_get_fd(vm), KVM_CREATE_DEVICE, &create_dev);
+	if (ret == -1)
+		return -errno;
+	return test ? 0 : create_dev.fd;
+}
+
+int kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
+{
+	int ret = _kvm_create_device(vm, type, test);
+
+	TEST_ASSERT(ret >= 0, "KVM_CREATE_DEVICE IOCTL failed,\n"
+		"  errno: %i", errno);
+	return ret;
+}
+
+int _kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
+		      void *val, bool write)
+{
+	struct kvm_device_attr kvmattr = {
+		.group = group,
+		.attr = attr,
+		.flags = 0,
+		.addr = (uintptr_t)val,
+	};
+	int ret;
+
+	ret = ioctl(dev_fd, write ? KVM_SET_DEVICE_ATTR : KVM_GET_DEVICE_ATTR,
+		    &kvmattr);
+	if (ret < 0)
+		return -errno;
+	return ret;
+}
+
+int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
+		      void *val, bool write)
+{
+	int ret = _kvm_device_access(dev_fd, group, attr, val, write);
+
+	TEST_ASSERT(ret >= 0, "KVM_SET|GET_DEVICE_ATTR IOCTL failed,\n"
+		    "  errno: %i", errno);
+	return ret;
+}
+
 /*
  * VM Dump
  *
-- 
2.26.3

Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests

[Andrew Jones]

Hi Eric,

It looks like Marc already picked this patch up, but, FWIW, here's
a few more comments you may consider.

On Mon, Apr 05, 2021 at 06:39:41PM +0200, Eric Auger wrote:
> The tests exercise the VGIC_V3 device creation including the
> associated KVM_DEV_ARM_VGIC_GRP_ADDR group attributes:
> 
> - KVM_VGIC_V3_ADDR_TYPE_DIST/REDIST
> - KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
> 
> Some other tests dedicate to KVM_DEV_ARM_VGIC_GRP_REDIST_REGS group
> and especially the GICR_TYPER read. The goal was to test the case
> recently fixed by commit 23bde34771f1
> ("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace").
> 
> The API under test can be found at
> Documentation/virt/kvm/devices/arm-vgic-v3.rst
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v4 -> v5:
> - simplify the last bit tests given the simpler interpretation
>   of the spec
> 
> v3 -> v4:
> - update .gitignore
> - More vgic-mmio-v3.c change into the previous patch
> - rename fuzz_dist_rdist into test_dist_rdist
> - cleanup in run_vcpu and guest_code
> - max_ipa_bits is global
> - s/fuzz/subtest
> - added test_kvm_device,
> - moved ucall_init() just before the cpu run
> - use vm_create_default_with_vcpus
> - use vm_gic struct, vm_gic_create, vm_gic_destroy
> - revwrite util.c helpers to comply with the usual style
> ---
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 585 ++++++++++++++++++
>  .../testing/selftests/kvm/include/kvm_util.h  |   9 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    |  77 +++
>  5 files changed, 673 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 7bd7e776c266..bb862f91f640 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  /aarch64/get-reg-list
>  /aarch64/get-reg-list-sve
> +/aarch64/vgic_init
>  /s390x/memop
>  /s390x/resets
>  /s390x/sync_regs_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 67eebb53235f..2fd4801de9ca 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -78,6 +78,7 @@ TEST_GEN_PROGS_x86_64 += steal_time
>  
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
> +TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
>  TEST_GEN_PROGS_aarch64 += demand_paging_test
>  TEST_GEN_PROGS_aarch64 += dirty_log_test
>  TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> new file mode 100644
> index 000000000000..be1a7c0d0527
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * vgic init sequence tests
> + *
> + * Copyright (C) 2020, Red Hat, Inc.
> + */
> +#define _GNU_SOURCE
> +#include <linux/kernel.h>
> +#include <sys/syscall.h>
> +#include <asm/kvm.h>
> +#include <asm/kvm_para.h>
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "processor.h"
> +
> +#define NR_VCPUS		4
> +
> +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) (((uint64_t)(count) << 52) | \
> +	((uint64_t)((base) >> 16) << 16) | ((uint64_t)(flags) << 12) | index)
> +#define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
> +
> +#define GICR_TYPER 0x8
> +
> +struct vm_gic {
> +	struct kvm_vm *vm;
> +	int gic_fd;
> +};
> +
> +int max_ipa_bits;

static

> +
> +/* helper to access a redistributor register */
> +static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
> +			     uint32_t *val, bool write)
> +{
> +	uint64_t attr = REG_OFFSET(vcpu, offset);
> +
> +	return _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS,
> +				  attr, val, write);
> +}
> +
> +/* dummy guest code */
> +static void guest_code(void)
> +{
> +	GUEST_SYNC(0);
> +	GUEST_SYNC(1);
> +	GUEST_SYNC(2);
> +	GUEST_DONE();
> +}
> +
> +/* we don't want to assert on run execution, hence that helper */
> +static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
> +{
> +	int ret;
> +
> +	vcpu_args_set(vm, vcpuid, 1);

You don't need the above vcpu_args_set call since guest_code doesn't take
any arguments.

> +	ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
> +	get_ucall(vm, vcpuid, NULL);

You're not checking the result of get_ucall, so there's no need for the
call.

> +
> +	if (ret)
> +		return -errno;
> +	return 0;
> +}
> +
> +static struct vm_gic vm_gic_create(void)
> +{
> +	struct vm_gic v;
> +
> +	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");

Why > 0? And why the assert here when you already have one for >= 0 in
kvm_create_device?

> +
> +	return v;
> +}
> +
> +static void vm_gic_destroy(struct vm_gic *v)
> +{
> +	close(v->gic_fd);
> +	kvm_vm_free(v->vm);
> +}
> +
> +/**
> + * Helper routine that performs KVM device tests in general and
> + * especially ARM_VGIC_V3 ones. Eventually the ARM_VGIC_V3
> + * device gets created, a legacy RDIST region is set at @0x0
> + * and a DIST region is set @0x60000
> + */
> +static void subtest_dist_rdist(struct vm_gic *v)
> +{
> +	int ret;
> +	uint64_t addr;
> +
> +	/* Check existing group/attributes */
> +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				    KVM_VGIC_V3_ADDR_TYPE_DIST);
> +	TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_DIST supported");

Assert is too harsh here. A SKIP on ENODEV would be better, because these
tests will also get run on gicv2 machines where the lack of gicv3 is not a
bug. If all the tests in this file are v3 specific, then please put this
check+skip in main.

> +
> +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				    KVM_VGIC_V3_ADDR_TYPE_REDIST);
> +	TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST supported");

Assert is OK now, since if there's a V3 _DIST, there must be a V3 _REDIST.

> +
> +	/* check non existing attribute */
> +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
> +	TEST_ASSERT(ret == -ENXIO, "attribute not supported");
> +
> +	/* misaligned DIST and REDIST address settings */
> +	addr = 0x1000;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "GICv3 dist base not 64kB aligned");
> +
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "GICv3 redist base not 64kB aligned");
> +
> +	/* out of range address */
> +	if (max_ipa_bits) {
> +		addr = 1ULL << max_ipa_bits;
> +		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +					 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> +		TEST_ASSERT(ret == -E2BIG, "dist address beyond IPA limit");
> +
> +		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +					 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +		TEST_ASSERT(ret == -E2BIG, "redist address beyond IPA limit");
> +	}
> +
> +	/* set REDIST base address @0x0*/
> +	addr = 0x00000;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +	TEST_ASSERT(!ret, "GICv3 redist base set");
> +
> +	/* Attempt to create a second legacy redistributor region */
> +	addr = 0xE0000;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +	TEST_ASSERT(ret == -EEXIST, "GICv3 redist base set again");
> +
> +	/* Attempt to mix legacy and new redistributor regions */
> +	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "attempt to mix GICv3 REDIST and REDIST_REGION");
> +
> +	/*
> +	 * Set overlapping DIST / REDIST, cannot be detected here. Will be detected
> +	 * on first vcpu run instead.
> +	 */
> +	addr = 3 * 2 * 0x10000;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_DIST,
> +				 &addr, true);
> +	TEST_ASSERT(!ret, "dist overlapping rdist");
> +}
> +
> +/* Test the new REDIST region API */
> +static void subtest_redist_regions(struct vm_gic *v)
> +{
> +	uint64_t addr, expected_addr;
> +	int ret;
> +
> +	ret = kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				     KVM_VGIC_V3_ADDR_TYPE_REDIST);
> +	TEST_ASSERT(!ret, "Multiple redist regions advertised");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 2, 0);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "redist region attr value with flags != 0");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(0, 0x100000, 0, 0);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "redist region attr value with count== 0");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 1);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "attempt to register the first rdist region with index != 0");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x201000, 0, 1);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "rdist region with misaligned address");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "First valid redist region with 2 rdist @ 0x200000, index 0");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 1);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "register an rdist region with already used index");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x210000, 0, 2);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "register an rdist region overlapping with another one");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 2);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "register redist region with index not +1");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 1);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "register valid redist region with 1 rdist @ 0x220000, index 1");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(1, 1ULL << max_ipa_bits, 0, 2);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -E2BIG, "register redist region with base address beyond IPA range");
> +
> +	addr = 0x260000;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "Mix KVM_VGIC_V3_ADDR_TYPE_REDIST and REDIST_REGION");
> +
> +	/*
> +	 * Now there are 2 redist regions:
> +	 * region 0 @ 0x200000 2 redists
> +	 * region 1 @ 0x240000 1 redist
> +	 * Attempt to read their characteristics
> +	 */
> +
> +	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 0);
> +	expected_addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
> +	TEST_ASSERT(!ret && addr == expected_addr, "read characteristics of region #0");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 1);
> +	expected_addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 1);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
> +	TEST_ASSERT(!ret && addr == expected_addr, "read characteristics of region #1");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 2);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
> +	TEST_ASSERT(ret == -ENOENT, "read characteristics of non existing region");
> +
> +	addr = 0x260000;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> +	TEST_ASSERT(!ret, "set dist region");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x260000, 0, 2);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "register redist region colliding with dist");
> +}
> +
> +/*
> + * VGIC KVM device is created and initialized before the secondary CPUs
> + * get created
> + */
> +static void test_vgic_then_vcpus(void)
> +{
> +	struct vm_gic v;
> +	int ret, i;
> +
> +	v.vm = vm_create_default(0, 0, guest_code);
> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
> +
> +	subtest_dist_rdist(&v);
> +
> +	/* Add the rest of the VCPUs */
> +	for (i = 1; i < NR_VCPUS; ++i)
> +		vm_vcpu_add_default(v.vm, i, guest_code);
> +
> +	ucall_init(v.vm, NULL);
> +	ret = run_vcpu(v.vm, 3);
> +	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
> +
> +	vm_gic_destroy(&v);
> +}
> +
> +/* All the VCPUs are created before the VGIC KVM device gets initialized */
> +static void test_vcpus_then_vgic(void)
> +{
> +	struct vm_gic v;
> +	int ret;
> +
> +	v = vm_gic_create();
> +
> +	subtest_dist_rdist(&v);
> +
> +	ucall_init(v.vm, NULL);
> +	ret = run_vcpu(v.vm, 3);
> +	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
> +
> +	vm_gic_destroy(&v);
> +}
> +
> +static void test_new_redist_regions(void)
> +{
> +	void *dummy = NULL;
> +	struct vm_gic v;
> +	uint64_t addr;
> +	int ret;
> +
> +	v = vm_gic_create();
> +	subtest_redist_regions(&v);
> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +	TEST_ASSERT(!ret, "init the vgic");
> +
> +	ucall_init(v.vm, NULL);
> +	ret = run_vcpu(v.vm, 3);
> +	TEST_ASSERT(ret == -ENXIO, "running without sufficient number of rdists");
> +	vm_gic_destroy(&v);
> +
> +	/* step2 */
> +
> +	v = vm_gic_create();
> +	subtest_redist_regions(&v);
> +
> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "register a third region allowing to cover the 4 vcpus");
> +
> +	ucall_init(v.vm, NULL);
> +	ret = run_vcpu(v.vm, 3);

Looks like ucall_init could be put in run_vcpu.

> +	TEST_ASSERT(ret == -EBUSY, "running without vgic explicit init");
> +
> +	vm_gic_destroy(&v);
> +
> +	/* step 3 */
> +
> +	v = vm_gic_create();
> +	subtest_redist_regions(&v);
> +
> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, dummy, true);
> +	TEST_ASSERT(ret == -EFAULT, "register a third region allowing to cover the 4 vcpus");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "register a third region allowing to cover the 4 vcpus");
> +
> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +	TEST_ASSERT(!ret, "init the vgic");
> +
> +	ucall_init(v.vm, NULL);
> +	ret = run_vcpu(v.vm, 3);
> +	TEST_ASSERT(!ret, "vcpu run");
> +
> +	vm_gic_destroy(&v);
> +}
> +
> +static void test_typer_accesses(void)
> +{
> +	int ret, i, gicv3_fd = -1;
> +	uint64_t addr;
> +	struct kvm_vm *vm;
> +	uint32_t val;
> +
> +	vm = vm_create_default(0, 0, guest_code);
> +
> +	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
> +
> +	vm_vcpu_add_default(vm, 3, guest_code);
> +
> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
> +	TEST_ASSERT(ret == -EINVAL, "attempting to read GICR_TYPER of non created vcpu");
> +
> +	vm_vcpu_add_default(vm, 1, guest_code);
> +
> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
> +	TEST_ASSERT(ret == -EBUSY, "read GICR_TYPER before GIC initialized");
> +
> +	vm_vcpu_add_default(vm, 2, guest_code);
> +
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
> +
> +	for (i = 0; i < NR_VCPUS ; i++) {
> +		ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
> +		TEST_ASSERT(!ret && !val, "read GICR_TYPER before rdist region setting");
> +	}
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "first rdist region with a capacity of 2 rdists");
> +
> +	/* The 2 first rdists should be put there (vcpu 0 and 3) */
> +	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && !val, "read typer of rdist #0");
> +
> +	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #1");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(10, 0x100000, 0, 1);
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret == -EINVAL, "collision with previous rdist region");
> +
> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x100,
> +		    "no redist region attached to vcpu #1 yet, last cannot be returned");
> +
> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x200,
> +		    "no redist region attached to vcpu #2, last cannot be returned");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(10, 0x20000, 0, 1);
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "second rdist region");
> +
> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
> +
> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x210,
> +		    "read typer of rdist #1, last properly returned");
> +
> +	close(gicv3_fd);
> +	kvm_vm_free(vm);
> +}
> +
> +/**
> + * Test GICR_TYPER last bit with new redist regions
> + * rdist regions #1 and #2 are contiguous
> + * rdist region #0 @0x100000 2 rdist capacity
> + *     rdists: 0, 3 (Last)
> + * rdist region #1 @0x240000 2 rdist capacity
> + *     rdists:  5, 4 (Last)
> + * rdist region #2 @0x200000 2 rdist capacity
> + *     rdists: 1, 2
> + */
> +static void test_last_bit_redist_regions(void)
> +{
> +	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
> +	int ret, gicv3_fd;
> +	uint64_t addr;
> +	struct kvm_vm *vm;
> +	uint32_t val;
> +
> +	vm = vm_create_default_with_vcpus(6, 0, 0, guest_code, vcpuids);
> +
> +	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
> +
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x100000, 0, 0);
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "rdist region #0 (2 rdist)");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x240000, 0, 1);
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "rdist region #1 (1 rdist) contiguous with #2");
> +
> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 2);
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(!ret, "rdist region #2 with a capacity of 2 rdists");
> +
> +	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
> +
> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
> +
> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x200, "read typer of rdist #2");
> +
> +	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #3");
> +
> +	ret = access_redist_reg(gicv3_fd, 5, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #5");
> +
> +	ret = access_redist_reg(gicv3_fd, 4, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x410, "read typer of rdist #4");
> +
> +	close(gicv3_fd);
> +	kvm_vm_free(vm);

Seems like sometimes we use the vm_gic_create/destroy and sometimes we
don't...  Maybe vm_gic_create needs to be more flexible. Or at least
the struct could be used more often like test_kvm_device does below.

> +}
> +
> +/* Test last bit with legacy region */
> +static void test_last_bit_single_rdist(void)
> +{
> +	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
> +	int ret, gicv3_fd;
> +	uint64_t addr;
> +	struct kvm_vm *vm;
> +	uint32_t val;
> +
> +	vm = vm_create_default_with_vcpus(6, 0, 0, guest_code, vcpuids);
> +
> +	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
> +
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
> +
> +	addr = 0x10000;
> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +
> +	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
> +
> +	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x300, "read typer of rdist #1");
> +
> +	ret = access_redist_reg(gicv3_fd, 5, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #2");
> +
> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #3");
> +
> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
> +	TEST_ASSERT(!ret && val == 0x210, "read typer of rdist #3");
> +
> +	close(gicv3_fd);
> +	kvm_vm_free(vm);
> +}
> +
> +void test_kvm_device(void)
> +{
> +	struct vm_gic v;
> +	int ret;
> +
> +	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
> +
> +	/* try to create a non existing KVM device */
> +	ret = _kvm_create_device(v.vm, 0, true);
> +	TEST_ASSERT(ret == -ENODEV, "unsupported device");
> +
> +	/* trial mode with VGIC_V3 device */
> +	ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> +	if (ret) {
> +		print_skip("GICv3 not supported");
> +		exit(KSFT_SKIP);
> +	}
> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	TEST_ASSERT(v.gic_fd, "create the GICv3 device");
> +
> +	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	TEST_ASSERT(ret == -EEXIST, "create GICv3 device twice");
> +
> +	ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> +	TEST_ASSERT(!ret, "create GICv3 in test mode while the same already is created");
> +
> +	if (!_kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, true)) {
> +		ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, false);
> +		TEST_ASSERT(ret == -EINVAL, "create GICv2 while v3 exists");
> +	}
> +
> +	vm_gic_destroy(&v);
> +}
> +
> +int main(int ac, char **av)
> +{
> +	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
> +
> +	test_kvm_device();
> +	test_vcpus_then_vgic();
> +	test_vgic_then_vcpus();
> +	test_new_redist_regions();
> +	test_typer_accesses();
> +	test_last_bit_redist_regions();
> +	test_last_bit_single_rdist();
> +
> +	return 0;
> +}
> +
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 0f4258eaa629..2b4b325cde01 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -225,6 +225,15 @@ int vcpu_nested_state_set(struct kvm_vm *vm, uint32_t vcpuid,
>  #endif
>  void *vcpu_map_dirty_ring(struct kvm_vm *vm, uint32_t vcpuid);
>  
> +int _kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr);
> +int kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr);
> +int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test);
> +int kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test);
> +int _kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
> +		       void *val, bool write);
> +int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
> +		      void *val, bool write);
> +
>  const char *exit_reason_str(unsigned int exit_reason);
>  
>  void virt_pgd_alloc(struct kvm_vm *vm, uint32_t pgd_memslot);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index b8849a1aca79..db2a252be917 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1733,6 +1733,83 @@ int _kvm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
>  	return ioctl(vm->kvm_fd, cmd, arg);
>  }
>  
> +/*
> + * Device Ioctl
> + */
> +
> +int _kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr)
> +{
> +	struct kvm_device_attr attribute = {
> +		.group = group,
> +		.attr = attr,
> +		.flags = 0,
> +	};
> +	int ret = ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attribute);
> +
> +	if (ret == -1)
> +		return -errno;

This still doesn't follow our pattern for the underscore prefixed ioctl
wrapping functions. Those functions pass back the return code as received.
The callers check errno themselves per the return code, e.g. if it's -1.
Take a look at _vcpu_run, for example.

> +	return 0;
> +}
> +
> +int kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr)
> +{
> +	int ret = _kvm_device_check_attr(dev_fd, group, attr);
> +
> +	TEST_ASSERT(ret >= 0, "KVM_HAS_DEVICE_ATTR failed, errno: %i", errno);
> +	return ret;
> +}
> +
> +int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
> +{
> +	struct kvm_create_device create_dev;
> +	int ret;
> +
> +	create_dev.type = type;
> +	create_dev.fd = -1;
> +	create_dev.flags = test ? KVM_CREATE_DEVICE_TEST : 0;
> +	ret = ioctl(vm_get_fd(vm), KVM_CREATE_DEVICE, &create_dev);
> +	if (ret == -1)
> +		return -errno;
> +	return test ? 0 : create_dev.fd;

Something like this belongs in the non underscore prefixed wrappers.

> +}
> +
> +int kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
> +{
> +	int ret = _kvm_create_device(vm, type, test);
> +
> +	TEST_ASSERT(ret >= 0, "KVM_CREATE_DEVICE IOCTL failed,\n"
> +		"  errno: %i", errno);
> +	return ret;
> +}
> +
> +int _kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
> +		      void *val, bool write)
> +{
> +	struct kvm_device_attr kvmattr = {
> +		.group = group,
> +		.attr = attr,
> +		.flags = 0,
> +		.addr = (uintptr_t)val,
> +	};
> +	int ret;
> +
> +	ret = ioctl(dev_fd, write ? KVM_SET_DEVICE_ATTR : KVM_GET_DEVICE_ATTR,
> +		    &kvmattr);
> +	if (ret < 0)
> +		return -errno;
> +	return ret;
> +}
> +
> +int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
> +		      void *val, bool write)
> +{
> +	int ret = _kvm_device_access(dev_fd, group, attr, val, write);
> +
> +	TEST_ASSERT(ret >= 0, "KVM_SET|GET_DEVICE_ATTR IOCTL failed,\n"
> +		    "  errno: %i", errno);
> +	return ret;
> +}
> +
>  /*
>   * VM Dump
>   *
> -- 
> 2.26.3
>

Thanks,
drew 

Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests

[Marc Zyngier]

On Tue, 06 Apr 2021 16:09:16 +0100,
Andrew Jones <drjones@redhat.com> wrote:
> 
> 
> Hi Eric,
> 
> It looks like Marc already picked this patch up, but, FWIW, here's
> a few more comments you may consider.

FWIW, I'm happy to drop this patch from the queue (the series is on
its own branch for this reason), or take reworks on top.

But given that we're getting close to release time and this series has
some userspace visibile impacts, I want to let it simmer in -next for
a few days before sending the pull request.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests

[Auger Eric]

Hi Drew,

On 4/6/21 5:09 PM, Andrew Jones wrote:
> 
> Hi Eric,
> 
> It looks like Marc already picked this patch up, but, FWIW, here's
> a few more comments you may consider.

I will send a fixup patch on top of the one taken my Marc.  Few comments
below.
> 
> On Mon, Apr 05, 2021 at 06:39:41PM +0200, Eric Auger wrote:
>> The tests exercise the VGIC_V3 device creation including the
>> associated KVM_DEV_ARM_VGIC_GRP_ADDR group attributes:
>>
>> - KVM_VGIC_V3_ADDR_TYPE_DIST/REDIST
>> - KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
>>
>> Some other tests dedicate to KVM_DEV_ARM_VGIC_GRP_REDIST_REGS group
>> and especially the GICR_TYPER read. The goal was to test the case
>> recently fixed by commit 23bde34771f1
>> ("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace").
>>
>> The API under test can be found at
>> Documentation/virt/kvm/devices/arm-vgic-v3.rst
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v4 -> v5:
>> - simplify the last bit tests given the simpler interpretation
>>   of the spec
>>
>> v3 -> v4:
>> - update .gitignore
>> - More vgic-mmio-v3.c change into the previous patch
>> - rename fuzz_dist_rdist into test_dist_rdist
>> - cleanup in run_vcpu and guest_code
>> - max_ipa_bits is global
>> - s/fuzz/subtest
>> - added test_kvm_device,
>> - moved ucall_init() just before the cpu run
>> - use vm_create_default_with_vcpus
>> - use vm_gic struct, vm_gic_create, vm_gic_destroy
>> - revwrite util.c helpers to comply with the usual style
>> ---
>>  tools/testing/selftests/kvm/.gitignore        |   1 +
>>  tools/testing/selftests/kvm/Makefile          |   1 +
>>  .../testing/selftests/kvm/aarch64/vgic_init.c | 585 ++++++++++++++++++
>>  .../testing/selftests/kvm/include/kvm_util.h  |   9 +
>>  tools/testing/selftests/kvm/lib/kvm_util.c    |  77 +++
>>  5 files changed, 673 insertions(+)
>>  create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c
>>
>> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
>> index 7bd7e776c266..bb862f91f640 100644
>> --- a/tools/testing/selftests/kvm/.gitignore
>> +++ b/tools/testing/selftests/kvm/.gitignore
>> @@ -1,6 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  /aarch64/get-reg-list
>>  /aarch64/get-reg-list-sve
>> +/aarch64/vgic_init
>>  /s390x/memop
>>  /s390x/resets
>>  /s390x/sync_regs_test
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index 67eebb53235f..2fd4801de9ca 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -78,6 +78,7 @@ TEST_GEN_PROGS_x86_64 += steal_time
>>  
>>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
>>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
>> +TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
>>  TEST_GEN_PROGS_aarch64 += demand_paging_test
>>  TEST_GEN_PROGS_aarch64 += dirty_log_test
>>  TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
>> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>> new file mode 100644
>> index 000000000000..be1a7c0d0527
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>> @@ -0,0 +1,585 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * vgic init sequence tests
>> + *
>> + * Copyright (C) 2020, Red Hat, Inc.
>> + */
>> +#define _GNU_SOURCE
>> +#include <linux/kernel.h>
>> +#include <sys/syscall.h>
>> +#include <asm/kvm.h>
>> +#include <asm/kvm_para.h>
>> +
>> +#include "test_util.h"
>> +#include "kvm_util.h"
>> +#include "processor.h"
>> +
>> +#define NR_VCPUS		4
>> +
>> +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) (((uint64_t)(count) << 52) | \
>> +	((uint64_t)((base) >> 16) << 16) | ((uint64_t)(flags) << 12) | index)
>> +#define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
>> +
>> +#define GICR_TYPER 0x8
>> +
>> +struct vm_gic {
>> +	struct kvm_vm *vm;
>> +	int gic_fd;
>> +};
>> +
>> +int max_ipa_bits;
> 
> static
done
> 
>> +
>> +/* helper to access a redistributor register */
>> +static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
>> +			     uint32_t *val, bool write)
>> +{
>> +	uint64_t attr = REG_OFFSET(vcpu, offset);
>> +
>> +	return _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS,
>> +				  attr, val, write);
>> +}
>> +
>> +/* dummy guest code */
>> +static void guest_code(void)
>> +{
>> +	GUEST_SYNC(0);
>> +	GUEST_SYNC(1);
>> +	GUEST_SYNC(2);
>> +	GUEST_DONE();
>> +}
>> +
>> +/* we don't want to assert on run execution, hence that helper */
>> +static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
>> +{
>> +	int ret;
>> +
>> +	vcpu_args_set(vm, vcpuid, 1);
> 
> You don't need the above vcpu_args_set call since guest_code doesn't take
> any arguments.
ok
> 
>> +	ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
>> +	get_ucall(vm, vcpuid, NULL);
> 
> You're not checking the result of get_ucall, so there's no need for the
> call.
removed
> 
>> +
>> +	if (ret)
>> +		return -errno;
>> +	return 0;
>> +}
>> +
>> +static struct vm_gic vm_gic_create(void)
>> +{
>> +	struct vm_gic v;
>> +
>> +	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
>> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>> +	TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
> 
> Why > 0? And why the assert here when you already have one for >= 0 in
> kvm_create_device?
removed
> 
>> +
>> +	return v;
>> +}
>> +
>> +static void vm_gic_destroy(struct vm_gic *v)
>> +{
>> +	close(v->gic_fd);
>> +	kvm_vm_free(v->vm);
>> +}
>> +
>> +/**
>> + * Helper routine that performs KVM device tests in general and
>> + * especially ARM_VGIC_V3 ones. Eventually the ARM_VGIC_V3
>> + * device gets created, a legacy RDIST region is set at @0x0
>> + * and a DIST region is set @0x60000
>> + */
>> +static void subtest_dist_rdist(struct vm_gic *v)
>> +{
>> +	int ret;
>> +	uint64_t addr;
>> +
>> +	/* Check existing group/attributes */
>> +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				    KVM_VGIC_V3_ADDR_TYPE_DIST);
>> +	TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_DIST supported");
> 
> Assert is too harsh here. A SKIP on ENODEV would be better, because these
> tests will also get run on gicv2 machines where the lack of gicv3 is not a
> bug. If all the tests in this file are v3 specific, then please put this
> check+skip in main.
In test_kvm_device() I do
>> +
>> +	/* trial mode with VGIC_V3 device */
>> +	ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
>> +	if (ret) {
>> +		print_skip("GICv3 not supported");
>> +		exit(KSFT_SKIP);
>> +	}

I tested the kvm selftests on Seattle and the test is skipped.

However I can repace _ prefixed calls by standard calls here.
> 
>> +
>> +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				    KVM_VGIC_V3_ADDR_TYPE_REDIST);
>> +	TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST supported");
> 
> Assert is OK now, since if there's a V3 _DIST, there must be a V3 _REDIST.
replaced by kvm_device_check_attr() call
> 
>> +
>> +	/* check non existing attribute */
>> +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
>> +	TEST_ASSERT(ret == -ENXIO, "attribute not supported");
>> +
>> +	/* misaligned DIST and REDIST address settings */
>> +	addr = 0x1000;
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "GICv3 dist base not 64kB aligned");
>> +
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "GICv3 redist base not 64kB aligned");
>> +
>> +	/* out of range address */
>> +	if (max_ipa_bits) {
>> +		addr = 1ULL << max_ipa_bits;
>> +		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +					 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
>> +		TEST_ASSERT(ret == -E2BIG, "dist address beyond IPA limit");
>> +
>> +		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +					 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> +		TEST_ASSERT(ret == -E2BIG, "redist address beyond IPA limit");
>> +	}
>> +
>> +	/* set REDIST base address @0x0*/
>> +	addr = 0x00000;
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> +	TEST_ASSERT(!ret, "GICv3 redist base set");
>> +
>> +	/* Attempt to create a second legacy redistributor region */
>> +	addr = 0xE0000;
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> +	TEST_ASSERT(ret == -EEXIST, "GICv3 redist base set again");
>> +
>> +	/* Attempt to mix legacy and new redistributor regions */
>> +	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "attempt to mix GICv3 REDIST and REDIST_REGION");
>> +
>> +	/*
>> +	 * Set overlapping DIST / REDIST, cannot be detected here. Will be detected
>> +	 * on first vcpu run instead.
>> +	 */
>> +	addr = 3 * 2 * 0x10000;
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_DIST,
>> +				 &addr, true);
>> +	TEST_ASSERT(!ret, "dist overlapping rdist");
>> +}
>> +
>> +/* Test the new REDIST region API */
>> +static void subtest_redist_regions(struct vm_gic *v)
>> +{
>> +	uint64_t addr, expected_addr;
>> +	int ret;
>> +
>> +	ret = kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				     KVM_VGIC_V3_ADDR_TYPE_REDIST);
>> +	TEST_ASSERT(!ret, "Multiple redist regions advertised");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 2, 0);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "redist region attr value with flags != 0");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(0, 0x100000, 0, 0);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "redist region attr value with count== 0");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 1);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "attempt to register the first rdist region with index != 0");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x201000, 0, 1);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "rdist region with misaligned address");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "First valid redist region with 2 rdist @ 0x200000, index 0");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 1);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "register an rdist region with already used index");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x210000, 0, 2);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "register an rdist region overlapping with another one");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 2);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "register redist region with index not +1");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 1);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "register valid redist region with 1 rdist @ 0x220000, index 1");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(1, 1ULL << max_ipa_bits, 0, 2);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -E2BIG, "register redist region with base address beyond IPA range");
>> +
>> +	addr = 0x260000;
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "Mix KVM_VGIC_V3_ADDR_TYPE_REDIST and REDIST_REGION");
>> +
>> +	/*
>> +	 * Now there are 2 redist regions:
>> +	 * region 0 @ 0x200000 2 redists
>> +	 * region 1 @ 0x240000 1 redist
>> +	 * Attempt to read their characteristics
>> +	 */
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 0);
>> +	expected_addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
>> +	TEST_ASSERT(!ret && addr == expected_addr, "read characteristics of region #0");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 1);
>> +	expected_addr = REDIST_REGION_ATTR_ADDR(1, 0x240000, 0, 1);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
>> +	TEST_ASSERT(!ret && addr == expected_addr, "read characteristics of region #1");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(0, 0, 0, 2);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, false);
>> +	TEST_ASSERT(ret == -ENOENT, "read characteristics of non existing region");
>> +
>> +	addr = 0x260000;
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
>> +	TEST_ASSERT(!ret, "set dist region");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x260000, 0, 2);
>> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "register redist region colliding with dist");
>> +}
>> +
>> +/*
>> + * VGIC KVM device is created and initialized before the secondary CPUs
>> + * get created
>> + */
>> +static void test_vgic_then_vcpus(void)
>> +{
>> +	struct vm_gic v;
>> +	int ret, i;
>> +
>> +	v.vm = vm_create_default(0, 0, guest_code);
>> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>> +	TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
>> +
>> +	subtest_dist_rdist(&v);
>> +
>> +	/* Add the rest of the VCPUs */
>> +	for (i = 1; i < NR_VCPUS; ++i)
>> +		vm_vcpu_add_default(v.vm, i, guest_code);
>> +
>> +	ucall_init(v.vm, NULL);
>> +	ret = run_vcpu(v.vm, 3);
>> +	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
>> +
>> +	vm_gic_destroy(&v);
>> +}
>> +
>> +/* All the VCPUs are created before the VGIC KVM device gets initialized */
>> +static void test_vcpus_then_vgic(void)
>> +{
>> +	struct vm_gic v;
>> +	int ret;
>> +
>> +	v = vm_gic_create();
>> +
>> +	subtest_dist_rdist(&v);
>> +
>> +	ucall_init(v.vm, NULL);
>> +	ret = run_vcpu(v.vm, 3);
>> +	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
>> +
>> +	vm_gic_destroy(&v);
>> +}
>> +
>> +static void test_new_redist_regions(void)
>> +{
>> +	void *dummy = NULL;
>> +	struct vm_gic v;
>> +	uint64_t addr;
>> +	int ret;
>> +
>> +	v = vm_gic_create();
>> +	subtest_redist_regions(&v);
>> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>> +	TEST_ASSERT(!ret, "init the vgic");
>> +
>> +	ucall_init(v.vm, NULL);
>> +	ret = run_vcpu(v.vm, 3);
>> +	TEST_ASSERT(ret == -ENXIO, "running without sufficient number of rdists");
>> +	vm_gic_destroy(&v);
>> +
>> +	/* step2 */
>> +
>> +	v = vm_gic_create();
>> +	subtest_redist_regions(&v);
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
>> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "register a third region allowing to cover the 4 vcpus");
>> +
>> +	ucall_init(v.vm, NULL);
>> +	ret = run_vcpu(v.vm, 3);
> 
> Looks like ucall_init could be put in run_vcpu.
done
> 
>> +	TEST_ASSERT(ret == -EBUSY, "running without vgic explicit init");
>> +
>> +	vm_gic_destroy(&v);
>> +
>> +	/* step 3 */
>> +
>> +	v = vm_gic_create();
>> +	subtest_redist_regions(&v);
>> +
>> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, dummy, true);
>> +	TEST_ASSERT(ret == -EFAULT, "register a third region allowing to cover the 4 vcpus");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
>> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "register a third region allowing to cover the 4 vcpus");
>> +
>> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>> +	TEST_ASSERT(!ret, "init the vgic");
>> +
>> +	ucall_init(v.vm, NULL);
>> +	ret = run_vcpu(v.vm, 3);
>> +	TEST_ASSERT(!ret, "vcpu run");
>> +
>> +	vm_gic_destroy(&v);
>> +}
>> +
>> +static void test_typer_accesses(void)
>> +{
>> +	int ret, i, gicv3_fd = -1;
>> +	uint64_t addr;
>> +	struct kvm_vm *vm;
>> +	uint32_t val;
>> +
>> +	vm = vm_create_default(0, 0, guest_code);
>> +
>> +	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>> +	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
>> +
>> +	vm_vcpu_add_default(vm, 3, guest_code);
>> +
>> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(ret == -EINVAL, "attempting to read GICR_TYPER of non created vcpu");
>> +
>> +	vm_vcpu_add_default(vm, 1, guest_code);
>> +
>> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(ret == -EBUSY, "read GICR_TYPER before GIC initialized");
>> +
>> +	vm_vcpu_add_default(vm, 2, guest_code);
>> +
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>> +	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
>> +
>> +	for (i = 0; i < NR_VCPUS ; i++) {
>> +		ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
>> +		TEST_ASSERT(!ret && !val, "read GICR_TYPER before rdist region setting");
>> +	}
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 0);
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "first rdist region with a capacity of 2 rdists");
>> +
>> +	/* The 2 first rdists should be put there (vcpu 0 and 3) */
>> +	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && !val, "read typer of rdist #0");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #1");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(10, 0x100000, 0, 1);
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(ret == -EINVAL, "collision with previous rdist region");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x100,
>> +		    "no redist region attached to vcpu #1 yet, last cannot be returned");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x200,
>> +		    "no redist region attached to vcpu #2, last cannot be returned");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(10, 0x20000, 0, 1);
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "second rdist region");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x210,
>> +		    "read typer of rdist #1, last properly returned");
>> +
>> +	close(gicv3_fd);
>> +	kvm_vm_free(vm);
>> +}
>> +
>> +/**
>> + * Test GICR_TYPER last bit with new redist regions
>> + * rdist regions #1 and #2 are contiguous
>> + * rdist region #0 @0x100000 2 rdist capacity
>> + *     rdists: 0, 3 (Last)
>> + * rdist region #1 @0x240000 2 rdist capacity
>> + *     rdists:  5, 4 (Last)
>> + * rdist region #2 @0x200000 2 rdist capacity
>> + *     rdists: 1, 2
>> + */
>> +static void test_last_bit_redist_regions(void)
>> +{
>> +	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
>> +	int ret, gicv3_fd;
>> +	uint64_t addr;
>> +	struct kvm_vm *vm;
>> +	uint32_t val;
>> +
>> +	vm = vm_create_default_with_vcpus(6, 0, 0, guest_code, vcpuids);
>> +
>> +	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>> +	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
>> +
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>> +	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x100000, 0, 0);
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "rdist region #0 (2 rdist)");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x240000, 0, 1);
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "rdist region #1 (1 rdist) contiguous with #2");
>> +
>> +	addr = REDIST_REGION_ATTR_ADDR(2, 0x200000, 0, 2);
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>> +	TEST_ASSERT(!ret, "rdist region #2 with a capacity of 2 rdists");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x200, "read typer of rdist #2");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #3");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 5, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #5");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 4, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x410, "read typer of rdist #4");
>> +
>> +	close(gicv3_fd);
>> +	kvm_vm_free(vm);
> 
> Seems like sometimes we use the vm_gic_create/destroy and sometimes we
> don't...  Maybe vm_gic_create needs to be more flexible. Or at least
> the struct could be used more often like test_kvm_device does below.
ok, I used the vm_gic structure in different places
> 
>> +}
>> +
>> +/* Test last bit with legacy region */
>> +static void test_last_bit_single_rdist(void)
>> +{
>> +	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
>> +	int ret, gicv3_fd;
>> +	uint64_t addr;
>> +	struct kvm_vm *vm;
>> +	uint32_t val;
>> +
>> +	vm = vm_create_default_with_vcpus(6, 0, 0, guest_code, vcpuids);
>> +
>> +	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>> +	TEST_ASSERT(gicv3_fd >= 0, "VGIC_V3 device created");
>> +
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>> +				 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>> +	TEST_ASSERT(!ret, "init the vgic after the vcpu creations");
>> +
>> +	addr = 0x10000;
>> +	ret = _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> +
>> +	ret = access_redist_reg(gicv3_fd, 0, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 3, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x300, "read typer of rdist #1");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 5, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #2");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 1, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #3");
>> +
>> +	ret = access_redist_reg(gicv3_fd, 2, GICR_TYPER, &val, false);
>> +	TEST_ASSERT(!ret && val == 0x210, "read typer of rdist #3");
>> +
>> +	close(gicv3_fd);
>> +	kvm_vm_free(vm);
>> +}
>> +
>> +void test_kvm_device(void)
>> +{
>> +	struct vm_gic v;
>> +	int ret;
>> +
>> +	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
>> +
>> +	/* try to create a non existing KVM device */
>> +	ret = _kvm_create_device(v.vm, 0, true);
>> +	TEST_ASSERT(ret == -ENODEV, "unsupported device");
>> +
>> +	/* trial mode with VGIC_V3 device */
>> +	ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
>> +	if (ret) {
>> +		print_skip("GICv3 not supported");
>> +		exit(KSFT_SKIP);
>> +	}
>> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>> +	TEST_ASSERT(v.gic_fd, "create the GICv3 device");
>> +
>> +	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>> +	TEST_ASSERT(ret == -EEXIST, "create GICv3 device twice");
>> +
>> +	ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
>> +	TEST_ASSERT(!ret, "create GICv3 in test mode while the same already is created");
>> +
>> +	if (!_kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, true)) {
>> +		ret = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, false);
>> +		TEST_ASSERT(ret == -EINVAL, "create GICv2 while v3 exists");
>> +	}
>> +
>> +	vm_gic_destroy(&v);
>> +}
>> +
>> +int main(int ac, char **av)
>> +{
>> +	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
>> +
>> +	test_kvm_device();
>> +	test_vcpus_then_vgic();
>> +	test_vgic_then_vcpus();
>> +	test_new_redist_regions();
>> +	test_typer_accesses();
>> +	test_last_bit_redist_regions();
>> +	test_last_bit_single_rdist();
>> +
>> +	return 0;
>> +}
>> +
>> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
>> index 0f4258eaa629..2b4b325cde01 100644
>> --- a/tools/testing/selftests/kvm/include/kvm_util.h
>> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
>> @@ -225,6 +225,15 @@ int vcpu_nested_state_set(struct kvm_vm *vm, uint32_t vcpuid,
>>  #endif
>>  void *vcpu_map_dirty_ring(struct kvm_vm *vm, uint32_t vcpuid);
>>  
>> +int _kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr);
>> +int kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr);
>> +int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test);
>> +int kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test);
>> +int _kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
>> +		       void *val, bool write);
>> +int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
>> +		      void *val, bool write);
>> +
>>  const char *exit_reason_str(unsigned int exit_reason);
>>  
>>  void virt_pgd_alloc(struct kvm_vm *vm, uint32_t pgd_memslot);
>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
>> index b8849a1aca79..db2a252be917 100644
>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
>> @@ -1733,6 +1733,83 @@ int _kvm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
>>  	return ioctl(vm->kvm_fd, cmd, arg);
>>  }
>>  
>> +/*
>> + * Device Ioctl
>> + */
>> +
>> +int _kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr)
>> +{
>> +	struct kvm_device_attr attribute = {
>> +		.group = group,
>> +		.attr = attr,
>> +		.flags = 0,
>> +	};
>> +	int ret = ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attribute);
>> +
>> +	if (ret == -1)
>> +		return -errno;
> 
> This still doesn't follow our pattern for the underscore prefixed ioctl
> wrapping functions. Those functions pass back the return code as received.
> The callers check errno themselves per the return code, e.g. if it's -1.
> Take a look at _vcpu_run, for example.
OK
> 
>> +	return 0;
>> +}
>> +
>> +int kvm_device_check_attr(int dev_fd, uint32_t group, uint64_t attr)
>> +{
>> +	int ret = _kvm_device_check_attr(dev_fd, group, attr);
>> +
>> +	TEST_ASSERT(ret >= 0, "KVM_HAS_DEVICE_ATTR failed, errno: %i", errno);
>> +	return ret;
>> +}
>> +
>> +int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
>> +{
>> +	struct kvm_create_device create_dev;
>> +	int ret;
>> +
>> +	create_dev.type = type;
>> +	create_dev.fd = -1;
>> +	create_dev.flags = test ? KVM_CREATE_DEVICE_TEST : 0;
>> +	ret = ioctl(vm_get_fd(vm), KVM_CREATE_DEVICE, &create_dev);
>> +	if (ret == -1)
>> +		return -errno;
>> +	return test ? 0 : create_dev.fd;
> 
> Something like this belongs in the non underscore prefixed wrappers.
I need at least to return the create_dev.fd or do you want me to add an
extra int *fd parameter?
What about:

        if (ret < 0)
                return ret;
        return test ? 0 : create_dev.fd;

> 
>> +}
>> +
>> +int kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
>> +{
>> +	int ret = _kvm_create_device(vm, type, test);
>> +
>> +	TEST_ASSERT(ret >= 0, "KVM_CREATE_DEVICE IOCTL failed,\n"
>> +		"  errno: %i", errno);
>> +	return ret;
>> +}
>> +
>> +int _kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
>> +		      void *val, bool write)
>> +{
>> +	struct kvm_device_attr kvmattr = {
>> +		.group = group,
>> +		.attr = attr,
>> +		.flags = 0,
>> +		.addr = (uintptr_t)val,
>> +	};
>> +	int ret;
>> +
>> +	ret = ioctl(dev_fd, write ? KVM_SET_DEVICE_ATTR : KVM_GET_DEVICE_ATTR,
>> +		    &kvmattr);
>> +	if (ret < 0)
>> +		return -errno;
>> +	return ret;
>> +}
>> +
>> +int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
>> +		      void *val, bool write)
>> +{
>> +	int ret = _kvm_device_access(dev_fd, group, attr, val, write);
>> +
>> +	TEST_ASSERT(ret >= 0, "KVM_SET|GET_DEVICE_ATTR IOCTL failed,\n"
>> +		    "  errno: %i", errno);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * VM Dump
>>   *
>> -- 
>> 2.26.3
>>
> 
> Thanks,
> drew 
> 
Thanks

Eric

Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests

[Andrew Jones]

On Wed, Apr 07, 2021 at 12:14:29PM +0200, Auger Eric wrote:
> >> +int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
> >> +{
> >> +	struct kvm_create_device create_dev;
> >> +	int ret;
> >> +
> >> +	create_dev.type = type;
> >> +	create_dev.fd = -1;
> >> +	create_dev.flags = test ? KVM_CREATE_DEVICE_TEST : 0;
> >> +	ret = ioctl(vm_get_fd(vm), KVM_CREATE_DEVICE, &create_dev);
> >> +	if (ret == -1)
> >> +		return -errno;
> >> +	return test ? 0 : create_dev.fd;
> > 
> > Something like this belongs in the non underscore prefixed wrappers.
> I need at least to return the create_dev.fd or do you want me to add an
> extra int *fd parameter?
> What about:
> 
>         if (ret < 0)
>                 return ret;
>         return test ? 0 : create_dev.fd;

Maybe the underscore version of kvm_create_device isn't necessary. If
the non-underscore version isn't flexible enough, then just use the
ioctl directly from the test code with its own struct kvm_create_device
Being able to call ioctls directly from test code is what vm_get_fd()
is for, otherwise you could just use vm->fd.

Thanks,
drew

Re: [PATCH v6 0/9] KVM/ARM: Some vgic fixes and init sequence KVM selftests

[Marc Zyngier]

On Mon, 5 Apr 2021 18:39:32 +0200, Eric Auger wrote:
> While writting vgic v3 init sequence KVM selftests I noticed some
> relatively minor issues. This was also the opportunity to try to
> fix the issue laterly reported by Zenghui, related to the RDIST_TYPER
> last bit emulation. The final patch is a first batch of VGIC init
> sequence selftests. Of course they can be augmented with a lot more
> register access tests, but let's try to move forward incrementally ...
> 
> [...]

Applied to kvm-arm64/vgic-5.13, thanks!

[1/9] KVM: arm64: vgic-v3: Fix some error codes when setting RDIST base
      commit: d9b201e99c616001b4a51627820377b293479384
[2/9] KVM: arm64: Fix KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION read
      commit: 53b16dd6ba5cf64ed147ac3523ec34651d553cb0
[3/9] KVM: arm64: vgic-v3: Fix error handling in vgic_v3_set_redist_base()
      commit: 8542a8f95a67ff6b76d6868ec0af58c464bdf041
[4/9] KVM: arm/arm64: vgic: Reset base address on kvm_vgic_dist_destroy()
      commit: 3a5211612764fa3948e5db5254734168e9e763de
[5/9] docs: kvm: devices/arm-vgic-v3: enhance KVM_DEV_ARM_VGIC_CTRL_INIT doc
      commit: 298c41b8fa1e02c5a35e2263d138583220ab6094
[6/9] KVM: arm64: Simplify argument passing to vgic_uaccess_[read|write]
      commit: da3853097679022e14a2d125983f11a67fd2f96a
[7/9] kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
      commit: e5a35635464bc5304674b84ea42615a3fd0bd949
[8/9] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
      commit: 28e9d4bce3be9b8fec6c854f87923db99c8fb874
[9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests
      commit: dc0e058eef42f61effe9fd4f0fa4b0c793cc1f14

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.