[PATCH 0/4] KVM: arm64: Userspace GICv3 sysreg access fixes and testing

[PATCH 0/4] KVM: arm64: Userspace GICv3 sysreg access fixes and testing

[Marc Zyngier]

As a follow-up to my earlier series at [1], here's a small set of
fixes to address an annoying bug that made ICH_HCR_EL2 unreachable
from userspace -- not something you'd expect.

So the first patch fixes the ordering the the sysreg table, which had
ICH_HCR_EL2 at the wrong spot. The next two ensure that we now check
for the table to be sorted (just like all the other tables). Finally,
the last patch augments the vgic_init selftest to actually check that
we can access these registers

[1] https://lore.kernel.org/r/20250714122634.3334816-1-maz@kernel.org

Marc Zyngier (4):
  KVM: arm64: vgic-v3: Fix ordering of ICH_HCR_EL2
  KVM: arm64: Clarify the check for reset callback in
    check_sysreg_table()
  KVM: arm64: Enforce the sorting of the GICv3 system register table
  KVM: arm64: selftest: vgic-v3: Add basic GICv3 sysreg userspace access
    test

 arch/arm64/kvm/sys_regs.c                     |  20 +-
 arch/arm64/kvm/vgic-sys-reg-v3.c              |   8 +-
 arch/arm64/kvm/vgic/vgic.h                    |   1 +
 tools/testing/selftests/kvm/arm64/vgic_init.c | 219 +++++++++++++++++-
 4 files changed, 237 insertions(+), 11 deletions(-)

-- 
2.39.2

[PATCH 1/4] KVM: arm64: vgic-v3: Fix ordering of ICH_HCR_EL2

[Marc Zyngier]

The sysreg tables are supposed to be sorted so that a binary search
can easily find them. However, ICH_HCR_EL2 is obviously at the wrong
spot.

Move it where it belongs.

Fixes: 9fe9663e47e21 ("KVM: arm64: Expose GICv3 EL2 registers via KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS")
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic-sys-reg-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
index 75aee0148936f..1850f1727eb93 100644
--- a/arch/arm64/kvm/vgic-sys-reg-v3.c
+++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
@@ -421,8 +421,8 @@ static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
 	EL2_REG(ICH_AP1R1_EL2, ich_apr),
 	EL2_REG(ICH_AP1R2_EL2, ich_apr),
 	EL2_REG(ICH_AP1R3_EL2, ich_apr),
-	EL2_REG(ICH_HCR_EL2, ich_reg),
 	EL2_REG_RO(ICC_SRE_EL2, icc_sre),
+	EL2_REG(ICH_HCR_EL2, ich_reg),
 	EL2_REG_RO(ICH_VTR_EL2, ich_vtr),
 	EL2_REG(ICH_VMCR_EL2, ich_reg),
 	EL2_REG(ICH_LR0_EL2, ich_reg),
-- 
2.39.2

Re: [PATCH 1/4] KVM: arm64: vgic-v3: Fix ordering of ICH_HCR_EL2

[Sebastian Ott]

On Fri, 18 Jul 2025, Marc Zyngier wrote:

> The sysreg tables are supposed to be sorted so that a binary search
> can easily find them. However, ICH_HCR_EL2 is obviously at the wrong
> spot.
>
> Move it where it belongs.
>
> Fixes: 9fe9663e47e21 ("KVM: arm64: Expose GICv3 EL2 registers via KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS")
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Sebastian Ott <sebott@redhat.com>

[PATCH 2/4] KVM: arm64: Clarify the check for reset callback in check_sysreg_table()

[Marc Zyngier]

check_sysreg_table() has a wonky 'is_32" parameter, which is really
an indication that we should enforce the presence of a reset helper.

Clean this up by naming the variable accordingly and inverting the
condition. Contrary to popular belief, system instructions don't
have a reset value (duh!), and therefore do not need to be checked
for reset (they escaped the check through luck...).

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index af743494538a2..f8b10966d0c3e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -4335,12 +4335,12 @@ static const struct sys_reg_desc cp15_64_regs[] = {
 };
 
 static bool check_sysreg_table(const struct sys_reg_desc *table, unsigned int n,
-			       bool is_32)
+			       bool reset_check)
 {
 	unsigned int i;
 
 	for (i = 0; i < n; i++) {
-		if (!is_32 && table[i].reg && !table[i].reset) {
+		if (reset_check && table[i].reg && !table[i].reset) {
 			kvm_err("sys_reg table %pS entry %d (%s) lacks reset\n",
 				&table[i], i, table[i].name);
 			return false;
@@ -5334,11 +5334,11 @@ int __init kvm_sys_reg_table_init(void)
 	int ret = 0;
 
 	/* Make sure tables are unique and in order. */
-	valid &= check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs), false);
-	valid &= check_sysreg_table(cp14_regs, ARRAY_SIZE(cp14_regs), true);
-	valid &= check_sysreg_table(cp14_64_regs, ARRAY_SIZE(cp14_64_regs), true);
-	valid &= check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs), true);
-	valid &= check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs), true);
+	valid &= check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs), true);
+	valid &= check_sysreg_table(cp14_regs, ARRAY_SIZE(cp14_regs), false);
+	valid &= check_sysreg_table(cp14_64_regs, ARRAY_SIZE(cp14_64_regs), false);
+	valid &= check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs), false);
+	valid &= check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs), false);
 	valid &= check_sysreg_table(sys_insn_descs, ARRAY_SIZE(sys_insn_descs), false);
 
 	if (!valid)
