[PATCH v2 1/3] ARM: implement ENTRY_FUNCTION_WITHSTACK

[PATCH v2 1/3] ARM: implement ENTRY_FUNCTION_WITHSTACK

[Ahmad Fatoum]

The point of ENTRY_FUNCTION is to write the entry point in C. Due to
lack of __naked on ARM64, the start of the entry point will have prologue
using stack and it's not possible to set up the stack safely without
branching into non-inline assembly[0]. On ARM32, where we got __naked, we
have the potential for a different problem: If BootROM sets up stack for
us and we branch to a naked function, which doesn't set up its own
stack, compiler may decide to spill local variables overwriting
instructions it had already run[1].

For code reuse between ARM and ARM64, it would be nice to use the same
entry point structure for both. Currently, the only way is to write it
in non-inline assembly using the ENTRY_PROC macro.

This introduces another way: the ARM64 barebox header has enough space
for 8 instructions of which 5 are unused (2 instructions compiler prologue
+ 1 instruction to jump after the header), we could place a stack setup
routine there to avoid having to write a separate assembly file.
For ARM32, we just call arm_setup_stack and branch out directly after,
freeing board porters of the burden of getting it right.

Add a new ENTRY_FUNCTION_WITHSTACK to realize this.

[0]: 76bced6fe146 ("ARM: document arm_setup_stack() pitfalls"),
[1]: b51b15ba1738 ("RISC-V: board-dt-2nd: move low level init into nonnaked function")

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
 - add __ARM_SETUP_STACK(0) to normal ENTRY_FUNCTION. On ARM32, it's a
   no-op, but on ARM64, it ensures the header isn't shifted
---
 arch/arm/include/asm/barebox-arm-head.h |  6 +---
 arch/arm/include/asm/barebox-arm.h      | 46 +++++++++++++++++++++++++
 arch/arm/lib/pbl.lds.S                  |  1 +
 3 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h
index fcb7e31d8531..099e1bef3cb8 100644
--- a/arch/arm/include/asm/barebox-arm-head.h
+++ b/arch/arm/include/asm/barebox-arm-head.h
@@ -46,14 +46,10 @@ static inline void __barebox_arm_head(void)
 		"1: b 1b\n"
 #endif
 #else
+		/* 5 instructions added by ENTRY_FUNCTION */
 		/* two instruction long function prologue */
 		/* only use if stack is initialized! */
 		"b 2f\n"
-		"nop\n"
-		"nop\n"
-		"nop\n"
-		"nop\n"
-		"nop\n"
 #endif
 		".asciz \"barebox\"\n"
 #ifdef CONFIG_CPU_32
diff --git a/arch/arm/include/asm/barebox-arm.h b/arch/arm/include/asm/barebox-arm.h
index cfb5943f33d6..5018b3e2f57a 100644
--- a/arch/arm/include/asm/barebox-arm.h
+++ b/arch/arm/include/asm/barebox-arm.h
@@ -161,6 +161,51 @@ static inline unsigned long arm_mem_barebox_image(unsigned long membase,
 	}
 }
 
