[PATCH v2 0/5] KVM: arm64: selftests: Fix get-reg-list

[PATCH v2 0/5] KVM: arm64: selftests: Fix get-reg-list

[Andrew Jones]

v2:
 - Removed some cruft left over from a previous more complex design of the
   config command line parser
 - Dropped the list printing factor out patch as it's not necessary
 - Added a 'PASS' output for passing tests to allow testers to feel good
 - Changed the "up to date with kernel" comment to reference 5.13.0-rc2


Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
userspace when not available") the get-reg-list* tests have been
failing with

  ...
  ... There are 74 missing registers.
  The following lines are missing registers:
  ...

where the 74 missing registers are all PMU registers. This isn't a
bug in KVM that the selftest found, even though it's true that a
KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
flag, but still expecting the PMU registers to be in the reg-list,
would suddenly no longer have their expectations met. In that case,
the expectations were wrong, though, so that KVM userspace needs to
be fixed, and so does this selftest.

We could fix the test with a one-liner since we just need a

  init->features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;

in prepare_vcpu_init(), but that's too easy, so here's a 5 patch patch
series instead :-)  The reason for all the patches and the heavy diffstat
is to prepare for other vcpu configuration testing, e.g. ptrauth and mte.
With the refactoring done in this series, we should now be able to easily
add register sublists and vcpu configs to the get-reg-list test, as the
last patch demonstrates with the pmu fix.

Thanks,
drew


Andrew Jones (5):
  KVM: arm64: selftests: get-reg-list: Introduce vcpu configs
  KVM: arm64: selftests: get-reg-list: Prepare to run multiple configs
    at once
  KVM: arm64: selftests: get-reg-list: Provide config selection option
  KVM: arm64: selftests: get-reg-list: Remove get-reg-list-sve
  KVM: arm64: selftests: get-reg-list: Split base and pmu registers

 tools/testing/selftests/kvm/.gitignore        |   1 -
 tools/testing/selftests/kvm/Makefile          |   1 -
 .../selftests/kvm/aarch64/get-reg-list-sve.c  |   3 -
 .../selftests/kvm/aarch64/get-reg-list.c      | 388 ++++++++++++------
 4 files changed, 271 insertions(+), 122 deletions(-)
 delete mode 100644 tools/testing/selftests/kvm/aarch64/get-reg-list-sve.c

-- 
2.30.2

[PATCH v2 1/5] KVM: arm64: selftests: get-reg-list: Introduce vcpu configs

[Andrew Jones]

We already break register lists into sublists that get selected based
on vcpu config. However, since we only had two configs (vregs and sve),
we didn't structure the code very well to manage them. Restructure it
now to more cleanly handle register sublists that are dependent on the
vcpu config.

This patch has no intended functional change (except for the vcpu
config name now being prepended to all output).

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../selftests/kvm/aarch64/get-reg-list.c      | 207 ++++++++++--------
 1 file changed, 118 insertions(+), 89 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index 486932164cf2..e5d9cb312717 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -37,7 +37,27 @@
 #define reg_list_sve() (false)
 #endif
 
-#define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_ARM_COPROC_MASK)
+static struct kvm_reg_list *reg_list;
+static __u64 *blessed_reg, blessed_n;
+
+struct reg_sublist {
+	__u64 *regs;
+	__u64 regs_n;
+	__u64 *rejects_set;
+	__u64 rejects_set_n;
+};
+
+struct vcpu_config {
+	const char *name;
+	bool sve;
+	struct reg_sublist sublists[];
+};
+
+static struct vcpu_config vregs_config;
+static struct vcpu_config sve_config;
+
+#define for_each_sublist(c, s)							\
+	for ((s) = &(c)->sublists[0]; (s)->regs; ++(s))
 
 #define for_each_reg(i)								\
 	for ((i) = 0; (i) < reg_list->n; ++(i))
@@ -54,13 +74,6 @@
 	for_each_reg_filtered(i)						\
 		if (!find_reg(blessed_reg, blessed_n, reg_list->reg[i]))
 