-- 
2.39.2

Re: [PATCH 2/4] KVM: arm64: Clarify the check for reset callback in check_sysreg_table()

[Sebastian Ott]

On Fri, 18 Jul 2025, Marc Zyngier wrote:

> check_sysreg_table() has a wonky 'is_32" parameter, which is really
> an indication that we should enforce the presence of a reset helper.
>
> Clean this up by naming the variable accordingly and inverting the
> condition. Contrary to popular belief, system instructions don't
> have a reset value (duh!), and therefore do not need to be checked
> for reset (they escaped the check through luck...).
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Sebastian Ott <sebott@redhat.com>

[PATCH 3/4] KVM: arm64: Enforce the sorting of the GICv3 system register table

[Marc Zyngier]

In order to avoid further embarassing bugs, enforce that the GICv3
sysreg table is actually sorted, just like all the other tables.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c        | 6 +++++-
 arch/arm64/kvm/vgic-sys-reg-v3.c | 6 ++++++
 arch/arm64/kvm/vgic/vgic.h       | 1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f8b10966d0c3e..9d8c47e706b96 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -5329,8 +5329,9 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
 
 int __init kvm_sys_reg_table_init(void)
 {
+	const struct sys_reg_desc *gicv3_regs;
 	bool valid = true;
-	unsigned int i;
+	unsigned int i, sz;
 	int ret = 0;
 
 	/* Make sure tables are unique and in order. */
@@ -5341,6 +5342,9 @@ int __init kvm_sys_reg_table_init(void)
 	valid &= check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs), false);
 	valid &= check_sysreg_table(sys_insn_descs, ARRAY_SIZE(sys_insn_descs), false);
 
+	gicv3_regs = vgic_v3_get_sysreg_table(&sz);
+	valid &= check_sysreg_table(gicv3_regs, sz, false);
+
 	if (!valid)
 		return -EINVAL;
 
diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
index 1850f1727eb93..bdc2d57370b27 100644
--- a/arch/arm64/kvm/vgic-sys-reg-v3.c
+++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
@@ -443,6 +443,12 @@ static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
 	EL2_REG(ICH_LR15_EL2, ich_reg),
 };
 
+const struct sys_reg_desc *vgic_v3_get_sysreg_table(unsigned int *sz)
+{
+	*sz = ARRAY_SIZE(gic_v3_icc_reg_descs);
+	return gic_v3_icc_reg_descs;
+}
+
 static u64 attr_to_id(u64 attr)
 {
 	return ARM64_SYS_REG(FIELD_GET(KVM_REG_ARM_VGIC_SYSREG_OP0_MASK, attr),
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 5fe554d40c8ef..6ea817d8a804a 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -315,6 +315,7 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu,
 				struct kvm_device_attr *attr, bool is_write);
 int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
+const struct sys_reg_desc *vgic_v3_get_sysreg_table(unsigned int *sz);
 int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 				    u32 intid, u32 *val);
 int kvm_register_vgic_device(unsigned long type);
-- 
2.39.2

Re: [PATCH 3/4] KVM: arm64: Enforce the sorting of the GICv3 system register table

[Sebastian Ott]

On Fri, 18 Jul 2025, Marc Zyngier wrote:

> In order to avoid further embarassing bugs, enforce that the GICv3
> sysreg table is actually sorted, just like all the other tables.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Sebastian Ott <sebott@redhat.com>

[PATCH 4/4] KVM: arm64: selftest: vgic-v3: Add basic GICv3 sysreg userspace access test

[Marc Zyngier]

We have a lot of more or less useful vgic tests, but none of them
tracks the availability of GICv3 system registers, which is a bit
annoying.

Add one such test, which covers both EL1 and EL2 registers.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 tools/testing/selftests/kvm/arm64/vgic_init.c | 219 +++++++++++++++++-
 1 file changed, 217 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/arm64/vgic_init.c b/tools/testing/selftests/kvm/arm64/vgic_init.c
