Fix board init code to use a valid C runtime environment

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 <[email protected]>

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

Hi Albert,

Quoted 12 lines · Albert ARIBAUD

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 <[email protected]>

Quoted 13 lines

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

Apply the patch.

$ git am "/work/tmp/[U-Boot] [PATCH] Fix board init code to use a valid 
C runtime environment.eml"
Applying: Fix board init code to use a valid C runtime environment
/work/nios2-linux/u-boot/.git/rebase-apply/patch:91: trailing whitespace.

/work/nios2-linux/u-boot/.git/rebase-apply/patch:292: trailing whitespace.
*
/work/nios2-linux/u-boot/.git/rebase-apply/patch:309: trailing whitespace.
*
/work/nios2-linux/u-boot/.git/rebase-apply/patch:325: trailing whitespace.
*
/work/nios2-linux/u-boot/.git/rebase-apply/patch:337: trailing whitespace.
*
warning: 5 lines add whitespace errors.

Make.

LD arch/nios2/lib/built-in.o
CC common/init/board_init.o
common/init/board_init.c:61:6: error: conflicting types for 'board_init_f_malloc'
void board_init_f_malloc(ulong malloc_base)

       ^
In file included from common/init/board_init.c:10:0:
include/common.h:265:7: note: previous declaration of 
'board_init_f_malloc' was here
  ulong board_init_f_malloc(ulong malloc_base);
        ^

scripts/Makefile.build:277: recipe for target 'common/init/board_init.o' failed
make[2]: *** [common/init/board_init.o] Error 1 scripts/Makefile.build:422: recipe for target 'common/init' failed make[1]: *** [common/init] Error 2
Makefile:1205: recipe for target 'common' failed
make: *** [common] Error 2

Fix common.h.

include/common.h +1 -1

index dbc2808..7eb3ba8 100644
@@ -262,7 +262,7 @@ ulong board_init_f_malloc_size(void);
   *
   * @malloc_base: the base of the malloc arena
   */
-ulong board_init_f_malloc(ulong malloc_base);
+void board_init_f_malloc(ulong malloc_base);
 
  /**
   * arch_setup_gd() - Set up the global_data pointer

Make again.

LDS u-boot.lds
LD u-boot
arch/nios2/cpu/start.o: In function `_reloc':
/work/nios2-linux/u-boot/arch/nios2/cpu/start.S:111: undefined reference to board_init_gd_size' /work/nios2-linux/u-boot/arch/nios2/cpu/start.S:120: undefined reference to board_init_malloc_size'
Makefile:1187: recipe for target 'u-boot' failed
make: *** [u-boot] Error 1

Fix start.S.

arch/nios2/cpu/start.S +2 -2

index 4d5f0b0..c163ce1 100644
@@ -108,7 +108,7 @@ _reloc:
 
  	/* Allocate and zero GD, update SP */
  	movhi	r2, %hi(board_init_f_gd_size at h)
-	ori	r2, r2, %lo(board_init_gd_size at h)
+	ori	r2, r2, %lo(board_init_f_gd_size at h)
  	callr	r2
  	sub	sp, sp, r4
  	mov	r4, sp

@@ -117,7 +117,7 @@ _reloc:
  	callr	r2
  	/* Allocate malloc arena, update SP */
  	movhi	r2, %hi(board_init_f_malloc_size at h)
-	ori	r2, r2, %lo(board_init_malloc_size at h)
+	ori	r2, r2, %lo(board_init_f_malloc_size at h)
  	callr	r2
  	sub	sp, sp, r4
  	mov	r4, sp

Make and run, but failed to boot. I will gdb and tell you the trace later.

Best regards,
Thomas

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 <[email protected]>

---

Hi Albert,

Quoted 4 lines · Albert ARIBAUD

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.

What C compiler's expectations conflicts with current board_init_f_mem()? Did you see any actual error on any board?

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 <[email protected]>

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

arch/arc/lib/start.S +11 -2

index 26a5934..d24d60b 100644
@@ -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

arch/arc/lib/start.S +0 -0

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

arch/arm/lib/crt0.S +8 -2

index 80548eb..26abab7 100644
@@ -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

arch/arm/lib/crt0_64.S +9 -3

index cef1c71..e716889 100644
@@ -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
 

arch/microblaze/cpu/start.S +2 -2

index 14f46a8..206be3e 100644
@@ -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

arch/nios2/cpu/start.S +15 -2

index 54787c5..4d5f0b0 100644
@@ -107,9 +107,22 @@ _reloc:
        mov     fp, sp
 
        /* Allocate and zero GD, update SP */
+       movhi   r2, %hi(board_init_f_gd_size at h)
+       ori     r2, r2, %lo(board_init_gd_size at h)
+       callr   r2
+       sub     sp, sp, r4
+       mov     r4, sp
+       movhi   r2, %hi(board_init_f_gd at h)
+       ori     r2, r2, %lo(board_init_f_gd at h)
+       callr   r2
+       /* Allocate malloc arena, update SP */
+       movhi   r2, %hi(board_init_f_malloc_size at h)
+       ori     r2, r2, %lo(board_init_malloc_size at h)
+       callr   r2
+       sub     sp, sp, r4
        mov     r4, sp
-       movhi   r2, %hi(board_init_f_mem at h)
-       ori     r2, r2, %lo(board_init_f_mem at h)
+       movhi   r2, %hi(board_init_f_malloc at h)
+       ori     r2, r2, %lo(board_init_f_malloc at h)
        callr   r2
 
        /* Update stack- and frame-pointers */

arch/powerpc/cpu/ppc4xx/start.S +14 -4

index 77d4040..c827c95 100644
@@ -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

arch/x86/cpu/start.S +8 -2

index 5b4ee79..25ac286 100644
@@ -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

arch/x86/lib/fsp/fsp_common.c +1 -1

index c78df94..cd7fd86 100644
@@ -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;

common/init/board_init.c +20 -13

index e74b63b..1313138 100644
@@ -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
 }

include/common.h +32 -21

index 09a131d..dbc2808 100644
@@ -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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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?

Regards,
Simon