Currently when we run guest code to validate that the values we wrote to the registers are seen by the guest we assert that these values match using a KVM selftests level assert, resulting in unclear diagnostics if the test fails. Replace this assert with reporting a kselftest test per register. In order to support getting the names of the registers we repaint the array of ID_ registers to store the names and open code the rest. Signed-off-by: Mark Brown --- tools/testing/selftests/kvm/arm64/set_id_regs.c | 74 +++++++++++++++++++------ 1 file changed, 57 insertions(+), 17 deletions(-) diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c index 5e24f77868b5..7a759e976c2c 100644 --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c @@ -40,6 +40,7 @@ struct reg_ftr_bits { }; struct test_feature_reg { + const char *name; uint32_t reg; const struct reg_ftr_bits *ftr_bits; }; @@ -218,24 +219,25 @@ static const struct reg_ftr_bits ftr_id_aa64zfr0_el1[] = { #define TEST_REG(id, table) \ { \ - .reg = id, \ + .name = #id, \ + .reg = SYS_ ## id, \ .ftr_bits = &((table)[0]), \ } static struct test_feature_reg test_regs[] = { - TEST_REG(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0_el1), - TEST_REG(SYS_ID_DFR0_EL1, ftr_id_dfr0_el1), - TEST_REG(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0_el1), - TEST_REG(SYS_ID_AA64ISAR1_EL1, ftr_id_aa64isar1_el1), - TEST_REG(SYS_ID_AA64ISAR2_EL1, ftr_id_aa64isar2_el1), - TEST_REG(SYS_ID_AA64ISAR3_EL1, ftr_id_aa64isar3_el1), - TEST_REG(SYS_ID_AA64PFR0_EL1, ftr_id_aa64pfr0_el1), - TEST_REG(SYS_ID_AA64PFR1_EL1, ftr_id_aa64pfr1_el1), - TEST_REG(SYS_ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0_el1), - TEST_REG(SYS_ID_AA64MMFR1_EL1, ftr_id_aa64mmfr1_el1), - TEST_REG(SYS_ID_AA64MMFR2_EL1, ftr_id_aa64mmfr2_el1), - TEST_REG(SYS_ID_AA64MMFR3_EL1, ftr_id_aa64mmfr3_el1), - TEST_REG(SYS_ID_AA64ZFR0_EL1, ftr_id_aa64zfr0_el1), + TEST_REG(ID_AA64DFR0_EL1, ftr_id_aa64dfr0_el1), + TEST_REG(ID_DFR0_EL1, ftr_id_dfr0_el1), + TEST_REG(ID_AA64ISAR0_EL1, ftr_id_aa64isar0_el1), + TEST_REG(ID_AA64ISAR1_EL1, ftr_id_aa64isar1_el1), + TEST_REG(ID_AA64ISAR2_EL1, ftr_id_aa64isar2_el1), + TEST_REG(ID_AA64ISAR3_EL1, ftr_id_aa64isar3_el1), + TEST_REG(ID_AA64PFR0_EL1, ftr_id_aa64pfr0_el1), + TEST_REG(ID_AA64PFR1_EL1, ftr_id_aa64pfr1_el1), + TEST_REG(ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0_el1), + TEST_REG(ID_AA64MMFR1_EL1, ftr_id_aa64mmfr1_el1), + TEST_REG(ID_AA64MMFR2_EL1, ftr_id_aa64mmfr2_el1), + TEST_REG(ID_AA64MMFR3_EL1, ftr_id_aa64mmfr3_el1), + TEST_REG(ID_AA64ZFR0_EL1, ftr_id_aa64zfr0_el1), }; #define GUEST_REG_SYNC(id) GUEST_SYNC_ARGS(0, id, read_sysreg_s(id), 0, 0); @@ -265,6 +267,34 @@ static void guest_code(void) GUEST_DONE(); } +#define GUEST_READ_TEST (ARRAY_SIZE(test_regs) + 6) + +static const char *get_reg_name(u64 id) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(test_regs); i++) + if (test_regs[i].reg == id) + return test_regs[i].name; + + switch (id) { + case SYS_MPIDR_EL1: + return "MPIDR_EL1"; + case SYS_CLIDR_EL1: + return "CLIDR_EL1"; + case SYS_CTR_EL0: + return "CTR_EL0"; + case SYS_MIDR_EL1: + return "MIDR_EL1"; + case SYS_REVIDR_EL1: + return "REVIDR_EL1"; + case SYS_AIDR_EL1: + return "AIDR_EL1"; + default: + TEST_FAIL("Unknown register"); + } +} + /* Return a safe value to a given ftr_bits an ftr value */ uint64_t get_safe_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr) { @@ -635,6 +665,8 @@ static void test_guest_reg_read(struct kvm_vcpu *vcpu) { bool done = false; struct ucall uc; + uint64_t reg_id, expected_val, guest_val; + bool match; while (!done) { vcpu_run(vcpu); @@ -645,8 +677,16 @@ static void test_guest_reg_read(struct kvm_vcpu *vcpu) break; case UCALL_SYNC: /* Make sure the written values are seen by guest */ - TEST_ASSERT_EQ(test_reg_vals[encoding_to_range_idx(uc.args[2])], - uc.args[3]); + reg_id = uc.args[2]; + guest_val = uc.args[3]; + expected_val = test_reg_vals[encoding_to_range_idx(reg_id)]; + match = expected_val == guest_val; + if (!match) + ksft_print_msg("%lx != %lx\n", + expected_val, guest_val); + ksft_test_result(match, + "%s value seen in guest\n", + get_reg_name(reg_id)); break; case UCALL_DONE: done = true; @@ -786,7 +826,7 @@ int main(void) ksft_print_header(); - test_cnt = 3 + MPAM_IDREG_TEST + MTE_IDREG_TEST; + test_cnt = 3 + MPAM_IDREG_TEST + MTE_IDREG_TEST + GUEST_READ_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++; -- 2.47.2 set_id_regs tests that registers have their values preserved over reset. Currently it reports all registers in a single test with an instantly fatal assert which isn't great for diagnostics, it's hard to tell which register failed or if it's just one register. Change this to report each register as a separate test so that it's clear from the program output which registers have problems. Signed-off-by: Mark Brown --- tools/testing/selftests/kvm/arm64/set_id_regs.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c index 7a759e976c2c..1a53f3a4be8d 100644 --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c @@ -775,11 +775,18 @@ static void test_assert_id_reg_unchanged(struct kvm_vcpu *vcpu, uint32_t encodin { size_t idx = encoding_to_range_idx(encoding); uint64_t observed; + bool pass; observed = vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(encoding)); - TEST_ASSERT_EQ(test_reg_vals[idx], observed); + pass = test_reg_vals[idx] == observed; + if (!pass) + ksft_print_msg("%lx != %lx\n", test_reg_vals[idx], observed); + ksft_test_result(pass, "%s unchanged by reset\n", + get_reg_name(encoding)); } +#define ID_REG_RESET_UNCHANGED_TEST (ARRAY_SIZE(test_regs) + 6) + static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu) { /* @@ -797,8 +804,6 @@ static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu) 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); - - ksft_test_result_pass("%s\n", __func__); } int main(void) @@ -826,7 +831,8 @@ int main(void) ksft_print_header(); - test_cnt = 3 + MPAM_IDREG_TEST + MTE_IDREG_TEST + GUEST_READ_TEST; + test_cnt = 2 + MPAM_IDREG_TEST + MTE_IDREG_TEST + GUEST_READ_TEST + + 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++; -- 2.47.2 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 --- 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 1a53f3a4be8d..abe97f9293c9 100644 --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c @@ -405,6 +405,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; @@ -417,7 +418,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; } @@ -429,6 +433,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; @@ -445,7 +450,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]; @@ -485,7 +493,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; } @@ -497,8 +509,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); } } } @@ -835,7 +845,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); -- 2.47.2