index b3b5fb0ff0a9a..7d508dcdbf23d 100644
--- a/tools/testing/selftests/kvm/arm64/vgic_init.c
+++ b/tools/testing/selftests/kvm/arm64/vgic_init.c
@@ -9,6 +9,8 @@
 #include <asm/kvm.h>
 #include <asm/kvm_para.h>
 
+#include <arm64/gic_v3.h>
+
 #include "test_util.h"
 #include "kvm_util.h"
 #include "processor.h"
@@ -18,8 +20,6 @@
 
 #define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
 
-#define GICR_TYPER 0x8
-
 #define VGIC_DEV_IS_V2(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V2)
 #define VGIC_DEV_IS_V3(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V3)
 
@@ -715,6 +715,220 @@ int test_kvm_device(uint32_t gic_dev_type)
 	return 0;
 }
 
+struct sr_def {
+	const char	*name;
+	u32		encoding;
+};
+
+#define PACK_SR(r)						\
+	((sys_reg_Op0(r) << 14) |				\
+	 (sys_reg_Op1(r) << 11) |				\
+	 (sys_reg_CRn(r) << 7) |				\
+	 (sys_reg_CRm(r) << 3) |				\
+	 (sys_reg_Op2(r)))
+
+#define SR(r)							\
+	{							\
+		.name		= #r,				\
+		.encoding	= r,				\
+	}
+
+static const struct sr_def sysregs_el1[] = {
+	SR(SYS_ICC_PMR_EL1),
+	SR(SYS_ICC_BPR0_EL1),
+	SR(SYS_ICC_AP0R0_EL1),
+	SR(SYS_ICC_AP0R1_EL1),
+	SR(SYS_ICC_AP0R2_EL1),
+	SR(SYS_ICC_AP0R3_EL1),
+	SR(SYS_ICC_AP1R0_EL1),
+	SR(SYS_ICC_AP1R1_EL1),
+	SR(SYS_ICC_AP1R2_EL1),
+	SR(SYS_ICC_AP1R3_EL1),
+	SR(SYS_ICC_BPR1_EL1),
+	SR(SYS_ICC_CTLR_EL1),
+	SR(SYS_ICC_SRE_EL1),
+	SR(SYS_ICC_IGRPEN0_EL1),
+	SR(SYS_ICC_IGRPEN1_EL1),
+};
+
+static const struct sr_def sysregs_el2[] = {
+	SR(SYS_ICH_AP0R0_EL2),
+	SR(SYS_ICH_AP0R1_EL2),
+	SR(SYS_ICH_AP0R2_EL2),
+	SR(SYS_ICH_AP0R3_EL2),
+	SR(SYS_ICH_AP1R0_EL2),
+	SR(SYS_ICH_AP1R1_EL2),
+	SR(SYS_ICH_AP1R2_EL2),
+	SR(SYS_ICH_AP1R3_EL2),
+	SR(SYS_ICH_HCR_EL2),
+	SR(SYS_ICC_SRE_EL2),
+	SR(SYS_ICH_VTR_EL2),
+	SR(SYS_ICH_VMCR_EL2),
+	SR(SYS_ICH_LR0_EL2),
+	SR(SYS_ICH_LR1_EL2),
+	SR(SYS_ICH_LR2_EL2),
+	SR(SYS_ICH_LR3_EL2),
+	SR(SYS_ICH_LR4_EL2),
+	SR(SYS_ICH_LR5_EL2),
+	SR(SYS_ICH_LR6_EL2),
+	SR(SYS_ICH_LR7_EL2),
+	SR(SYS_ICH_LR8_EL2),
+	SR(SYS_ICH_LR9_EL2),
+	SR(SYS_ICH_LR10_EL2),
+	SR(SYS_ICH_LR11_EL2),
+	SR(SYS_ICH_LR12_EL2),
+	SR(SYS_ICH_LR13_EL2),
+	SR(SYS_ICH_LR14_EL2),
+	SR(SYS_ICH_LR15_EL2),
+};
+
+static void test_sysreg_array(int gic, const struct sr_def *sr, int nr,
+			      int (*check)(int, const struct sr_def *, const char *))
+{
+	for (int i = 0; i < nr; i++) {
+		u64 val;
+		u64 attr;
+		int ret;
+
+		/* Assume MPIDR_EL1.Aff*=0 */
+		attr = PACK_SR(sr[i].encoding);
+
+		/*
+		 * The API is braindead. A register can be advertised as
+		 * available, and yet not be readable or writable.
+		 * ICC_APnR{1,2,3}_EL1 are examples of such non-sense, and
+		 * ICH_APnR{1,2,3}_EL2 do follow suit for consistency.
+		 *
+		 * On the bright side, no known HW is implementing more than
+		 * 5 bits of priority, so we're safe. Sort of...
+		 */
+		ret = __kvm_has_device_attr(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+					    attr);
+		TEST_ASSERT(ret == 0, "%s unavailable", sr[i].name);
+
+		/* Check that we can write back what we read */
+		ret = __kvm_device_attr_get(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+					    attr, &val);
+		TEST_ASSERT(ret == 0 || !check(gic, &sr[i], "read"), "%s unreadable", sr[i].name);
+		ret = __kvm_device_attr_set(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+					    attr, &val);
+		TEST_ASSERT(ret == 0 || !check(gic, &sr[i], "write"), "%s unwritable", sr[i].name);
+	}
+}
+
+static u8 get_ctlr_pribits(int gic)
+{
+	int ret;
+	u64 val;
+	u8 pri;
+
+	ret = __kvm_device_attr_get(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+				    PACK_SR(SYS_ICC_CTLR_EL1), &val);
+	TEST_ASSERT(ret == 0, "ICC_CTLR_EL1 unreadable");
+
+	pri = FIELD_GET(ICC_CTLR_EL1_PRI_BITS_MASK, val) + 1;
+	TEST_ASSERT(pri >= 5 && pri <= 7, "Bad pribits %d", pri);
+
+	return pri;
+}
+
+static int check_unaccessible_el1_regs(int gic, const struct sr_def *sr, const char *what)
+{
+	switch (sr->encoding) {
+	case SYS_ICC_AP0R1_EL1:
+	case SYS_ICC_AP1R1_EL1:
+		if (get_ctlr_pribits(gic) >= 6)
+			return -EINVAL;
+		break;
+	case SYS_ICC_AP0R2_EL1:
+	case SYS_ICC_AP0R3_EL1:
+	case SYS_ICC_AP1R2_EL1:
+	case SYS_ICC_AP1R3_EL1:
+		if (get_ctlr_pribits(gic) == 7)
+			return 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	pr_info("SKIP %s for %s\n", sr->name, what);
+	return 0;
+}
+
+static u8 get_vtr_pribits(int gic)
+{
+	int ret;
+	u64 val;
+	u8 pri;
+
+	ret = __kvm_device_attr_get(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+				    PACK_SR(SYS_ICH_VTR_EL2), &val);
+	TEST_ASSERT(ret == 0, "ICH_VTR_EL2 unreadable");
+
+	pri = FIELD_GET(ICH_VTR_EL2_PRIbits, val) + 1;
+	TEST_ASSERT(pri >= 5 && pri <= 7, "Bad pribits %d", pri);
+
+	return pri;
+}
+
+static int check_unaccessible_el2_regs(int gic, const struct sr_def *sr, const char *what)
+{
+	switch (sr->encoding) {
+	case SYS_ICH_AP0R1_EL2:
+	case SYS_ICH_AP1R1_EL2:
+		if (get_vtr_pribits(gic) >= 6)
+			return -EINVAL;
+		break;
+	case SYS_ICH_AP0R2_EL2:
+	case SYS_ICH_AP0R3_EL2:
+	case SYS_ICH_AP1R2_EL2:
+	case SYS_ICH_AP1R3_EL2:
+		if (get_vtr_pribits(gic) == 7)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	pr_info("SKIP %s for %s\n", sr->name, what);
+	return 0;
+}
+
+static void test_v3_sysregs(void)
+{
+	struct kvm_vcpu_init init = {};
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	u32 feat = 0;
+	int gic;
+
+	if (kvm_check_cap(KVM_CAP_ARM_EL2))
+		feat |= BIT(KVM_ARM_VCPU_HAS_EL2);
+
+	vm = vm_create(1);
+
+	vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init);
+	init.features[0] |= feat;
+
+	vcpu = aarch64_vcpu_add(vm, 0, &init, NULL);
+	TEST_ASSERT(vcpu, "Can't create a vcpu?");
+
+	gic = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3);
+	TEST_ASSERT(gic >= 0, "No GIC???");
+
+	kvm_device_attr_set(gic, KVM_DEV_ARM_VGIC_GRP_CTRL,
+			    KVM_DEV_ARM_VGIC_CTRL_INIT, NULL);
+
+	test_sysreg_array(gic, sysregs_el1, ARRAY_SIZE(sysregs_el1), check_unaccessible_el1_regs);
+	if (feat)
+		test_sysreg_array(gic, sysregs_el2, ARRAY_SIZE(sysregs_el2), check_unaccessible_el2_regs);
+	else
+		pr_info("SKIP EL2 registers, not available\n");
+
+	close(gic);
+	kvm_vm_free(vm);
+}
+
 void run_tests(uint32_t gic_dev_type)
 {
 	test_vcpus_then_vgic(gic_dev_type);
@@ -730,6 +944,7 @@ void run_tests(uint32_t gic_dev_type)
 		test_v3_last_bit_single_rdist();
 		test_v3_redist_ipa_range_check_at_vcpu_run();
 		test_v3_its_region();
+		test_v3_sysregs();
 	}
 }
 
