Re: [PATCH 21/22] KVM: arm64: selftests: Test AT emulation for FEAT_HAFT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing `isb()` instructions between consecutive writes to control-plane system registers.
- [Low] Typo `sysrec_clear_set_s` instead of `sysreg_clear_set_s` causes a build failure.
--

commit 72540780d6ff6cb4b6ed1c45760fb4372cfa7e9b
Author: Oliver Upton <oupton@kernel.org>

KVM: arm64: selftests: Test AT emulation for FEAT_HAFT

Test that KVM's AT emulation sets the access flag on table descriptors
when FEAT_HAFT is enabled at stage-1. Additionally, add test coverage
that asserts the access flag is clear on table/page descriptors when the
AT instruction generates an access flag fault.

> diff --git a/tools/testing/selftests/kvm/arm64/at.c b/tools/testing/selftests/kvm/arm64/at.c
> index d7289f3df04fa..fb7399736f442 100644
> --- a/tools/testing/selftests/kvm/arm64/at.c
> +++ b/tools/testing/selftests/kvm/arm64/at.c

[ ... ]

> @@ -93,9 +103,43 @@ static void guest_code(void)
>  	isb();
>  	test_at(false);
>  
> +	if (!guest_has_haft())
> +		GUEST_DONE();
> +
> +	sysrec_clear_set_s(SYS_HCRX_EL2, 0, HCRX_EL2_TCR2En);

[Severity: Low]
There is a typo here: sysrec_clear_set_s instead of sysreg_clear_set_s.

> +	sysreg_clear_set_s(SYS_TCR2_EL12, 0, TCR2_EL1_HAFT);

[Severity: Medium]
Does this sequence need an isb() between the register updates?

Updates to control-plane system registers must be synchronized with an
explicit Context Synchronization Event before any subsequent sysreg accesses.
Because sysreg_clear_set_s() performs reads, issuing back-to-back calls
without an intervening isb() causes the second access to execute against an
architecturally undefined pipeline state.

> +	isb();
> +	test_at(false);
> +
> +	/* The effective value of HAFT is 0 if HA is 0 */
> +	sysreg_clear_set_s(SYS_TCR_EL12, TCR_HA, 0);
> +	isb();
> +	test_at(true);
> +
> +	/* The effective value of HAFT is 0 if HCRX_EL2.TCR2En is 0 */
> +	sysreg_clear_set_s(SYS_HCRX_EL2, HCRX_EL2_TCR2En, 0);
> +	sysreg_clear_set_s(SYS_TCR_EL12, 0, TCR_HA);

[Severity: Medium]
Similar to the above, does this code need an isb() between modifying
SYS_HCRX_EL2 and SYS_TCR_EL12?

> +	isb();
> +	test_at(false);
> +
>  	GUEST_DONE();
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623184201.1518871-1-oupton@kernel.org?part=21