Re: [U-Boot] [PATCH] Fix board init code to use a valid C runtime environment

Hi Albert,

On 9 November 2015 at 17:20, Albert ARIBAUD albert.u.boot@aribaud.net wrote:

board_init_f_mem() alters the C runtime environment's stack it ls actually already using. This is not a valid C runtime environment and may conflict with the C compiler's expectations.

Split board_init_f_mem into C functions which do not alter their own stack and therefore function in a valid C runtime environment.

NOTE: this has not been tested with all architectures.

Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net

arch/arc/lib/start.S | 20 +++++++++++++--- arch/arm/lib/crt0.S | 10 ++++++-- arch/arm/lib/crt0_64.S | 12 +++++++--- arch/microblaze/cpu/start.S | 4 ++-- arch/nios2/cpu/start.S | 17 ++++++++++++-- arch/powerpc/cpu/ppc4xx/start.S | 18 ++++++++++---- arch/x86/cpu/start.S | 10 ++++++-- arch/x86/lib/fsp/fsp_common.c | 2 +- common/init/board_init.c | 33 +++++++++++++++----------- include/common.h | 52 +++++++++++++++++++++++++---------------- 10 files changed, 126 insertions(+), 52 deletions(-)

diff --git a/arch/arc/lib/start.S b/arch/arc/lib/start.S index 26a5934..d24d60b 100644 --- a/arch/arc/lib/start.S +++ b/arch/arc/lib/start.S @@ -54,14 +54,28 @@ ENTRY(_start) mov %sp, CONFIG_SYS_INIT_SP_ADDR mov %fp, %sp

•   /* Allocate and zero GD, update SP */
  

•   /* Allocate GD, update SP */
    mov     %r0, %sp
  

•   bl      board_init_f_mem
  

•   bl      board_init_f_gd_size
  

•   /* Update stack- and frame-pointers */
  

•   sub     %sp, %sp, %r0
  

•   mov     %fp, %sp
  

•   /* Zero GD */
  

•   mov     %r0, %sp
  

•   bl      board_init_f_gd
  
  

•   /* Allocate malloc, update SP */
  

•   mov     %r0, %sp
  

•   bl      board_init_f_malloc_size
    /* Update stack- and frame-pointers */
  

•   mov     %sp, %r0
  

•   sub     %sp, %sp, %r0
    mov     %fp, %sp
  
  

•   /* update GD with malloc base */
  

•   mov     %r0, %sp
  

•   bl      board_init_f_malloc
  

I'm hoping we don't have to bring back this assembler. Do we really need to put everything in separate functions?

Perhaps you could add a bit more detail in the commit message as to what problem this solves?

    /* Zero the one and only argument of "board_init_f" */
    mov_s   %r0, 0
    j       board_init_f

diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S index 80548eb..26abab7 100644 --- a/arch/arm/lib/crt0.S +++ b/arch/arm/lib/crt0.S @@ -82,9 +82,15 @@ ENTRY(_main) #else bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ #endif

•   bl      board_init_f_gd_size
  

•   sub     sp, r0
    mov     r0, sp
  

•   bl      board_init_f_mem
  

•   mov     sp, r0
  

•   mov     r9, r0
  

•   bl      board_init_f_gd
  

•   bl      board_init_f_malloc_size
  

•   sub     sp, r0
  

•   mov     r0, sp
  

•   bl      board_init_f_malloc
  
    mov     r0, #0
    bl      board_init_f
  

diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S index cef1c71..e716889 100644 --- a/arch/arm/lib/crt0_64.S +++ b/arch/arm/lib/crt0_64.S @@ -75,9 +75,15 @@ ENTRY(_main) ldr x0, =(CONFIG_SYS_INIT_SP_ADDR) #endif bic sp, x0, #0xf /* 16-byte alignment for ABI compliance */

•   bl      board_init_f_mem
  

•   mov     sp, x0
  

•   bl      board_init_f_gd_size
  

•   sub     sp, x0
  

•   mov     x0, sp
  

•   bl      board_init_f_gd
  

•   bl      board_init_f_malloc_size
  

•   sub     sp, x0
  

•   mov     x0, sp
  

•   bl      board_init_f_malloc
  

•   mov     x0, #0
    bl      board_init_f
  
  

diff --git a/arch/microblaze/cpu/start.S b/arch/microblaze/cpu/start.S index 14f46a8..206be3e 100644 --- a/arch/microblaze/cpu/start.S +++ b/arch/microblaze/cpu/start.S @@ -25,7 +25,7 @@ _start:

    addi    r8, r0, __end
    mts     rslr, r8

•   /* TODO: Redo this code to call board_init_f_mem() */
  

•   /* TODO: Redo this code to call board_init_f_*() */
  