-- 
2.39.2

Re: [PATCH 4/4] KVM: arm64: selftest: vgic-v3: Add basic GICv3 sysreg userspace access test

[Itaru Kitayama]

On Fri, Jul 18, 2025 at 12:11:54PM +0100, Marc Zyngier wrote:
> We have a lot of more or less useful vgic tests, but none of them
> tracks the availability of GICv3 system registers, which is a bit
> annoying.
> 
> Add one such test, which covers both EL1 and EL2 registers.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>

I've tested this selftest on the RevC FVP with kvm-arm.mode=nested.

Tested-by: Itaru Kitayama <itaru.kitayama@fujitsu.com>

Running GIC_v3 tests.
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='682'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='682'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
SKIP SYS_ICC_AP0R1_EL1 for read
SKIP SYS_ICC_AP0R1_EL1 for write
SKIP SYS_ICC_AP0R2_EL1 for read
SKIP SYS_ICC_AP0R2_EL1 for write
SKIP SYS_ICC_AP0R3_EL1 for read
SKIP SYS_ICC_AP0R3_EL1 for write
SKIP SYS_ICC_AP1R1_EL1 for read
SKIP SYS_ICC_AP1R1_EL1 for write
SKIP SYS_ICC_AP1R2_EL1 for read
SKIP SYS_ICC_AP1R2_EL1 for write
SKIP SYS_ICC_AP1R3_EL1 for read
SKIP SYS_ICC_AP1R3_EL1 for write
SKIP SYS_ICH_AP0R1_EL2 for read
SKIP SYS_ICH_AP0R1_EL2 for write
SKIP SYS_ICH_AP0R2_EL2 for read
SKIP SYS_ICH_AP0R2_EL2 for write
SKIP SYS_ICH_AP0R3_EL2 for read
SKIP SYS_ICH_AP0R3_EL2 for write
SKIP SYS_ICH_AP1R1_EL2 for read
SKIP SYS_ICH_AP1R1_EL2 for write
SKIP SYS_ICH_AP1R2_EL2 for read
SKIP SYS_ICH_AP1R2_EL2 for write
SKIP SYS_ICH_AP1R3_EL2 for read
SKIP SYS_ICH_AP1R3_EL2 for write
__vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'




