Re: [PATCH] KVM: selftests: Test the PV_UNHALT feature, not just its CPUID bit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The arguments to `KVM_HC_KICK_CPU` are swapped in the selftest's `kvm_hypercall()` invocation, causing the test to falsely pass only because the target vCPU's APIC ID happens to be 0.
- [Low] Incorrect error reporting for `pthread_create` and `pthread_join`, as they do not set `errno`.
--

commit 6ba52c6d2b00ca080ff07b971fc3f8bbdf50de71
Author: Hemanth Selam <hemanth.selam@gmail.com>

KVM: selftests: Test the PV_UNHALT feature, not just its CPUID bit

This commit implements a two-vCPU test for the KVM PV_UNHALT feature to
actually exercise it instead of just checking its CPUID bit. It verifies
that one vCPU can successfully kick and wake up a halted vCPU using
KVM_HC_KICK_CPU.

> diff --git a/tools/testing/selftests/kvm/x86/kvm_pv_test.c b/tools/testing/selftests/kvm/x86/kvm_pv_test.c
> index 8ed5fa6350211..d14d55d3a9ba7 100644
> --- a/tools/testing/selftests/kvm/x86/kvm_pv_test.c
> +++ b/tools/testing/selftests/kvm/x86/kvm_pv_test.c
[ ... ]
> +static void pv_unhalt_kicker_guest_code(u32 halter_apic_id)
> +{
> +	GUEST_ASSERT_EQ(kvm_hypercall(KVM_HC_KICK_CPU, halter_apic_id, 0, 0, 0), 0);

[Severity: Medium]
Are the arguments to kvm_hypercall() in the correct order here?

Passing halter_apic_id as the first argument places it into a0 instead of a1.
Since KVM's handler uses a1 for the APIC ID, it will always kick APIC ID 0
instead of the dynamically determined halter_apic_id.

Does this regression falsely allow the test to pass only because the target
vCPU is created first and its APIC ID happens to be 0?

> +	GUEST_DONE();
> +}
[ ... ]
> +	r = pthread_create(&thread, NULL, pv_unhalt_halter_thread, halter);
> +	TEST_ASSERT(!r, "pthread_create halter failed, errno=%d", errno);

[Severity: Low]
Does pthread_create() set errno?

It returns the error code directly in r. Should this print r instead of
errno to avoid showing a stale or zero value when it fails?

[ ... ]
> +	/* Nothing except the kick can get the halter to GUEST_DONE(). */
> +	r = pthread_join(thread, NULL);
> +	TEST_ASSERT(!r, "pthread_join halter failed, errno=%d", errno);

[Severity: Low]
Similarly here, pthread_join() returns the error code directly. Should this
log r instead of errno?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826101627.2873660-1-hemanth.selam@gmail.com?part=1