Hi Mark,
On 12/19/25 19:28, Mark Brown wrote:
> 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 <[email protected]>
> ---
> 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;
> + }
> +
Unnecessary? The decision for which regs are testing is made in
guest_code().
> 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) {
Doesn't this exclude more registers than needed? E.g. MIDR?
> + 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);
>
>
Thanks,
Ben
