Re: [PATCH v7 3/3] KVM: selftests: arm64: Make set_id_regs bitfield validatity checks non-fatal

[Lorenzo Stoakes (ARM)]

Typo in subject: validatity -> validity

On Tue, Sep 01, 2026 at 07:50:03PM +0100, Mark Brown wrote:
> 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.
>
> Reviewed-by: Ben Horgan <ben.horgan@xxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>

One NIT below and typo noted above, with those addressed LGTM so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

> ---
>  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 3aa8886e8b70..385e1f1c0133 100644
> --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> @@ -422,6 +422,7 @@ static u64 test_reg_set_success(struct kvm_vcpu *vcpu, u64 reg,
>  	u8 shift = ftr_bits->shift;
>  	u64 mask = ftr_bits->mask;
>  	u64 val, new_val, ftr;
> +	bool match;
>
>  	val = vcpu_get_reg(vcpu, reg);
>  	ftr = (val & mask) >> shift;
> @@ -434,7 +435,10 @@ static u64 test_reg_set_success(struct kvm_vcpu *vcpu, u64 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);

Same pattern (I wonder, not for this series, but perhaps a follow up whether
this could be generalised?)

>
>  	return new_val;
>  }
> @@ -446,6 +450,7 @@ static void test_reg_set_fail(struct kvm_vcpu *vcpu, u64 reg,
>  	u64 mask = ftr_bits->mask;
>  	u64 val, old_val, ftr;
>  	int r;
> +	bool match;
>
>  	val = vcpu_get_reg(vcpu, reg);
>  	ftr = (val & mask) >> shift;
> @@ -462,7 +467,10 @@ static void test_reg_set_fail(struct kvm_vcpu *vcpu, u64 reg,
>  		    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
>

NIT:

There's a:

	TEST_ASSERT(r < 0 && errno == EINVAL,
		    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);

Above, do we want to make that non-fatal too?

>  	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 u64 test_reg_vals[KVM_ARM_FEATURE_ID_RANGE_SIZE];
> @@ -502,7 +510,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;
>  			}
> @@ -514,8 +526,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);
>  		}
>  	}
>  }
> @@ -882,7 +892,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.3
>
>

--
Cheers, Lorenzo