-
-static struct kvm_reg_list *reg_list;
-
-static __u64 base_regs[], vregs[], sve_regs[], rejects_set[];
-static __u64 base_regs_n, vregs_n, sve_regs_n, rejects_set_n;
-static __u64 *blessed_reg, blessed_n;
-
 static bool filter_reg(__u64 reg)
 {
 	/*
@@ -96,11 +109,13 @@ static const char *str_with_index(const char *template, __u64 index)
 	return (const char *)str;
 }
 
+#define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_ARM_COPROC_MASK)
+
 #define CORE_REGS_XX_NR_WORDS	2
 #define CORE_SPSR_XX_NR_WORDS	2
 #define CORE_FPREGS_XX_NR_WORDS	4
 
-static const char *core_id_to_str(__u64 id)
+static const char *core_id_to_str(struct vcpu_config *c, __u64 id)
 {
 	__u64 core_off = id & ~REG_MASK, idx;
 
@@ -111,7 +126,7 @@ static const char *core_id_to_str(__u64 id)
 	case KVM_REG_ARM_CORE_REG(regs.regs[0]) ...
 	     KVM_REG_ARM_CORE_REG(regs.regs[30]):
 		idx = (core_off - KVM_REG_ARM_CORE_REG(regs.regs[0])) / CORE_REGS_XX_NR_WORDS;
-		TEST_ASSERT(idx < 31, "Unexpected regs.regs index: %lld", idx);
+		TEST_ASSERT(idx < 31, "%s: Unexpected regs.regs index: %lld", c->name, idx);
 		return str_with_index("KVM_REG_ARM_CORE_REG(regs.regs[##])", idx);
 	case KVM_REG_ARM_CORE_REG(regs.sp):
 		return "KVM_REG_ARM_CORE_REG(regs.sp)";
@@ -126,12 +141,12 @@ static const char *core_id_to_str(__u64 id)
 	case KVM_REG_ARM_CORE_REG(spsr[0]) ...
 	     KVM_REG_ARM_CORE_REG(spsr[KVM_NR_SPSR - 1]):
 		idx = (core_off - KVM_REG_ARM_CORE_REG(spsr[0])) / CORE_SPSR_XX_NR_WORDS;
-		TEST_ASSERT(idx < KVM_NR_SPSR, "Unexpected spsr index: %lld", idx);
+		TEST_ASSERT(idx < KVM_NR_SPSR, "%s: Unexpected spsr index: %lld", c->name, idx);
 		return str_with_index("KVM_REG_ARM_CORE_REG(spsr[##])", idx);
 	case KVM_REG_ARM_CORE_REG(fp_regs.vregs[0]) ...
 	     KVM_REG_ARM_CORE_REG(fp_regs.vregs[31]):
 		idx = (core_off - KVM_REG_ARM_CORE_REG(fp_regs.vregs[0])) / CORE_FPREGS_XX_NR_WORDS;
-		TEST_ASSERT(idx < 32, "Unexpected fp_regs.vregs index: %lld", idx);
+		TEST_ASSERT(idx < 32, "%s: Unexpected fp_regs.vregs index: %lld", c->name, idx);
 		return str_with_index("KVM_REG_ARM_CORE_REG(fp_regs.vregs[##])", idx);
 	case KVM_REG_ARM_CORE_REG(fp_regs.fpsr):
 		return "KVM_REG_ARM_CORE_REG(fp_regs.fpsr)";
@@ -139,11 +154,11 @@ static const char *core_id_to_str(__u64 id)
 		return "KVM_REG_ARM_CORE_REG(fp_regs.fpcr)";
 	}
 
-	TEST_FAIL("Unknown core reg id: 0x%llx", id);
+	TEST_FAIL("%s: Unknown core reg id: 0x%llx", c->name, id);
 	return NULL;
 }
 
-static const char *sve_id_to_str(__u64 id)
+static const char *sve_id_to_str(struct vcpu_config *c, __u64 id)
 {
 	__u64 sve_off, n, i;
 
@@ -153,37 +168,37 @@ static const char *sve_id_to_str(__u64 id)
 	sve_off = id & ~(REG_MASK | ((1ULL << 5) - 1));
 	i = id & (KVM_ARM64_SVE_MAX_SLICES - 1);
 
-	TEST_ASSERT(i == 0, "Currently we don't expect slice > 0, reg id 0x%llx", id);
+	TEST_ASSERT(i == 0, "%s: Currently we don't expect slice > 0, reg id 0x%llx", c->name, id);
 
 	switch (sve_off) {
 	case KVM_REG_ARM64_SVE_ZREG_BASE ...
 	     KVM_REG_ARM64_SVE_ZREG_BASE + (1ULL << 5) * KVM_ARM64_SVE_NUM_ZREGS - 1:
 		n = (id >> 5) & (KVM_ARM64_SVE_NUM_ZREGS - 1);
 		TEST_ASSERT(id == KVM_REG_ARM64_SVE_ZREG(n, 0),
-			    "Unexpected bits set in SVE ZREG id: 0x%llx", id);
+			    "%s: Unexpected bits set in SVE ZREG id: 0x%llx", c->name, id);
 		return str_with_index("KVM_REG_ARM64_SVE_ZREG(##, 0)", n);
 	case KVM_REG_ARM64_SVE_PREG_BASE ...
 	     KVM_REG_ARM64_SVE_PREG_BASE + (1ULL << 5) * KVM_ARM64_SVE_NUM_PREGS - 1:
 		n = (id >> 5) & (KVM_ARM64_SVE_NUM_PREGS - 1);
 		TEST_ASSERT(id == KVM_REG_ARM64_SVE_PREG(n, 0),
-			    "Unexpected bits set in SVE PREG id: 0x%llx", id);
+			    "%s: Unexpected bits set in SVE PREG id: 0x%llx", c->name, id);
 		return str_with_index("KVM_REG_ARM64_SVE_PREG(##, 0)", n);
 	case KVM_REG_ARM64_SVE_FFR_BASE:
 		TEST_ASSERT(id == KVM_REG_ARM64_SVE_FFR(0),
-			    "Unexpected bits set in SVE FFR id: 0x%llx", id);
+			    "%s: Unexpected bits set in SVE FFR id: 0x%llx", c->name, id);
 		return "KVM_REG_ARM64_SVE_FFR(0)";
 	}
 
 	return NULL;
 }
 
-static void print_reg(__u64 id)
+static void print_reg(struct vcpu_config *c, __u64 id)
 {
 	unsigned op0, op1, crn, crm, op2;
 	const char *reg_size = NULL;
 
 	TEST_ASSERT((id & KVM_REG_ARCH_MASK) == KVM_REG_ARM64,
-		    "KVM_REG_ARM64 missing in reg id: 0x%llx", id);
+		    "%s: KVM_REG_ARM64 missing in reg id: 0x%llx", c->name, id);
 
 	switch (id & KVM_REG_SIZE_MASK) {
 	case KVM_REG_SIZE_U8:
@@ -214,17 +229,17 @@ static void print_reg(__u64 id)
 		reg_size = "KVM_REG_SIZE_U2048";
 		break;
 	default:
-		TEST_FAIL("Unexpected reg size: 0x%llx in reg id: 0x%llx",
-			  (id & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT, id);
+		TEST_FAIL("%s: Unexpected reg size: 0x%llx in reg id: 0x%llx",
+			  c->name, (id & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT, id);
 	}
 
 	switch (id & KVM_REG_ARM_COPROC_MASK) {
 	case KVM_REG_ARM_CORE:
-		printf("\tKVM_REG_ARM64 | %s | KVM_REG_ARM_CORE | %s,\n", reg_size, core_id_to_str(id));
+		printf("\tKVM_REG_ARM64 | %s | KVM_REG_ARM_CORE | %s,\n", reg_size, core_id_to_str(c, id));
 		break;
 	case KVM_REG_ARM_DEMUX:
 		TEST_ASSERT(!(id & ~(REG_MASK | KVM_REG_ARM_DEMUX_ID_MASK | KVM_REG_ARM_DEMUX_VAL_MASK)),
-			    "Unexpected bits set in DEMUX reg id: 0x%llx", id);
+			    "%s: Unexpected bits set in DEMUX reg id: 0x%llx", c->name, id);
 		printf("\tKVM_REG_ARM64 | %s | KVM_REG_ARM_DEMUX | KVM_REG_ARM_DEMUX_ID_CCSIDR | %lld,\n",
 		       reg_size, id & KVM_REG_ARM_DEMUX_VAL_MASK);
 		break;
@@ -235,23 +250,23 @@ static void print_reg(__u64 id)
 		crm = (id & KVM_REG_ARM64_SYSREG_CRM_MASK) >> KVM_REG_ARM64_SYSREG_CRM_SHIFT;
 		op2 = (id & KVM_REG_ARM64_SYSREG_OP2_MASK) >> KVM_REG_ARM64_SYSREG_OP2_SHIFT;
 		TEST_ASSERT(id == ARM64_SYS_REG(op0, op1, crn, crm, op2),
-			    "Unexpected bits set in SYSREG reg id: 0x%llx", id);
+			    "%s: Unexpected bits set in SYSREG reg id: 0x%llx", c->name, id);
 		printf("\tARM64_SYS_REG(%d, %d, %d, %d, %d),\n", op0, op1, crn, crm, op2);
 		break;
 	case KVM_REG_ARM_FW:
 		TEST_ASSERT(id == KVM_REG_ARM_FW_REG(id & 0xffff),
-			    "Unexpected bits set in FW reg id: 0x%llx", id);
+			    "%s: Unexpected bits set in FW reg id: 0x%llx", c->name, id);
 		printf("\tKVM_REG_ARM_FW_REG(%lld),\n", id & 0xffff);
 		break;
 	case KVM_REG_ARM64_SVE:
-		if (reg_list_sve())
-			printf("\t%s,\n", sve_id_to_str(id));
+		if (c->sve)
+			printf("\t%s,\n", sve_id_to_str(c, id));
 		else
-			TEST_FAIL("KVM_REG_ARM64_SVE is an unexpected coproc type in reg id: 0x%llx", id);
+			TEST_FAIL("%s: KVM_REG_ARM64_SVE is an unexpected coproc type in reg id: 0x%llx", c->name, id);
 		break;
 	default:
-		TEST_FAIL("Unexpected coproc type: 0x%llx in reg id: 0x%llx",
-			  (id & KVM_REG_ARM_COPROC_MASK) >> KVM_REG_ARM_COPROC_SHIFT, id);
+		TEST_FAIL("%s: Unexpected coproc type: 0x%llx in reg id: 0x%llx",
+			  c->name, (id & KVM_REG_ARM_COPROC_MASK) >> KVM_REG_ARM_COPROC_SHIFT, id);
 	}
 }
 
@@ -312,40 +327,41 @@ static void core_reg_fixup(void)
 	reg_list = tmp;
 }
 
-static void prepare_vcpu_init(struct kvm_vcpu_init *init)
+static void prepare_vcpu_init(struct vcpu_config *c, struct kvm_vcpu_init *init)
 {
-	if (reg_list_sve())
+	if (c->sve)
 		init->features[0] |= 1 << KVM_ARM_VCPU_SVE;
 }
 
-static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
+static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid, struct vcpu_config *c)
 {
 	int feature;
 
-	if (reg_list_sve()) {
+	if (c->sve) {
 		feature = KVM_ARM_VCPU_SVE;
 		vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
 	}
 }
 
-static void check_supported(void)
+static void check_supported(struct vcpu_config *c)
 {
-	if (reg_list_sve() && !kvm_check_cap(KVM_CAP_ARM_SVE)) {
-		fprintf(stderr, "SVE not available, skipping tests\n");
+	if (c->sve && !kvm_check_cap(KVM_CAP_ARM_SVE)) {
+		fprintf(stderr, "%s: SVE not available, skipping tests\n", c->name);
 		exit(KSFT_SKIP);
 	}
 }
 
 int main(int ac, char **av)
 {
+	struct vcpu_config *c = reg_list_sve() ? &sve_config : &vregs_config;
 	struct kvm_vcpu_init init = { .target = -1, };
-	int new_regs = 0, missing_regs = 0, i;
+	int new_regs = 0, missing_regs = 0, i, n;
 	int failed_get = 0, failed_set = 0, failed_reject = 0;
 	bool print_list = false, print_filtered = false, fixup_core_regs = false;
 	struct kvm_vm *vm;
-	__u64 *vec_regs;
+	struct reg_sublist *s;
 
-	check_supported();
+	check_supported(c);
 
 	for (i = 1; i < ac; ++i) {
 		if (strcmp(av[i], "--core-reg-fixup") == 0)
@@ -359,9 +375,9 @@ int main(int ac, char **av)
 	}
 
 	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
-	prepare_vcpu_init(&init);
+	prepare_vcpu_init(c, &init);
 	aarch64_vcpu_add_default(vm, 0, &init, NULL);
-	finalize_vcpu(vm, 0);
+	finalize_vcpu(vm, 0, c);
 
 	reg_list = vcpu_get_reg_list(vm, 0);
 
@@ -374,7 +390,7 @@ int main(int ac, char **av)
 			__u64 id = reg_list->reg[i];
 			if ((print_list && !filter_reg(id)) ||
 			    (print_filtered && filter_reg(id)))
-				print_reg(id);
+				print_reg(c, id);
 		}
 		putchar('\n');
 		return 0;
@@ -396,50 +412,52 @@ int main(int ac, char **av)
 			.id = reg_list->reg[i],
 			.addr = (__u64)&addr,
 		};
+		bool reject_reg = false;
 		int ret;
 
 		ret = _vcpu_ioctl(vm, 0, KVM_GET_ONE_REG, &reg);
 		if (ret) {
-			puts("Failed to get ");
-			print_reg(reg.id);
+			printf("%s: Failed to get ", c->name);
+			print_reg(c, reg.id);
 			putchar('\n');
 			++failed_get;
 		}
 
 		/* rejects_set registers are rejected after KVM_ARM_VCPU_FINALIZE */
-		if (find_reg(rejects_set, rejects_set_n, reg.id)) {
-			ret = _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, &reg);
-			if (ret != -1 || errno != EPERM) {
-				printf("Failed to reject (ret=%d, errno=%d) ", ret, errno);
-				print_reg(reg.id);
-				putchar('\n');
-				++failed_reject;
+		for_each_sublist(c, s) {
+			if (s->rejects_set && find_reg(s->rejects_set, s->rejects_set_n, reg.id)) {
+				reject_reg = true;
+				ret = _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, &reg);
+				if (ret != -1 || errno != EPERM) {
+					printf("%s: Failed to reject (ret=%d, errno=%d) ", c->name, ret, errno);
+					print_reg(c, reg.id);
+					putchar('\n');
+					++failed_reject;
+				}
+				break;
 			}
-			continue;
 		}
 
-		ret = _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, &reg);
-		if (ret) {
-			puts("Failed to set ");
-			print_reg(reg.id);
-			putchar('\n');
-			++failed_set;
+		if (!reject_reg) {
+			ret = _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, &reg);
+			if (ret) {
+				printf("%s: Failed to set ", c->name);
+				print_reg(c, reg.id);
+				putchar('\n');
+				++failed_set;
+			}
 		}
 	}
 
-	if (reg_list_sve()) {
-		blessed_n = base_regs_n + sve_regs_n;
-		vec_regs = sve_regs;
-	} else {
-		blessed_n = base_regs_n + vregs_n;
-		vec_regs = vregs;
-	}
-
+	for_each_sublist(c, s)
+		blessed_n += s->regs_n;
 	blessed_reg = calloc(blessed_n, sizeof(__u64));
-	for (i = 0; i < base_regs_n; ++i)
-		blessed_reg[i] = base_regs[i];
-	for (i = 0; i < blessed_n - base_regs_n; ++i)
-		blessed_reg[base_regs_n + i] = vec_regs[i];
+
+	n = 0;
+	for_each_sublist(c, s) {
+		for (i = 0; i < s->regs_n; ++i)
+			blessed_reg[n++] = s->regs[i];
+	}
 
 	for_each_new_reg(i)
 		++new_regs;
@@ -448,31 +466,31 @@ int main(int ac, char **av)
 		++missing_regs;
 
 	if (new_regs || missing_regs) {
-		printf("Number blessed registers: %5lld\n", blessed_n);
-		printf("Number registers:         %5lld\n", reg_list->n);
+		printf("%s: Number blessed registers: %5lld\n", c->name, blessed_n);
+		printf("%s: Number registers:         %5lld\n", c->name, reg_list->n);
 	}
 
 	if (new_regs) {
-		printf("\nThere are %d new registers.\n"
+		printf("\n%s: There are %d new registers.\n"
 		       "Consider adding them to the blessed reg "
-		       "list with the following lines:\n\n", new_regs);
+		       "list with the following lines:\n\n", c->name, new_regs);
 		for_each_new_reg(i)
-			print_reg(reg_list->reg[i]);
+			print_reg(c, reg_list->reg[i]);
 		putchar('\n');
 	}
 
 	if (missing_regs) {
-		printf("\nThere are %d missing registers.\n"
-		       "The following lines are missing registers:\n\n", missing_regs);
+		printf("\n%s: There are %d missing registers.\n"
+		       "The following lines are missing registers:\n\n", c->name, missing_regs);
 		for_each_missing_reg(i)
-			print_reg(blessed_reg[i]);
+			print_reg(c, blessed_reg[i]);
 		putchar('\n');
 	}
 
 	TEST_ASSERT(!missing_regs && !failed_get && !failed_set && !failed_reject,
-		    "There are %d missing registers; "
+		    "%s: There are %d missing registers; "
 		    "%d registers failed get; %d registers failed set; %d registers failed reject",
-		    missing_regs, failed_get, failed_set, failed_reject);
+		    c->name, missing_regs, failed_get, failed_set, failed_reject);
 
 	return 0;
 }