+#ifdef CONFIG_CPU_64
+
+#define ____emit_entry_prologue(instr, ...) do { \
+	static __attribute__ ((unused,section(".text_head_prologue"))) \
+		const u32 __entry_prologue[] = {(instr), ##__VA_ARGS__}; \
+	barrier_data(__entry_prologue); \
+} while(0)
+
+#define __emit_entry_prologue(instr1, instr2, instr3, instr4, instr5) \
+	____emit_entry_prologue(instr1, instr2, instr3, instr4, instr5)
+
+#define __ARM_SETUP_STACK(stack_top) \
+	__emit_entry_prologue(0x14000002	/* b pc+0x8 */,		\
+			      stack_top		/* 32-bit literal */,	\
+			      0x18ffffe9	/* ldr w9, top */,	\
+			      0xb4000049	/* cbz x9, pc+0x8 */,	\
+			      0x9100013f	/* mov sp, x9 */)
+#else
+#define __ARM_SETUP_STACK(stack_top) if (stack_top) arm_setup_stack(stack_top)
+#endif
+
+/*
+ * Unlike ENTRY_FUNCTION, this can be used to setup stack for a C entry
+ * point on both ARM32 and ARM64. ENTRY_FUNCTION on ARM64 can only be used
+ * if preceding boot stage has initialized the stack pointer.
+ *
+ * Stack top of 0 means stack is already set up. In that case, the follow-up
+ * code block will not be inlined and may spill to stack right away.
+ */
+#define ENTRY_FUNCTION_WITHSTACK(name, stack_top, arg0, arg1, arg2)	\
+	void name(ulong r0, ulong r1, ulong r2);			\
+									\
+	static void __##name(ulong, ulong, ulong);			\
+									\
+	void NAKED __section(.text_head_entry_##name)	name		\
+				(ulong r0, ulong r1, ulong r2)		\
+		{							\
+			__barebox_arm_head();				\
+			__ARM_SETUP_STACK(stack_top);			\
+			__##name(r0, r1, r2);				\
+		}							\
+		static void noinline __##name				\
+			(ulong arg0, ulong arg1, ulong arg2)
+
+
 #define ENTRY_FUNCTION(name, arg0, arg1, arg2)				\
 	void name(ulong r0, ulong r1, ulong r2);			\
 									\
@@ -170,6 +215,7 @@ static inline unsigned long arm_mem_barebox_image(unsigned long membase,
 				(ulong r0, ulong r1, ulong r2)		\
 		{							\
 			__barebox_arm_head();				\
+			__ARM_SETUP_STACK(0);				\
 			__##name(r0, r1, r2);				\
 		}							\
 		static void NAKED noinline __##name			\
diff --git a/arch/arm/lib/pbl.lds.S b/arch/arm/lib/pbl.lds.S
index 0a0fb8b5ac13..e77b3220fcb0 100644
--- a/arch/arm/lib/pbl.lds.S
+++ b/arch/arm/lib/pbl.lds.S
@@ -31,6 +31,7 @@ SECTIONS
 	.text      :
 	{
 		_stext = .;
+		*(.text_head_prologue*)
 		*(.text_head_entry*)
 		__bare_init_start = .;
 		*(.text_bare_init*)
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/3] ARM: turn arm_mem_stack_top into a macro

[Ahmad Fatoum]

This makes it possible to use for static initialization, like done in a
follow-up commit.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - drop OPTEE_SIZE on non OPTEE builds
---
 arch/arm/include/asm/barebox-arm.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/barebox-arm.h b/arch/arm/include/asm/barebox-arm.h
index 5018b3e2f57a..1c8cdfd16e62 100644
--- a/arch/arm/include/asm/barebox-arm.h
+++ b/arch/arm/include/asm/barebox-arm.h
@@ -97,14 +97,13 @@ static inline void arm_fixup_vectors(void)
 
 void *barebox_arm_boot_dtb(void);
 
-static inline unsigned long arm_mem_stack_top(unsigned long membase,
-					      unsigned long endmem)
-{
-	if (IS_ENABLED(CONFIG_BOOTM_OPTEE) || IS_ENABLED(CONFIG_PBL_OPTEE))
-		endmem -= OPTEE_SIZE;
+#define __arm_mem_stack_top(membase, endmem) ((endmem) - SZ_64K)
 
-	return endmem - SZ_64K;
-}
+#if defined(CONFIG_BOOTM_OPTEE) || defined(CONFIG_PBL_OPTEE)
+#define arm_mem_stack_top(membase, endmem) (__arm_mem_stack_top(membase, endmem) - OPTEE_SIZE)
+#else
+#define arm_mem_stack_top(membase, endmem)  __arm_mem_stack_top(membase, endmem)
+#endif
 
 static inline unsigned long arm_mem_stack(unsigned long membase,
 					  unsigned long endmem)
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/3] ARM: rpi: use ENTRY_FUNCTION_WITHSTACK to prepare for ARM64 support

[Ahmad Fatoum]

arm_setup_stack used in start_raspberry_pi is not safe on ARM64, due to
lack of __naked support. since 76bced6fe146 ("ARM: document
arm_setup_stack() pitfalls"), its use in ARM64 code is a compile-time
error.

The Raspberry Pi 4 support will be ARM64. To allow it to reuse
the existing low-level code, use the new ENTRY_FUNCTION_WITHSTACK,
which will set up stack appropriately for both ARM32 and ARM64.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/boards/raspberry-pi/lowlevel.c | 35 ++++++++++++-------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/arch/arm/boards/raspberry-pi/lowlevel.c b/arch/arm/boards/raspberry-pi/lowlevel.c
index 29162ff02d54..e9314fb27b25 100644
--- a/arch/arm/boards/raspberry-pi/lowlevel.c
+++ b/arch/arm/boards/raspberry-pi/lowlevel.c
@@ -33,52 +33,51 @@ static void copy_vc_fdt(void *dest, void *src, unsigned long max_size)
 	memmove(dest, src, size);
 }
 
+/* A pointer to the FDT created by VideoCore was passed to us in r2. We
+ * reserve some memory just above the region used for Barebox and copy
+ * this FDT there. We fetch it from there later in rpi_devices_init().
+ */
+#define rpi_stack_top(memsize) \
+	arm_mem_stack_top(BCM2835_SDRAM_BASE, BCM2835_SDRAM_BASE + memsize - VIDEOCORE_FDT_SZ)
+
 /* Must be inline since stack isn't setup yet. */
 static inline void start_raspberry_pi(unsigned long memsize, void *fdt,
 								void *vc_fdt)
 {
-	void *saved_vc_fdt;
-	unsigned long membase = BCM2835_SDRAM_BASE;
-
-	/* A pointer to the FDT created by VideoCore was passed to us in r2. We
-	 * reserve some memory just above the region used for Basebox and copy
-	 * this FDT there. We fetch it from there later in rpi_devices_init().*/
-	memsize -= VIDEOCORE_FDT_SZ;
+	unsigned long endmem = rpi_stack_top(memsize);
 
 	arm_cpu_lowlevel_init();
 
-	/* Copied from barebox_arm_entry(). We need stack here early
-	 * for normal function calls to work. */
-	arm_setup_stack(arm_mem_stack_top(membase, membase + memsize));
+	copy_vc_fdt((void *)endmem, vc_fdt, VIDEOCORE_FDT_SZ);
 
 	fdt += get_runtime_offset();
 
-	saved_vc_fdt = (void *)(membase + memsize);
-	copy_vc_fdt(saved_vc_fdt, vc_fdt, VIDEOCORE_FDT_SZ);
-
-	barebox_arm_entry(membase, memsize, fdt);
+	barebox_arm_entry(BCM2835_SDRAM_BASE, endmem - BCM2835_SDRAM_BASE, fdt);
 }
 
+#define RPI_ENTRY_FUNCTION(name, memsize, r2) \
+	ENTRY_FUNCTION_WITHSTACK(name, rpi_stack_top(memsize), __r0, __r1, r2)
+
 extern char __dtb_bcm2835_rpi_start[];
-ENTRY_FUNCTION(start_raspberry_pi1, r0, r1, r2)
+RPI_ENTRY_FUNCTION(start_raspberry_pi1, SZ_128M, r2)
 {
 	start_raspberry_pi(SZ_128M, __dtb_bcm2835_rpi_start, (void *)r2);
 }
 
 extern char __dtb_bcm2836_rpi_2_start[];
-ENTRY_FUNCTION(start_raspberry_pi2, r0, r1, r2)
+RPI_ENTRY_FUNCTION(start_raspberry_pi2, SZ_512M, r2)
 {
 	start_raspberry_pi(SZ_512M, __dtb_bcm2836_rpi_2_start, (void *)r2);
 }
 
 extern char __dtb_bcm2837_rpi_3_start[];
-ENTRY_FUNCTION(start_raspberry_pi3, r0, r1, r2)
+RPI_ENTRY_FUNCTION(start_raspberry_pi3, SZ_512M, r2)
 {
 	start_raspberry_pi(SZ_512M, __dtb_bcm2837_rpi_3_start, (void *)r2);
 }
 
 extern char __dtb_bcm2837_rpi_cm3_start[];
-ENTRY_FUNCTION(start_raspberry_pi_cm3, r0, r1, r2)
+RPI_ENTRY_FUNCTION(start_raspberry_pi_cm3, SZ_512M, r2)
 {
 	start_raspberry_pi(SZ_512M, __dtb_bcm2837_rpi_cm3_start, (void *)r2);
 }
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] ARM: implement ENTRY_FUNCTION_WITHSTACK

