Re: [PATCH v3 4/4] KVM: selftests: arm64: Skip all 32 bit IDs when set_id_regs is aarch64 only

[Ben Horgan <ben.horgan@arm.com>]

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 <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;
> +			}
> +

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