Re: [PATCH 6.6 v3 3/6] arm64: Fix early handling of FEAT_E2H0 not being implemented

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but writes to control-plane system registers (such as `SYS_SCTLR_EL12`, `sctlr_el1`, `cnthctl_el2`, `cntvoff_el2`, `SYS_HCRX_EL2`, and fine-grained trap registers like `SYS_HDFGRTR_EL2`) are not immediately followed by an `isb()` instruction. This violates architectural context synchronization rules which mandate that every control-plane sysreg write must be followed by an `isb()` as the very next instruction.
--

--- Patch [3]: [PATCH 6.6 v3 3/6] arm64: Fix early handling of FEAT_E2H0 not being implemented ---
commit 7771c286d908263811f7e18fed1ebdcc2d0d16e0
Author: Marc Zyngier <maz@kernel.org>

arm64: Fix early handling of FEAT_E2H0 not being implemented

This commit fixes a bug where the check for ID_AA64MMFR4_EL1.E2H0
was incorrect and happened too late in the initialization sequence.
It updates the check and publishes the HCR_EL2.E2H bit earlier to
ensure hVHE mode boots successfully as a nested guest.

Link: https://lore.kernel.org/r/20240321115414.3169115-1-maz@kernel.org

> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index e32c8dd0b17a7..e0e710b36da37 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
[ ... ]
> @@ -588,22 +603,10 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
>  
>  	mov_q	x1, INIT_SCTLR_EL1_MMU_OFF
>  
> -	/*
> -	 * Compliant CPUs advertise their VHE-onlyness with
> -	 * ID_AA64MMFR4_EL1.E2H0 < 0. HCR_EL2.E2H can be
> -	 * RES1 in that case.
> -	 *
> -	 * Fruity CPUs seem to have HCR_EL2.E2H set to RES1, but
> -	 * don't advertise it (they predate this relaxation).
> -	 */
> -	mrs_s	x0, SYS_ID_AA64MMFR4_EL1
> -	ubfx	x0, x0, #ID_AA64MMFR4_EL1_E2H0_SHIFT, #ID_AA64MMFR4_EL1_E2H0_WIDTH
> -	tbnz	x0, #(ID_AA64MMFR4_EL1_E2H0_SHIFT + ID_AA64MMFR4_EL1_E2H0_WIDTH - 1), 1f
> -
>  	mrs	x0, hcr_el2
>  	and	x0, x0, #HCR_E2H
>  	cbz	x0, 2f
> -1:
> +
>  	/* Set a sane SCTLR_EL1, the VHE way */
>  	pre_disable_mmu_workaround
>  	msr_s	SYS_SCTLR_EL12, x1

[Severity: High]
This isn't a bug introduced by this patch, but do the writes to control-plane
system registers here and in nearby setup functions need to be immediately
followed by an isb() instruction?

Looking at init_el2() in arch/arm64/kernel/head.S, the write to
SYS_SCTLR_EL12 is immediately followed by a mov instruction instead of
an isb():

        msr_s   SYS_SCTLR_EL12, x1
        mov     x2, #BOOT_CPU_FLAG_E2H

Operating with a stale pipeline configuration after control-plane sysreg writes
can cause unexpected CPU traps, undefined behavior, or boot failures.

The same pattern appears in init_el2() where sctlr_el1 is written:

2:
        pre_disable_mmu_workaround
        msr     sctlr_el1, x1
        mov     x2, xzr

This also seems to occur in arch/arm64/include/asm/el2_setup.h:

- __init_el2_timers() writes cnthctl_el2 and cntvoff_el2 without an
  intervening isb().
- __init_el2_hcrx() writes to SYS_HCRX_EL2.
- __init_el2_fgt() writes to fine-grained trap registers like
  SYS_HDFGRTR_EL2.

Could these missing synchronization barriers cause boot failures on
microarchitectures that require strict enforcement?

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