Re: [PATCH 6.6 v2 2/6] arm64: Treat HCR_EL2.E2H as RES1 when ID_AA64MMFR4_EL1.E2H0 is negative

[sashiko-bot@kernel.org]

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

New issues:
- [High] The feature check for `ID_AA64MMFR4_EL1.E2H0 < 0` will always evaluate to false because `tbnz` tests a bit that has been unconditionally cleared by `ubfx`.

Pre-existing issues:
- [High] Writes to control-plane system registers (`SYS_SCTLR_EL12` and `sctlr_el1`) are not immediately followed by an `isb()` instruction.
--

--- Patch [2]: [PATCH 6.6 v2 2/6] arm64: Treat HCR_EL2.E2H as RES1 when ID_AA64MMFR4_EL1.E2H0 is negative ---
commit c23e23c763b1ca2a9cc5f6843036bd8b47bf9194
Author: Marc Zyngier <maz@kernel.org>

arm64: Treat HCR_EL2.E2H as RES1 when ID_AA64MMFR4_EL1.E2H0 is negative
    
For CPUs that have ID_AA64MMFR4_EL1.E2H0 as negative, it is important
to avoid the boot path that sets HCR_EL2.E2H=0. This patch tweaks init_el2
to look at ID_AA64MMFR4_EL1.E2H0 first.
    
Link: https://lore.kernel.org/r/20240122181344.258974-8-maz@kernel.org

> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 6517bf2644a08..e32c8dd0b17a7 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -589,25 +589,32 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
>  	mov_q	x1, INIT_SCTLR_EL1_MMU_OFF
>  
>  	/*
> -	 * Fruity CPUs seem to have HCR_EL2.E2H set to RES1,
> -	 * making it impossible to start in nVHE mode. Is that
> -	 * compliant with the architecture? Absolutely not!
> +	 * 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

[Severity: High]
Does this bit test evaluate correctly? The ubfx instruction extracts 4 bits
starting at the shift and places them into bits 3:0 of x0, zero-extending
the rest of the register.

However, the tbnz instruction here tests a bit based on the original shift
amount plus the width minus 1. Because the upper bits of x0 are
unconditionally cleared by ubfx, will this branch ever be taken?

> +
>  	mrs	x0, hcr_el2
>  	and	x0, x0, #HCR_E2H
> -	cbz	x0, 1f
> -
> +	cbz	x0, 2f
> +1:
>  	/* Set a sane SCTLR_EL1, the VHE way */
>  	pre_disable_mmu_workaround
>  	msr_s	SYS_SCTLR_EL12, x1
>  	mov	x2, #BOOT_CPU_FLAG_E2H

[Severity: High]
This is a pre-existing issue, but does this system register write need an
immediate isb()?

According to ARM64 context synchronization rules, writes to control-plane
system registers like SYS_SCTLR_EL12 must be followed by an isb() as the
very next instruction. Here, a mov instruction intervenes, which might
observe architecturally undefined pipeline state.

> -	b	2f
> +	b	3f
>  
> -1:
> +2:
>  	pre_disable_mmu_workaround
>  	msr	sctlr_el1, x1
>  	mov	x2, xzr

[Severity: High]
This is a pre-existing issue, but is the nVHE path missing an isb() here
as well? 

Similar to the VHE path above, msr sctlr_el1, x1 is immediately followed by a
mov instruction rather than a synchronization barrier.

> -2:
> +3:
>  	__init_el2_nvhe_prepare_eret

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