[boot-wrapper PATCH v3 2/8] aarch64: Rename labels and prepare for lower EL booting

[Jaxson Han]

Hi Mark,

> -----Original Message-----
> From: Mark Rutland <mark.rutland at arm.com>
> Sent: Saturday, July 24, 2021 12:49 AM
> To: Jaxson Han <Jaxson.Han at arm.com>
> Cc: Andre Przywara <Andre.Przywara at arm.com>; linux-arm-
> kernel at lists.infradead.org; Wei Chen <Wei.Chen at arm.com>
> Subject: Re: [boot-wrapper PATCH v3 2/8] aarch64: Rename labels and
> prepare for lower EL booting
> 
> On Tue, May 25, 2021 at 02:25:03PM +0800, Jaxson Han wrote:
> > Prepare for booting from lower EL. Rename *_el3 relavant labels with
> > *_el_max and *_no_el3 with *_keep_el. Since the original _no_el3 means
> > "We neither do init sequence at this highest EL nor drop to lower EL
> > when entering to kernel", we rename it with _keep_el to make it more
> > clear for lower EL initialisation.
> 
> I think the existing behaviour here where we don't initialize EL2 in the
> _no_el3 case is an accident rather than a deliberate design decision, and is
> unsound (e.g. as SCTLR_ELx.EE resets to an IMPLEMENTATION DEFINED value,
> but we assume it's little-endian).
> 
> I think we need to clean that up before we alter this further, since the
> existing code supposedly supports cases that cannot work correctly.
> 
> I also think that we should mandate that the boot-wrapper is started at the
> highest implemented exception level. We used to care about being a TF-A
> payload, but that hasn't been the case since TF-A commit:
> 
>   32412a8a6b0da74d ("Replace bootwrapped kernel instructions from User
> Guide")
> 
> ... and it will be significantly easier to do the right thing if the boot-wrapper is
> directly in charge of the HW. Does that fit with your use-case?

Currently, yes, it does. I will let you know if there's a change in the future. 

> 
> I've started work on that, and I have some WIP patches at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/mark/boot-wrapper-
> aarch64.git/log/?h=cleanup
> 
> ... which I'll spend some more time on next week, and I'll have a go at picking
> up portions of this series atop that.

Many thanks, I got it.

> 
> Thanks,
> Mark.
> 
> >
> > Signed-off-by: Jaxson Han <jaxson.han at arm.com>
> > ---
> >  arch/aarch64/boot.S | 31 +++++++++++++++++++++----------
> > arch/aarch64/psci.S | 13 +++++++------  arch/aarch64/spin.S |  8
> > ++++----
> >  3 files changed, 32 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S index
> > a9264de..1a5da35 100644
> > --- a/arch/aarch64/boot.S
> > +++ b/arch/aarch64/boot.S
> > @@ -12,7 +12,7 @@
> >  	.section .init
> >
> >  	.globl	_start
> > -	.globl jump_kernel
> > +	.globl	jump_kernel
> >
> >  _start:
> >  	cpuid	x0, x1
> > @@ -22,20 +22,31 @@ _start:
> >  	bl	setup_stack
> >
> >  	/*
> > -	 * EL3 initialisation
> > +	 * Boot sequence
> > +	 * If CurrentEL == EL3, then goto EL3 initialisation and drop to
> > +	 *   lower EL before entering the kernel.
> > +	 * Else, no initialisation and keep the current EL before
> > +	 *   entering the kernel.
> >  	 */
> >  	mrs	x0, CurrentEL
> >  	cmp	x0, #CURRENTEL_EL3
> > -	b.eq	1f
> > +	beq	el3_init
> 
> As above, I think our failure to initialize anything other than EL3 is a bug,
> since e.g. we don't know what a number of SCTLR_ELx bits are out-of-reset,
> including EE.
> 
> I think we need to rework this into something like:
> 
> 	mrs     x0, CurrentEL
> 	cmp     x0, #CURRENTEL_EL3
> 	b.eq	init_el3
> 	cmp     x0, #CURRENTEL_EL2
> 	b.eq	init_el2
> 	cmp	x0, #CURRENTEL_EL1
> 	b.eq	init_el1
> 
> 	/* this should not happen */
> 	b	.
> 
> >
> > +	/*
> > +	 * We stay in the current EL for entering the kernel
> > +	 */
> >  	mov	w0, #1
> > -	ldr	x1, =flag_no_el3
> > +	ldr	x1, =flag_keep_el
> >  	str	w0, [x1]
> 
> Can we get rid of the start_el3/start_no_el3 distinction, and pass the EL in as
> a parameter to the boot method?
> 
> Since this it boot-method dependent, I'd like to decide this elsewhere.

Yes, that makes perfect sense!

> 
> >
> >  	bl	setup_stack
> > -	b	start_no_el3
> > +	b	start_keep_el
> >
> > -1:	mov	x0, #0x30			// RES1
> > +	/*
> > +	 * EL3 initialisation
> > +	 */
> > +el3_init:
> > +	mov	x0, #0x30			// RES1
> >  	orr	x0, x0, #(1 << 0)		// Non-secure EL1
> >  	orr	x0, x0, #(1 << 8)		// HVC enable
> >
> > @@ -114,7 +125,7 @@ _start:
> >
> >  	bl	gic_secure_init
> >
> > -	b	start_el3
> > +	b	start_el_max
> >
> >  err_invalid_id:
> >  	b	.
> > @@ -141,7 +152,7 @@ jump_kernel:
> >  	bl	find_logical_id
> >  	bl	setup_stack		// Reset stack pointer
> >
> > -	ldr	w0, flag_no_el3
> > +	ldr	w0, flag_keep_el
> >  	cmp	w0, #0			// Prepare Z flag
> >
> >  	mov	x0, x20
> > @@ -150,7 +161,7 @@ jump_kernel:
> >  	mov	x3, x23
> >
> >  	b.eq	1f
> > -	br	x19			// No EL3
> > +	br	x19			// Keep current EL
> >
> >  1:	mov	x4, #SPSR_KERNEL
> >
> > @@ -168,5 +179,5 @@ jump_kernel:
> >
> >  	.data
> >  	.align 3
> > -flag_no_el3:
> > +flag_keep_el:
> >  	.long 0
> > diff --git a/arch/aarch64/psci.S b/arch/aarch64/psci.S index
> > 01ebe7d..ae02fd6 100644
> > --- a/arch/aarch64/psci.S
> > +++ b/arch/aarch64/psci.S
> > @@ -45,8 +45,8 @@ vector:
> >
> >  	.text
> >
> > -	.globl start_no_el3
> > -	.globl start_el3
> > +	.globl start_keep_el
> > +	.globl start_el_max
> >
> >  err_exception:
> >  	b err_exception
> > @@ -101,7 +101,7 @@ smc_exit:
> >  	eret
> >
> >
> > -start_el3:
> > +start_el_max:
> >  	ldr	x0, =vector
> >  	bl	setup_vector
> >
> > @@ -111,10 +111,11 @@ start_el3:
> >  	b	psci_first_spin
> >
> >  /*
> > - * This PSCI implementation requires EL3. Without EL3 we'll only boot
> > the
> > - * primary cpu, all others will be trapped in an infinite loop.
> > + * This PSCI implementation requires the highest EL(EL3 or Armv8-R EL2).
> > + * Without the highest EL, we'll only boot the primary cpu, all
> > + others
> > + * will be trapped in an infinite loop.
> >   */
> > -start_no_el3:
> > +start_keep_el:
> >  	cpuid	x0, x1
> >  	bl	find_logical_id
> >  	cbz	x0, psci_first_spin
> > diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S index
> > 72603cf..533177c 100644
> > --- a/arch/aarch64/spin.S
> > +++ b/arch/aarch64/spin.S
> > @@ -11,11 +11,11 @@
> >
> >  	.text
> >
> > -	.globl start_no_el3
> > -	.globl start_el3
> > +	.globl start_keep_el
> > +	.globl start_el_max
> >
> > -start_el3:
> > -start_no_el3:
> > +start_el_max:
> > +start_keep_el:
> >  	cpuid	x0, x1
> >  	bl	find_logical_id
> >
> > --
> > 2.25.1
> >