> ---
>  tools/testing/selftests/kvm/arm64/vgic_init.c | 219 +++++++++++++++++-
>  1 file changed, 217 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/arm64/vgic_init.c b/tools/testing/selftests/kvm/arm64/vgic_init.c
> index b3b5fb0ff0a9a..7d508dcdbf23d 100644
> --- a/tools/testing/selftests/kvm/arm64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/arm64/vgic_init.c
> @@ -9,6 +9,8 @@
>  #include <asm/kvm.h>
>  #include <asm/kvm_para.h>
>  
> +#include <arm64/gic_v3.h>
> +
>  #include "test_util.h"
>  #include "kvm_util.h"
>  #include "processor.h"
> @@ -18,8 +20,6 @@
>  
>  #define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
>  
> -#define GICR_TYPER 0x8
> -
>  #define VGIC_DEV_IS_V2(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V2)
>  #define VGIC_DEV_IS_V3(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V3)
>  
> @@ -715,6 +715,220 @@ int test_kvm_device(uint32_t gic_dev_type)
>  	return 0;
>  }
>  
> +struct sr_def {
> +	const char	*name;
> +	u32		encoding;
> +};
> +
> +#define PACK_SR(r)						\
> +	((sys_reg_Op0(r) << 14) |				\
> +	 (sys_reg_Op1(r) << 11) |				\
> +	 (sys_reg_CRn(r) << 7) |				\
> +	 (sys_reg_CRm(r) << 3) |				\
> +	 (sys_reg_Op2(r)))
> +
> +#define SR(r)							\
> +	{							\
> +		.name		= #r,				\
> +		.encoding	= r,				\
> +	}
> +
> +static const struct sr_def sysregs_el1[] = {
> +	SR(SYS_ICC_PMR_EL1),
> +	SR(SYS_ICC_BPR0_EL1),
> +	SR(SYS_ICC_AP0R0_EL1),
> +	SR(SYS_ICC_AP0R1_EL1),
> +	SR(SYS_ICC_AP0R2_EL1),
> +	SR(SYS_ICC_AP0R3_EL1),
> +	SR(SYS_ICC_AP1R0_EL1),
> +	SR(SYS_ICC_AP1R1_EL1),
> +	SR(SYS_ICC_AP1R2_EL1),
> +	SR(SYS_ICC_AP1R3_EL1),
> +	SR(SYS_ICC_BPR1_EL1),
> +	SR(SYS_ICC_CTLR_EL1),
> +	SR(SYS_ICC_SRE_EL1),
> +	SR(SYS_ICC_IGRPEN0_EL1),
> +	SR(SYS_ICC_IGRPEN1_EL1),
> +};
> +
> +static const struct sr_def sysregs_el2[] = {
> +	SR(SYS_ICH_AP0R0_EL2),
> +	SR(SYS_ICH_AP0R1_EL2),
> +	SR(SYS_ICH_AP0R2_EL2),
> +	SR(SYS_ICH_AP0R3_EL2),
> +	SR(SYS_ICH_AP1R0_EL2),
> +	SR(SYS_ICH_AP1R1_EL2),
> +	SR(SYS_ICH_AP1R2_EL2),
> +	SR(SYS_ICH_AP1R3_EL2),
> +	SR(SYS_ICH_HCR_EL2),
> +	SR(SYS_ICC_SRE_EL2),
> +	SR(SYS_ICH_VTR_EL2),
> +	SR(SYS_ICH_VMCR_EL2),
> +	SR(SYS_ICH_LR0_EL2),
> +	SR(SYS_ICH_LR1_EL2),
> +	SR(SYS_ICH_LR2_EL2),
> +	SR(SYS_ICH_LR3_EL2),
> +	SR(SYS_ICH_LR4_EL2),
> +	SR(SYS_ICH_LR5_EL2),
> +	SR(SYS_ICH_LR6_EL2),
> +	SR(SYS_ICH_LR7_EL2),
> +	SR(SYS_ICH_LR8_EL2),
> +	SR(SYS_ICH_LR9_EL2),
> +	SR(SYS_ICH_LR10_EL2),
> +	SR(SYS_ICH_LR11_EL2),
> +	SR(SYS_ICH_LR12_EL2),
> +	SR(SYS_ICH_LR13_EL2),
> +	SR(SYS_ICH_LR14_EL2),
> +	SR(SYS_ICH_LR15_EL2),
> +};
> +
> +static void test_sysreg_array(int gic, const struct sr_def *sr, int nr,
> +			      int (*check)(int, const struct sr_def *, const char *))
> +{
> +	for (int i = 0; i < nr; i++) {
> +		u64 val;
> +		u64 attr;
> +		int ret;
> +
> +		/* Assume MPIDR_EL1.Aff*=0 */
> +		attr = PACK_SR(sr[i].encoding);
> +
> +		/*
> +		 * The API is braindead. A register can be advertised as
> +		 * available, and yet not be readable or writable.
> +		 * ICC_APnR{1,2,3}_EL1 are examples of such non-sense, and
> +		 * ICH_APnR{1,2,3}_EL2 do follow suit for consistency.
> +		 *
> +		 * On the bright side, no known HW is implementing more than
> +		 * 5 bits of priority, so we're safe. Sort of...
> +		 */
> +		ret = __kvm_has_device_attr(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +					    attr);
> +		TEST_ASSERT(ret == 0, "%s unavailable", sr[i].name);
> +
> +		/* Check that we can write back what we read */
> +		ret = __kvm_device_attr_get(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +					    attr, &val);
> +		TEST_ASSERT(ret == 0 || !check(gic, &sr[i], "read"), "%s unreadable", sr[i].name);
> +		ret = __kvm_device_attr_set(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +					    attr, &val);
> +		TEST_ASSERT(ret == 0 || !check(gic, &sr[i], "write"), "%s unwritable", sr[i].name);
> +	}
> +}
> +
> +static u8 get_ctlr_pribits(int gic)
> +{
> +	int ret;
> +	u64 val;
> +	u8 pri;
> +
> +	ret = __kvm_device_attr_get(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +				    PACK_SR(SYS_ICC_CTLR_EL1), &val);
> +	TEST_ASSERT(ret == 0, "ICC_CTLR_EL1 unreadable");
> +
> +	pri = FIELD_GET(ICC_CTLR_EL1_PRI_BITS_MASK, val) + 1;
> +	TEST_ASSERT(pri >= 5 && pri <= 7, "Bad pribits %d", pri);
> +
> +	return pri;
> +}
> +
> +static int check_unaccessible_el1_regs(int gic, const struct sr_def *sr, const char *what)
> +{
> +	switch (sr->encoding) {
> +	case SYS_ICC_AP0R1_EL1:
> +	case SYS_ICC_AP1R1_EL1:
> +		if (get_ctlr_pribits(gic) >= 6)
> +			return -EINVAL;
> +		break;
> +	case SYS_ICC_AP0R2_EL1:
> +	case SYS_ICC_AP0R3_EL1:
> +	case SYS_ICC_AP1R2_EL1:
> +	case SYS_ICC_AP1R3_EL1:
> +		if (get_ctlr_pribits(gic) == 7)
> +			return 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	pr_info("SKIP %s for %s\n", sr->name, what);
> +	return 0;
> +}
> +
> +static u8 get_vtr_pribits(int gic)
> +{
> +	int ret;
> +	u64 val;
> +	u8 pri;
> +
> +	ret = __kvm_device_attr_get(gic, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +				    PACK_SR(SYS_ICH_VTR_EL2), &val);
> +	TEST_ASSERT(ret == 0, "ICH_VTR_EL2 unreadable");
> +
> +	pri = FIELD_GET(ICH_VTR_EL2_PRIbits, val) + 1;
> +	TEST_ASSERT(pri >= 5 && pri <= 7, "Bad pribits %d", pri);
> +
> +	return pri;
> +}
> +
> +static int check_unaccessible_el2_regs(int gic, const struct sr_def *sr, const char *what)
> +{
> +	switch (sr->encoding) {
> +	case SYS_ICH_AP0R1_EL2:
> +	case SYS_ICH_AP1R1_EL2:
> +		if (get_vtr_pribits(gic) >= 6)
> +			return -EINVAL;
> +		break;
> +	case SYS_ICH_AP0R2_EL2:
> +	case SYS_ICH_AP0R3_EL2:
> +	case SYS_ICH_AP1R2_EL2:
> +	case SYS_ICH_AP1R3_EL2:
> +		if (get_vtr_pribits(gic) == 7)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	pr_info("SKIP %s for %s\n", sr->name, what);
> +	return 0;
> +}
> +
> +static void test_v3_sysregs(void)
> +{
> +	struct kvm_vcpu_init init = {};
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	u32 feat = 0;
> +	int gic;
> +
> +	if (kvm_check_cap(KVM_CAP_ARM_EL2))
> +		feat |= BIT(KVM_ARM_VCPU_HAS_EL2);
> +
> +	vm = vm_create(1);
> +
> +	vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init);
> +	init.features[0] |= feat;
> +
> +	vcpu = aarch64_vcpu_add(vm, 0, &init, NULL);
> +	TEST_ASSERT(vcpu, "Can't create a vcpu?");
> +
> +	gic = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3);
> +	TEST_ASSERT(gic >= 0, "No GIC???");
> +
> +	kvm_device_attr_set(gic, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +			    KVM_DEV_ARM_VGIC_CTRL_INIT, NULL);
> +
> +	test_sysreg_array(gic, sysregs_el1, ARRAY_SIZE(sysregs_el1), check_unaccessible_el1_regs);
> +	if (feat)
> +		test_sysreg_array(gic, sysregs_el2, ARRAY_SIZE(sysregs_el2), check_unaccessible_el2_regs);
> +	else
> +		pr_info("SKIP EL2 registers, not available\n");
> +
> +	close(gic);
> +	kvm_vm_free(vm);
> +}
> +
>  void run_tests(uint32_t gic_dev_type)
>  {
>  	test_vcpus_then_vgic(gic_dev_type);
> @@ -730,6 +944,7 @@ void run_tests(uint32_t gic_dev_type)
>  		test_v3_last_bit_single_rdist();
>  		test_v3_redist_ipa_range_check_at_vcpu_run();
>  		test_v3_its_region();
> +		test_v3_sysregs();
>  	}
>  }
>  
> -- 
> 2.39.2
> 