[Sascha Hauer]

On Fri, Jan 14, 2022 at 09:42:25AM +0100, Ahmad Fatoum wrote:
> The point of ENTRY_FUNCTION is to write the entry point in C. Due to
> lack of __naked on ARM64, the start of the entry point will have prologue
> using stack and it's not possible to set up the stack safely without
> branching into non-inline assembly[0]. On ARM32, where we got __naked, we
> have the potential for a different problem: If BootROM sets up stack for
> us and we branch to a naked function, which doesn't set up its own
> stack, compiler may decide to spill local variables overwriting
> instructions it had already run[1].
> 
> For code reuse between ARM and ARM64, it would be nice to use the same
> entry point structure for both. Currently, the only way is to write it
> in non-inline assembly using the ENTRY_PROC macro.
> 
> This introduces another way: the ARM64 barebox header has enough space
> for 8 instructions of which 5 are unused (2 instructions compiler prologue
> + 1 instruction to jump after the header), we could place a stack setup
> routine there to avoid having to write a separate assembly file.
> For ARM32, we just call arm_setup_stack and branch out directly after,
> freeing board porters of the burden of getting it right.
> 
> Add a new ENTRY_FUNCTION_WITHSTACK to realize this.
> 
> [0]: 76bced6fe146 ("ARM: document arm_setup_stack() pitfalls"),
> [1]: b51b15ba1738 ("RISC-V: board-dt-2nd: move low level init into nonnaked function")
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
>  - add __ARM_SETUP_STACK(0) to normal ENTRY_FUNCTION. On ARM32, it's a
>    no-op, but on ARM64, it ensures the header isn't shifted
> ---
>  arch/arm/include/asm/barebox-arm-head.h |  6 +---
>  arch/arm/include/asm/barebox-arm.h      | 46 +++++++++++++++++++++++++
>  arch/arm/lib/pbl.lds.S                  |  1 +
>  3 files changed, 48 insertions(+), 5 deletions(-)

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox