Re: [PATCH v8 19/20] KVM: x86: selftests: Add APX state and ABI test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kselftests: missing volatile in read_egpr() causes spurious test failures
--

commit 9da537583adffa94be39a84efcfb15a0c9610def
Author: Chang S. Bae <chang.seok.bae@intel.com>

KVM: x86: selftests: Add APX state and ABI test

This commit introduces selftests for APX-specific behavior and ABI
interactions in KVM. It implements guest code to read and write EGPR state
and verifies the state transitions after userspace updates and APX
disablement.

> diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h
> index 6e6f70035508a..6e3e7d96c571e 100644
> --- a/tools/testing/selftests/kvm/include/x86/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86/processor.h

[ ... ]

> @@ -917,6 +919,124 @@ static inline void write_sse_reg(int reg, const sse128_t *data)
>  	}
>  }
>  
> +static inline unsigned long read_egpr(int reg)
> +{
> +	unsigned long data = 0;
> +
> +	/* mov %r16..%r31, %rax */
> +	switch (reg) {
> +	case 16:
> +		asm(".byte 0xd5, 0x48, 0x89, 0xc0" : "=a"(data));

[Severity: Medium]
Does read_egpr() need the volatile qualifier on its inline assembly
statements?

Since the asm statements only have an output operand, standard compiler
optimizations can treat them as pure computations. When called multiple
times, the compiler might cache the result of the first read and skip
subsequent executions, leading to stale register reads.

[ ... ]

> diff --git a/tools/testing/selftests/kvm/x86/apx_test.c b/tools/testing/selftests/kvm/x86/apx_test.c
> new file mode 100644
> index 0000000000000..4825003d0616b
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/apx_test.c

[ ... ]

> +static void test_userspace_update(void)
> +{
> +	/* Userspace updates EGPR state via the KVM XSAVE ABI */
> +	GUEST_SYNC(USERSPACE_UPDATE);
> +	GUEST_ASSERT(validate_egprs(USERSPACE_UPDATE));

[Severity: Medium]
Will compiler hoisting cause this validation to fail spuriously?

This sequence expects the EGPR state to be read strictly after the userspace
update and the hypercall returns. Because read_egpr() lacks a volatile
qualifier, the read inside validate_egprs() might be optimized away. This
could cause the test to use a stale cached value from earlier and trigger a
spurious failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930210750.1487547-1-chang.seok.bae@intel.com?part=19