@@ -761,7 +779,6 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(3, 4, 5, 0, 1),	/* IFSR32_EL2 */
 	ARM64_SYS_REG(3, 4, 5, 3, 0),	/* FPEXC32_EL2 */
 };
-static __u64 base_regs_n = ARRAY_SIZE(base_regs);
 
 static __u64 vregs[] = {
 	KVM_REG_ARM64 | KVM_REG_SIZE_U128 | KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(fp_regs.vregs[0]),
@@ -797,7 +814,6 @@ static __u64 vregs[] = {
 	KVM_REG_ARM64 | KVM_REG_SIZE_U128 | KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(fp_regs.vregs[30]),
 	KVM_REG_ARM64 | KVM_REG_SIZE_U128 | KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(fp_regs.vregs[31]),
 };
-static __u64 vregs_n = ARRAY_SIZE(vregs);
 
 static __u64 sve_regs[] = {
 	KVM_REG_ARM64_SVE_VLS,
@@ -852,11 +868,24 @@ static __u64 sve_regs[] = {
 	KVM_REG_ARM64_SVE_FFR(0),
 	ARM64_SYS_REG(3, 0, 1, 2, 0),   /* ZCR_EL1 */
 };
-static __u64 sve_regs_n = ARRAY_SIZE(sve_regs);
 
-static __u64 rejects_set[] = {
-#ifdef REG_LIST_SVE
+static __u64 sve_rejects_set[] = {
 	KVM_REG_ARM64_SVE_VLS,
-#endif
 };
-static __u64 rejects_set_n = ARRAY_SIZE(rejects_set);
+
+static struct vcpu_config vregs_config = {
+	"vregs",
+	.sublists = {
+	{ base_regs,	ARRAY_SIZE(base_regs), },
+	{ vregs,	ARRAY_SIZE(vregs), },
+	{0},
+	},
+};
+static struct vcpu_config sve_config = {
+	"sve", .sve = true,
+	.sublists = {
+	{ base_regs,	ARRAY_SIZE(base_regs), },
+	{ sve_regs,	ARRAY_SIZE(sve_regs),	sve_rejects_set,	ARRAY_SIZE(sve_rejects_set), },
+	{0},
+	},
+};
-- 
2.30.2

[PATCH v2 2/5] KVM: arm64: selftests: get-reg-list: Prepare to run multiple configs at once

[Andrew Jones]

We don't want to have to create a new binary for each vcpu config, so
prepare to run the test for multiple vcpu configs in a single binary.
We do this by factoring out the test from main() and then looping over
configs. When given '--list' we still never print more than a single
reg-list for a single vcpu config though, because it would be confusing
otherwise.

No functional change intended.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../selftests/kvm/aarch64/get-reg-list.c      | 68 ++++++++++++++-----
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index e5d9cb312717..06f737973511 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -53,8 +53,8 @@ struct vcpu_config {
 	struct reg_sublist sublists[];
 };
 
-static struct vcpu_config vregs_config;
-static struct vcpu_config sve_config;
+static struct vcpu_config *vcpu_configs[];
+static int vcpu_configs_n;
 
 #define for_each_sublist(c, s)							\
 	for ((s) = &(c)->sublists[0]; (s)->regs; ++(s))
@@ -351,29 +351,20 @@ static void check_supported(struct vcpu_config *c)
 	}
 }
 
-int main(int ac, char **av)
+static bool print_list;
+static bool print_filtered;
+static bool fixup_core_regs;
+
+static void run_test(struct vcpu_config *c)
 {
-	struct vcpu_config *c = reg_list_sve() ? &sve_config : &vregs_config;
 	struct kvm_vcpu_init init = { .target = -1, };
 	int new_regs = 0, missing_regs = 0, i, n;
 	int failed_get = 0, failed_set = 0, failed_reject = 0;
-	bool print_list = false, print_filtered = false, fixup_core_regs = false;
 	struct kvm_vm *vm;
 	struct reg_sublist *s;
 
 	check_supported(c);
 
-	for (i = 1; i < ac; ++i) {
-		if (strcmp(av[i], "--core-reg-fixup") == 0)
-			fixup_core_regs = true;
-		else if (strcmp(av[i], "--list") == 0)
-			print_list = true;
-		else if (strcmp(av[i], "--list-filtered") == 0)
-			print_filtered = true;
-		else
-			TEST_FAIL("Unknown option: %s\n", av[i]);
-	}
-
 	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
 	prepare_vcpu_init(c, &init);
 	aarch64_vcpu_add_default(vm, 0, &init, NULL);
@@ -393,7 +384,7 @@ int main(int ac, char **av)
 				print_reg(c, id);
 		}
 		putchar('\n');
-		return 0;
+		return;
 	}
 
 	/*
@@ -492,6 +483,44 @@ int main(int ac, char **av)
 		    "%d registers failed get; %d registers failed set; %d registers failed reject",
 		    c->name, missing_regs, failed_get, failed_set, failed_reject);
 
+	pr_info("%s: PASS\n", c->name);
+	blessed_n = 0;
+	free(blessed_reg);
+	free(reg_list);
+	kvm_vm_free(vm);
+}
+
+int main(int ac, char **av)
+{
+	struct vcpu_config *c, *sel = NULL;
+	int i;
+
+	for (i = 1; i < ac; ++i) {
+		if (strcmp(av[i], "--core-reg-fixup") == 0)
+			fixup_core_regs = true;
+		else if (strcmp(av[i], "--list") == 0)
+			print_list = true;
+		else if (strcmp(av[i], "--list-filtered") == 0)
+			print_filtered = true;
+		else
+			TEST_FAIL("Unknown option: %s\n", av[i]);
+	}
+
+	if (print_list || print_filtered) {
+		/*
+		 * We only want to print the register list of a single config.
+		 * TODO: Add command line support to pick which config.
+		 */
+		sel = vcpu_configs[0];
+	}
+
+	for (i = 0; i < vcpu_configs_n; ++i) {
+		c = vcpu_configs[i];
+		if (sel && c != sel)
+			continue;
+		run_test(c);
+	}
+
 	return 0;
 }
 
@@ -889,3 +918,8 @@ static struct vcpu_config sve_config = {
 	{0},
 	},
 };
+
+static struct vcpu_config *vcpu_configs[] = {
+	reg_list_sve() ? &sve_config : &vregs_config,
+};
+static int vcpu_configs_n = ARRAY_SIZE(vcpu_configs);
-- 
2.30.2

[PATCH v2 3/5] KVM: arm64: selftests: get-reg-list: Provide config selection option

[Andrew Jones]

Add a new command line option that allows the user to select a specific
configuration, e.g. --config=sve will give the sve config. Also provide
help text and the --help/-h options.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../selftests/kvm/aarch64/get-reg-list.c      | 56 ++++++++++++++++++-
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index 06f737973511..be1a2877d7fa 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -490,6 +490,52 @@ static void run_test(struct vcpu_config *c)
 	kvm_vm_free(vm);
 }
 
+static void help(void)
+{
+	struct vcpu_config *c;
+	int i;
+
+	printf(
+	"\n"
+	"usage: get-reg-list [--config=<selection>] [--list] [--list-filtered] [--core-reg-fixup]\n\n"
+	" --config=<selection>        Used to select a specific vcpu configuration for the test/listing\n"
+	"                             '<selection>' may be\n");
+
+	for (i = 0; i < vcpu_configs_n; ++i) {
+		c = vcpu_configs[i];
+		printf(
+	"                               '%s'\n", c->name);
+	}
+
+	printf(
+	"\n"
+	" --list                      Print the register list rather than test it (requires --config)\n"
+	" --list-filtered             Print registers that would normally be filtered out (requires --config)\n"
+	" --core-reg-fixup            Needed when running on old kernels with broken core reg listings\n"
+	"\n"
+	);
+}
+
+static struct vcpu_config *parse_config(const char *config)
+{
+	struct vcpu_config *c;
+	int i;
+
+	if (config[8] != '=')
+		help(), exit(1);
+
+	for (i = 0; i < vcpu_configs_n; ++i) {
+		c = vcpu_configs[i];
+		if (strcmp(c->name, &config[9]) == 0)
+			break;
+	}
+
+	if (i == vcpu_configs_n)
+		help(), exit(1);
+
+	return c;
+}
+
 int main(int ac, char **av)
 {
 	struct vcpu_config *c, *sel = NULL;
@@ -498,20 +544,24 @@ int main(int ac, char **av)
 	for (i = 1; i < ac; ++i) {
 		if (strcmp(av[i], "--core-reg-fixup") == 0)
 			fixup_core_regs = true;
+		else if (strncmp(av[i], "--config", 8) == 0)
+			sel = parse_config(av[i]);
 		else if (strcmp(av[i], "--list") == 0)
 			print_list = true;
 		else if (strcmp(av[i], "--list-filtered") == 0)
 			print_filtered = true;
+		else if (strcmp(av[i], "--help") == 0 || strcmp(av[1], "-h") == 0)
+			help(), exit(0);
 		else
-			TEST_FAIL("Unknown option: %s\n", av[i]);
+			help(), exit(1);
 	}
 
 	if (print_list || print_filtered) {
 		/*
 		 * We only want to print the register list of a single config.
-		 * TODO: Add command line support to pick which config.
 		 */
-		sel = vcpu_configs[0];
+		if (!sel)
+			help(), exit(1);
 	}
 
 	for (i = 0; i < vcpu_configs_n; ++i) {
-- 
2.30.2

[PATCH v2 4/5] KVM: arm64: selftests: get-reg-list: Remove get-reg-list-sve

[Andrew Jones]

Now that we can easily run the test for multiple vcpu configs, let's
merge get-reg-list and get-reg-list-sve into just get-reg-list. We
also add a final change to make it more possible to run multiple
tests, which is to fork the test, rather than directly run it. That
allows a test to fail, but subsequent tests can still run.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/.gitignore        |  1 -
 tools/testing/selftests/kvm/Makefile          |  1 -
 .../selftests/kvm/aarch64/get-reg-list-sve.c  |  3 --
 .../selftests/kvm/aarch64/get-reg-list.c      | 31 +++++++++++++------
 4 files changed, 21 insertions(+), 15 deletions(-)
 delete mode 100644 tools/testing/selftests/kvm/aarch64/get-reg-list-sve.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index bd83158e0e0b..e3745c90d8b2 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -1,6 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 /aarch64/get-reg-list
-/aarch64/get-reg-list-sve
 /aarch64/vgic_init
 /s390x/memop
 /s390x/resets
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index e439d027939d..bf54fc23bcff 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -78,7 +78,6 @@ TEST_GEN_PROGS_x86_64 += set_memory_region_test
 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
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list-sve.c b/tools/testing/selftests/kvm/aarch64/get-reg-list-sve.c
deleted file mode 100644
index efba76682b4b..000000000000
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list-sve.c
+++ /dev/null
@@ -1,3 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#define REG_LIST_SVE
-#include "get-reg-list.c"
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index be1a2877d7fa..dc06a28bfb74 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -27,16 +27,13 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 #include "kvm_util.h"
 #include "test_util.h"
 #include "processor.h"
 
-#ifdef REG_LIST_SVE
-#define reg_list_sve() (true)
-#else
-#define reg_list_sve() (false)
-#endif
-
 static struct kvm_reg_list *reg_list;
 static __u64 *blessed_reg, blessed_n;
 
@@ -539,7 +536,8 @@ static struct vcpu_config *parse_config(const char *config)
 int main(int ac, char **av)
 {
 	struct vcpu_config *c, *sel = NULL;
-	int i;
+	int i, ret = 0;
+	pid_t pid;
 
 	for (i = 1; i < ac; ++i) {
 		if (strcmp(av[i], "--core-reg-fixup") == 0)
@@ -568,10 +566,22 @@ int main(int ac, char **av)
 		c = vcpu_configs[i];
 		if (sel && c != sel)
 			continue;
-		run_test(c);
+
+		pid = fork();
+
+		if (!pid) {
+			run_test(c);
+			exit(0);
+		} else {
+			int wstatus;
+			pid_t wpid = wait(&wstatus);
+			TEST_ASSERT(wpid == pid && WIFEXITED(wstatus), "wait: Unexpected return");
+			if (WEXITSTATUS(wstatus) && WEXITSTATUS(wstatus) != KSFT_SKIP)
+				ret = KSFT_FAIL;
+		}
 	}
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -970,6 +980,7 @@ static struct vcpu_config sve_config = {
 };
 
 static struct vcpu_config *vcpu_configs[] = {
-	reg_list_sve() ? &sve_config : &vregs_config,
+	&vregs_config,
+	&sve_config,
 };
 static int vcpu_configs_n = ARRAY_SIZE(vcpu_configs);
-- 
2.30.2

[PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Andrew Jones]

Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
userspace when not available") the get-reg-list* tests have been
failing with

  ...
  ... There are 74 missing registers.
  The following lines are missing registers:
  ...

where the 74 missing registers are all PMU registers. This isn't a
bug in KVM that the selftest found, even though it's true that a
KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
flag, but still expecting the PMU registers to be in the reg-list,
would suddenly no longer have their expectations met. In that case,
the expectations were wrong, though, so that KVM userspace needs to
be fixed, and so does this selftest. The fix for this selftest is to
pull the PMU registers out of the base register sublist into their
own sublist and then create new, pmu-enabled vcpu configs which can
be tested.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index dc06a28bfb74..78d8949bddbd 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -47,6 +47,7 @@ struct reg_sublist {
 struct vcpu_config {
 	const char *name;
 	bool sve;
+	bool pmu;
 	struct reg_sublist sublists[];
 };
 
@@ -328,6 +329,8 @@ static void prepare_vcpu_init(struct vcpu_config *c, struct kvm_vcpu_init *init)
 {
 	if (c->sve)
 		init->features[0] |= 1 << KVM_ARM_VCPU_SVE;
+	if (c->pmu)
+		init->features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
 }
 
 static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid, struct vcpu_config *c)
@@ -346,6 +349,10 @@ static void check_supported(struct vcpu_config *c)
 		fprintf(stderr, "%s: SVE not available, skipping tests\n", c->name);
 		exit(KSFT_SKIP);
 	}
+	if (c->pmu && !kvm_check_cap(KVM_CAP_ARM_PMU_V3)) {
+		fprintf(stderr, "%s: PMU not available, skipping tests\n", c->name);
+		exit(KSFT_SKIP);
+	}
 }
 
 static bool print_list;
@@ -588,7 +595,7 @@ int main(int ac, char **av)
  * The current blessed list was primed with the output of kernel version
  * v4.15 with --core-reg-fixup and then later updated with new registers.
  *
- * The blessed list is up to date with kernel version v5.10-rc5
+ * The blessed list is up to date with kernel version 5.13.0-rc2
  */
 static __u64 base_regs[] = {
 	KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(regs.regs[0]),
@@ -780,8 +787,6 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(3, 0, 5, 2, 0),	/* ESR_EL1 */
 	ARM64_SYS_REG(3, 0, 6, 0, 0),	/* FAR_EL1 */
 	ARM64_SYS_REG(3, 0, 7, 4, 0),	/* PAR_EL1 */
-	ARM64_SYS_REG(3, 0, 9, 14, 1),	/* PMINTENSET_EL1 */
-	ARM64_SYS_REG(3, 0, 9, 14, 2),	/* PMINTENCLR_EL1 */
 	ARM64_SYS_REG(3, 0, 10, 2, 0),	/* MAIR_EL1 */
 	ARM64_SYS_REG(3, 0, 10, 3, 0),	/* AMAIR_EL1 */
 	ARM64_SYS_REG(3, 0, 12, 0, 0),	/* VBAR_EL1 */
@@ -790,6 +795,16 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(3, 0, 13, 0, 4),	/* TPIDR_EL1 */
 	ARM64_SYS_REG(3, 0, 14, 1, 0),	/* CNTKCTL_EL1 */
 	ARM64_SYS_REG(3, 2, 0, 0, 0),	/* CSSELR_EL1 */
+	ARM64_SYS_REG(3, 3, 13, 0, 2),	/* TPIDR_EL0 */
+	ARM64_SYS_REG(3, 3, 13, 0, 3),	/* TPIDRRO_EL0 */
+	ARM64_SYS_REG(3, 4, 3, 0, 0),	/* DACR32_EL2 */
+	ARM64_SYS_REG(3, 4, 5, 0, 1),	/* IFSR32_EL2 */
+	ARM64_SYS_REG(3, 4, 5, 3, 0),	/* FPEXC32_EL2 */
+};
+
+static __u64 pmu_regs[] = {
+	ARM64_SYS_REG(3, 0, 9, 14, 1),	/* PMINTENSET_EL1 */
+	ARM64_SYS_REG(3, 0, 9, 14, 2),	/* PMINTENCLR_EL1 */
 	ARM64_SYS_REG(3, 3, 9, 12, 0),	/* PMCR_EL0 */
 	ARM64_SYS_REG(3, 3, 9, 12, 1),	/* PMCNTENSET_EL0 */
 	ARM64_SYS_REG(3, 3, 9, 12, 2),	/* PMCNTENCLR_EL0 */
@@ -799,8 +814,6 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(3, 3, 9, 13, 0),	/* PMCCNTR_EL0 */
 	ARM64_SYS_REG(3, 3, 9, 14, 0),	/* PMUSERENR_EL0 */
 	ARM64_SYS_REG(3, 3, 9, 14, 3),	/* PMOVSSET_EL0 */
-	ARM64_SYS_REG(3, 3, 13, 0, 2),	/* TPIDR_EL0 */
-	ARM64_SYS_REG(3, 3, 13, 0, 3),	/* TPIDRRO_EL0 */
 	ARM64_SYS_REG(3, 3, 14, 8, 0),
 	ARM64_SYS_REG(3, 3, 14, 8, 1),
 	ARM64_SYS_REG(3, 3, 14, 8, 2),
@@ -864,9 +877,6 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(3, 3, 14, 15, 5),
 	ARM64_SYS_REG(3, 3, 14, 15, 6),
 	ARM64_SYS_REG(3, 3, 14, 15, 7),	/* PMCCFILTR_EL0 */
-	ARM64_SYS_REG(3, 4, 3, 0, 0),	/* DACR32_EL2 */
-	ARM64_SYS_REG(3, 4, 5, 0, 1),	/* IFSR32_EL2 */
-	ARM64_SYS_REG(3, 4, 5, 3, 0),	/* FPEXC32_EL2 */
 };
 
 static __u64 vregs[] = {
@@ -970,6 +980,15 @@ static struct vcpu_config vregs_config = {
 	{0},
 	},
 };
+static struct vcpu_config vregs_pmu_config = {
+	"vregs+pmu", .pmu = true,
+	.sublists = {
+	{ base_regs,	ARRAY_SIZE(base_regs), },
+	{ vregs,	ARRAY_SIZE(vregs), },
+	{ pmu_regs,	ARRAY_SIZE(pmu_regs), },
+	{0},
+	},
+};
 static struct vcpu_config sve_config = {
 	"sve", .sve = true,
 	.sublists = {
@@ -978,9 +997,20 @@ static struct vcpu_config sve_config = {
 	{0},
 	},
 };
+static struct vcpu_config sve_pmu_config = {
+	"sve+pmu", .sve = true, .pmu = true,
+	.sublists = {
+	{ base_regs,	ARRAY_SIZE(base_regs), },
+	{ sve_regs,	ARRAY_SIZE(sve_regs),	sve_rejects_set,	ARRAY_SIZE(sve_rejects_set), },
+	{ pmu_regs,	ARRAY_SIZE(pmu_regs), },
+	{0},
+	},
+};
 
 static struct vcpu_config *vcpu_configs[] = {
 	&vregs_config,
+	&vregs_pmu_config,
 	&sve_config,
+	&sve_pmu_config,
 };
 static int vcpu_configs_n = ARRAY_SIZE(vcpu_configs);
-- 
2.30.2

Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Ricardo Koller]

On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> userspace when not available") the get-reg-list* tests have been
> failing with
> 
>   ...
>   ... There are 74 missing registers.
>   The following lines are missing registers:
>   ...
> 
> where the 74 missing registers are all PMU registers. This isn't a
> bug in KVM that the selftest found, even though it's true that a
> KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> flag, but still expecting the PMU registers to be in the reg-list,
> would suddenly no longer have their expectations met. In that case,
> the expectations were wrong, though, so that KVM userspace needs to
> be fixed, and so does this selftest. The fix for this selftest is to
> pull the PMU registers out of the base register sublist into their
> own sublist and then create new, pmu-enabled vcpu configs which can
> be tested.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
>  1 file changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> index dc06a28bfb74..78d8949bddbd 100644
> --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> @@ -47,6 +47,7 @@ struct reg_sublist {
>  struct vcpu_config {
>  	const char *name;
>  	bool sve;
> +	bool pmu;
>  	struct reg_sublist sublists[];
>  };

I think it's possible that the number of sublists keeps increasing: it
would be very nice/useful if KVM allowed enabling/disabling more
features from userspace (besides SVE, PMU etc). In that case, it might
be easier if adding a new feature to get-reg-list just requires defining
a new config and not dealing with the internals of vcpu_config.

Do you think it's possible in general to associate a sublist to a
capability and a feature? It works for the PMU and SVE. If that is
possible, what do you think of something like this? this would be the
config for sve+pmu:

static struct vcpu_config sve_pmu_config = {
      "sve+pmu",
       .sublists = {
       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
       { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
       { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
       {0},
       },
};

Appended a rough patch at the end to make this idea more concrete.

Thanks,
Ricardo

>  
> @@ -328,6 +329,8 @@ static void prepare_vcpu_init(struct vcpu_config *c, struct kvm_vcpu_init *init)
>  {
>  	if (c->sve)
>  		init->features[0] |= 1 << KVM_ARM_VCPU_SVE;
> +	if (c->pmu)
> +		init->features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
>  }
>  
>  static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid, struct vcpu_config *c)
> @@ -346,6 +349,10 @@ static void check_supported(struct vcpu_config *c)
>  		fprintf(stderr, "%s: SVE not available, skipping tests\n", c->name);
>  		exit(KSFT_SKIP);
>  	}
> +	if (c->pmu && !kvm_check_cap(KVM_CAP_ARM_PMU_V3)) {
> +		fprintf(stderr, "%s: PMU not available, skipping tests\n", c->name);
> +		exit(KSFT_SKIP);
> +	}
>  }
>  
>  static bool print_list;
> @@ -588,7 +595,7 @@ int main(int ac, char **av)
>   * The current blessed list was primed with the output of kernel version
>   * v4.15 with --core-reg-fixup and then later updated with new registers.
>   *
> - * The blessed list is up to date with kernel version v5.10-rc5
> + * The blessed list is up to date with kernel version 5.13.0-rc2
>   */
>  static __u64 base_regs[] = {
>  	KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(regs.regs[0]),
> @@ -780,8 +787,6 @@ static __u64 base_regs[] = {
>  	ARM64_SYS_REG(3, 0, 5, 2, 0),	/* ESR_EL1 */
>  	ARM64_SYS_REG(3, 0, 6, 0, 0),	/* FAR_EL1 */
>  	ARM64_SYS_REG(3, 0, 7, 4, 0),	/* PAR_EL1 */
> -	ARM64_SYS_REG(3, 0, 9, 14, 1),	/* PMINTENSET_EL1 */
> -	ARM64_SYS_REG(3, 0, 9, 14, 2),	/* PMINTENCLR_EL1 */
>  	ARM64_SYS_REG(3, 0, 10, 2, 0),	/* MAIR_EL1 */
>  	ARM64_SYS_REG(3, 0, 10, 3, 0),	/* AMAIR_EL1 */
>  	ARM64_SYS_REG(3, 0, 12, 0, 0),	/* VBAR_EL1 */
> @@ -790,6 +795,16 @@ static __u64 base_regs[] = {
>  	ARM64_SYS_REG(3, 0, 13, 0, 4),	/* TPIDR_EL1 */
>  	ARM64_SYS_REG(3, 0, 14, 1, 0),	/* CNTKCTL_EL1 */
>  	ARM64_SYS_REG(3, 2, 0, 0, 0),	/* CSSELR_EL1 */
> +	ARM64_SYS_REG(3, 3, 13, 0, 2),	/* TPIDR_EL0 */
> +	ARM64_SYS_REG(3, 3, 13, 0, 3),	/* TPIDRRO_EL0 */
> +	ARM64_SYS_REG(3, 4, 3, 0, 0),	/* DACR32_EL2 */
> +	ARM64_SYS_REG(3, 4, 5, 0, 1),	/* IFSR32_EL2 */
> +	ARM64_SYS_REG(3, 4, 5, 3, 0),	/* FPEXC32_EL2 */
> +};
> +
> +static __u64 pmu_regs[] = {
> +	ARM64_SYS_REG(3, 0, 9, 14, 1),	/* PMINTENSET_EL1 */
> +	ARM64_SYS_REG(3, 0, 9, 14, 2),	/* PMINTENCLR_EL1 */
>  	ARM64_SYS_REG(3, 3, 9, 12, 0),	/* PMCR_EL0 */
>  	ARM64_SYS_REG(3, 3, 9, 12, 1),	/* PMCNTENSET_EL0 */
>  	ARM64_SYS_REG(3, 3, 9, 12, 2),	/* PMCNTENCLR_EL0 */
> @@ -799,8 +814,6 @@ static __u64 base_regs[] = {
>  	ARM64_SYS_REG(3, 3, 9, 13, 0),	/* PMCCNTR_EL0 */
>  	ARM64_SYS_REG(3, 3, 9, 14, 0),	/* PMUSERENR_EL0 */
>  	ARM64_SYS_REG(3, 3, 9, 14, 3),	/* PMOVSSET_EL0 */
> -	ARM64_SYS_REG(3, 3, 13, 0, 2),	/* TPIDR_EL0 */
> -	ARM64_SYS_REG(3, 3, 13, 0, 3),	/* TPIDRRO_EL0 */
>  	ARM64_SYS_REG(3, 3, 14, 8, 0),
>  	ARM64_SYS_REG(3, 3, 14, 8, 1),
>  	ARM64_SYS_REG(3, 3, 14, 8, 2),
> @@ -864,9 +877,6 @@ static __u64 base_regs[] = {
>  	ARM64_SYS_REG(3, 3, 14, 15, 5),
>  	ARM64_SYS_REG(3, 3, 14, 15, 6),
>  	ARM64_SYS_REG(3, 3, 14, 15, 7),	/* PMCCFILTR_EL0 */
> -	ARM64_SYS_REG(3, 4, 3, 0, 0),	/* DACR32_EL2 */
> -	ARM64_SYS_REG(3, 4, 5, 0, 1),	/* IFSR32_EL2 */
> -	ARM64_SYS_REG(3, 4, 5, 3, 0),	/* FPEXC32_EL2 */
>  };
>  
>  static __u64 vregs[] = {
> @@ -970,6 +980,15 @@ static struct vcpu_config vregs_config = {
>  	{0},
>  	},
>  };
> +static struct vcpu_config vregs_pmu_config = {
> +	"vregs+pmu", .pmu = true,
> +	.sublists = {
> +	{ base_regs,	ARRAY_SIZE(base_regs), },
> +	{ vregs,	ARRAY_SIZE(vregs), },
> +	{ pmu_regs,	ARRAY_SIZE(pmu_regs), },
> +	{0},
> +	},
> +};
>  static struct vcpu_config sve_config = {
>  	"sve", .sve = true,
>  	.sublists = {
> @@ -978,9 +997,20 @@ static struct vcpu_config sve_config = {
>  	{0},
>  	},
>  };
> +static struct vcpu_config sve_pmu_config = {
> +	"sve+pmu", .sve = true, .pmu = true,
> +	.sublists = {
> +	{ base_regs,	ARRAY_SIZE(base_regs), },
> +	{ sve_regs,	ARRAY_SIZE(sve_regs),	sve_rejects_set,	ARRAY_SIZE(sve_rejects_set), },
> +	{ pmu_regs,	ARRAY_SIZE(pmu_regs), },
> +	{0},
> +	},
> +};
>  
>  static struct vcpu_config *vcpu_configs[] = {
>  	&vregs_config,
> +	&vregs_pmu_config,
>  	&sve_config,
> +	&sve_pmu_config,
>  };
>  static int vcpu_configs_n = ARRAY_SIZE(vcpu_configs);
> -- 
> 2.30.2
>

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index 78d8949bddbd..33b8735bdb15 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -38,6 +38,11 @@ static struct kvm_reg_list *reg_list;
 static __u64 *blessed_reg, blessed_n;
 
 struct reg_sublist {
+       const char *name;
+       bool base;
+       int feature;
+       int capability;
+       bool finalize;
        __u64 *regs;
        __u64 regs_n;
        __u64 *rejects_set;
@@ -46,8 +51,6 @@ struct reg_sublist {
 
 struct vcpu_config {
        const char *name;
-       bool sve;
-       bool pmu;
        struct reg_sublist sublists[];
 };
 
@@ -257,10 +260,7 @@ static void print_reg(struct vcpu_config *c, __u64 id)
                printf("\tKVM_REG_ARM_FW_REG(%lld),\n", id & 0xffff);
                break;
        case KVM_REG_ARM64_SVE:
-               if (c->sve)
-                       printf("\t%s,\n", sve_id_to_str(c, id));
-               else
-                       TEST_FAIL("%s: KVM_REG_ARM64_SVE is an unexpected coproc type in reg id: 0x%llx", c->name, id);
+               printf("\t%s,\n", sve_id_to_str(c, id));
                break;
        default:
                TEST_FAIL("%s: Unexpected coproc type: 0x%llx in reg id: 0x%llx",
@@ -327,31 +327,42 @@ static void core_reg_fixup(void)
 
 static void prepare_vcpu_init(struct vcpu_config *c, struct kvm_vcpu_init *init)
 {
-       if (c->sve)
-               init->features[0] |= 1 << KVM_ARM_VCPU_SVE;
-       if (c->pmu)
-               init->features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
+       struct reg_sublist *s;
+
+       for_each_sublist(c, s) {
+               if (s->base)
+                       continue;
+               init->features[0] |= 1 << s->feature;
+       }
 }
 
 static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid, struct vcpu_config *c)
 {
+       struct reg_sublist *s;
        int feature;
 
-       if (c->sve) {
-               feature = KVM_ARM_VCPU_SVE;
-               vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
+       for_each_sublist(c, s) {
+               if (s->base)
+                       continue;
+               if (s->finalize) {
+                       feature = s->feature;
+                       vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
+               }
        }
 }
 
 static void check_supported(struct vcpu_config *c)
 {
-       if (c->sve && !kvm_check_cap(KVM_CAP_ARM_SVE)) {
-               fprintf(stderr, "%s: SVE not available, skipping tests\n", c->name);
-               exit(KSFT_SKIP);
-       }
-       if (c->pmu && !kvm_check_cap(KVM_CAP_ARM_PMU_V3)) {
-               fprintf(stderr, "%s: PMU not available, skipping tests\n", c->name);
-               exit(KSFT_SKIP);
+       struct reg_sublist *s;
+
+       for_each_sublist(c, s) {
+               if (s->base)
+                       continue;
+               if (!kvm_check_cap(s->capability)) {
+                       fprintf(stderr, "%s: %s not available, skipping tests\n", c->name, s->name);
+                       exit(KSFT_SKIP);
+
+               }
        }
 }
 
@@ -975,34 +986,34 @@ static __u64 sve_rejects_set[] = {
 static struct vcpu_config vregs_config = {
        "vregs",
        .sublists = {
-       { base_regs,    ARRAY_SIZE(base_regs), },
-       { vregs,        ARRAY_SIZE(vregs), },
+       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
+       { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
        {0},
        },
 };
 static struct vcpu_config vregs_pmu_config = {
-       "vregs+pmu", .pmu = true,
+       "vregs+pmu",
        .sublists = {
-       { base_regs,    ARRAY_SIZE(base_regs), },
-       { vregs,        ARRAY_SIZE(vregs), },
-       { pmu_regs,     ARRAY_SIZE(pmu_regs), },
+       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
+       { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
+       { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
        {0},
        },
 };
 static struct vcpu_config sve_config = {
-       "sve", .sve = true,
+       "sve",
        .sublists = {
-       { base_regs,    ARRAY_SIZE(base_regs), },
-       { sve_regs,     ARRAY_SIZE(sve_regs),   sve_rejects_set,        ARRAY_SIZE(sve_rejects_set), },
+       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
+       { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
        {0},
        },
 };
 static struct vcpu_config sve_pmu_config = {
-       "sve+pmu", .sve = true, .pmu = true,
+       "sve+pmu",
        .sublists = {
-       { base_regs,    ARRAY_SIZE(base_regs), },
-       { sve_regs,     ARRAY_SIZE(sve_regs),   sve_rejects_set,        ARRAY_SIZE(sve_rejects_set), },
-       { pmu_regs,     ARRAY_SIZE(pmu_regs), },
+       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
+       { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
+       { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
        {0},
        },
 };

Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Andrew Jones]

On Tue, May 25, 2021 at 01:09:22PM -0700, Ricardo Koller wrote:
> On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> > Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> > userspace when not available") the get-reg-list* tests have been
> > failing with
> > 
> >   ...
> >   ... There are 74 missing registers.
> >   The following lines are missing registers:
> >   ...
> > 
> > where the 74 missing registers are all PMU registers. This isn't a
> > bug in KVM that the selftest found, even though it's true that a
> > KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> > flag, but still expecting the PMU registers to be in the reg-list,
> > would suddenly no longer have their expectations met. In that case,
> > the expectations were wrong, though, so that KVM userspace needs to
> > be fixed, and so does this selftest. The fix for this selftest is to
> > pull the PMU registers out of the base register sublist into their
> > own sublist and then create new, pmu-enabled vcpu configs which can
> > be tested.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
> >  1 file changed, 38 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > index dc06a28bfb74..78d8949bddbd 100644
> > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > @@ -47,6 +47,7 @@ struct reg_sublist {
> >  struct vcpu_config {
> >  	const char *name;
> >  	bool sve;
> > +	bool pmu;
> >  	struct reg_sublist sublists[];
> >  };
> 
> I think it's possible that the number of sublists keeps increasing: it
> would be very nice/useful if KVM allowed enabling/disabling more
> features from userspace (besides SVE, PMU etc). In that case, it might
> be easier if adding a new feature to get-reg-list just requires defining
> a new config and not dealing with the internals of vcpu_config.

Yes, adding the bools is a bit ugly, but how will we easily check if a
given feature is present in a given config? We could put a copy of the
vcpu_init features bitmap in vcpu_config, but I'm not sure if not touching
the vcpu_config structure is worth having to use test_bit() and friends
everywhere.

> 
> Do you think it's possible in general to associate a sublist to a
> capability and a feature? It works for the PMU and SVE. If that is
> possible, what do you think of something like this? this would be the
> config for sve+pmu:
> 
> static struct vcpu_config sve_pmu_config = {
>       "sve+pmu",
>        .sublists = {
>        { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
>        { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
>        { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
>        {0},
>        },
> };
> 
> Appended a rough patch at the end to make this idea more concrete.

Comments below

> diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> index 78d8949bddbd..33b8735bdb15 100644
> --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> @@ -38,6 +38,11 @@ static struct kvm_reg_list *reg_list;
>  static __u64 *blessed_reg, blessed_n;
>  
>  struct reg_sublist {
> +       const char *name;
> +       bool base;
> +       int feature;
> +       int capability;
> +       bool finalize;
>         __u64 *regs;
>         __u64 regs_n;
>         __u64 *rejects_set;
> @@ -46,8 +51,6 @@ struct reg_sublist {
>  
>  struct vcpu_config {
>         const char *name;
> -       bool sve;
> -       bool pmu;
>         struct reg_sublist sublists[];
>  };
>  
> @@ -257,10 +260,7 @@ static void print_reg(struct vcpu_config *c, __u64 id)
>                 printf("\tKVM_REG_ARM_FW_REG(%lld),\n", id & 0xffff);
>                 break;
>         case KVM_REG_ARM64_SVE:
> -               if (c->sve)
> -                       printf("\t%s,\n", sve_id_to_str(c, id));
> -               else
> -                       TEST_FAIL("%s: KVM_REG_ARM64_SVE is an unexpected coproc type in reg id: 0x%llx", c->name, id);
> +               printf("\t%s,\n", sve_id_to_str(c, id));

I'd rather not lose this test. What we were doing here is making sure we
don't see registers with KVM_REG_ARM64_SVE when sve is not enabled.

>                 break;
>         default:
>                 TEST_FAIL("%s: Unexpected coproc type: 0x%llx in reg id: 0x%llx",
> @@ -327,31 +327,42 @@ static void core_reg_fixup(void)
>  
>  static void prepare_vcpu_init(struct vcpu_config *c, struct kvm_vcpu_init *init)
>  {
> -       if (c->sve)
> -               init->features[0] |= 1 << KVM_ARM_VCPU_SVE;
> -       if (c->pmu)
> -               init->features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> +       struct reg_sublist *s;
> +
> +       for_each_sublist(c, s) {
> +               if (s->base)
> +                       continue;
> +               init->features[0] |= 1 << s->feature;
> +       }

If we want this to be general then we should ensure s->feature is < 32,
otherwise we need to move to the next word. Granted we only have a few
features so far for all the years we've had Arm KVM, so we probably don't
need to worry about this any time soon...

>  }
>  
>  static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid, struct vcpu_config *c)
>  {
> +       struct reg_sublist *s;
>         int feature;
>  
> -       if (c->sve) {
> -               feature = KVM_ARM_VCPU_SVE;
> -               vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
> +       for_each_sublist(c, s) {
> +               if (s->base)
> +                       continue;

Probably don't need the if (s->base) continue, since base registers won't
have s->finalize.

> +               if (s->finalize) {
> +                       feature = s->feature;
> +                       vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
> +               }
>         }
>  }
>  
>  static void check_supported(struct vcpu_config *c)
>  {
> -       if (c->sve && !kvm_check_cap(KVM_CAP_ARM_SVE)) {
> -               fprintf(stderr, "%s: SVE not available, skipping tests\n", c->name);
> -               exit(KSFT_SKIP);
> -       }
> -       if (c->pmu && !kvm_check_cap(KVM_CAP_ARM_PMU_V3)) {
> -               fprintf(stderr, "%s: PMU not available, skipping tests\n", c->name);
> -               exit(KSFT_SKIP);
> +       struct reg_sublist *s;
> +
> +       for_each_sublist(c, s) {
> +               if (s->base)
> +                       continue;

Also don't need the if (s->base) continue, since base registers won't have
capabilities.

> +               if (!kvm_check_cap(s->capability)) {
> +                       fprintf(stderr, "%s: %s not available, skipping tests\n", c->name, s->name);
> +                       exit(KSFT_SKIP);
> +
> +               }
>         }
>  }
>  
> @@ -975,34 +986,34 @@ static __u64 sve_rejects_set[] = {
>  static struct vcpu_config vregs_config = {
>         "vregs",
>         .sublists = {
> -       { base_regs,    ARRAY_SIZE(base_regs), },
> -       { vregs,        ARRAY_SIZE(vregs), },
> +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> +       { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
>         {0},
>         },
>  };
>  static struct vcpu_config vregs_pmu_config = {
> -       "vregs+pmu", .pmu = true,
> +       "vregs+pmu",
>         .sublists = {
> -       { base_regs,    ARRAY_SIZE(base_regs), },
> -       { vregs,        ARRAY_SIZE(vregs), },
> -       { pmu_regs,     ARRAY_SIZE(pmu_regs), },
> +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> +       { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
> +       { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
>         {0},
>         },
>  };
>  static struct vcpu_config sve_config = {
> -       "sve", .sve = true,
> +       "sve",
>         .sublists = {
> -       { base_regs,    ARRAY_SIZE(base_regs), },
> -       { sve_regs,     ARRAY_SIZE(sve_regs),   sve_rejects_set,        ARRAY_SIZE(sve_rejects_set), },
> +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> +       { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
>         {0},
>         },
>  };
>  static struct vcpu_config sve_pmu_config = {
> -       "sve+pmu", .sve = true, .pmu = true,
> +       "sve+pmu",
>         .sublists = {
> -       { base_regs,    ARRAY_SIZE(base_regs), },
> -       { sve_regs,     ARRAY_SIZE(sve_regs),   sve_rejects_set,        ARRAY_SIZE(sve_rejects_set), },
> -       { pmu_regs,     ARRAY_SIZE(pmu_regs), },
> +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> +       { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> +       { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
>         {0},
>         },
>  };
> 

It looks pretty good to me. While I don't really care about needing to add
booleans to vcpu_config, the biggest advantage I see is not needing to
modify prepare_vcpu_init, finalize_vcpu, and check_supported, and that the
feature bits and caps are better associated with the sublists.

These tables are getting wordy, though, so we'll probably want some
macros.

I'll experiment with this to see if I can integrate some of your
suggestions into a v3.

Thanks,
drew

Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Ricardo Koller]

On Wed, May 26, 2021 at 08:57:24AM +0200, Andrew Jones wrote:
> On Tue, May 25, 2021 at 01:09:22PM -0700, Ricardo Koller wrote:
> > On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> > > Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> > > userspace when not available") the get-reg-list* tests have been
> > > failing with
> > > 
> > >   ...
> > >   ... There are 74 missing registers.
> > >   The following lines are missing registers:
> > >   ...
> > > 
> > > where the 74 missing registers are all PMU registers. This isn't a
> > > bug in KVM that the selftest found, even though it's true that a
> > > KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> > > flag, but still expecting the PMU registers to be in the reg-list,
> > > would suddenly no longer have their expectations met. In that case,
> > > the expectations were wrong, though, so that KVM userspace needs to
> > > be fixed, and so does this selftest. The fix for this selftest is to
> > > pull the PMU registers out of the base register sublist into their
> > > own sublist and then create new, pmu-enabled vcpu configs which can
> > > be tested.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
> > >  1 file changed, 38 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > index dc06a28bfb74..78d8949bddbd 100644
> > > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > @@ -47,6 +47,7 @@ struct reg_sublist {
> > >  struct vcpu_config {
> > >  	const char *name;
> > >  	bool sve;
> > > +	bool pmu;
> > >  	struct reg_sublist sublists[];
> > >  };
> > 
> > I think it's possible that the number of sublists keeps increasing: it
> > would be very nice/useful if KVM allowed enabling/disabling more
> > features from userspace (besides SVE, PMU etc). In that case, it might
> > be easier if adding a new feature to get-reg-list just requires defining
> > a new config and not dealing with the internals of vcpu_config.
> 
> Yes, adding the bools is a bit ugly, but how will we easily check if a
> given feature is present in a given config? We could put a copy of the
> vcpu_init features bitmap in vcpu_config, but I'm not sure if not touching
> the vcpu_config structure is worth having to use test_bit() and friends
> everywhere.
> 

I agree, the bools are better than the bits.

My solution was to completely ignore the SVE test in print_reg (not the
best solution).

> > 
> > Do you think it's possible in general to associate a sublist to a
> > capability and a feature? It works for the PMU and SVE. If that is
> > possible, what do you think of something like this? this would be the
> > config for sve+pmu:
> > 
> > static struct vcpu_config sve_pmu_config = {
> >       "sve+pmu",
> >        .sublists = {
> >        { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> >        { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> >        { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
> >        {0},
> >        },
> > };
> > 
> > Appended a rough patch at the end to make this idea more concrete.
> 
> Comments below
>

Ack on all the comments.

> > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > index 78d8949bddbd..33b8735bdb15 100644
> > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > @@ -38,6 +38,11 @@ static struct kvm_reg_list *reg_list;
> >  static __u64 *blessed_reg, blessed_n;
> >  
> >  struct reg_sublist {
> > +       const char *name;
> > +       bool base;
> > +       int feature;
> > +       int capability;
> > +       bool finalize;
> >         __u64 *regs;
> >         __u64 regs_n;
> >         __u64 *rejects_set;
> > @@ -46,8 +51,6 @@ struct reg_sublist {
> >  
> >  struct vcpu_config {
> >         const char *name;
> > -       bool sve;
> > -       bool pmu;
> >         struct reg_sublist sublists[];
> >  };
> >  
> > @@ -257,10 +260,7 @@ static void print_reg(struct vcpu_config *c, __u64 id)
> >                 printf("\tKVM_REG_ARM_FW_REG(%lld),\n", id & 0xffff);
> >                 break;
> >         case KVM_REG_ARM64_SVE:
> > -               if (c->sve)
> > -                       printf("\t%s,\n", sve_id_to_str(c, id));
> > -               else
> > -                       TEST_FAIL("%s: KVM_REG_ARM64_SVE is an unexpected coproc type in reg id: 0x%llx", c->name, id);
> > +               printf("\t%s,\n", sve_id_to_str(c, id));
> 
> I'd rather not lose this test. What we were doing here is making sure we
> don't see registers with KVM_REG_ARM64_SVE when sve is not enabled.
> 
> >                 break;
> >         default:
> >                 TEST_FAIL("%s: Unexpected coproc type: 0x%llx in reg id: 0x%llx",
> > @@ -327,31 +327,42 @@ static void core_reg_fixup(void)
> >  
> >  static void prepare_vcpu_init(struct vcpu_config *c, struct kvm_vcpu_init *init)
> >  {
> > -       if (c->sve)
> > -               init->features[0] |= 1 << KVM_ARM_VCPU_SVE;
> > -       if (c->pmu)
> > -               init->features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> > +       struct reg_sublist *s;
> > +
> > +       for_each_sublist(c, s) {
> > +               if (s->base)
> > +                       continue;
> > +               init->features[0] |= 1 << s->feature;
> > +       }
> 
> If we want this to be general then we should ensure s->feature is < 32,
> otherwise we need to move to the next word. Granted we only have a few
> features so far for all the years we've had Arm KVM, so we probably don't
> need to worry about this any time soon...
> 
> >  }
> >  
> >  static void finalize_vcpu(struct kvm_vm *vm, uint32_t vcpuid, struct vcpu_config *c)
> >  {
> > +       struct reg_sublist *s;
> >         int feature;
> >  
> > -       if (c->sve) {
> > -               feature = KVM_ARM_VCPU_SVE;
> > -               vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
> > +       for_each_sublist(c, s) {
> > +               if (s->base)
> > +                       continue;
> 
> Probably don't need the if (s->base) continue, since base registers won't
> have s->finalize.
> 
> > +               if (s->finalize) {
> > +                       feature = s->feature;
> > +                       vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_FINALIZE, &feature);
> > +               }
> >         }
> >  }
> >  
> >  static void check_supported(struct vcpu_config *c)
> >  {
> > -       if (c->sve && !kvm_check_cap(KVM_CAP_ARM_SVE)) {
> > -               fprintf(stderr, "%s: SVE not available, skipping tests\n", c->name);
> > -               exit(KSFT_SKIP);
> > -       }
> > -       if (c->pmu && !kvm_check_cap(KVM_CAP_ARM_PMU_V3)) {
> > -               fprintf(stderr, "%s: PMU not available, skipping tests\n", c->name);
> > -               exit(KSFT_SKIP);
> > +       struct reg_sublist *s;
> > +
> > +       for_each_sublist(c, s) {
> > +               if (s->base)
> > +                       continue;
> 
> Also don't need the if (s->base) continue, since base registers won't have
> capabilities.
> 
> > +               if (!kvm_check_cap(s->capability)) {
> > +                       fprintf(stderr, "%s: %s not available, skipping tests\n", c->name, s->name);
> > +                       exit(KSFT_SKIP);
> > +
> > +               }
> >         }
> >  }
> >  
> > @@ -975,34 +986,34 @@ static __u64 sve_rejects_set[] = {
> >  static struct vcpu_config vregs_config = {
> >         "vregs",
> >         .sublists = {
> > -       { base_regs,    ARRAY_SIZE(base_regs), },
> > -       { vregs,        ARRAY_SIZE(vregs), },
> > +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> > +       { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
> >         {0},
> >         },
> >  };
> >  static struct vcpu_config vregs_pmu_config = {
> > -       "vregs+pmu", .pmu = true,
> > +       "vregs+pmu",
> >         .sublists = {
> > -       { base_regs,    ARRAY_SIZE(base_regs), },
> > -       { vregs,        ARRAY_SIZE(vregs), },
> > -       { pmu_regs,     ARRAY_SIZE(pmu_regs), },
> > +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> > +       { "vregs", true, 0, 0, false, vregs, ARRAY_SIZE(vregs), },
> > +       { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
> >         {0},
> >         },
> >  };
> >  static struct vcpu_config sve_config = {
> > -       "sve", .sve = true,
> > +       "sve",
> >         .sublists = {
> > -       { base_regs,    ARRAY_SIZE(base_regs), },
> > -       { sve_regs,     ARRAY_SIZE(sve_regs),   sve_rejects_set,        ARRAY_SIZE(sve_rejects_set), },
> > +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> > +       { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> >         {0},
> >         },
> >  };
> >  static struct vcpu_config sve_pmu_config = {
> > -       "sve+pmu", .sve = true, .pmu = true,
> > +       "sve+pmu",
> >         .sublists = {
> > -       { base_regs,    ARRAY_SIZE(base_regs), },
> > -       { sve_regs,     ARRAY_SIZE(sve_regs),   sve_rejects_set,        ARRAY_SIZE(sve_rejects_set), },
> > -       { pmu_regs,     ARRAY_SIZE(pmu_regs), },
> > +       { "base", true, 0, 0, false, base_regs, ARRAY_SIZE(base_regs), },
> > +       { "sve", false, KVM_ARM_VCPU_SVE, KVM_CAP_ARM_SVE, true, sve_regs, ARRAY_SIZE(sve_regs), sve_rejects_set, ARRAY_SIZE(sve_rejects_set), },
> > +       { "pmu", false, KVM_ARM_VCPU_PMU_V3, KVM_CAP_ARM_PMU_V3, false, pmu_regs, ARRAY_SIZE(pmu_regs), },
> >         {0},
> >         },
> >  };
> > 
> 
> It looks pretty good to me. While I don't really care about needing to add
> booleans to vcpu_config, the biggest advantage I see is not needing to
> modify prepare_vcpu_init, finalize_vcpu, and check_supported, and that the
> feature bits and caps are better associated with the sublists.
> 
> These tables are getting wordy, though, so we'll probably want some
> macros.
> 
> I'll experiment with this to see if I can integrate some of your
> suggestions into a v3.

Thanks for considering the changes.

> 
> Thanks,
> drew
>

Thanks,
Ricardo

Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Marc Zyngier]

On Tue, 25 May 2021 21:09:22 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> > Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> > userspace when not available") the get-reg-list* tests have been
> > failing with
> > 
> >   ...
> >   ... There are 74 missing registers.
> >   The following lines are missing registers:
> >   ...
> > 
> > where the 74 missing registers are all PMU registers. This isn't a
> > bug in KVM that the selftest found, even though it's true that a
> > KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> > flag, but still expecting the PMU registers to be in the reg-list,
> > would suddenly no longer have their expectations met. In that case,
> > the expectations were wrong, though, so that KVM userspace needs to
> > be fixed, and so does this selftest. The fix for this selftest is to
> > pull the PMU registers out of the base register sublist into their
> > own sublist and then create new, pmu-enabled vcpu configs which can
> > be tested.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
> >  1 file changed, 38 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > index dc06a28bfb74..78d8949bddbd 100644
> > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > @@ -47,6 +47,7 @@ struct reg_sublist {
> >  struct vcpu_config {
> >  	const char *name;
> >  	bool sve;
> > +	bool pmu;
> >  	struct reg_sublist sublists[];
> >  };
> 
> I think it's possible that the number of sublists keeps increasing: it
> would be very nice/useful if KVM allowed enabling/disabling more
> features from userspace (besides SVE, PMU etc).

[tangential semi-rant]

While this is a very noble goal, it also doubles the validation space
each time you add an option. Given how little testing gets done
relative to the diversity of features and implementations, that's a
*big* problem.

I'm not against it for big ticket items that result in a substantial
amount of state to be context-switched (SVE, NV). However, doing that
for more discrete features would require a radical change in the way
we develop, review and test KVM/arm64.

Thanks,

	M.

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

Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Andrew Jones]

On Wed, May 26, 2021 at 09:44:56AM +0100, Marc Zyngier wrote:
> On Tue, 25 May 2021 21:09:22 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> > > Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> > > userspace when not available") the get-reg-list* tests have been
> > > failing with
> > > 
> > >   ...
> > >   ... There are 74 missing registers.
> > >   The following lines are missing registers:
> > >   ...
> > > 
> > > where the 74 missing registers are all PMU registers. This isn't a
> > > bug in KVM that the selftest found, even though it's true that a
> > > KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> > > flag, but still expecting the PMU registers to be in the reg-list,
> > > would suddenly no longer have their expectations met. In that case,
> > > the expectations were wrong, though, so that KVM userspace needs to
> > > be fixed, and so does this selftest. The fix for this selftest is to
> > > pull the PMU registers out of the base register sublist into their
> > > own sublist and then create new, pmu-enabled vcpu configs which can
> > > be tested.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
> > >  1 file changed, 38 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > index dc06a28bfb74..78d8949bddbd 100644
> > > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > @@ -47,6 +47,7 @@ struct reg_sublist {
> > >  struct vcpu_config {
> > >  	const char *name;
> > >  	bool sve;
> > > +	bool pmu;
> > >  	struct reg_sublist sublists[];
> > >  };
> > 
> > I think it's possible that the number of sublists keeps increasing: it
> > would be very nice/useful if KVM allowed enabling/disabling more
> > features from userspace (besides SVE, PMU etc).
> 
> [tangential semi-rant]
> 
> While this is a very noble goal, it also doubles the validation space
> each time you add an option. Given how little testing gets done
> relative to the diversity of features and implementations, that's a
> *big* problem.

It's my hope that this test, especially now after its refactoring, will
allow us to test all configurations easily and therefore frequently.

> 
> I'm not against it for big ticket items that result in a substantial
> amount of state to be context-switched (SVE, NV). However, doing that
> for more discrete features would require a radical change in the way
> we develop, review and test KVM/arm64.
>

I'm not sure I understand how we should change the development and
review processes. As for testing, with simple tests like this one,
we can actually achieve exhaustive configuration testing fast, at
least with respect to checking for expected registers and checking
that we can get/set_one_reg on them. If we were to try and setup
QEMU migration tests for all the possible configurations, then it
would take way too long.

Thanks,
drew

Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Marc Zyngier]

On Wed, 26 May 2021 10:32:11 +0100,
Andrew Jones <drjones@redhat.com> wrote:
> 
> On Wed, May 26, 2021 at 09:44:56AM +0100, Marc Zyngier wrote:
> > On Tue, 25 May 2021 21:09:22 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> > > > Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> > > > userspace when not available") the get-reg-list* tests have been
> > > > failing with
> > > > 
> > > >   ...
> > > >   ... There are 74 missing registers.
> > > >   The following lines are missing registers:
> > > >   ...
> > > > 
> > > > where the 74 missing registers are all PMU registers. This isn't a
> > > > bug in KVM that the selftest found, even though it's true that a
> > > > KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> > > > flag, but still expecting the PMU registers to be in the reg-list,
> > > > would suddenly no longer have their expectations met. In that case,
> > > > the expectations were wrong, though, so that KVM userspace needs to
> > > > be fixed, and so does this selftest. The fix for this selftest is to
> > > > pull the PMU registers out of the base register sublist into their
> > > > own sublist and then create new, pmu-enabled vcpu configs which can
> > > > be tested.
> > > > 
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > >  .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
> > > >  1 file changed, 38 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > > index dc06a28bfb74..78d8949bddbd 100644
> > > > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > > @@ -47,6 +47,7 @@ struct reg_sublist {
> > > >  struct vcpu_config {
> > > >  	const char *name;
> > > >  	bool sve;
> > > > +	bool pmu;
> > > >  	struct reg_sublist sublists[];
> > > >  };
> > > 
> > > I think it's possible that the number of sublists keeps increasing: it
> > > would be very nice/useful if KVM allowed enabling/disabling more
> > > features from userspace (besides SVE, PMU etc).
> > 
> > [tangential semi-rant]
> > 
> > While this is a very noble goal, it also doubles the validation space
> > each time you add an option. Given how little testing gets done
> > relative to the diversity of features and implementations, that's a
> > *big* problem.
> 
> It's my hope that this test, especially now after its refactoring, will
> allow us to test all configurations easily and therefore frequently.
> 
> > 
> > I'm not against it for big ticket items that result in a substantial
> > amount of state to be context-switched (SVE, NV). However, doing that
> > for more discrete features would require a radical change in the way
> > we develop, review and test KVM/arm64.
> >
> 
> I'm not sure I understand how we should change the development and
> review processes.

I'm worried that the current ratio of development vs review vs testing
is simply not right. We have a huge reviewing deficit, and we end-up
merging buggy code. Some of the features we simply cannot test. It was
OK up to 3 years ago, but I'm not sure it is sustainable anymore.

So making more and more things optional seems to go further in the
direction of an uncontrolled bitrot.

> As for testing, with simple tests like this one,
> we can actually achieve exhaustive configuration testing fast, at
> least with respect to checking for expected registers and checking
> that we can get/set_one_reg on them. If we were to try and setup
> QEMU migration tests for all the possible configurations, then it
> would take way too long.

I'm not worried about this get/set thing. I'm worried about the full
end-to-end migration, which hardly anyone tests in anger, with all the
variability of the architecture and options.

	M.

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

Re: [PATCH v2 5/5] KVM: arm64: selftests: get-reg-list: Split base and pmu registers

[Andrew Jones]

On Wed, May 26, 2021 at 11:15:01AM +0100, Marc Zyngier wrote:
> On Wed, 26 May 2021 10:32:11 +0100,
> Andrew Jones <drjones@redhat.com> wrote:
> > 
> > On Wed, May 26, 2021 at 09:44:56AM +0100, Marc Zyngier wrote:
> > > On Tue, 25 May 2021 21:09:22 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > On Wed, May 19, 2021 at 04:07:26PM +0200, Andrew Jones wrote:
> > > > > Since KVM commit 11663111cd49 ("KVM: arm64: Hide PMU registers from
> > > > > userspace when not available") the get-reg-list* tests have been
> > > > > failing with
> > > > > 
> > > > >   ...
> > > > >   ... There are 74 missing registers.
> > > > >   The following lines are missing registers:
> > > > >   ...
> > > > > 
> > > > > where the 74 missing registers are all PMU registers. This isn't a
> > > > > bug in KVM that the selftest found, even though it's true that a
> > > > > KVM userspace that wasn't setting the KVM_ARM_VCPU_PMU_V3 VCPU
> > > > > flag, but still expecting the PMU registers to be in the reg-list,
> > > > > would suddenly no longer have their expectations met. In that case,
> > > > > the expectations were wrong, though, so that KVM userspace needs to
> > > > > be fixed, and so does this selftest. The fix for this selftest is to
> > > > > pull the PMU registers out of the base register sublist into their
> > > > > own sublist and then create new, pmu-enabled vcpu configs which can
> > > > > be tested.
> > > > > 
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  .../selftests/kvm/aarch64/get-reg-list.c      | 46 +++++++++++++++----
> > > > >  1 file changed, 38 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > > > index dc06a28bfb74..78d8949bddbd 100644
> > > > > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > > > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> > > > > @@ -47,6 +47,7 @@ struct reg_sublist {
> > > > >  struct vcpu_config {
> > > > >  	const char *name;
> > > > >  	bool sve;
> > > > > +	bool pmu;
> > > > >  	struct reg_sublist sublists[];
> > > > >  };
> > > > 
> > > > I think it's possible that the number of sublists keeps increasing: it
> > > > would be very nice/useful if KVM allowed enabling/disabling more
> > > > features from userspace (besides SVE, PMU etc).
> > > 
> > > [tangential semi-rant]
> > > 
> > > While this is a very noble goal, it also doubles the validation space
> > > each time you add an option. Given how little testing gets done
> > > relative to the diversity of features and implementations, that's a
> > > *big* problem.
> > 
> > It's my hope that this test, especially now after its refactoring, will
> > allow us to test all configurations easily and therefore frequently.
> > 
> > > 
> > > I'm not against it for big ticket items that result in a substantial
> > > amount of state to be context-switched (SVE, NV). However, doing that
> > > for more discrete features would require a radical change in the way
> > > we develop, review and test KVM/arm64.
> > >
> > 
> > I'm not sure I understand how we should change the development and
> > review processes.
> 
> I'm worried that the current ratio of development vs review vs testing
> is simply not right. We have a huge reviewing deficit, and we end-up
> merging buggy code. Some of the features we simply cannot test. It was
> OK up to 3 years ago, but I'm not sure it is sustainable anymore.
> 
> So making more and more things optional seems to go further in the
> direction of an uncontrolled bitrot.

I guess the optional CPU features are just going to keep on coming. And,
while more reviewers would help, there will never be enough. I think the
only solution is to get more CI.

> 
> > As for testing, with simple tests like this one,
> > we can actually achieve exhaustive configuration testing fast, at
> > least with respect to checking for expected registers and checking
> > that we can get/set_one_reg on them. If we were to try and setup
> > QEMU migration tests for all the possible configurations, then it
> > would take way too long.
> 
> I'm not worried about this get/set thing. I'm worried about the full
> end-to-end migration, which hardly anyone tests in anger, with all the
> variability of the architecture and options.

It does get tested downstream, but certain configurations will likely
be neglected. For example, we've rarely, if ever, tested with the PMU
disabled. Also, testing downstream is a bit late. It'd be better if
tests were running on upstream branches, before even being merged to
master.

As for testing migration of devices other than the CPU, we do have
some QEMU unit tests for that which gate merger to QEMU master.

Anyway, while unit tests like this one may seem too simple to be useful,
assuming the tests mimic key parts of the fully integrated function, and
are run frequently, then they may catch regressions sooner, even during
development. The less frequently run integrated tests which happen later,
and with limited configs, may then be sufficient.

BTW, kvm-unit-tests can also test migrations. The VM configs are limited,
but CPU feature combinations could be tested thoroughly without too
much difficulty. That would at least include QEMU in the integration
testing, but unless we modify the tests to migrate between hosts with
different kernel versions (it's nice to try and support older -> newer),
then we're not testing the same type of thing that we're testing here with
this test.

Thanks,
drew