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

quoted

Hi Andre,

-----Original Message-----
From: Andre Przywara <andre.przywara@arm.com>
Sent: Monday, April 26, 2021 7:41 PM
To: Jaxson Han <redacted>
Cc: Mark Rutland <Mark.Rutland@arm.com>; linux-arm-
kernel@lists.infradead.org; Wei Chen [off-list ref]
Subject: Re: [boot-wrapper PATCH 2/5] aarch64: Rename labels and prepare
for lower EL booting

On Tue, 20 Apr 2021 15:24:35 +0800
Jaxson Han [off-list ref] 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.
Yes, I will.

Some more below:

Also in jump_kernel, skip sctlr_el2 initialisation when CurrentEL < EL2.

Signed-off-by: Jaxson Han <redacted>
---
 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?
Yes, I will make this separate.


-	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.
Yes, I will


+
+el_max_init:
 	ldr	x0, =CNTFRQ
 	msr	cntfrq_el0, x0
+	isb
Why this isb here? I don't see that we would require this?
No need here. I'll remove it.


 	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.
Yes

Thanks,
Jaxson

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help