#if defined(CONFIG_SPL_BUILD) addi r1, r0, CONFIG_SPL_STACK_ADDR mts rshr, r1 @@ -142,7 +142,7 @@ _start: ori r12, r12, 0x1a0 mts rmsr, r12

•   /* TODO: Redo this code to call board_init_f_mem() */
  

•   /* TODO: Redo this code to call board_init_f_*() */
  

clear_bss: /* clear BSS segments */ addi r5, r0, __bss_start diff --git a/arch/nios2/cpu/start.S b/arch/nios2/cpu/start.S index 54787c5..4d5f0b0 100644 --- a/arch/nios2/cpu/start.S +++ b/arch/nios2/cpu/start.S @@ -107,9 +107,22 @@ _reloc: mov fp, sp

    /* Allocate and zero GD, update SP */

•   movhi   r2, %hi(board_init_f_gd_size@h)
  

•   ori     r2, r2, %lo(board_init_gd_size@h)
  

•   callr   r2
  

•   sub     sp, sp, r4
  

•   mov     r4, sp
  

•   movhi   r2, %hi(board_init_f_gd@h)
  

•   ori     r2, r2, %lo(board_init_f_gd@h)
  

•   callr   r2
  

•   /* Allocate malloc arena, update SP */
  

•   movhi   r2, %hi(board_init_f_malloc_size@h)
  

•   ori     r2, r2, %lo(board_init_malloc_size@h)
  

•   callr   r2
  

•   sub     sp, sp, r4
    mov     r4, sp
  

•   movhi   r2, %hi(board_init_f_mem@h)
  

•   ori     r2, r2, %lo(board_init_f_mem@h)
  

•   movhi   r2, %hi(board_init_f_malloc@h)
  

•   ori     r2, r2, %lo(board_init_f_malloc@h)
    callr   r2
  
    /* Update stack- and frame-pointers */
  

diff --git a/arch/powerpc/cpu/ppc4xx/start.S b/arch/powerpc/cpu/ppc4xx/start.S index 77d4040..c827c95 100644 --- a/arch/powerpc/cpu/ppc4xx/start.S +++ b/arch/powerpc/cpu/ppc4xx/start.S @@ -761,9 +761,14 @@ _start:

    bl      cpu_init_f      /* run low-level CPU init code     (from Flash) */

#ifdef CONFIG_SYS_GENERIC_BOARD

•   bl      board_init_f_gd_size
  

•   sub     r1, r1, r3
    mr      r3, r1
  

•   bl      board_init_f_mem
  

•   mr      r1, r3
  

•   bl      board_init_f_gd
  

•   bl      board_init_f_malloc_size
  

•   sub     r1, r1, r3
  

•   mr      r3, r1
  

•   bl      board_init_f_malloc
    li      r0,0
    stwu    r0, -4(r1)
    stwu    r0, -4(r1)
  

@@ -1037,9 +1042,14 @@ _start:

    bl      cpu_init_f      /* run low-level CPU init code     (from Flash) */

#ifdef CONFIG_SYS_GENERIC_BOARD

•   bl      board_init_f_gd_size
  

•   sub     r1, r1, r3
  

•   mr      r3, r1
  

•   bl      board_init_f_gd
  

•   bl      board_init_f_malloc_size
  

•   sub     r1, r1, r3
    mr      r3, r1
  

•   bl      board_init_f_mem
  

•   mr      r1, r3
  

•   bl      board_init_f_malloc
    stwu    r0, -4(r1)
    stwu    r0, -4(r1)
  

#endif diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S index 5b4ee79..25ac286 100644 --- a/arch/x86/cpu/start.S +++ b/arch/x86/cpu/start.S @@ -122,9 +122,15 @@ car_init_ret: */ #endif /* Set up global data */

•   call    board_init_f_gd_size
  

•   subl    %esp, %eax
    mov     %esp, %eax
  

•   call    board_init_f_mem
  

•   mov     %eax, %esp
  

•   call    board_init_f_gd
  

•   /* Set up malloc arena */
  

•   call    board_init_f_malloc_size
  

•   subl    %esp, %eax
  

•   mov     %esp, %eax
  

•   call    board_init_f_malloc
  
  

#ifdef CONFIG_DEBUG_UART call debug_uart_init diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c index c78df94..cd7fd86 100644 --- a/arch/x86/lib/fsp/fsp_common.c +++ b/arch/x86/lib/fsp/fsp_common.c @@ -95,7 +95,7 @@ int x86_fsp_init(void) /* * The second time we enter here, adjust the size of malloc() * pool before relocation. Given gd->malloc_base was adjusted

•            * after the call to board_init_f_mem() in arch/x86/cpu/start.S,
  