Re: [PATCH 4/4] KVM: arm64: selftest: vgic-v3: Add basic GICv3 sysreg userspace access test

[Marc Zyngier]

On Wed, 23 Jul 2025 06:00:40 +0100,
Itaru Kitayama <itaru.kitayama@linux.dev> wrote:
> 
> On Fri, Jul 18, 2025 at 12:11:54PM +0100, Marc Zyngier wrote:
> > We have a lot of more or less useful vgic tests, but none of them
> > tracks the availability of GICv3 system registers, which is a bit
> > annoying.
> > 
> > Add one such test, which covers both EL1 and EL2 registers.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> I've tested this selftest on the RevC FVP with kvm-arm.mode=nested.
> 
> Tested-by: Itaru Kitayama <itaru.kitayama@fujitsu.com>
> 
> Running GIC_v3 tests.
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='682'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='682'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'

I have no idea what you tested it on, because I get none of this
nonsense.

Where is it coming from?

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH 4/4] KVM: arm64: selftest: vgic-v3: Add basic GICv3 sysreg userspace access test

[Itaru Kitayama]

> On Jul 23, 2025, at 17:15, Marc Zyngier <maz@kernel.org> wrote:
> 
> On Wed, 23 Jul 2025 06:00:40 +0100,
> Itaru Kitayama <itaru.kitayama@linux.dev> wrote:
>> 
>> On Fri, Jul 18, 2025 at 12:11:54PM +0100, Marc Zyngier wrote:
>>> We have a lot of more or less useful vgic tests, but none of them
>>> tracks the availability of GICv3 system registers, which is a bit
>>> annoying.
>>> 
>>> Add one such test, which covers both EL1 and EL2 registers.
>>> 
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> 
>> I've tested this selftest on the RevC FVP with kvm-arm.mode=nested.
>> 
>> Tested-by: Itaru Kitayama <itaru.kitayama@fujitsu.com>
>> 
>> Running GIC_v3 tests.
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='682'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='682'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='672'
>> __vm_create: mode='PA-bits:40,  VA-bits:48,  4K pages' type='0', pages='657'
> 
> I have no idea what you tested it on, because I get none of this
> nonsense.
> 
> Where is it coming from?

