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

Hi Andre,

-----Original Message-----
From: Andre Przywara <andre.przywara@arm.com>
Sent: Monday, May 24, 2021 5:32 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 v2 2/8] aarch64: Rename labels and
prepare for lower EL booting

On Fri, 21 May 2021 18:48:01 +0800
Jaxson Han [off-list ref] wrote:

quoted

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.

Thanks for splitting out this patch, I verified that most of it is indeed just
renames, apart from the things below:

quoted

Signed-off-by: Jaxson Han <redacted>
---
 arch/aarch64/boot.S            | 33 ++++++++++++++++++++++-----------
 arch/aarch64/include/asm/cpu.h |  3 +++
 arch/aarch64/psci.S            | 13 +++++++------
 arch/aarch64/spin.S            |  8 ++++----
 4 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S index

a9264de..e4f5f3d 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

+	/*
+	 * 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
-	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,9 +161,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_KERNEL

This looks both premature and wrong: The value of SPSR_KERNEL doesn't
change here, so doesn't need any change in this instruction. Plus: ldr without
'=' requires an address, not a value, and this in fact already breaks
compilation:
arch/aarch64/boot.S:166: Error: immediate value must be a multiple of 4 at
operand 2 -- `ldr w4,#((1<<8)|(1<<9)|(1<<7)|(1<<6)|(9<<0))'

So please remove this change, and also the changes in cpu.h below, and
introduce them only when they are actually needed.

Apart from the rest is then really just renames.

Right, I made a mistake. I will fix this and move them into patch 5.

Thanks,
Jaxson

Cheers,
Andre

quoted

 	/*
 	 * If bit 0 of the kernel address is set, we're entering in AArch32

@@ -168,5 +179,5 @@ jump_kernel:

 	.data
 	.align 3
-flag_no_el3:
+flag_keep_el:
 	.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 */

quoted

@@ -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)

quoted

+#define SPSR_KERNEL_EL1		(SPSR_A | SPSR_D | SPSR_I | SPSR_F |

SPSR_EL1H)

quoted

 #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