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

Andre Przywara andre.przywara at arm.com
Mon Apr 26 12:40:54 BST 2021


On Tue, 20 Apr 2021 15:24:35 +0800
Jaxson Han <jaxson.han at arm.com> 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.

So this cleanup and rename is fine, however the patch is hard to read
since various other changes are mixed in.
Can you try to isolate this more? If this patch would just rename the
labels, and wouldn't carry any functional change, it might be easier to
reason about. At least the SPSR value change should be separate.

Some more below:

> Also in jump_kernel, skip sctlr_el2 initialisation when CurrentEL < EL2.
> 
> Signed-off-by: Jaxson Han <jaxson.han at arm.com>
> ---
>  arch/aarch64/boot.S            | 54 +++++++++++++++++++++++++---------
>  arch/aarch64/include/asm/cpu.h |  3 ++
>  arch/aarch64/psci.S            | 13 ++++----
>  arch/aarch64/spin.S            |  8 ++---
>  4 files changed, 54 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> index e47cf59..f7dbf3f 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,30 @@ _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
>  
> +	/*
> +	 * 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]
>  
> -	bl	setup_stack

I think this is indeed redundant, but can you either make this a
separate patch or at least mention it in the commit message?

> -	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
>  
> @@ -93,13 +103,23 @@ _start:
>  	mov	x0, #ZCR_EL3_LEN_MASK		// SVE: Enable full vector len
>  	msr	ZCR_EL3, x0			// for EL2.
>  
> -1:
> +	/*
> +	 * Save SPSR_KERNEL into spsr_to_elx.
> +	 * The jump_kernel will load spsr_to_elx into spsr_el3
> +	 */
> +1:	mov	w0, #SPSR_KERNEL
> +	ldr	x1, =spsr_to_elx
> +	str	w0, [x1]
> +	b	el_max_init

This (and the other SPSR related changes below) should be in a separate
patch.

> +
> +el_max_init:
>  	ldr	x0, =CNTFRQ
>  	msr	cntfrq_el0, x0
> +	isb

Why this isb here? I don't see that we would require this?

>  	bl	gic_secure_init
>  
> -	b	start_el3
> +	b	start_el_max
>  
>  err_invalid_id:
>  	b	.
> @@ -119,14 +139,18 @@ jump_kernel:
>  	ldr	x0, =SCTLR_EL1_RESET
>  	msr	sctlr_el1, x0
>  
> +	mrs	x0, CurrentEL
> +	cmp	x0, #CURRENTEL_EL2
> +	b.lt	1f
> +

This is also a change which might be separate.

Cheers,
Andre


>  	ldr	x0, =SCTLR_EL2_RESET
>  	msr	sctlr_el2, x0
>  
> -	cpuid	x0, x1
> +1:	cpuid	x0, x1
>  	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
> @@ -135,9 +159,9 @@ jump_kernel:
>  	mov	x3, x23
>  
>  	b.eq	1f
> -	br	x19			// No EL3
> +	br	x19			// Keep current EL
>  
> -1:	mov	x4, #SPSR_KERNEL
> +1:	ldr	w4, spsr_to_elx
>  
>  	/*
>  	 * If bit 0 of the kernel address is set, we're entering in AArch32
> @@ -153,5 +177,7 @@ jump_kernel:
>  
>  	.data
>  	.align 3
> -flag_no_el3:
> +flag_keep_el:
> +	.long 0
> +spsr_to_elx:
>  	.long 0
> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
> index ccb5397..2b3a0a4 100644
> --- a/arch/aarch64/include/asm/cpu.h
> +++ b/arch/aarch64/include/asm/cpu.h
> @@ -11,6 +11,7 @@
>  
>  #define MPIDR_ID_BITS		0xff00ffffff
>  
> +#define CURRENTEL_EL2		(2 << 2)
>  #define CURRENTEL_EL3		(3 << 2)
>  
>  /*
> @@ -24,6 +25,7 @@
>  #define SPSR_I			(1 << 7)	/* IRQ masked */
>  #define SPSR_F			(1 << 6)	/* FIQ masked */
>  #define SPSR_T			(1 << 5)	/* Thumb */
> +#define SPSR_EL1H		(5 << 0)	/* EL1 Handler mode */
>  #define SPSR_EL2H		(9 << 0)	/* EL2 Handler mode */
>  #define SPSR_HYP		(0x1a << 0)	/* M[3:0] = hyp, M[4] = AArch32 */
>  
> @@ -42,6 +44,7 @@
>  #else
>  #define SCTLR_EL1_RESET		SCTLR_EL1_RES1
>  #define SPSR_KERNEL		(SPSR_A | SPSR_D | SPSR_I | SPSR_F | SPSR_EL2H)
> +#define SPSR_KERNEL_EL1		(SPSR_A | SPSR_D | SPSR_I | SPSR_F | SPSR_EL1H)
>  #endif
>  
>  #ifndef __ASSEMBLY__
> 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
>  




More information about the linux-arm-kernel mailing list