•            * after the call to board_init_f_malloc() in arch/x86/cpu/start.S,
             * we should fix up gd->malloc_limit here.
             */
            gd->malloc_limit += CONFIG_FSP_SYS_MALLOC_F_LEN;
  

diff --git a/common/init/board_init.c b/common/init/board_init.c index e74b63b..1313138 100644 --- a/common/init/board_init.c +++ b/common/init/board_init.c @@ -29,32 +29,39 @@ __weak void arch_setup_gd(struct global_data *gd_ptr) } #endif /* !CONFIG_X86 */

-ulong board_init_f_mem(ulong top) +ulong board_init_f_gd_size(void) +{

•   return roundup(sizeof(struct global_data), 16);
  

+}

+void board_init_f_gd(struct global_data *gd_ptr) {

•   struct global_data *gd_ptr;
  

#ifndef _USE_MEMCPY int *ptr; #endif

•   /* Leave space for the stack we are running with now */
  

•   top -= 0x40;
  

•   top -= sizeof(struct global_data);
  

•   top = ALIGN(top, 16);
  

•   gd_ptr = (struct global_data *)top;
  

#ifdef _USE_MEMCPY memset(gd_ptr, '\0', sizeof(*gd)); #else for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); ) *ptr++ = 0; #endif

•   arch_setup_gd(gd_ptr);
  

+}

+ulong board_init_f_malloc_size(void) +{ #if defined(CONFIG_SYS_MALLOC_F) && \

•   (!defined(CONFIG_SPL_BUILD) || !defined(CONFIG_SYS_SPL_MALLOC_START))
  

•   top -= CONFIG_SYS_MALLOC_F_LEN;
  

•   gd->malloc_base = top;
  

• (!defined(CONFIG_SPL_BUILD) || !defined(CONFIG_SYS_SPL_MALLOC_START))

•   return CONFIG_SYS_MALLOC_F_LEN;
  

+#else

•   return 0;
  

#endif +}

•   return top;
  

+void board_init_f_malloc(ulong malloc_base) +{ +#if defined(CONFIG_SYS_MALLOC_F) && \

• (!defined(CONFIG_SPL_BUILD) || !defined(CONFIG_SYS_SPL_MALLOC_START))

•   gd->malloc_base = malloc_base;
  

+#endif } diff --git a/include/common.h b/include/common.h index 09a131d..dbc2808 100644 --- a/include/common.h +++ b/include/common.h @@ -225,32 +225,44 @@ void board_init_f(ulong); void board_init_r(gd_t *, ulong) __attribute__ ((noreturn));

/**

• • board_init_f_mem() - Allocate global data and set stack position

• • board_init_f_gd_size() - Return the size of the global data
  • This function is called by each architecture very early in the start-up

• • code to set up the environment for board_init_f(). It allocates space for
• • global_data (see include/asm-generic/global_data.h) and places the stack
• • below this.

• • code to allow the C runtime to reserve space for the GD on the stack.
• • @return: GD size
• */

+ulong board_init_f_gd_size(void);

+/**

• • board_init_f_gd() - Initialize (zero) the global data

• • This function requires a stack[1] Normally this is at @top. The function
• • starts allocating space from 64 bytes below @top. First it creates space
• • for global_data. Then it calls arch_setup_gd() which sets gd to point to
• • the global_data space and can reserve additional bytes of space if
• • required). Finally it allocates early malloc() memory
• • (CONFIG_SYS_MALLOC_F_LEN). The new top of the stack is just below this,
• • and it returned by this function.

• • This function is called once the C runtime has allocated the GD on
• • the stack. It must initialize the GD.
• • @gd_ptr: the pointer to the GD to initialize
• */

+void board_init_f_gd(struct global_data *gd_ptr);

+/**

• • board_init_f_malloc_size() - Return the size of the malloc arena

• • [1] Strictly speaking it would be possible to implement this function
• • in C on many archs such that it does not require a stack. However this
• • does not seem hugely important as only 64 byte are wasted. The 64 bytes
• • are used to handle the calling standard which generally requires pushing
• • addresses or registers onto the stack. We should be able to get away with
• • less if this becomes important.

• • This function is called once the GD is initialized, to allow the C
• • runtime to allocate the malloc arena on the stack.
• • @return: Malloc arena size
• */

+ulong board_init_f_malloc_size(void);

+/**

• • board_init_f_malloc() - Mark the malloc arena base in the global data

• • @top: Top of available memory, also normally the top of the stack
• • @return: New stack location

• • This function is called once the malloc arena is allocated on the
• • stack. It marks the malloc arena base in the global data.
• • @malloc_base: the base of the malloc arena
  */

-ulong board_init_f_mem(ulong top); +ulong board_init_f_malloc(ulong malloc_base);

/**

• arch_setup_gd() - Set up the global_data pointer

-- 2.5.0

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon