While debugging issues related to aarch64 only systems I ran into speedbumps due to the lack of detail in the results reported when the guest register read and reset value preservation tests were run, they generated an immediately fatal assert without indicating which register was being tested. Update these tests to report a result per register, making it much easier to see what the problem being reported is.
A similar, though less severe, issue exists with the validation of the individual bitfields in registers due to the use of immediately fatal asserts. Update those asserts to be standard kselftest reports.
Finally we have a fix for spurious errors on some NV systems.
Signed-off-by: Mark Brown broonie@kernel.org --- Changes in v3: - Rebase onto v6.19-rc1. - Link to v2: https://patch.msgid.link/20251114-kvm-arm64-set-id-regs-aarch64-v2-0-672f214...
Changes in v2: - Add a fix for spurious failures with 64 bit only guests. - Link to v1: https://patch.msgid.link/20251030-kvm-arm64-set-id-regs-aarch64-v1-0-96fe0d2...
--- Mark Brown (4): KVM: selftests: arm64: Report set_id_reg reads of test registers as tests KVM: selftests: arm64: Report register reset tests individually KVM: selftests: arm64: Make set_id_regs bitfield validatity checks non-fatal KVM: selftests: arm64: Skip all 32 bit IDs when set_id_regs is aarch64 only
tools/testing/selftests/kvm/arm64/set_id_regs.c | 150 ++++++++++++++++++------ 1 file changed, 111 insertions(+), 39 deletions(-) --- base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8 change-id: 20251028-kvm-arm64-set-id-regs-aarch64-ebb77969401c
Best regards, -- Mark Brown broonie@kernel.org
Currently when set_id_regs encounters a problem checking validation of writes to feature registers it uses an immediately fatal assert to report the problem. This is not idiomatic for kselftest, and it is also not great for usability. The affected bitfield is not clearly reported and further tests do not have their results reported.
Switch to using standard kselftest result reporting for the two asserts we do, these are non-fatal asserts so allow the program to continue and the test names include the affected field.
Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/kvm/arm64/set_id_regs.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c index b61942895808..5837da63e9b9 100644 --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c @@ -409,6 +409,7 @@ static uint64_t test_reg_set_success(struct kvm_vcpu *vcpu, uint64_t reg, uint8_t shift = ftr_bits->shift; uint64_t mask = ftr_bits->mask; uint64_t val, new_val, ftr; + bool match;
val = vcpu_get_reg(vcpu, reg); ftr = (val & mask) >> shift; @@ -421,7 +422,10 @@ static uint64_t test_reg_set_success(struct kvm_vcpu *vcpu, uint64_t reg,
vcpu_set_reg(vcpu, reg, val); new_val = vcpu_get_reg(vcpu, reg); - TEST_ASSERT_EQ(new_val, val); + match = new_val == val; + if (!match) + ksft_print_msg("%lx != %lx\n", new_val, val); + ksft_test_result(match, "%s valid write succeeded\n", ftr_bits->name);
return new_val; } @@ -433,6 +437,7 @@ static void test_reg_set_fail(struct kvm_vcpu *vcpu, uint64_t reg, uint64_t mask = ftr_bits->mask; uint64_t val, old_val, ftr; int r; + bool match;
val = vcpu_get_reg(vcpu, reg); ftr = (val & mask) >> shift; @@ -449,7 +454,10 @@ static void test_reg_set_fail(struct kvm_vcpu *vcpu, uint64_t reg, "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
val = vcpu_get_reg(vcpu, reg); - TEST_ASSERT_EQ(val, old_val); + match = val == old_val; + if (!match) + ksft_print_msg("%lx != %lx\n", val, old_val); + ksft_test_result(match, "%s invalid write rejected\n", ftr_bits->name); }
static uint64_t test_reg_vals[KVM_ARM_FEATURE_ID_RANGE_SIZE]; @@ -489,7 +497,11 @@ static void test_vm_ftr_id_regs(struct kvm_vcpu *vcpu, bool aarch64_only) for (int j = 0; ftr_bits[j].type != FTR_END; j++) { /* Skip aarch32 reg on aarch64 only system, since they are RAZ/WI. */ if (aarch64_only && sys_reg_CRm(reg_id) < 4) { - ksft_test_result_skip("%s on AARCH64 only system\n", + ksft_print_msg("%s on AARCH64 only system\n", + ftr_bits[j].name); + ksft_test_result_skip("%s invalid write rejected\n", + ftr_bits[j].name); + ksft_test_result_skip("%s valid write succeeded\n", ftr_bits[j].name); continue; } @@ -501,8 +513,6 @@ static void test_vm_ftr_id_regs(struct kvm_vcpu *vcpu, bool aarch64_only)
test_reg_vals[idx] = test_reg_set_success(vcpu, reg, &ftr_bits[j]); - - ksft_test_result_pass("%s\n", ftr_bits[j].name); } } } @@ -839,7 +849,7 @@ int main(void) ID_REG_RESET_UNCHANGED_TEST; for (i = 0; i < ARRAY_SIZE(test_regs); i++) for (j = 0; test_regs[i].ftr_bits[j].type != FTR_END; j++) - test_cnt++; + test_cnt += 2;
ksft_set_plan(test_cnt);
On an aarch64 only system the 32 bit ID registers have UNDEFINED values. As a result set_id_regs skips tests for setting fields in these registers when testing an aarch64 only guest. This has the side effect of meaning that we don't record an expected value for these registers, meaning that when the subsequent tests for values being visible in guests and preserved over reset check the value they can spuriously fail. This can be seen by running on an emulated system with both NV and 32 bit enabled, NV will result in the guests created by the test program being 64 bit only but the 32 bit ID registers will have values.
Also skip those tests that use the values set in the field setting tests for aarch64 only guests in order to avoid these spurious failures.
Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/kvm/arm64/set_id_regs.c | 42 +++++++++++++++++-------- 1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c index 5837da63e9b9..f19ba949aa18 100644 --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c @@ -675,7 +675,7 @@ static void test_user_set_mte_reg(struct kvm_vcpu *vcpu) ksft_test_result_pass("ID_AA64PFR1_EL1.MTE_frac no longer 0xF\n"); }
-static void test_guest_reg_read(struct kvm_vcpu *vcpu) +static void test_guest_reg_read(struct kvm_vcpu *vcpu, bool aarch64_only) { bool done = false; struct ucall uc; @@ -694,6 +694,13 @@ static void test_guest_reg_read(struct kvm_vcpu *vcpu) reg_id = uc.args[2]; guest_val = uc.args[3]; expected_val = test_reg_vals[encoding_to_range_idx(reg_id)]; + + if (aarch64_only && sys_reg_CRm(reg_id) < 4) { + ksft_test_result_skip("%s value seen in guest\n", + get_reg_name(reg_id)); + break; + } + match = expected_val == guest_val; if (!match) ksft_print_msg("%lx != %lx\n", @@ -785,12 +792,19 @@ static void test_vcpu_non_ftr_id_regs(struct kvm_vcpu *vcpu) ksft_test_result_pass("%s\n", __func__); }
-static void test_assert_id_reg_unchanged(struct kvm_vcpu *vcpu, uint32_t encoding) +static void test_assert_id_reg_unchanged(struct kvm_vcpu *vcpu, uint32_t encoding, + bool aarch64_only) { size_t idx = encoding_to_range_idx(encoding); uint64_t observed; bool pass;
+ if (aarch64_only && sys_reg_CRm(encoding) < 4) { + ksft_test_result_skip("%s unchanged by reset\n", + get_reg_name(encoding)); + return; + } + observed = vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(encoding)); pass = test_reg_vals[idx] == observed; if (!pass) @@ -801,7 +815,8 @@ static void test_assert_id_reg_unchanged(struct kvm_vcpu *vcpu, uint32_t encodin
#define ID_REG_RESET_UNCHANGED_TEST (ARRAY_SIZE(test_regs) + 6)
-static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu) +static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu, + bool aarch64_only) { /* * Calls KVM_ARM_VCPU_INIT behind the scenes, which will do an @@ -810,14 +825,15 @@ static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu) aarch64_vcpu_setup(vcpu, NULL);
for (int i = 0; i < ARRAY_SIZE(test_regs); i++) - test_assert_id_reg_unchanged(vcpu, test_regs[i].reg); - - test_assert_id_reg_unchanged(vcpu, SYS_MPIDR_EL1); - test_assert_id_reg_unchanged(vcpu, SYS_CLIDR_EL1); - test_assert_id_reg_unchanged(vcpu, SYS_CTR_EL0); - test_assert_id_reg_unchanged(vcpu, SYS_MIDR_EL1); - test_assert_id_reg_unchanged(vcpu, SYS_REVIDR_EL1); - test_assert_id_reg_unchanged(vcpu, SYS_AIDR_EL1); + test_assert_id_reg_unchanged(vcpu, test_regs[i].reg, + aarch64_only); + + test_assert_id_reg_unchanged(vcpu, SYS_MPIDR_EL1, aarch64_only); + test_assert_id_reg_unchanged(vcpu, SYS_CLIDR_EL1, aarch64_only); + test_assert_id_reg_unchanged(vcpu, SYS_CTR_EL0, aarch64_only); + test_assert_id_reg_unchanged(vcpu, SYS_MIDR_EL1, aarch64_only); + test_assert_id_reg_unchanged(vcpu, SYS_REVIDR_EL1, aarch64_only); + test_assert_id_reg_unchanged(vcpu, SYS_AIDR_EL1, aarch64_only); }
int main(void) @@ -859,9 +875,9 @@ int main(void) test_user_set_mpam_reg(vcpu); test_user_set_mte_reg(vcpu);
- test_guest_reg_read(vcpu); + test_guest_reg_read(vcpu, aarch64_only);
- test_reset_preserves_id_regs(vcpu); + test_reset_preserves_id_regs(vcpu, aarch64_only);
kvm_vm_free(vm);
linux-kselftest-mirror@lists.linaro.org
-
Mark Brown