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> --- arch/arm/include/asm/barebox-arm-head.h | 6 +--- arch/arm/include/asm/barebox-arm.h | 45 +++++++++++++++++++++++++ arch/arm/lib/pbl.lds.S | 1 + 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h index 187d12c9fc8d..3e8da0da7c0a 100644 --- a/arch/arm/include/asm/barebox-arm-head.h +++ b/arch/arm/include/asm/barebox-arm-head.h @@ -44,14 +44,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..d70de6d5e6e6 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); \ \ 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
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> --- 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 d70de6d5e6e6..0481d3391c26 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) - OPTEE_SIZE) +#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
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 d58beb605255..ab5b47051926 100644 --- a/arch/arm/boards/raspberry-pi/lowlevel.c +++ b/arch/arm/boards/raspberry-pi/lowlevel.c @@ -31,52 +31,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
We want same prologue for both. Otherwise header is shifted. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- arch/arm/include/asm/barebox-arm.h | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/include/asm/barebox-arm.h b/arch/arm/include/asm/barebox-arm.h index d70de6d5e6e6..5018b3e2f57a 100644 --- a/arch/arm/include/asm/barebox-arm.h +++ b/arch/arm/include/asm/barebox-arm.h @@ -215,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 \ -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On 02.12.21 08:42, Ahmad Fatoum wrote: > 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> > --- > 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 d70de6d5e6e6..0481d3391c26 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) - OPTEE_SIZE) Argh. Should have looked more closely before sending out. I'll respin later, but looking forward to feedback in the mean time. > +#endif > > static inline unsigned long arm_mem_stack(unsigned long membase, > unsigned long endmem) > -- 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
On Thu, Dec 02, 2021 at 09:01:10AM +0100, Ahmad Fatoum wrote: > On 02.12.21 08:42, Ahmad Fatoum wrote: > > 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> > > --- > > 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 d70de6d5e6e6..0481d3391c26 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) - OPTEE_SIZE) > > Argh. Should have looked more closely before sending out. I'll respin later, > but looking forward to feedback in the mean time. Apart from this error this series looks good to me (at least in the range of how good such a preprocessor/assembly wasteland can look). 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