Re: [PATCH 07/12] arm64: Treat HCR_EL2.E2H as RES1 when ID_AA64MMFR4_EL1.E2H0 is non-zero

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>
Subject: Re: [PATCH 07/12] arm64: Treat HCR_EL2.E2H as RES1 when ID_AA64MMFR4_EL1.E2H0 is non-zero
Date: Fri, 17 Nov 2023 12:17:25 +0000	[thread overview]
Message-ID: <86wmugzi96.wl-maz@kernel.org> (raw)
In-Reply-To: <ZVaycYt51sUWS0h+@thinky-boi>

On Fri, 17 Nov 2023 00:23:13 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hey Marc,
> 
> On Mon, Nov 13, 2023 at 05:42:39PM +0000, Marc Zyngier wrote:
> > For CPUs that have ID_AA64MMFR4_EL1.E2H0 as non-zero, it is important
> > to avoid the boot path that sets HCR_EL2.E2H=0. Fortunately, we
> > already have this path to cope with fruity CPUs.
> > 
> > Tweak init_el2 to look at ID_AA64MMFR4_EL1.E2H0 first.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kernel/head.S | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 7b236994f0e1..5853540f7809 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -584,25 +584,34 @@ 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=0b111x. 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
> > +	and	x0, x0, #0b1110
> > +	cmp	x0, #0b1110
> > +	b.eq	1f
> 
> Wouldn't it be more consistent with the architectural definition of
> signed feature fields to just test the sign bit in this case?
> 
> Maybe like:
> 
> 	ubfx	x0, x0, #ID_AA64MMFR4_EL1_E2H0_SHIFT, #ID_AA64MMFR4_EL1_E2H0_WIDTH
> 	tbnz	x0, #3, 1f
> 
> You'll also save 8 measly bytes of text while you're at it :)

At this stage, why not try this:

tbnz x0, #(ID_AA64MMFR4_EL1_E2H0_SHIFT + ID_AA64MMFR4_EL1_E2H0_WIDTH - 1), 1f

and we save a total of 12 bytes!

	M.

-- 
Without deviation from the norm, progress is not possible.