Re: [PATCH 6.6 v2 6/6] arm64: Revamp HCR_EL2.E2H RES1 detection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing immediate Context Synchronization Event (ISB) after writes to control-plane system registers.
--

--- Patch [6]: [PATCH 6.6 v2 6/6] arm64: Revamp HCR_EL2.E2H RES1 detection ---
commit 75069b0b3126a153b742f982dd5dff757dc34d3e
Author: Marc Zyngier <maz@kernel.org>

arm64: Revamp HCR_EL2.E2H RES1 detection

This patch replaces the RAO/WI test for VHE-only CPUs with a sequence
utilizing VHE register remapping. This allows Neoverse V2 NV guests to
reliably detect they are in a VHE-only configuration despite the absence
of FEAT_FGT.

Link: https://lore.kernel.org/r/15A85F2B-1A0C-4FA7-9FE4-EEC2203CC09E@global.cadence.com

> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index 76b0d50d286d5..4c7467b382b60 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h

[ ... ]

> @@ -24,22 +24,48 @@
>  	 * ID_AA64MMFR4_EL1.E2H0 < 0. On such CPUs HCR_EL2.E2H is RES1, but it
>  	 * can reset into an UNKNOWN state and might not read as 1 until it has
>  	 * been initialized explicitly.
> -	 *
> -	 * Fruity CPUs seem to have HCR_EL2.E2H set to RAO/WI, but
> -	 * don't advertise it (they predate this relaxation).
> -	 *
>  	 * Initalize HCR_EL2.E2H so that later code can rely upon HCR_EL2.E2H
>  	 * indicating whether the CPU is running in E2H mode.
>  	 */
>  	mrs_s	x1, SYS_ID_AA64MMFR4_EL1
>  	sbfx	x1, x1, #ID_AA64MMFR4_EL1_E2H0_SHIFT, #ID_AA64MMFR4_EL1_E2H0_WIDTH
>  	cmp	x1, #0
> -	b.ge	.LnVHE_\@
> +	b.lt	.LnE2H0_\@

[ ... ]

> +.LnE2H0_\@:
>  	orr	x0, x0, #HCR_E2H
> -.LnVHE_\@:
>  	msr	hcr_el2, x0
>  	isb
> +.LnVHE_\@:
>  .endm

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file
I noticed there might be missing Context Synchronization Events (ISB)
after writes to control-plane system registers.

In the __init_el2_timers macro:

	msr	cnthctl_el2, x0
	msr	cntvoff_el2, xzr		// Clear virtual offset

Does the write to cnthctl_el2 require an isb instruction immediately after it,
before executing the write to cntvoff_el2?

Similarly, in the __init_el2_fgt macro, there are multiple writes executed
without synchronization:

	msr_s	SYS_HFGRTR_EL2, x0
	msr_s	SYS_HFGWTR_EL2, x0
	msr_s	SYS_HFGITR_EL2, xzr

	mrs	x1, id_aa64pfr0_el1		// AMU traps UNDEF without AMU

Could executing subsequent instructions without an isb cause the CPU to
operate under a stale configuration here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708225124.4130846-1-coltonlewis@google.com?part=6