From lib/kvm_util.c file __vm_create()’s pr_debug().

Anyway, I rebuilt the kernel your recent two patch sets on top of kvm-next/next and did the test. 

# ./arm64/vgic_init
Random seed: 0x6b8b4567
Running GIC_v3 tests.
SKIP SYS_ICC_AP0R1_EL1 for read
SKIP SYS_ICC_AP0R1_EL1 for write
SKIP SYS_ICC_AP0R2_EL1 for read
SKIP SYS_ICC_AP0R2_EL1 for write
SKIP SYS_ICC_AP0R3_EL1 for read
SKIP SYS_ICC_AP0R3_EL1 for write
SKIP SYS_ICC_AP1R1_EL1 for read
SKIP SYS_ICC_AP1R1_EL1 for write
SKIP SYS_ICC_AP1R2_EL1 for read
SKIP SYS_ICC_AP1R2_EL1 for write
SKIP SYS_ICC_AP1R3_EL1 for read
SKIP SYS_ICC_AP1R3_EL1 for write
SKIP SYS_ICH_AP0R1_EL2 for read
SKIP SYS_ICH_AP0R1_EL2 for write
SKIP SYS_ICH_AP0R2_EL2 for read
SKIP SYS_ICH_AP0R2_EL2 for write
SKIP SYS_ICH_AP0R3_EL2 for read
SKIP SYS_ICH_AP0R3_EL2 for write
SKIP SYS_ICH_AP1R1_EL2 for read
SKIP SYS_ICH_AP1R1_EL2 for write
SKIP SYS_ICH_AP1R2_EL2 for read
SKIP SYS_ICH_AP1R2_EL2 for write
SKIP SYS_ICH_AP1R3_EL2 for read
SKIP SYS_ICH_AP1R3_EL2 for write

