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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] KVM: selftests: arm64: fatal vcpu_set_reg() in test_reg_set_success()
--

commit 528a199af5ef8c602f0139ccc2860e91e650c762
Author: Mark Brown <broonie@kernel.org>

KVM: selftests: arm64: Make set_id_regs bitfield validity checks non-fatal

This commit converts fatal assertions in set_id_regs feature register
validation checks into non-fatal kselftest result reports that include the
affected bitfield name and allow subsequent tests to continue running.

> diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> index 37107eadd9854..d7f8b8efe1e28 100644
> --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> @@ -423,6 +423,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;
> @@ -435,7 +436,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);

[Severity: Low]
As reported by Sashiko on v8 [1], in test_reg_set_success() in
tools/testing/selftests/kvm/arm64/set_id_regs.c, vcpu_set_reg() is still used
to write the register value before checking whether the write succeeded.

In tools/testing/selftests/kvm/include/kvm_util.h, vcpu_set_reg() calls
vcpu_ioctl(), which triggers a fatal assertion if the KVM_SET_ONE_REG ioctl
returns a non-zero value:

tools/testing/selftests/kvm/include/kvm_util.h:vcpu_set_reg() {
	struct kvm_one_reg reg = { .id = id, .addr = (u64)&val };

	TEST_ASSERT(KVM_REG_SIZE(id) <= sizeof(val), "Reg %lx too big", id);

	vcpu_ioctl(vcpu, KVM_SET_ONE_REG, &reg);
}

When KVM rejects an ID register bitfield write in arch/arm64/kvm/sys_regs.c,
set_id_reg() returns -EINVAL from the ioctl rather than succeeding and
leaving the register unchanged:

arch/arm64/kvm/sys_regs.c:set_id_reg() {
	...
	ret = arm64_check_features(vcpu, rd, val);
	if (!ret)
		kvm_set_vm_id_reg(vcpu->kvm, id, val);

	mutex_unlock(&vcpu->kvm->arch.config_lock);

	if (ret == -E2BIG)
		ret = -EINVAL;
	return ret;
}

If KVM unexpectedly rejects a valid write during
main() -> test_vm_ftr_id_regs() -> test_reg_set_success(), wouldn't
vcpu_set_reg() abort the test binary immediately before reaching
ksft_test_result() to report ftr_bits->name and continue running the remaining
tests? Should test_reg_set_success() use __vcpu_set_reg() and handle a
non-zero return value non-fatally, similar to test_reg_set_fail()?

[1] https://lore.kernel.org/r/20260929163445.0D9331F000FF@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-kvm-arm64-set-id-regs-aarch64-v9-0-ab32bc79fa9c@kernel.org?part=3