Re: [BOOT-WRAPPER 03/11] aarch64: Implement cpu_init_arch()

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Aug 02, 2024 at 10:29:36AM +0100, Marc Zyngier wrote:
> On Mon, 29 Jul 2024 17:14:53 +0100,
> Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > When the boot-wrapper is entered at EL2 it does not initialise
> > CNTFRQ_EL0, and in future it may need to initialize other CPU state
> > regardless of the exeption level it was entered at.
> > 
> > Use a common cpu_init_arch() function to initialize CPU state regardless
> > of the exception level the boot-wrapper was entered at.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Akos Denke <akos.denke@arm.com>
> > Cc: Andre Przywara <andre.przywara@arm.com>
> > Cc: Luca Fancellu <luca.fancellu@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/aarch64/boot.S |  4 +++-
> >  arch/aarch64/init.c | 12 +++++++++---
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> > index 51ef41b..d8d38dd 100644
> > --- a/arch/aarch64/boot.S
> > +++ b/arch/aarch64/boot.S
> > @@ -51,7 +51,7 @@ reset_at_el3:
> >  
> >  	bl	cpu_init_bootwrapper
> >  
> > -	bl	cpu_init_el3
> > +	bl	cpu_init_arch
> >  
> >  	bl	gic_secure_init
> >  
> > @@ -82,6 +82,8 @@ reset_at_el2:
> >  
> >  	bl	cpu_init_bootwrapper
> >  
> > +	bl	cpu_init_arch
> > +
> >  	b	start_bootmethod
> >  
> >  err_invalid_id:
> > diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
> > index c9fc7f1..49abdf7 100644
> > --- a/arch/aarch64/init.c
> > +++ b/arch/aarch64/init.c
> > @@ -52,7 +52,7 @@ static inline bool cpu_has_permission_indirection(void)
> >  	return mrs(ID_AA64MMFR3_EL1) & mask;
> >  }
> >  
> > -void cpu_init_el3(void)
> > +static void cpu_init_el3(void)
> >  {
> >  	unsigned long scr = SCR_EL3_RES1 | SCR_EL3_NS | SCR_EL3_HCE;
> >  	unsigned long mdcr = 0;
> > @@ -153,8 +153,6 @@ void cpu_init_el3(void)
> >  
> >  		msr(SMCR_EL3, smcr);
> >  	}
> > -
> > -	msr(CNTFRQ_EL0, COUNTER_FREQ);
> >  }
> >  
> >  #ifdef PSCI
> > @@ -171,3 +169,11 @@ bool cpu_init_psci_arch(void)
> >  	return true;
> >  }
> >  #endif
> > +
> > +void cpu_init_arch(void)
> > +{
> > +	if (mrs(CurrentEL) == CURRENTEL_EL3)
> > +		cpu_init_el3();
> > +
> > +	msr(CNTFRQ_EL0, COUNTER_FREQ);
> > +}
> 
> Hmmm. This means that you cannot use the BW on a system where EL3 is
> implemented, but where you decide to enter at EL2 anyway (the write to
> CNTFRQ_EL0 will UNDEF).
> 
> I don't care much (I always want the BW to be the first piece of SW to
> run), but this is a rather subtle change in behaviour, and we'd better
> capture it in the commit message.

Booting in that way has been explciitly documented as not supported since
January 2022 in commit:

  286b8ecc86393c26 ("Document entry requirements")

... which added documentation to the start of boot.S which says:

| The boot-wrapper must be entered from the reset vector at the
| highest implemented exception level.

.. but regardless I can add some text to the commit message to say that
violating this requirement will now cause a silent failure.

> With that,
> 
> Acked-by: Marc Zyngier <maz@kernel.org>

Thanks!

Mark.