Thanks,
Itaru.

> M.
> 
> -- 
> Jazz isn't dead. It just smells funny.

Re: [PATCH 4/4] KVM: arm64: selftest: vgic-v3: Add basic GICv3 sysreg userspace access test

[Sebastian Ott]

On Fri, 18 Jul 2025, Marc Zyngier wrote:

> We have a lot of more or less useful vgic tests, but none of them
> tracks the availability of GICv3 system registers, which is a bit
> annoying.
>
> Add one such test, which covers both EL1 and EL2 registers.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Sebastian Ott <sebott@redhat.com>

Re: [PATCH 0/4] KVM: arm64: Userspace GICv3 sysreg access fixes and testing

[Oliver Upton]

On Fri, 18 Jul 2025 12:11:50 +0100, Marc Zyngier wrote:
> As a follow-up to my earlier series at [1], here's a small set of
> fixes to address an annoying bug that made ICH_HCR_EL2 unreachable
> from userspace -- not something you'd expect.
> 
> So the first patch fixes the ordering the the sysreg table, which had
> ICH_HCR_EL2 at the wrong spot. The next two ensure that we now check
> for the table to be sorted (just like all the other tables). Finally,
> the last patch augments the vgic_init selftest to actually check that
> we can access these registers
> 
> [...]

Applied to next, thanks!

[1/4] KVM: arm64: vgic-v3: Fix ordering of ICH_HCR_EL2
      https://git.kernel.org/kvmarm/kvmarm/c/0f3046c8f68c
[2/4] KVM: arm64: Clarify the check for reset callback in check_sysreg_table()
      https://git.kernel.org/kvmarm/kvmarm/c/f5e6ebf285e1
[3/4] KVM: arm64: Enforce the sorting of the GICv3 system register table
      https://git.kernel.org/kvmarm/kvmarm/c/8af3e8ab09d0
[4/4] KVM: arm64: selftest: vgic-v3: Add basic GICv3 sysreg userspace access test
      https://git.kernel.org/kvmarm/kvmarm/c/3435bd79ec13

--
Best,
Oliver