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

[PATCH 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-selftests-and-fixes-v1

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()
  KVM: arm: move has_run_once after the map_resources
  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: 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/arm.c                          |   4 +-
 arch/arm64/kvm/vgic/vgic-init.c               |   7 +-
 arch/arm64/kvm/vgic/vgic-kvm-device.c         |   3 +
 arch/arm64/kvm/vgic/vgic-mmio-v3.c            |  24 +-
 arch/arm64/kvm/vgic/vgic-mmio.c               |  10 +-
 include/kvm/arm_vgic.h                        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/aarch64/vgic_init.c | 453 ++++++++++++++++++
 .../testing/selftests/kvm/include/kvm_util.h  |   5 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  51 ++
 11 files changed, 546 insertions(+), 15 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c

-- 
2.21.3

[PATCH 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>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 15a6c98ee92f..8e8a862def76 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -792,8 +792,13 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
 	int ret;
 
 	/* single rdist region already set ?*/
-	if (!count && !list_empty(rd_regions))
-		return -EINVAL;
+	if (!count && !list_empty(rd_regions)) {
+		rdreg = list_last_entry(rd_regions,
+				       struct vgic_redist_region, list);
+		if (rdreg->count)
+			return -EINVAL; /* Mixing REDIST and REDIST_REGION API */
+		return -EEXIST;
+	}
 
 	/* cross the end of memory ? */
 	if (base + size < base)
-- 
2.21.3

[PATCH 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
and thus the index always is 0.

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>
---
 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.21.3

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

[Eric Auger]

vgic_register_all_redist_iodevs may succeed while
vgic_register_all_redist_iodevs fails. For example this can happen
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 such a case, remove the newly added redistributor region and free
it.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 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 8e8a862def76..581f0f490000 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -866,8 +866,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 =
+			vgic_v3_rdist_region_from_index(kvm, index);
+
+		list_del(&rdreg->list);
+		kfree(rdreg);
 		return ret;
+	}
 
 	return 0;
 }
-- 
2.21.3

[PATCH 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>
---
 arch/arm64/kvm/vgic/vgic-init.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 32e32d67a127..6147bed56b1b 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -335,14 +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
+		kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
 
 	if (vgic_has_its(kvm))
 		vgic_lpi_translation_cache_destroy(kvm);
@@ -362,6 +364,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.21.3

[PATCH 5/9] KVM: arm: move has_run_once after the map_resources

[Eric Auger]

has_run_once is set to true at the beginning of
kvm_vcpu_first_run_init(). This generally is not an issue
except when exercising the code with KVM selftests. Indeed,
if kvm_vgic_map_resources() fails due to erroneous user settings,
has_run_once is set and this prevents from continuing
executing the test. This patch moves the assignment after the
kvm_vgic_map_resources().

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arch/arm64/kvm/arm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index c0ffb019ca8b..331fae6bff31 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 	if (!kvm_arm_vcpu_is_finalized(vcpu))
 		return -EPERM;
 
-	vcpu->arch.has_run_once = true;
-
 	if (likely(irqchip_in_kernel(kvm))) {
 		/*
 		 * Map the VGIC hardware resources before running a vcpu the
@@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 		static_branch_inc(&userspace_irqchip_in_use);
 	}
 
+	vcpu->arch.has_run_once = true;
+
 	ret = kvm_timer_enable(vcpu);
 	if (ret)
 		return ret;
-- 
2.21.3

[PATCH 6/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>
---
 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..322de6aebdec 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 vcpu creations.
     KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES
       save all LPI pending bits into guest RAM pending tables.
 
-- 
2.21.3

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

[Eric Auger]

Instead of converting the vgic_io_device handle to a kvm_io_device
handled and then do the oppositive, pass a vgic_io_device pointer all
along the call chain.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 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.21.3

[PATCH 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. This patch restores its
support (if the redistributor region was set) while keeping the
code safe.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++-
 include/kvm/arm_vgic.h             | 1 +
 2 files 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 581f0f490000..2f9ef6058f6e 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -277,6 +277,8 @@ 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);
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
 	int target_vcpu_id = vcpu->vcpu_id;
 	u64 value;
 
@@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
 	if (vgic_has_its(vcpu->kvm))
 		value |= GICR_TYPER_PLPIS;
 
-	/* reporting of the Last bit is not supported for userspace */
+	if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
+		value |= GICR_TYPER_LAST;
+
 	return extract_bytes(value, addr & 7, len);
 }
 
@@ -714,6 +718,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 a8d8fdcd3723..596c069263a7 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.21.3

[PATCH 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 (vcpu_first and vgic_first)
- KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION (redist_regions).

Another test dedicates 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>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/aarch64/vgic_init.c | 453 ++++++++++++++++++
 .../testing/selftests/kvm/include/kvm_util.h  |   5 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  51 ++
 4 files changed, 510 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 3d14ef77755e..dc2fd33bd580 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -68,6 +68,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 += kvm_create_max_vcpus
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..e8caa64c0395
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -0,0 +1,453 @@
+// 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
+
+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);
+}
+
+static void guest_code(int cpu)
+{
+	GUEST_SYNC(0);
+	GUEST_SYNC(1);
+	GUEST_SYNC(2);
+	GUEST_DONE();
+}
+
+static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
+{
+	static int run;
+	struct ucall uc;
+	int ret;
+
+	vcpu_args_set(vm, vcpuid, 1, vcpuid);
+	ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
+	get_ucall(vm, vcpuid, &uc);
+	run++;
+
+	if (ret)
+		return -errno;
+	return 0;
+}
+
+int dist_rdist_tests(struct kvm_vm *vm)
+{
+	int ret, gicv3_fd, max_ipa_bits;
+	uint64_t addr;
+
+	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
+
+	ret = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
+	if (ret) {
+		print_skip("GICv3 not supported");
+		exit(KSFT_SKIP);
+	}
+
+	ret = kvm_create_device(vm, 0, true);
+	TEST_ASSERT(ret == -ENODEV, "unsupported device");
+
+	/* Create the device */
+
+	gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(gicv3_fd > 0, "GICv3 device created");
+
+	/* Check attributes */
+
+	ret = kvm_device_check_attr(gicv3_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(gicv3_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");
+
+	ret = kvm_device_check_attr(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
+	TEST_ASSERT(ret == -ENXIO, "attribute not supported");
+
+	/* misaligned DIST and REDIST addresses */
+
+	addr = 0x1000;
+	ret = kvm_device_access(gicv3_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(gicv3_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(gicv3_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(gicv3_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 */
+	addr = 0x00000;
+	ret = kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+	TEST_ASSERT(!ret, "GICv3 redist base set");
+
+	addr = 0xE0000;
+	ret = kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+	TEST_ASSERT(ret == -EEXIST, "GICv3 redist base set again");
+
+	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 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 == -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(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_DIST,
+				&addr, true);
+	TEST_ASSERT(!ret, "dist overlapping rdist");
+
+	ret = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	TEST_ASSERT(ret == -EEXIST, "create GICv3 device twice");
+
+	ret = kvm_create_device(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(vm, KVM_DEV_TYPE_ARM_VGIC_V2, true)) {
+		ret = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V2, true);
+		TEST_ASSERT(ret == -EINVAL, "create GICv2 while v3 exists");
+	}
+
+	return gicv3_fd;
+}
+
+static int redist_region_tests(struct kvm_vm *vm, int gicv3_fd)
+{
+	int ret, max_ipa_bits;
+	uint64_t addr, expected_addr;
+
+	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
+
+	ret = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
+	if (ret) {
+		print_skip("GICv3 not supported");
+		exit(KSFT_SKIP);
+	}
+
+	if (gicv3_fd < 0) {
+		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_check_attr(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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
+	 * now 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(gicv3_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(gicv3_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(gicv3_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(gicv3_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(gicv3_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");
+
+	return gicv3_fd;
+}
+
+static void vgic_first(void)
+{
+	int ret, i, gicv3_fd;
+	struct kvm_vm *vm;
+
+	vm = vm_create_default(0, 0, guest_code);
+
+	gicv3_fd = dist_rdist_tests(vm);
+
+	/* Add the rest of the VCPUs */
+	for (i = 1; i < NR_VCPUS; ++i)
+		vm_vcpu_add_default(vm, i, guest_code);
+
+	ret = run_vcpu(vm, 3);
+	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
+
+	close(gicv3_fd);
+	kvm_vm_free(vm);
+}
+
+
+static void vcpu_first(void)
+{
+	int ret, i, gicv3_fd;
+	struct kvm_vm *vm;
+
+	vm = vm_create_default(0, 0, guest_code);
+
+	/* Add the rest of the VCPUs */
+	for (i = 1; i < NR_VCPUS; ++i)
+		vm_vcpu_add_default(vm, i, guest_code);
+
+	gicv3_fd = dist_rdist_tests(vm);
+
+	ret = run_vcpu(vm, 3);
+	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
+
+	close(gicv3_fd);
+	kvm_vm_free(vm);
+}
+
+static void redist_regions(void)
+{
+	int ret, i, gicv3_fd = -1;
+	struct kvm_vm *vm;
+	uint64_t addr;
+	void *dummy = NULL;
+
+	vm = vm_create_default(0, 0, guest_code);
+	ucall_init(vm, NULL);
+
+	/* Add the rest of the VCPUs */
+	for (i = 1; i < NR_VCPUS; ++i)
+		vm_vcpu_add_default(vm, i, guest_code);
+
+	gicv3_fd = redist_region_tests(vm, gicv3_fd);
+
+	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");
+
+	ret = run_vcpu(vm, 3);
+	TEST_ASSERT(ret == -ENXIO, "running without sufficient number of rdists");
+
+	/*
+	 * At this time the kvm_vgic_map_resources destroyed the vgic
+	 * Redo everything
+	 */
+	gicv3_fd = redist_region_tests(vm, gicv3_fd);
+
+	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 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, "register a third region allowing to cover the 4 vcpus");
+
+	ret = run_vcpu(vm, 3);
+	TEST_ASSERT(ret == -EBUSY, "running without vgic explicit init");
+
+	/* again need to redo init and this time do the explicit init*/
+	gicv3_fd = redist_region_tests(vm, gicv3_fd);
+
+	ret = kvm_device_access(gicv3_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(gicv3_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(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+				KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+	TEST_ASSERT(!ret, "init the vgic");
+
+	ret = run_vcpu(vm, 3);
+	TEST_ASSERT(!ret, "vcpu run");
+
+	close(gicv3_fd);
+	kvm_vm_free(vm);
+}
+
+static void 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);
+	ucall_init(vm, NULL);
+
+	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);
+}
+
+int main(int ac, char **av)
+{
+	vcpu_first();
+	vgic_first();
+	redist_regions();
+	typer_accesses();
+
+	return 0;
+}
+
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 7d29aa786959..0fecea13a570 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -200,6 +200,11 @@ int vcpu_nested_state_set(struct kvm_vm *vm, uint32_t vcpuid,
 			  struct kvm_nested_state *state, bool ignore_error);
 #endif
 
+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_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 126c6727a6b0..e3ec381e8c0c 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1582,6 +1582,57 @@ void vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
 		cmd, ret, errno, strerror(errno));
 }
 
+/*
+ * 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;
+
+	ret = ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attribute);
+	if (ret == -1)
+		return -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_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)
+		return -errno;
+	return ret;
+}
+
 /*
  * VM Dump
  *
-- 
2.21.3

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

[Marc Zyngier]

Hi Eric,

On Sat, 12 Dec 2020 18:50:04 +0000,
Eric Auger <eric.auger@redhat.com> wrote:
> 
> vgic_register_all_redist_iodevs may succeed while
> vgic_register_all_redist_iodevs fails. For example this can happen

The same function cannot both fail and succeed ;-) Can you shed some
light on what you had in mind?

> 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 such a case, remove the newly added redistributor region and free
> it.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  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 8e8a862def76..581f0f490000 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -866,8 +866,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 =
> +			vgic_v3_rdist_region_from_index(kvm, index);
> +

nit: consider splitting declaration and assignment so that we avoid
the line split if you insist on the 80 character limit.

> +		list_del(&rdreg->list);
> +		kfree(rdreg);
>  		return ret;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.21.3
> 
> 

Thanks,

	M.

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

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

[Marc Zyngier]

On Sat, 12 Dec 2020 18:50:05 +0000,
Eric Auger <eric.auger@redhat.com> wrote:
> 
> 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>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 32e32d67a127..6147bed56b1b 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -335,14 +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
> +		kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;

Since you have converted the hunk above to use dist->, you could do
the same thing here. And the coding style dictates that you need {} on
the else side as well.

>
>  	if (vgic_has_its(kvm))
>  		vgic_lpi_translation_cache_destroy(kvm);
> @@ -362,6 +364,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.21.3
> 
> 

Thanks,

	M.

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

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

[Alexandru Elisei]

Hi Eric,

On 12/12/20 6:50 PM, Eric Auger wrote:
> 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>
> ---
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 15a6c98ee92f..8e8a862def76 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -792,8 +792,13 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
>  	int ret;
>  
>  	/* single rdist region already set ?*/
> -	if (!count && !list_empty(rd_regions))
> -		return -EINVAL;
> +	if (!count && !list_empty(rd_regions)) {
> +		rdreg = list_last_entry(rd_regions,
> +				       struct vgic_redist_region, list);
> +		if (rdreg->count)
> +			return -EINVAL; /* Mixing REDIST and REDIST_REGION API */
> +		return -EEXIST;
> +	}

A few instructions below:

    if (list_empty(rd_regions)) {
        [..]
    } else {
        rdreg = list_last_entry(rd_regions,
                    struct vgic_redist_region, list);
        [..]

        /* Cannot add an explicitly sized regions after legacy region */
        if (!rdreg->count)
            return -EINVAL;
    }

Isn't this testing for the same thing, but using the opposite condition? Or am I
misunderstanding the code (quite likely)?

Looks to me like KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST{,_REGION})
used to return -EEXIST (from vgic_check_ioaddr()) before commit ccc27bf5be7b7
("KVM: arm/arm64: Helper to register a new redistributor region") which added the
vgic_v3_insert_redist_region() function, so bringing back the -EEXIST return code
looks the right thing to me.

Thanks,
Alex
>  
>  	/* cross the end of memory ? */
>  	if (base + size < base)

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

[Alexandru Elisei]

Hi Eric,

The patch looks correct to me. kvm_vgic_addr() masks out all the bits except index
from addr, so we don't need to do it in vgic_get_common_attr():

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

One nitpick below.

On 12/12/20 6:50 PM, Eric Auger wrote:
> 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
> and thus the index always is 0.

addr is allocated on the stack, I don't think it will always be 0.

Thanks,
Alex
>
> 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>
> ---
>  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;

Re: [PATCH 5/9] KVM: arm: move has_run_once after the map_resources

[Alexandru Elisei]

Hi Eric,

On 12/12/20 6:50 PM, Eric Auger wrote:
> has_run_once is set to true at the beginning of
> kvm_vcpu_first_run_init(). This generally is not an issue
> except when exercising the code with KVM selftests. Indeed,
> if kvm_vgic_map_resources() fails due to erroneous user settings,
> has_run_once is set and this prevents from continuing
> executing the test. This patch moves the assignment after the
> kvm_vgic_map_resources().
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arch/arm64/kvm/arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index c0ffb019ca8b..331fae6bff31 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  	if (!kvm_arm_vcpu_is_finalized(vcpu))
>  		return -EPERM;
>  
> -	vcpu->arch.has_run_once = true;
> -
>  	if (likely(irqchip_in_kernel(kvm))) {
>  		/*
>  		 * Map the VGIC hardware resources before running a vcpu the
> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  		static_branch_inc(&userspace_irqchip_in_use);
>  	}
>  
> +	vcpu->arch.has_run_once = true;

I have a few concerns regarding this:

1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable()
and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user
values. If there's a reason why the assignment cannot be moved at the end of the
function, I think it should be clearly stated in a comment for the people who
might be tempted to write similar tests for the timer or pmu.

2. There are many ways that kvm_vgic_map_resources() can fail, other than
incorrect user settings. I started digging into how
kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this
is what I managed to find before I gave up:

* vgic_init() can fail in:
    - kvm_vgic_dist_init()
    - vgic_v3_init()
    - kvm_vgic_setup_default_irq_routing()
* vgic_register_dist_iodev() can fail in:
    - vgic_v3_init_dist_iodev()
    - kvm_io_bus_register_dev()(*)
* kvm_phys_addr_ioremap() can fail in:
    - kvm_mmu_topup_memory_cache()
    - kvm_pgtable_stage2_map()

So if any of the functions below fail, are we 100% sure it is safe to allow the
user to execute kvm_vgic_map_resources() again?

(*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a
caller that tries to register the same device address range and it will create
another identical range. Is this intentional? Is it a bug that should be fixed? Or
am I misunderstanding the function?

Thanks,
Alex
> +
>  	ret = kvm_timer_enable(vcpu);
>  	if (ret)
>  		return ret;

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

[Alexandru Elisei]

Hi Eric,

On 12/12/20 6:50 PM, Eric Auger wrote:
> 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.

Checked and this is indeed the case,
kvm_vm_ioctl_create_vcpu()->kvm_arch_vcpu_precreate() returns -EBUSY is
vgic_initialized() is true.

>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  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..322de6aebdec 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 vcpu creations.

Nitpick here: the document writes VCPU with all caps. This also sounds a bit
weird, I think something like "Must be called after all VCPUs have been created"
is clearer.

Thanks,
Alex
>      KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES
>        save all LPI pending bits into guest RAM pending tables.
>  

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

[Alexandru Elisei]

Hi Eric,

On 12/12/20 6:50 PM, Eric Auger wrote:
> Instead of converting the vgic_io_device handle to a kvm_io_device
> handled and then do the oppositive, pass a vgic_io_device pointer all
> along the call chain.

To me, it looks like the commit message describes what the patch does instead of
why it does it.

What are "vgic_io_device handle" and "kvm_io_device handled"?

Thanks,
Alex
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  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,

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

[Alexandru Elisei]

Hi Eric,

On 1/12/21 4:04 PM, Alexandru Elisei wrote:
> Hi Eric,
>
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> Instead of converting the vgic_io_device handle to a kvm_io_device
>> handled and then do the oppositive, pass a vgic_io_device pointer all
>> along the call chain.
> To me, it looks like the commit message describes what the patch does instead of
> why it does it.
>
> What are "vgic_io_device handle" and "kvm_io_device handled"?

Sorry, I think I got it now. You were referring to the argument types struct
vgic_io_device and struct kvm_io_device. The patch looks like a very good cleanup.

How changing to commit message to sound something like this (feel free to
ignore/change it if you think of something else):

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).

Thanks,
Alex
>
> Thanks,
> Alex
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  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,
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Alexandru Elisei]

Hi Eric,

On 12/12/20 6:50 PM, Eric Auger wrote:
> 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. This patch restores its
> support (if the redistributor region was set) while keeping the
> code safe.

I suppose the reason for emulating GICR_TYPER.Last is for architecture compliance,
right? I think that should be in the commit message.

>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++-
>  include/kvm/arm_vgic.h             | 1 +
>  2 files 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 581f0f490000..2f9ef6058f6e 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -277,6 +277,8 @@ 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);
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>  	int target_vcpu_id = vcpu->vcpu_id;
>  	u64 value;
>  
> @@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
>  	if (vgic_has_its(vcpu->kvm))
>  		value |= GICR_TYPER_PLPIS;
>  
> -	/* reporting of the Last bit is not supported for userspace */
> +	if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
> +		value |= GICR_TYPER_LAST;
> +
>  	return extract_bytes(value, addr & 7, len);
>  }
>  
> @@ -714,6 +718,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  		return -EINVAL;
>  
>  	vgic_cpu->rdreg = rdreg;
> +	vgic_cpu->rdreg_index = rdreg->free_index;

What happens if the next redistributor region we register has the base address
adjacent to this one?

I'm really not familiar with the code, but is it not possible to create two
Redistributor regions (via
KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST)) where the second
Redistributor region start address is immediately after the last Redistributor in
the preceding region?

Thanks,
Alex
>  
>  	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 a8d8fdcd3723..596c069263a7 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;

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

[Alexandru Elisei]

Hi Eric,

On 12/12/20 6:50 PM, Eric Auger wrote:
> 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. This patch restores its
> support (if the redistributor region was set) while keeping the
> code safe.

If I understand your patch correctly, it is possible for the GICR_TYPER.Last bit
to be transiently 1 if the register is accessed before all the redistributors
regions have been configured.

Arm IHI 0069F states that accesses to the GICR_TYPER register are RO. I haven't
found exactly what RO means (please point me to the definition if you find it in
the architecture!), but I assume it means read-only and I'm not sure how correct
(from an architectural point of view) it is for two subsequent reads of this
register to return different values. Maybe Marc can shed some light on this.

Thanks,
Alex
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++-
>  include/kvm/arm_vgic.h             | 1 +
>  2 files 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 581f0f490000..2f9ef6058f6e 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -277,6 +277,8 @@ 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);
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>  	int target_vcpu_id = vcpu->vcpu_id;
>  	u64 value;
>  
> @@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
>  	if (vgic_has_its(vcpu->kvm))
>  		value |= GICR_TYPER_PLPIS;
>  
> -	/* reporting of the Last bit is not supported for userspace */
> +	if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
> +		value |= GICR_TYPER_LAST;
> +
>  	return extract_bytes(value, addr & 7, len);
>  }
>  
> @@ -714,6 +718,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 a8d8fdcd3723..596c069263a7 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;

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

[Marc Zyngier]

On 2021-01-12 17:28, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> 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. This patch restores its
>> support (if the redistributor region was set) while keeping the
>> code safe.
> 
> If I understand your patch correctly, it is possible for the 
> GICR_TYPER.Last bit
> to be transiently 1 if the register is accessed before all the 
> redistributors
> regions have been configured.
> 
> Arm IHI 0069F states that accesses to the GICR_TYPER register are RO. I 
> haven't
> found exactly what RO means (please point me to the definition if you 
> find it in
> the architecture!), but I assume it means read-only and I'm not sure 
> how correct
> (from an architectural point of view) it is for two subsequent reads of 
> this
> register to return different values. Maybe Marc can shed some light on 
> this.

RO = Read-Only indeed. Not sure that's documented anywhere in the 
architecture,
but this is enough of a well known acronym that even the ARM ARM doesn't 
feel
the need to invent a new definition for it.

As for your concern, I don't think it is a problem to return different 
values
if the HW has changed in between. This may need to be documented though.

Thanks,

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

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

[Auger Eric]

Hi Alexandru,

On 1/12/21 4:39 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> 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.
> 
> Checked and this is indeed the case,
> kvm_vm_ioctl_create_vcpu()->kvm_arch_vcpu_precreate() returns -EBUSY is
> vgic_initialized() is true.
thanks!
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  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..322de6aebdec 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 vcpu creations.
> 
> Nitpick here: the document writes VCPU with all caps. This also sounds a bit
> weird, I think something like "Must be called after all VCPUs have been created"
> is clearer.
I took your suggestion.

Thanks

Eric
> 
> Thanks,
> Alex
>>      KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES
>>        save all LPI pending bits into guest RAM pending tables.
>>  
> 

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

[Auger Eric]

Hi Alexandru,
On 1/12/21 5:16 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 1/12/21 4:04 PM, Alexandru Elisei wrote:
>> Hi Eric,
>>
>> On 12/12/20 6:50 PM, Eric Auger wrote:
>>> Instead of converting the vgic_io_device handle to a kvm_io_device
>>> handled and then do the oppositive, pass a vgic_io_device pointer all
>>> along the call chain.
>> To me, it looks like the commit message describes what the patch does instead of
>> why it does it.
>>
>> What are "vgic_io_device handle" and "kvm_io_device handled"?
Yes unfortunate typo, sorry.
> 
> Sorry, I think I got it now. You were referring to the argument types struct
> vgic_io_device and struct kvm_io_device. The patch looks like a very good cleanup.
> 
> How changing to commit message to sound something like this (feel free to
> ignore/change it if you think of something else):
> 
> 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).
I reworded the commit message as you suggested.

Thanks

Eric
> 
> Thanks,
> Alex
>>
>> Thanks,
>> Alex
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  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,
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

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

[Auger Eric]

Hi Alexandru,

On 1/6/21 6:12 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> The patch looks correct to me. kvm_vgic_addr() masks out all the bits except index
> from addr, so we don't need to do it in vgic_get_common_attr():
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> One nitpick below.
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> 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
>> and thus the index always is 0.
> 
> addr is allocated on the stack, I don't think it will always be 0.
I removed this statement in the commit message. Thanks!

Eric
> 
> Thanks,
> Alex
>>
>> 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>
>> ---
>>  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;
> 

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

[Auger Eric]

Hi Marc,

On 12/28/20 4:41 PM, Marc Zyngier wrote:
> On Sat, 12 Dec 2020 18:50:05 +0000,
> Eric Auger <eric.auger@redhat.com> wrote:
>>
>> 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>
>> ---
>>  arch/arm64/kvm/vgic/vgic-init.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
>> index 32e32d67a127..6147bed56b1b 100644
>> --- a/arch/arm64/kvm/vgic/vgic-init.c
>> +++ b/arch/arm64/kvm/vgic/vgic-init.c
>> @@ -335,14 +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
>> +		kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
> 
> Since you have converted the hunk above to use dist->, you could do
> the same thing here. And the coding style dictates that you need {} on
> the else side as well.
sure

Thanks

Eric
> 
>>
>>  	if (vgic_has_its(kvm))
>>  		vgic_lpi_translation_cache_destroy(kvm);
>> @@ -362,6 +364,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.21.3
>>
>>
> 
> Thanks,
> 
> 	M.
> 

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

[Auger Eric]

Hi Marc,

On 12/28/20 4:35 PM, Marc Zyngier wrote:
> Hi Eric,
> 
> On Sat, 12 Dec 2020 18:50:04 +0000,
> Eric Auger <eric.auger@redhat.com> wrote:
>>
>> vgic_register_all_redist_iodevs may succeed while
>> vgic_register_all_redist_iodevs fails. For example this can happen
> 
> The same function cannot both fail and succeed ;-) Can you shed some
> light on what you had in mind?

Damn, I meant vgic_v3_insert_redist_region() can be successful and then
vgic_register_all_redist_iodevs() fails due to detection of overlap.
> 
>> 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 such a case, remove the newly added redistributor region and free
>> it.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  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 8e8a862def76..581f0f490000 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -866,8 +866,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 =
>> +			vgic_v3_rdist_region_from_index(kvm, index);
>> +
> 
> nit: consider splitting declaration and assignment so that we avoid
> the line split if you insist on the 80 character limit.
Sure

Thanks

Eric
> 
>> +		list_del(&rdreg->list);
>> +		kfree(rdreg);
>>  		return ret;
>> +	}
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.21.3
>>
>>
> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH 5/9] KVM: arm: move has_run_once after the map_resources

[Auger Eric]

Hi Alexandru,

On 1/12/21 3:55 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> has_run_once is set to true at the beginning of
>> kvm_vcpu_first_run_init(). This generally is not an issue
>> except when exercising the code with KVM selftests. Indeed,
>> if kvm_vgic_map_resources() fails due to erroneous user settings,
>> has_run_once is set and this prevents from continuing
>> executing the test. This patch moves the assignment after the
>> kvm_vgic_map_resources().
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  arch/arm64/kvm/arm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index c0ffb019ca8b..331fae6bff31 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>  	if (!kvm_arm_vcpu_is_finalized(vcpu))
>>  		return -EPERM;
>>  
>> -	vcpu->arch.has_run_once = true;
>> -
>>  	if (likely(irqchip_in_kernel(kvm))) {
>>  		/*
>>  		 * Map the VGIC hardware resources before running a vcpu the
>> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>  		static_branch_inc(&userspace_irqchip_in_use);
>>  	}
>>  
>> +	vcpu->arch.has_run_once = true;
> 
> I have a few concerns regarding this:
> 
> 1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable()
> and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user
> values. If there's a reason why the assignment cannot be moved at the end of the
> function, I think it should be clearly stated in a comment for the people who
> might be tempted to write similar tests for the timer or pmu.

Setting has_run_once = true at the entry of the function looks to me
even more arbitrary. I agree with you that eventually has_run_once may
be moved at the very end but maybe this can be done later once timer,
pmu tests haven ben written
> 
> 2. There are many ways that kvm_vgic_map_resources() can fail, other than
> incorrect user settings. I started digging into how
> kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this
> is what I managed to find before I gave up:
> 
> * vgic_init() can fail in:
>     - kvm_vgic_dist_init()
>     - vgic_v3_init()
>     - kvm_vgic_setup_default_irq_routing()
> * vgic_register_dist_iodev() can fail in:
>     - vgic_v3_init_dist_iodev()
>     - kvm_io_bus_register_dev()(*)
> * kvm_phys_addr_ioremap() can fail in:
>     - kvm_mmu_topup_memory_cache()
>     - kvm_pgtable_stage2_map()

I changed the commit msg so that "incorrect user settings" sounds as an
example.
> 
> So if any of the functions below fail, are we 100% sure it is safe to allow the
> user to execute kvm_vgic_map_resources() again?

I think additional tests will confirm this. However at the moment,
moving the assignment, which does not look wrong to me, allows to
greatly simplify the tests so I would tend to say that it is worth.
> 
> (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a
> caller that tries to register the same device address range and it will create
> another identical range. Is this intentional? Is it a bug that should be fixed? Or
> am I misunderstanding the function?

doesn't kvm_io_bus_cmp() do the check?

Thanks

Eric
> 
> Thanks,
> Alex
>> +
>>  	ret = kvm_timer_enable(vcpu);
>>  	if (ret)
>>  		return ret;
> 

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

[Auger Eric]

Hi Alexandru,

On 1/6/21 5:32 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> 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>
>> ---
>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> index 15a6c98ee92f..8e8a862def76 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -792,8 +792,13 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
>>  	int ret;
>>  
>>  	/* single rdist region already set ?*/
>> -	if (!count && !list_empty(rd_regions))
>> -		return -EINVAL;
>> +	if (!count && !list_empty(rd_regions)) {
>> +		rdreg = list_last_entry(rd_regions,
>> +				       struct vgic_redist_region, list);
>> +		if (rdreg->count)
>> +			return -EINVAL; /* Mixing REDIST and REDIST_REGION API */
>> +		return -EEXIST;
>> +	}
> 
> A few instructions below:
> 
>     if (list_empty(rd_regions)) {
>         [..]
>     } else {
>         rdreg = list_last_entry(rd_regions,
>                     struct vgic_redist_region, list);
>         [..]
> 
>         /* Cannot add an explicitly sized regions after legacy region */
>         if (!rdreg->count)
>             return -EINVAL;
>     }
> 
> Isn't this testing for the same thing, but using the opposite condition? Or am I
> misunderstanding the code (quite likely)?
the 1st test sequence handles the case where the legacy
KVM_VGIC_V3_ADDR_TYPE_REDIST is used (!count) while the second handles
the case where the REDIST_REGION is used. Nevertheless I think this can
be simplified into:

        if (list_empty(rd_regions)) {
                if (index != 0)
                        return -EINVAL;
        } else {
                rdreg = list_last_entry(rd_regions,
                                        struct vgic_redist_region, list);

                if ((!count) != (!rdreg->count))
                        return -EINVAL; /* Mix REDIST and REDIST_REGION */

                if (!count)
                        return -EEXIST;

                if (index != rdreg->index + 1)
                        return -EINVAL;
        }






> 
> Looks to me like KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST{,_REGION})
> used to return -EEXIST (from vgic_check_ioaddr()) before commit ccc27bf5be7b7
> ("KVM: arm/arm64: Helper to register a new redistributor region") which added the
> vgic_v3_insert_redist_region() function, so bringing back the -EEXIST return code
> looks the right thing to me.

OK thank you for the detailed study.

Eric
> 
> Thanks,
> Alex
>>  
>>  	/* cross the end of memory ? */
>>  	if (base + size < base)
> 

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

[Auger Eric]

Hi Alexandru,

On 1/12/21 6:02 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> 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. This patch restores its
>> support (if the redistributor region was set) while keeping the
>> code safe.
> 
> I suppose the reason for emulating GICR_TYPER.Last is for architecture compliance,
> right? I think that should be in the commit message.
OK added this in the commit msg.
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++-
>>  include/kvm/arm_vgic.h             | 1 +
>>  2 files 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 581f0f490000..2f9ef6058f6e 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -277,6 +277,8 @@ 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);
>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> +	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>>  	int target_vcpu_id = vcpu->vcpu_id;
>>  	u64 value;
>>  
>> @@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
>>  	if (vgic_has_its(vcpu->kvm))
>>  		value |= GICR_TYPER_PLPIS;
>>  
>> -	/* reporting of the Last bit is not supported for userspace */
>> +	if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
>> +		value |= GICR_TYPER_LAST;
>> +
>>  	return extract_bytes(value, addr & 7, len);
>>  }
>>  
>> @@ -714,6 +718,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>  		return -EINVAL;
>>  
>>  	vgic_cpu->rdreg = rdreg;
>> +	vgic_cpu->rdreg_index = rdreg->free_index;
> 
> What happens if the next redistributor region we register has the base address
> adjacent to this one?
> 
> I'm really not familiar with the code, but is it not possible to create two
> Redistributor regions (via
> KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST)) where the second
> Redistributor region start address is immediately after the last Redistributor in
> the preceding region?
KVM_VGIC_V3_ADDR_TYPE_REDIST only allows to create a single rdist
region. Only KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION allows to register
several of them.

with KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, it is possible to register
adjacent rdist regions. vgic_v3_rdist_free_slot() previously returned
the 1st rdist region where enough space remains for inserting the new
reg. We put the rdist at the free index there.

But maybe I misunderstood your question?

Thanks

Eric
> 
> Thanks,
> Alex
>>  
>>  	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 a8d8fdcd3723..596c069263a7 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;
> 

Re: [PATCH 5/9] KVM: arm: move has_run_once after the map_resources

[Alexandru Elisei]

Hi Eric,

On 1/14/21 10:02 AM, Auger Eric wrote:
> Hi Alexandru,
>
> On 1/12/21 3:55 PM, Alexandru Elisei wrote:
>> Hi Eric,
>>
>> On 12/12/20 6:50 PM, Eric Auger wrote:
>>> has_run_once is set to true at the beginning of
>>> kvm_vcpu_first_run_init(). This generally is not an issue
>>> except when exercising the code with KVM selftests. Indeed,
>>> if kvm_vgic_map_resources() fails due to erroneous user settings,
>>> has_run_once is set and this prevents from continuing
>>> executing the test. This patch moves the assignment after the
>>> kvm_vgic_map_resources().
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  arch/arm64/kvm/arm.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>> index c0ffb019ca8b..331fae6bff31 100644
>>> --- a/arch/arm64/kvm/arm.c
>>> +++ b/arch/arm64/kvm/arm.c
>>> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>>  	if (!kvm_arm_vcpu_is_finalized(vcpu))
>>>  		return -EPERM;
>>>  
>>> -	vcpu->arch.has_run_once = true;
>>> -
>>>  	if (likely(irqchip_in_kernel(kvm))) {
>>>  		/*
>>>  		 * Map the VGIC hardware resources before running a vcpu the
>>> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>>  		static_branch_inc(&userspace_irqchip_in_use);
>>>  	}
>>>  
>>> +	vcpu->arch.has_run_once = true;
>> I have a few concerns regarding this:
>>
>> 1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable()
>> and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user
>> values. If there's a reason why the assignment cannot be moved at the end of the
>> function, I think it should be clearly stated in a comment for the people who
>> might be tempted to write similar tests for the timer or pmu.
> Setting has_run_once = true at the entry of the function looks to me
> even more arbitrary. I agree with you that eventually has_run_once may

Or it could be it's there to prevent the user from calling
kvm_vgic_map_resources() a second time after it failed. This is what I'm concerned
about and I think deserves more investigation.

Thanks,
Alex
> be moved at the very end but maybe this can be done later once timer,
> pmu tests haven ben written
>> 2. There are many ways that kvm_vgic_map_resources() can fail, other than
>> incorrect user settings. I started digging into how
>> kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this
>> is what I managed to find before I gave up:
>>
>> * vgic_init() can fail in:
>>     - kvm_vgic_dist_init()
>>     - vgic_v3_init()
>>     - kvm_vgic_setup_default_irq_routing()
>> * vgic_register_dist_iodev() can fail in:
>>     - vgic_v3_init_dist_iodev()
>>     - kvm_io_bus_register_dev()(*)
>> * kvm_phys_addr_ioremap() can fail in:
>>     - kvm_mmu_topup_memory_cache()
>>     - kvm_pgtable_stage2_map()
> I changed the commit msg so that "incorrect user settings" sounds as an
> example.
>> So if any of the functions below fail, are we 100% sure it is safe to allow the
>> user to execute kvm_vgic_map_resources() again?
> I think additional tests will confirm this. However at the moment,
> moving the assignment, which does not look wrong to me, allows to
> greatly simplify the tests so I would tend to say that it is worth.
>> (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a
>> caller that tries to register the same device address range and it will create
>> another identical range. Is this intentional? Is it a bug that should be fixed? Or
>> am I misunderstanding the function?
> doesn't kvm_io_bus_cmp() do the check?
>
> Thanks
>
> Eric
>> Thanks,
>> Alex
>>> +
>>>  	ret = kvm_timer_enable(vcpu);
>>>  	if (ret)
>>>  		return ret;

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

[Alexandru Elisei]

Hi Eric,

On 1/14/21 10:16 AM, Auger Eric wrote:
> Hi Alexandru,
>
> On 1/12/21 6:02 PM, Alexandru Elisei wrote:
>> Hi Eric,
>>
>> On 12/12/20 6:50 PM, Eric Auger wrote:
>>> 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. This patch restores its
>>> support (if the redistributor region was set) while keeping the
>>> code safe.
>> I suppose the reason for emulating GICR_TYPER.Last is for architecture compliance,
>> right? I think that should be in the commit message.
> OK added this in the commit msg.
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++-
>>>  include/kvm/arm_vgic.h             | 1 +
>>>  2 files 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 581f0f490000..2f9ef6058f6e 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> @@ -277,6 +277,8 @@ 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);
>>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> +	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>>>  	int target_vcpu_id = vcpu->vcpu_id;
>>>  	u64 value;
>>>  
>>> @@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
>>>  	if (vgic_has_its(vcpu->kvm))
>>>  		value |= GICR_TYPER_PLPIS;
>>>  
>>> -	/* reporting of the Last bit is not supported for userspace */
>>> +	if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
>>> +		value |= GICR_TYPER_LAST;
>>> +
>>>  	return extract_bytes(value, addr & 7, len);
>>>  }
>>>  
>>> @@ -714,6 +718,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>>  		return -EINVAL;
>>>  
>>>  	vgic_cpu->rdreg = rdreg;
>>> +	vgic_cpu->rdreg_index = rdreg->free_index;
>> What happens if the next redistributor region we register has the base address
>> adjacent to this one?
>>
>> I'm really not familiar with the code, but is it not possible to create two
>> Redistributor regions (via
>> KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST)) where the second
>> Redistributor region start address is immediately after the last Redistributor in
>> the preceding region?
> KVM_VGIC_V3_ADDR_TYPE_REDIST only allows to create a single rdist
> region. Only KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION allows to register
> several of them.
>
> with KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, it is possible to register
> adjacent rdist regions. vgic_v3_rdist_free_slot() previously returned
> the 1st rdist region where enough space remains for inserting the new
> reg. We put the rdist at the free index there.
>
> But maybe I misunderstood your question?

Yes, I think you did a good job at answering my poorly worded question.

This is the case I am concerned about:

1. Userspace sets first redistributor base address to 0x0 via
KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION(count = 1, base = 0x0, flags = 0, index = 0).

2. Userspace sets first redistributor base address to 0x0 + 128K, immediately
following the previous Redistributor.

In that case the two Redistributors will be represented by two separate struct
vgic_redist_region, but they are adjacent to one another and represent one
contiguous memory region.

From what I understand from your patch, GICR_TYPER.Last will be set for both
Redistributors, when it should be set only for the second Redistributor. Does any
of that make sense?

Thanks,
Alex
>
> Thanks
>
> Eric
>> Thanks,
>> Alex
>>>  
>>>  	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 a8d8fdcd3723..596c069263a7 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;

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

[Auger Eric]

Hi Alexandru,

On 1/20/21 5:13 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 1/14/21 10:16 AM, Auger Eric wrote:
>> Hi Alexandru,
>>
>> On 1/12/21 6:02 PM, Alexandru Elisei wrote:
>>> Hi Eric,
>>>
>>> On 12/12/20 6:50 PM, Eric Auger wrote:
>>>> 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. This patch restores its
>>>> support (if the redistributor region was set) while keeping the
>>>> code safe.
>>> I suppose the reason for emulating GICR_TYPER.Last is for architecture compliance,
>>> right? I think that should be in the commit message.
>> OK added this in the commit msg.
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++-
>>>>  include/kvm/arm_vgic.h             | 1 +
>>>>  2 files 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 581f0f490000..2f9ef6058f6e 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>> @@ -277,6 +277,8 @@ 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);
>>>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>> +	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>>>>  	int target_vcpu_id = vcpu->vcpu_id;
>>>>  	u64 value;
>>>>  
>>>> @@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
>>>>  	if (vgic_has_its(vcpu->kvm))
>>>>  		value |= GICR_TYPER_PLPIS;
>>>>  
>>>> -	/* reporting of the Last bit is not supported for userspace */
>>>> +	if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
>>>> +		value |= GICR_TYPER_LAST;
>>>> +
>>>>  	return extract_bytes(value, addr & 7, len);
>>>>  }
>>>>  
>>>> @@ -714,6 +718,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>>>  		return -EINVAL;
>>>>  
>>>>  	vgic_cpu->rdreg = rdreg;
>>>> +	vgic_cpu->rdreg_index = rdreg->free_index;
>>> What happens if the next redistributor region we register has the base address
>>> adjacent to this one?
>>>
>>> I'm really not familiar with the code, but is it not possible to create two
>>> Redistributor regions (via
>>> KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST)) where the second
>>> Redistributor region start address is immediately after the last Redistributor in
>>> the preceding region?
>> KVM_VGIC_V3_ADDR_TYPE_REDIST only allows to create a single rdist
>> region. Only KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION allows to register
>> several of them.
>>
>> with KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, it is possible to register
>> adjacent rdist regions. vgic_v3_rdist_free_slot() previously returned
>> the 1st rdist region where enough space remains for inserting the new
>> reg. We put the rdist at the free index there.
>>
>> But maybe I misunderstood your question?
> 
> Yes, I think you did a good job at answering my poorly worded question.
> 
> This is the case I am concerned about:
> 
> 1. Userspace sets first redistributor base address to 0x0 via
> KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION(count = 1, base = 0x0, flags = 0, index = 0).
> 
> 2. Userspace sets first redistributor base address to 0x0 + 128K, immediately
> following the previous Redistributor.
> 
> In that case the two Redistributors will be represented by two separate struct
> vgic_redist_region, but they are adjacent to one another and represent one
> contiguous memory region.
> 
> From what I understand from your patch, GICR_TYPER.Last will be set for both
> Redistributors, when it should be set only for the second Redistributor. Does any
> of that make sense?

Please forgive me for not having replied before on this thread.

This is a valid concern. Nothing prevents the redistributor regions from
being contiguous although this is not the goal. Also nothing prevents
vcpu rdists to be laid out within a redist region in non ascending
order. Also redist regions with ascending indices may not have
increasing base addresses.

So this becomes a gas factory for emulating a single bit but I have
reworked this in v3 ;-)

Thanks

Eric





> 
> Thanks,
> Alex
>>
>> Thanks
>>
>> Eric
>>> Thanks,
>>> Alex
>>>>  
>>>>  	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 a8d8fdcd3723..596c069263a7 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;
> 

Re: [PATCH 5/9] KVM: arm: move has_run_once after the map_resources

[Auger Eric]

Hi Alexandru,

On 1/20/21 4:56 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 1/14/21 10:02 AM, Auger Eric wrote:
>> Hi Alexandru,
>>
>> On 1/12/21 3:55 PM, Alexandru Elisei wrote:
>>> Hi Eric,
>>>
>>> On 12/12/20 6:50 PM, Eric Auger wrote:
>>>> has_run_once is set to true at the beginning of
>>>> kvm_vcpu_first_run_init(). This generally is not an issue
>>>> except when exercising the code with KVM selftests. Indeed,
>>>> if kvm_vgic_map_resources() fails due to erroneous user settings,
>>>> has_run_once is set and this prevents from continuing
>>>> executing the test. This patch moves the assignment after the
>>>> kvm_vgic_map_resources().
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  arch/arm64/kvm/arm.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>>> index c0ffb019ca8b..331fae6bff31 100644
>>>> --- a/arch/arm64/kvm/arm.c
>>>> +++ b/arch/arm64/kvm/arm.c
>>>> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>>>  	if (!kvm_arm_vcpu_is_finalized(vcpu))
>>>>  		return -EPERM;
>>>>  
>>>> -	vcpu->arch.has_run_once = true;
>>>> -
>>>>  	if (likely(irqchip_in_kernel(kvm))) {
>>>>  		/*
>>>>  		 * Map the VGIC hardware resources before running a vcpu the
>>>> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>>>  		static_branch_inc(&userspace_irqchip_in_use);
>>>>  	}
>>>>  
>>>> +	vcpu->arch.has_run_once = true;
>>> I have a few concerns regarding this:
>>>
>>> 1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable()
>>> and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user
>>> values. If there's a reason why the assignment cannot be moved at the end of the
>>> function, I think it should be clearly stated in a comment for the people who
>>> might be tempted to write similar tests for the timer or pmu.
>> Setting has_run_once = true at the entry of the function looks to me
>> even more arbitrary. I agree with you that eventually has_run_once may
> 
> Or it could be it's there to prevent the user from calling
> kvm_vgic_map_resources() a second time after it failed. This is what I'm concerned
> about and I think deserves more investigation.

I have reworked my kvm selftests to live without that change.

Thanks

Eric
> 
> Thanks,
> Alex
>> be moved at the very end but maybe this can be done later once timer,
>> pmu tests haven ben written
>>> 2. There are many ways that kvm_vgic_map_resources() can fail, other than
>>> incorrect user settings. I started digging into how
>>> kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this
>>> is what I managed to find before I gave up:
>>>
>>> * vgic_init() can fail in:
>>>     - kvm_vgic_dist_init()
>>>     - vgic_v3_init()
>>>     - kvm_vgic_setup_default_irq_routing()
>>> * vgic_register_dist_iodev() can fail in:
>>>     - vgic_v3_init_dist_iodev()
>>>     - kvm_io_bus_register_dev()(*)
>>> * kvm_phys_addr_ioremap() can fail in:
>>>     - kvm_mmu_topup_memory_cache()
>>>     - kvm_pgtable_stage2_map()
>> I changed the commit msg so that "incorrect user settings" sounds as an
>> example.
>>> So if any of the functions below fail, are we 100% sure it is safe to allow the
>>> user to execute kvm_vgic_map_resources() again?
>> I think additional tests will confirm this. However at the moment,
>> moving the assignment, which does not look wrong to me, allows to
>> greatly simplify the tests so I would tend to say that it is worth.
>>> (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a
>>> caller that tries to register the same device address range and it will create
>>> another identical range. Is this intentional? Is it a bug that should be fixed? Or
>>> am I misunderstanding the function?
>> doesn't kvm_io_bus_cmp() do the check?
>>
>> Thanks
>>
>> Eric
>>> Thanks,
>>> Alex
>>>> +
>>>>  	ret = kvm_timer_enable(vcpu);
>>>>  	if (ret)
>>>>  		return ret;
>