[U-Boot] [PATCH v7 1/2] Fix board init code to respect the C runtime environment

[U-Boot] [PATCH v7 1/2] Fix board init code to respect the C runtime environment

[Albert ARIBAUD]

board_init_f_mem() alters the C runtime environment's
stack it is actually already using. This is not a valid
behaviour within a C runtime environment.

Split board_init_f_mem into C functions which do not alter
their own stack and always behave properly with respect to
their C runtime environment.

Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
Copying custodians for all architectures which this
patch affects.

This patch has been build-tested for all affected
architectures except NIOS2, and run-tested on ARM
OpenRD Client.

For NIOS2, this patch contains all manual v1 and v2
fixes by Thomas.

For x86, this patch contains all manual v2 fixes by Bin.

Changes in v7: None
Changes in v6:
- Switch from size- to address-based reservation
- Add comments on gdp_tr vs gd use wrt arch_setup_gd.
- Clarify that this is about the *early* malloc arena

Changes in v5:
- Reword commit message (including summary/subject)
- Reword comments in ARC code

Changes in v4:
- Add comments on reserving GD at the lowest location
- Add comments on post-incrementing base for each "chunk"

Changes in v3:
- Rebase after commit 9ac4fc82
- Simplify malloc conditional as per commit 9ac4fc82
- Fix NIOS2 return value register (r2, not r4)
- Fix x86 subl argument inversion
- Group allocations in a single function
- Group initializations in a single function

Changes in v2:
- Fix all checkpatch issues
- Fix board_init_f_malloc prototype mismatch
- Fix board_init_[f_]xxx typo in NIOS2
- Fix aarch64 asm 'sub' syntax error

 arch/arc/lib/start.S            |  12 +++--
 arch/arm/lib/crt0.S             |   3 +-
 arch/arm/lib/crt0_64.S          |   4 +-
 arch/microblaze/cpu/start.S     |   4 +-
 arch/nios2/cpu/start.S          |  14 ++++--
 arch/powerpc/cpu/ppc4xx/start.S |   6 ++-
 arch/x86/cpu/start.S            |   3 +-
 arch/x86/lib/fsp/fsp_common.c   |   4 +-
 common/init/board_init.c        | 109 ++++++++++++++++++++++++++++++++++++----
 include/common.h                |  34 ++++++-------
 10 files changed, 144 insertions(+), 49 deletions(-)

diff --git a/arch/arc/lib/start.S b/arch/arc/lib/start.S
index 26a5934..90ee7e0 100644
--- a/arch/arc/lib/start.S
+++ b/arch/arc/lib/start.S
@@ -50,18 +50,20 @@ ENTRY(_start)
 1:
 #endif
 
-	/* Setup stack- and frame-pointers */
+	/* Establish C runtime stack and frame */
 	mov	%sp, CONFIG_SYS_INIT_SP_ADDR
 	mov	%fp, %sp
 
-	/* Allocate and zero GD, update SP */
+	/* Allocate reserved area from current top of stack */
 	mov	%r0, %sp
-	bl	board_init_f_mem
-
-	/* Update stack- and frame-pointers */
+	bl	board_init_f_alloc_reserve
+	/* Set stack below reserved area, adjust frame pointer accordingly */
 	mov	%sp, %r0
 	mov	%fp, %sp
 
+	/* Initialize reserved area - note: r0 already contains address */
+	bl	board_init_f_init_reserve
+
 	/* 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..4f2a712 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -83,8 +83,9 @@ ENTRY(_main)
 	bic	sp, sp, #7	/* 8-byte alignment for ABI compliance */
 #endif
 	mov	r0, sp
-	bl	board_init_f_mem
+	bl	board_init_f_alloc_reserve
 	mov	sp, r0
+	bl	board_init_f_init_reserve
 
 	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..b4fc760 100644
--- a/arch/arm/lib/crt0_64.S
+++ b/arch/arm/lib/crt0_64.S
@@ -75,8 +75,10 @@ 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	x0, sp
+	bl	board_init_f_alloc_reserve
 	mov	sp, x0
+	bl	board_init_f_init_reserve
 
 	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..204d0cd 100644
--- a/arch/nios2/cpu/start.S
+++ b/arch/nios2/cpu/start.S
@@ -106,14 +106,18 @@ _reloc:
 	stw	r0, 4(sp)
 	mov	fp, sp
 
-	/* Allocate and zero GD, update SP */
+	/* Allocate and initialize reserved area, update SP */
 	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_alloc_reserve at h)
+	ori	r2, r2, %lo(board_init_f_alloc_reserve at h)
 	callr	r2
-
-	/* Update stack- and frame-pointers */
 	mov	sp, r2
+	mov	r4, sp
+	movhi	r2, %hi(board_init_f_init_reserve at h)
+	ori	r2, r2, %lo(board_init_f_init_reserve at h)
+	callr	r2
+
+	/* Update frame-pointer */
 	mov	fp, sp
 
 	/* Call board_init_f -- never returns */
diff --git a/arch/powerpc/cpu/ppc4xx/start.S b/arch/powerpc/cpu/ppc4xx/start.S
index 77d4040..cb63ab1 100644
--- a/arch/powerpc/cpu/ppc4xx/start.S
+++ b/arch/powerpc/cpu/ppc4xx/start.S
@@ -762,8 +762,9 @@ _start:
 	bl	cpu_init_f	/* run low-level CPU init code	   (from Flash) */
 #ifdef CONFIG_SYS_GENERIC_BOARD
 	mr	r3, r1
-	bl	board_init_f_mem
+	bl	board_init_f_alloc_reserve
 	mr	r1, r3
+	bl	board_init_f_init_reserve
 	li	r0,0
 	stwu	r0, -4(r1)
 	stwu	r0, -4(r1)
@@ -1038,8 +1039,9 @@ _start:
 	bl	cpu_init_f	/* run low-level CPU init code	   (from Flash) */
 #ifdef CONFIG_SYS_GENERIC_BOARD
 	mr	r3, r1
-	bl	board_init_f_mem
+	bl	board_init_f_alloc_reserve
 	mr	r1, r3
+	bl	board_init_f_init_reserve
 	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..485868f 100644
--- a/arch/x86/cpu/start.S
+++ b/arch/x86/cpu/start.S
@@ -123,8 +123,9 @@ car_init_ret:
 #endif
 	/* Set up global data */
 	mov	%esp, %eax
-	call	board_init_f_mem
+	call	board_init_f_alloc_reserve
 	mov	%eax, %esp
+	call	board_init_f_init_reserve
 
 #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 5276ce6..8479af1 100644
--- a/arch/x86/lib/fsp/fsp_common.c
+++ b/arch/x86/lib/fsp/fsp_common.c
@@ -90,8 +90,8 @@ 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,
-		 * we should fix up gd->malloc_limit here.
+		 * after the call to board_init_f_init_reserve() 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 1c6126d..e649e07 100644
--- a/common/init/board_init.c
+++ b/common/init/board_init.c
@@ -29,31 +29,120 @@ __weak void arch_setup_gd(struct global_data *gd_ptr)
 }
 #endif /* !CONFIG_X86 */
 
-ulong board_init_f_mem(ulong top)
+/*
+ * Allocate reserved space for use as 'globals' from 'top' address and
+ * return 'bottom' address of allocated space
+ *
+ * Notes:
+ *
+ * Actual reservation cannot be done from within this function as
+ * it requires altering the C stack pointer, so this will be done by
+ * the caller upon return from this function.
+ *
+ * IMPORTANT:
+ *
+ * Alignment constraints may differ for each 'chunk' allocated. For now:
+ *
+ * - GD is aligned down on a 16-byte boundary
+ *
+ *  - the early malloc arena is not aligned, therefore it follows the stack
+ *   alignment constraint of the architecture for which we are bulding.
+ *
+ *  - GD is allocated last, so that the return value of this functions is
+ *   both the bottom of the reserved area and the address of GD, should
+ *   the calling context need it.
+ */
+
+ulong board_init_f_alloc_reserve(ulong top)
+{
+	/* Reserve early malloc arena */
+#if defined(CONFIG_SYS_MALLOC_F)
+	top -= CONFIG_SYS_MALLOC_F_LEN;
+#endif
+	/* LAST : reserve GD (rounded up to a multiple of 16 bytes) */
+	top = rounddown(top-sizeof(struct global_data), 16);
+
+	return top;
+}
+
+/*
+ * Initialize reserved space (which has been safely allocated on the C
+ * stack from the C runtime environment handling code).
+ *
+ * Notes:
+ *
+ * Actual reservation was done by the caller; the locations from base
+ * to base+size-1 (where 'size' is the value returned by the allocation
+ * function above) can be accessed freely without risk of corrupting the
+ * C runtime environment.
+ *
+ * IMPORTANT:
+ *
+ * Upon return from the allocation function above, on some architectures
+ * the caller will set gd to the lowest reserved location. Therefore, in
+ * this initialization function, the global data MUST be placed at base.
+ *
+ * ALSO IMPORTANT:
+ *
+ * On some architectures, gd will already be good when entering this
+ * function. On others, it will only be good once arch_setup_gd() returns.
+ * Therefore, global data accesses must be done:
+ *
+ * - through gd_ptr if before the call to arch_setup_gd();
+ *
+ * - through gd once arch_setup_gd() has been called.
+ *
+ * Do not use 'gd->' until arch_setup_gd() has been called!
+ *
+ * IMPORTANT TOO:
+ *
+ * Initialization for each "chunk" (GD, early malloc arena...) ends with
+ * an incrementation line of the form 'base += <some size>'. The last of
+ * these incrementations seems useless, as base will not be used any
+ * more after this incrementation; but if/when a new "chunk" is appended,
+ * this increment will be essential as it will give base right value for
+ * this new chunk (which will have to end with its own incrementation
+ * statement). Besides, the compiler's optimizer will silently detect
+ * and remove the last base incrementation, therefore leaving that last
+ * (seemingly useless) incrementation causes no code increase.
+ */
+
+void board_init_f_init_reserve(ulong base)
 {
 	struct global_data *gd_ptr;
 #ifndef _USE_MEMCPY
 	int *ptr;
 #endif
 
-	/* Leave space for the stack we are running with now */
-	top -= 0x40;
+	/*
+	 * clear GD entirely and set it up.
+	 * Use gd_ptr, as gd may not be properly set yet.
+	 */
 
-	top -= sizeof(struct global_data);
-	top = ALIGN(top, 16);
-	gd_ptr = (struct global_data *)top;
+	gd_ptr = (struct global_data *)base;
+	/* zero the area */
 #ifdef _USE_MEMCPY
 	memset(gd_ptr, '\0', sizeof(*gd));
 #else
 	for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); )
 		*ptr++ = 0;
 #endif
+	/* set GD unless architecture did it already */
+#ifndef CONFIG_X86
 	arch_setup_gd(gd_ptr);
+#endif
+	/* next alloc will be higher by one GD plus 16-byte alignment */
+	base += roundup(sizeof(struct global_data), 16);
+
+	/*
+	 * record early malloc arena start.
+	 * Use gd as it is now properly set for all architectures.
+	 */
 
 #if defined(CONFIG_SYS_MALLOC_F)
-	top -= CONFIG_SYS_MALLOC_F_LEN;
-	gd->malloc_base = top;
+	/* go down one 'early malloc arena' */
+	gd->malloc_base = base;
+	/* next alloc will be higher by one 'early malloc arena' size */
+	base += CONFIG_SYS_MALLOC_F_LEN;
 #endif
-
-	return top;
 }
diff --git a/include/common.h b/include/common.h
index e910730..3d87de3 100644
--- a/include/common.h
+++ b/include/common.h
@@ -224,32 +224,26 @@ 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
+ * ulong board_init_f_alloc_reserve - allocate reserved area
  *
  * 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 on the stack for writable
+ * 'globals' such as GD and the malloc arena.
  *
- * 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.
+ * @top:	top of the reserve area, growing down.
+ * @return:	bottom of reserved area
+ */
+ulong board_init_f_alloc_reserve(ulong top);
+
+/**
+ * board_init_f_init_reserve - initialize the reserved area(s)
  *
- * [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 C runtime has allocated the reserved
+ * area on the stack. It must initialize the GD at the base of that area.
  *
- * @top:	Top of available memory, also normally the top of the stack
- * @return:	New stack location
+ * @base:	top from which reservation was done
  */
-ulong board_init_f_mem(ulong top);
+void board_init_f_init_reserve(ulong base);
 
 /**
  * arch_setup_gd() - Set up the global_data pointer
-- 
2.5.0

[U-Boot] [PATCH v7 2/2] arm: move gd handling outside of C code

[Albert ARIBAUD]

As of gcc 5.2.1 for Thumb-1, it is not possible any
more to assign gd from C code, as gd is mapped to r9,
and r9 may now be saved in the prolog sequence, and
restored in the epilog sequence, of any C functions.

Therefore arch_setup_gd(), which is supposed to set
r9, may actually have no effect, causing U-Boot to
use a bad address to access GD.

Fix this by never calling arch_setup_gd() for ARM,
and instead setting r9 in arch/arm/lib/crt0.S, to
the value returned by board_init_f_alloc_reserve().

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

Changes in v7:
- also fix common/spl/spl.c assignment to gd

Changes in v6:
- moved ARM r9 fix into its own patch

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm/lib/crt0.S      |  3 +++
 common/init/board_init.c |  8 ++++----
 common/spl/spl.c         | 26 +++++++++++++++-----------
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index 4f2a712..2f4c14e 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -85,6 +85,8 @@ ENTRY(_main)
 	mov	r0, sp
 	bl	board_init_f_alloc_reserve
 	mov	sp, r0
+	/* set up gd here, outside any C code */
+	mov	r9, r0
 	bl	board_init_f_init_reserve
 
 	mov	r0, #0
@@ -134,6 +136,7 @@ here:
 	bl	spl_relocate_stack_gd
 	cmp	r0, #0
 	movne	sp, r0
+	movne	r9, r0
 # endif
 	ldr	r0, =__bss_start	/* this is auto-relocated! */
 
diff --git a/common/init/board_init.c b/common/init/board_init.c
index e649e07..d98648e 100644
--- a/common/init/board_init.c
+++ b/common/init/board_init.c
@@ -21,13 +21,13 @@ DECLARE_GLOBAL_DATA_PTR;
 #define _USE_MEMCPY
 #endif
 
-/* Unfortunately x86 can't compile this code as gd cannot be assigned */
-#ifndef CONFIG_X86
+/* Unfortunately x86 or ARM can't compile this code as gd cannot be assigned */
+#if !defined(CONFIG_X86) && !defined(CONFIG_ARM)
 __weak void arch_setup_gd(struct global_data *gd_ptr)
 {
 	gd = gd_ptr;
 }
-#endif /* !CONFIG_X86 */
+#endif /* !CONFIG_X86 && !CONFIG_ARM */
 
 /*
  * Allocate reserved space for use as 'globals' from 'top' address and
@@ -128,7 +128,7 @@ void board_init_f_init_reserve(ulong base)
 		*ptr++ = 0;
 #endif
 	/* set GD unless architecture did it already */
-#ifndef CONFIG_X86
+#if !defined(CONFIG_X86) && !defined(CONFIG_ARM)
 	arch_setup_gd(gd_ptr);
 #endif
 	/* next alloc will be higher by one GD plus 16-byte alignment */
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 7a393dc..61d0786 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -431,8 +431,13 @@ void preloader_console_init(void)
  * more stack space for things like the MMC sub-system.
  *
  * This function calculates the stack position, copies the global_data into
- * place and returns the new stack position. The caller is responsible for
- * setting up the sp register.
+ * place, sets the new gd (except for ARM, for which setting GD within a C
+ * function may not always work) and returns the new stack position. The
+ * caller is responsible for setting up the sp register and, in the case
+ * of ARM, setting up gd.
+ *
+ * All of this is done using the same layout and alignments as done in
+ * board_init_f_init_reserve() / board_init_f_alloc_reserve().
  *
  * @return new stack location, or 0 to use the same stack
  */
@@ -440,14 +445,7 @@ ulong spl_relocate_stack_gd(void)
 {
 #ifdef CONFIG_SPL_STACK_R
 	gd_t *new_gd;
-	ulong ptr;
-
-	/* Get stack position: use 8-byte alignment for ABI compliance */
-	ptr = CONFIG_SPL_STACK_R_ADDR - sizeof(gd_t);
-	ptr &= ~7;
-	new_gd = (gd_t *)ptr;
-	memcpy(new_gd, (void *)gd, sizeof(gd_t));
-	gd = new_gd;
+	ulong ptr = CONFIG_SPL_STACK_R_ADDR;
 
 #ifdef CONFIG_SPL_SYS_MALLOC_SIMPLE
 	if (CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN) {
@@ -460,7 +458,13 @@ ulong spl_relocate_stack_gd(void)
 		gd->malloc_ptr = 0;
 	}
 #endif
-
+	/* Get stack position: use 8-byte alignment for ABI compliance */
+	ptr = CONFIG_SPL_STACK_R_ADDR - roundup(sizeof(gd_t),16);
+	new_gd = (gd_t *)ptr;
+	memcpy(new_gd, (void *)gd, sizeof(gd_t));
+#if !defined(CONFIG_ARM)
+	gd = new_gd;
+#endif
 	return ptr;
 #else
 	return 0;
-- 
2.5.0

[U-Boot] [PATCH v7 1/2] Fix board init code to respect the C runtime environment

[Simon Glass]

Hi Albert,

On 25 November 2015 at 08:56, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> board_init_f_mem() alters the C runtime environment's
> stack it is actually already using. This is not a valid
> behaviour within a C runtime environment.
>
> Split board_init_f_mem into C functions which do not alter
> their own stack and always behave properly with respect to
> their C runtime environment.
>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Copying custodians for all architectures which this
> patch affects.
>
> This patch has been build-tested for all affected
> architectures except NIOS2, and run-tested on ARM
> OpenRD Client.
>
> For NIOS2, this patch contains all manual v1 and v2
> fixes by Thomas.
>
> For x86, this patch contains all manual v2 fixes by Bin.
>
> Changes in v7: None
> Changes in v6:
> - Switch from size- to address-based reservation
> - Add comments on gdp_tr vs gd use wrt arch_setup_gd.
> - Clarify that this is about the *early* malloc arena
>
> Changes in v5:
> - Reword commit message (including summary/subject)
> - Reword comments in ARC code
>
> Changes in v4:
> - Add comments on reserving GD at the lowest location
> - Add comments on post-incrementing base for each "chunk"
>
> Changes in v3:
> - Rebase after commit 9ac4fc82
> - Simplify malloc conditional as per commit 9ac4fc82
> - Fix NIOS2 return value register (r2, not r4)
> - Fix x86 subl argument inversion
> - Group allocations in a single function
> - Group initializations in a single function
>
> Changes in v2:
> - Fix all checkpatch issues
> - Fix board_init_f_malloc prototype mismatch
> - Fix board_init_[f_]xxx typo in NIOS2
> - Fix aarch64 asm 'sub' syntax error
>
>  arch/arc/lib/start.S            |  12 +++--
>  arch/arm/lib/crt0.S             |   3 +-
>  arch/arm/lib/crt0_64.S          |   4 +-
>  arch/microblaze/cpu/start.S     |   4 +-
>  arch/nios2/cpu/start.S          |  14 ++++--
>  arch/powerpc/cpu/ppc4xx/start.S |   6 ++-
>  arch/x86/cpu/start.S            |   3 +-
>  arch/x86/lib/fsp/fsp_common.c   |   4 +-
>  common/init/board_init.c        | 109 ++++++++++++++++++++++++++++++++++++----
>  include/common.h                |  34 ++++++-------
>  10 files changed, 144 insertions(+), 49 deletions(-)

This patch looks right except for one thing below.

>
> diff --git a/arch/arc/lib/start.S b/arch/arc/lib/start.S
> index 26a5934..90ee7e0 100644
> --- a/arch/arc/lib/start.S
> +++ b/arch/arc/lib/start.S
> @@ -50,18 +50,20 @@ ENTRY(_start)
>  1:
>  #endif
>
> -       /* Setup stack- and frame-pointers */
> +       /* Establish C runtime stack and frame */
>         mov     %sp, CONFIG_SYS_INIT_SP_ADDR
>         mov     %fp, %sp
>
> -       /* Allocate and zero GD, update SP */
> +       /* Allocate reserved area from current top of stack */
>         mov     %r0, %sp
> -       bl      board_init_f_mem
> -
> -       /* Update stack- and frame-pointers */
> +       bl      board_init_f_alloc_reserve
> +       /* Set stack below reserved area, adjust frame pointer accordingly */
>         mov     %sp, %r0
>         mov     %fp, %sp
>
> +       /* Initialize reserved area - note: r0 already contains address */
> +       bl      board_init_f_init_reserve
> +
>         /* 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..4f2a712 100644
> --- a/arch/arm/lib/crt0.S
> +++ b/arch/arm/lib/crt0.S
> @@ -83,8 +83,9 @@ ENTRY(_main)
>         bic     sp, sp, #7      /* 8-byte alignment for ABI compliance */
>  #endif
>         mov     r0, sp
> -       bl      board_init_f_mem
> +       bl      board_init_f_alloc_reserve
>         mov     sp, r0
> +       bl      board_init_f_init_reserve
>
>         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..b4fc760 100644
> --- a/arch/arm/lib/crt0_64.S
> +++ b/arch/arm/lib/crt0_64.S
> @@ -75,8 +75,10 @@ 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     x0, sp
> +       bl      board_init_f_alloc_reserve
>         mov     sp, x0
> +       bl      board_init_f_init_reserve
>
>         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..204d0cd 100644
> --- a/arch/nios2/cpu/start.S
> +++ b/arch/nios2/cpu/start.S
> @@ -106,14 +106,18 @@ _reloc:
>         stw     r0, 4(sp)
>         mov     fp, sp
>
> -       /* Allocate and zero GD, update SP */
> +       /* Allocate and initialize reserved area, update SP */
>         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_alloc_reserve at h)
> +       ori     r2, r2, %lo(board_init_f_alloc_reserve at h)
>         callr   r2
> -
> -       /* Update stack- and frame-pointers */
>         mov     sp, r2
> +       mov     r4, sp
> +       movhi   r2, %hi(board_init_f_init_reserve at h)
> +       ori     r2, r2, %lo(board_init_f_init_reserve at h)
> +       callr   r2
> +
> +       /* Update frame-pointer */
>         mov     fp, sp
>
>         /* Call board_init_f -- never returns */
> diff --git a/arch/powerpc/cpu/ppc4xx/start.S b/arch/powerpc/cpu/ppc4xx/start.S
> index 77d4040..cb63ab1 100644
> --- a/arch/powerpc/cpu/ppc4xx/start.S
> +++ b/arch/powerpc/cpu/ppc4xx/start.S
> @@ -762,8 +762,9 @@ _start:
>         bl      cpu_init_f      /* run low-level CPU init code     (from Flash) */
>  #ifdef CONFIG_SYS_GENERIC_BOARD
>         mr      r3, r1
> -       bl      board_init_f_mem
> +       bl      board_init_f_alloc_reserve
>         mr      r1, r3
> +       bl      board_init_f_init_reserve
>         li      r0,0
>         stwu    r0, -4(r1)
>         stwu    r0, -4(r1)
> @@ -1038,8 +1039,9 @@ _start:
>         bl      cpu_init_f      /* run low-level CPU init code     (from Flash) */
>  #ifdef CONFIG_SYS_GENERIC_BOARD
>         mr      r3, r1
> -       bl      board_init_f_mem
> +       bl      board_init_f_alloc_reserve
>         mr      r1, r3
> +       bl      board_init_f_init_reserve
>         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..485868f 100644
> --- a/arch/x86/cpu/start.S
> +++ b/arch/x86/cpu/start.S
> @@ -123,8 +123,9 @@ car_init_ret:
>  #endif
>         /* Set up global data */
>         mov     %esp, %eax
> -       call    board_init_f_mem
> +       call    board_init_f_alloc_reserve
>         mov     %eax, %esp
> +       call    board_init_f_init_reserve
>
>  #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 5276ce6..8479af1 100644
> --- a/arch/x86/lib/fsp/fsp_common.c
> +++ b/arch/x86/lib/fsp/fsp_common.c
> @@ -90,8 +90,8 @@ 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,
> -                * we should fix up gd->malloc_limit here.
> +                * after the call to board_init_f_init_reserve() 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 1c6126d..e649e07 100644
> --- a/common/init/board_init.c
> +++ b/common/init/board_init.c
> @@ -29,31 +29,120 @@ __weak void arch_setup_gd(struct global_data *gd_ptr)
>  }
>  #endif /* !CONFIG_X86 */
>
> -ulong board_init_f_mem(ulong top)
> +/*
> + * Allocate reserved space for use as 'globals' from 'top' address and
> + * return 'bottom' address of allocated space
> + *
> + * Notes:
> + *
> + * Actual reservation cannot be done from within this function as
> + * it requires altering the C stack pointer, so this will be done by
> + * the caller upon return from this function.
> + *
> + * IMPORTANT:
> + *
> + * Alignment constraints may differ for each 'chunk' allocated. For now:
> + *
> + * - GD is aligned down on a 16-byte boundary
> + *
> + *  - the early malloc arena is not aligned, therefore it follows the stack
> + *   alignment constraint of the architecture for which we are bulding.
> + *
> + *  - GD is allocated last, so that the return value of this functions is
> + *   both the bottom of the reserved area and the address of GD, should
> + *   the calling context need it.
> + */
> +
> +ulong board_init_f_alloc_reserve(ulong top)
> +{
> +       /* Reserve early malloc arena */
> +#if defined(CONFIG_SYS_MALLOC_F)
> +       top -= CONFIG_SYS_MALLOC_F_LEN;
> +#endif
> +       /* LAST : reserve GD (rounded up to a multiple of 16 bytes) */
> +       top = rounddown(top-sizeof(struct global_data), 16);
> +
> +       return top;
> +}
> +
> +/*
> + * Initialize reserved space (which has been safely allocated on the C
> + * stack from the C runtime environment handling code).
> + *
> + * Notes:
> + *
> + * Actual reservation was done by the caller; the locations from base
> + * to base+size-1 (where 'size' is the value returned by the allocation
> + * function above) can be accessed freely without risk of corrupting the
> + * C runtime environment.
> + *
> + * IMPORTANT:
> + *
> + * Upon return from the allocation function above, on some architectures
> + * the caller will set gd to the lowest reserved location. Therefore, in
> + * this initialization function, the global data MUST be placed at base.
> + *
> + * ALSO IMPORTANT:
> + *
> + * On some architectures, gd will already be good when entering this
> + * function. On others, it will only be good once arch_setup_gd() returns.
> + * Therefore, global data accesses must be done:
> + *
> + * - through gd_ptr if before the call to arch_setup_gd();
> + *
> + * - through gd once arch_setup_gd() has been called.
> + *
> + * Do not use 'gd->' until arch_setup_gd() has been called!
> + *
> + * IMPORTANT TOO:
> + *
> + * Initialization for each "chunk" (GD, early malloc arena...) ends with
> + * an incrementation line of the form 'base += <some size>'. The last of
> + * these incrementations seems useless, as base will not be used any
> + * more after this incrementation; but if/when a new "chunk" is appended,
> + * this increment will be essential as it will give base right value for
> + * this new chunk (which will have to end with its own incrementation
> + * statement). Besides, the compiler's optimizer will silently detect
> + * and remove the last base incrementation, therefore leaving that last
> + * (seemingly useless) incrementation causes no code increase.
> + */
> +
> +void board_init_f_init_reserve(ulong base)
>  {
>         struct global_data *gd_ptr;
>  #ifndef _USE_MEMCPY
>         int *ptr;
>  #endif
>
> -       /* Leave space for the stack we are running with now */
> -       top -= 0x40;
> +       /*
> +        * clear GD entirely and set it up.
> +        * Use gd_ptr, as gd may not be properly set yet.
> +        */
>
> -       top -= sizeof(struct global_data);
> -       top = ALIGN(top, 16);
> -       gd_ptr = (struct global_data *)top;
> +       gd_ptr = (struct global_data *)base;
> +       /* zero the area */
>  #ifdef _USE_MEMCPY
>         memset(gd_ptr, '\0', sizeof(*gd));
>  #else
>         for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); )
>                 *ptr++ = 0;
>  #endif
> +       /* set GD unless architecture did it already */
> +#ifndef CONFIG_X86
>         arch_setup_gd(gd_ptr);
> +#endif

For x86 we need to set the global_data pointer here. So I think you
need to remove this #ifndef.

> +       /* next alloc will be higher by one GD plus 16-byte alignment */
> +       base += roundup(sizeof(struct global_data), 16);
> +
> +       /*
> +        * record early malloc arena start.
> +        * Use gd as it is now properly set for all architectures.
> +        */
>
>  #if defined(CONFIG_SYS_MALLOC_F)
> -       top -= CONFIG_SYS_MALLOC_F_LEN;
> -       gd->malloc_base = top;
> +       /* go down one 'early malloc arena' */
> +       gd->malloc_base = base;
> +       /* next alloc will be higher by one 'early malloc arena' size */
> +       base += CONFIG_SYS_MALLOC_F_LEN;
>  #endif
> -
> -       return top;
>  }
> diff --git a/include/common.h b/include/common.h
> index e910730..3d87de3 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -224,32 +224,26 @@ 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
> + * ulong board_init_f_alloc_reserve - allocate reserved area
>   *
>   * 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 on the stack for writable
> + * 'globals' such as GD and the malloc arena.
>   *
> - * 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.
> + * @top:       top of the reserve area, growing down.
> + * @return:    bottom of reserved area
> + */
> +ulong board_init_f_alloc_reserve(ulong top);
> +
> +/**
> + * board_init_f_init_reserve - initialize the reserved area(s)
>   *
> - * [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 C runtime has allocated the reserved
> + * area on the stack. It must initialize the GD at the base of that area.
>   *
> - * @top:       Top of available memory, also normally the top of the stack
> - * @return:    New stack location
> + * @base:      top from which reservation was done
>   */
> -ulong board_init_f_mem(ulong top);
> +void board_init_f_init_reserve(ulong base);
>
>  /**
>   * arch_setup_gd() - Set up the global_data pointer
> --
> 2.5.0
>

Regards,
Simon

[U-Boot] [PATCH v7 2/2] arm: move gd handling outside of C code

[Simon Glass]

On 25 November 2015 at 08:56, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> As of gcc 5.2.1 for Thumb-1, it is not possible any
> more to assign gd from C code, as gd is mapped to r9,
> and r9 may now be saved in the prolog sequence, and
> restored in the epilog sequence, of any C functions.
>
> Therefore arch_setup_gd(), which is supposed to set
> r9, may actually have no effect, causing U-Boot to
> use a bad address to access GD.
>
> Fix this by never calling arch_setup_gd() for ARM,
> and instead setting r9 in arch/arm/lib/crt0.S, to
> the value returned by board_init_f_alloc_reserve().
>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
>
> Changes in v7:
> - also fix common/spl/spl.c assignment to gd
>
> Changes in v6:
> - moved ARM r9 fix into its own patch
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  arch/arm/lib/crt0.S      |  3 +++
>  common/init/board_init.c |  8 ++++----
>  common/spl/spl.c         | 26 +++++++++++++++-----------
>  3 files changed, 22 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v7 1/2] Fix board init code to respect the C runtime environment

[Thomas Chou]

Hi Albert,

On 2015?11?26? 00:56, Albert ARIBAUD wrote:
> board_init_f_mem() alters the C runtime environment's
> stack it is actually already using. This is not a valid
> behaviour within a C runtime environment.
>
> Split board_init_f_mem into C functions which do not alter
> their own stack and always behave properly with respect to
> their C runtime environment.
>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Copying custodians for all architectures which this
> patch affects.
>
> This patch has been build-tested for all affected
> architectures except NIOS2, and run-tested on ARM
> OpenRD Client.
>
> For NIOS2, this patch contains all manual v1 and v2
> fixes by Thomas.
>
> For x86, this patch contains all manual v2 fixes by Bin.
>
> Changes in v7: None
> Changes in v6:
> - Switch from size- to address-based reservation
> - Add comments on gdp_tr vs gd use wrt arch_setup_gd.
> - Clarify that this is about the *early* malloc arena
>
> Changes in v5:
> - Reword commit message (including summary/subject)
> - Reword comments in ARC code
>
> Changes in v4:
> - Add comments on reserving GD at the lowest location
> - Add comments on post-incrementing base for each "chunk"
>
> Changes in v3:
> - Rebase after commit 9ac4fc82
> - Simplify malloc conditional as per commit 9ac4fc82
> - Fix NIOS2 return value register (r2, not r4)
> - Fix x86 subl argument inversion
> - Group allocations in a single function
> - Group initializations in a single function
>
> Changes in v2:
> - Fix all checkpatch issues
> - Fix board_init_f_malloc prototype mismatch
> - Fix board_init_[f_]xxx typo in NIOS2
> - Fix aarch64 asm 'sub' syntax error
>
>   arch/arc/lib/start.S            |  12 +++--
>   arch/arm/lib/crt0.S             |   3 +-
>   arch/arm/lib/crt0_64.S          |   4 +-
>   arch/microblaze/cpu/start.S     |   4 +-
>   arch/nios2/cpu/start.S          |  14 ++++--
>   arch/powerpc/cpu/ppc4xx/start.S |   6 ++-
>   arch/x86/cpu/start.S            |   3 +-
>   arch/x86/lib/fsp/fsp_common.c   |   4 +-
>   common/init/board_init.c        | 109 ++++++++++++++++++++++++++++++++++++----
>   include/common.h                |  34 ++++++-------
>   10 files changed, 144 insertions(+), 49 deletions(-)
>

Acked-by: Thomas Chou <thomas@wytron.com.tw>

[U-Boot] [U-Boot, v7, 1/2] Fix board init code to respect the C runtime environment

[Tom Rini]

On Wed, Nov 25, 2015 at 05:56:32PM +0100, Albert ARIBAUD wrote:

> board_init_f_mem() alters the C runtime environment's
> stack it is actually already using. This is not a valid
> behaviour within a C runtime environment.
> 
> Split board_init_f_mem into C functions which do not alter
> their own stack and always behave properly with respect to
> their C runtime environment.
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Acked-by: Thomas Chou <thomas@wytron.com.tw>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/cde78157/attachment.sig>

[U-Boot] [U-Boot, v7, 2/2] arm: move gd handling outside of C code

[Tom Rini]

On Wed, Nov 25, 2015 at 05:56:33PM +0100, Albert ARIBAUD wrote:

> As of gcc 5.2.1 for Thumb-1, it is not possible any
> more to assign gd from C code, as gd is mapped to r9,
> and r9 may now be saved in the prolog sequence, and
> restored in the epilog sequence, of any C functions.
> 
> Therefore arch_setup_gd(), which is supposed to set
> r9, may actually have no effect, causing U-Boot to
> use a bad address to access GD.
> 
> Fix this by never calling arch_setup_gd() for ARM,
> and instead setting r9 in arch/arm/lib/crt0.S, to
> the value returned by board_init_f_alloc_reserve().
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/32fb17e2/attachment.sig>

[U-Boot] [U-Boot, v7, 2/2] arm: move gd handling outside of C code

[Stephen Warren]

On 01/14/2016 06:20 AM, Tom Rini wrote:
> On Wed, Nov 25, 2015 at 05:56:33PM +0100, Albert ARIBAUD wrote:
>
>> As of gcc 5.2.1 for Thumb-1, it is not possible any
>> more to assign gd from C code, as gd is mapped to r9,
>> and r9 may now be saved in the prolog sequence, and
>> restored in the epilog sequence, of any C functions.
>>
>> Therefore arch_setup_gd(), which is supposed to set
>> r9, may actually have no effect, causing U-Boot to
>> use a bad address to access GD.
>>
>> Fix this by never calling arch_setup_gd() for ARM,
>> and instead setting r9 in arch/arm/lib/crt0.S, to
>> the value returned by board_init_f_alloc_reserve().
>>
>> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Applied to u-boot/master, thanks!

FYI, this commit causes U-Boot to fail (crash or hang during very early 
startup with zero UART output) on at least an NVIDIA Jetson TX1 
(p2371-2180) board. Reverting just this in u-boot/master solves the 
issue. I have not tested other boards or looked at the code itself yet.

(As an interesting datapoint, this is the first issue caught by the 
Jenkins system I'm setting up to run test/py on a few Tegra boards)

[U-Boot] [U-Boot, v7, 2/2] arm: move gd handling outside of C code

[Tom Rini]

On Thu, Jan 14, 2016 at 11:35:39AM -0700, Stephen Warren wrote:
> On 01/14/2016 06:20 AM, Tom Rini wrote:
> >On Wed, Nov 25, 2015 at 05:56:33PM +0100, Albert ARIBAUD wrote:
> >
> >>As of gcc 5.2.1 for Thumb-1, it is not possible any
> >>more to assign gd from C code, as gd is mapped to r9,
> >>and r9 may now be saved in the prolog sequence, and
> >>restored in the epilog sequence, of any C functions.
> >>
> >>Therefore arch_setup_gd(), which is supposed to set
> >>r9, may actually have no effect, causing U-Boot to
> >>use a bad address to access GD.
> >>
> >>Fix this by never calling arch_setup_gd() for ARM,
> >>and instead setting r9 in arch/arm/lib/crt0.S, to
> >>the value returned by board_init_f_alloc_reserve().
> >>
> >>Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >>Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> >Applied to u-boot/master, thanks!
> 
> FYI, this commit causes U-Boot to fail (crash or hang during very
> early startup with zero UART output) on at least an NVIDIA Jetson
> TX1 (p2371-2180) board. Reverting just this in u-boot/master solves
> the issue. I have not tested other boards or looked at the code
> itself yet.

Is that one of the systems where we have an ARM9 and then a Cortex-A?
FWIW, my pandaboard is up in Fedora currently.  I'm trying to do some
boot testing on what I have more often and then a bigger round of
unboxing and testing at -rc1/release time.

> (As an interesting datapoint, this is the first issue caught by the
> Jenkins system I'm setting up to run test/py on a few Tegra boards)

Yay for that at least.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/95aedb91/attachment.sig>

[U-Boot] [U-Boot, v7, 2/2] arm: move gd handling outside of C code

[Stephen Warren]

On 01/14/2016 12:11 PM, Tom Rini wrote:
> On Thu, Jan 14, 2016 at 11:35:39AM -0700, Stephen Warren wrote:
>> On 01/14/2016 06:20 AM, Tom Rini wrote:
>>> On Wed, Nov 25, 2015 at 05:56:33PM +0100, Albert ARIBAUD wrote:
>>>
>>>> As of gcc 5.2.1 for Thumb-1, it is not possible any
>>>> more to assign gd from C code, as gd is mapped to r9,
>>>> and r9 may now be saved in the prolog sequence, and
>>>> restored in the epilog sequence, of any C functions.
>>>>
>>>> Therefore arch_setup_gd(), which is supposed to set
>>>> r9, may actually have no effect, causing U-Boot to
>>>> use a bad address to access GD.
>>>>
>>>> Fix this by never calling arch_setup_gd() for ARM,
>>>> and instead setting r9 in arch/arm/lib/crt0.S, to
>>>> the value returned by board_init_f_alloc_reserve().
>>>>
>>>> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> Applied to u-boot/master, thanks!
>>
>> FYI, this commit causes U-Boot to fail (crash or hang during very
>> early startup with zero UART output) on at least an NVIDIA Jetson
>> TX1 (p2371-2180) board. Reverting just this in u-boot/master solves
>> the issue. I have not tested other boards or looked at the code
>> itself yet.
>
> Is that one of the systems where we have an ARM9 and then a Cortex-A?
> FWIW, my pandaboard is up in Fedora currently.  I'm trying to do some
> boot testing on what I have more often and then a bigger round of
> unboxing and testing at -rc1/release time.

This board is AArch64. There's no SPL or dual-architecture U-Boot on 
this system; a boot CPU runs the boot ROM and and NVIDIA binary 
bootloader which loads U-Boot from disk and sets up the main CPU, then 
the main ARM CPU essentially jumps straight into the main U-Boot binary.

For more precise details, see:

ftp://download.nvidia.com/tegra-public-appnotes/t210-nvtboot-flow.html

or a bunch of stuff accessible from the following for more background:

ftp://download.nvidia.com/tegra-public-appnotes/index.html

[U-Boot] [U-Boot, v7, 2/2] arm: move gd handling outside of C code

[Tom Rini]

On Thu, Jan 14, 2016 at 12:27:02PM -0700, Stephen Warren wrote:
> On 01/14/2016 12:11 PM, Tom Rini wrote:
> >On Thu, Jan 14, 2016 at 11:35:39AM -0700, Stephen Warren wrote:
> >>On 01/14/2016 06:20 AM, Tom Rini wrote:
> >>>On Wed, Nov 25, 2015 at 05:56:33PM +0100, Albert ARIBAUD wrote:
> >>>
> >>>>As of gcc 5.2.1 for Thumb-1, it is not possible any
> >>>>more to assign gd from C code, as gd is mapped to r9,
> >>>>and r9 may now be saved in the prolog sequence, and
> >>>>restored in the epilog sequence, of any C functions.
> >>>>
> >>>>Therefore arch_setup_gd(), which is supposed to set
> >>>>r9, may actually have no effect, causing U-Boot to
> >>>>use a bad address to access GD.
> >>>>
> >>>>Fix this by never calling arch_setup_gd() for ARM,
> >>>>and instead setting r9 in arch/arm/lib/crt0.S, to
> >>>>the value returned by board_init_f_alloc_reserve().
> >>>>
> >>>>Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >>>>Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>
> >>>Applied to u-boot/master, thanks!
> >>
> >>FYI, this commit causes U-Boot to fail (crash or hang during very
> >>early startup with zero UART output) on at least an NVIDIA Jetson
> >>TX1 (p2371-2180) board. Reverting just this in u-boot/master solves
> >>the issue. I have not tested other boards or looked at the code
> >>itself yet.
> >
> >Is that one of the systems where we have an ARM9 and then a Cortex-A?
> >FWIW, my pandaboard is up in Fedora currently.  I'm trying to do some
> >boot testing on what I have more often and then a bigger round of
> >unboxing and testing at -rc1/release time.
> 
> This board is AArch64. There's no SPL or dual-architecture U-Boot on
> this system; a boot CPU runs the boot ROM and and NVIDIA binary
> bootloader which loads U-Boot from disk and sets up the main CPU,
> then the main ARM CPU essentially jumps straight into the main
> U-Boot binary.

Oh, it's one of the aarch64 ones, OK.  Yeah, I need to get some for that
architecture in my setup.  It's annoyingly complex to get hikey flashed
but I can get my hands on a dragonboard soon, so once that's in I'll be
using that.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/f6fb83d8/attachment.sig>

[U-Boot] [U-Boot, v7, 2/2] arm: move gd handling outside of C code

[Stephen Warren]

On 01/14/2016 12:45 PM, Tom Rini wrote:
> On Thu, Jan 14, 2016 at 12:27:02PM -0700, Stephen Warren wrote:
>> On 01/14/2016 12:11 PM, Tom Rini wrote:
>>> On Thu, Jan 14, 2016 at 11:35:39AM -0700, Stephen Warren wrote:
>>>> On 01/14/2016 06:20 AM, Tom Rini wrote:
>>>>> On Wed, Nov 25, 2015 at 05:56:33PM +0100, Albert ARIBAUD wrote:
>>>>>
>>>>>> As of gcc 5.2.1 for Thumb-1, it is not possible any
>>>>>> more to assign gd from C code, as gd is mapped to r9,
>>>>>> and r9 may now be saved in the prolog sequence, and
>>>>>> restored in the epilog sequence, of any C functions.
>>>>>>
>>>>>> Therefore arch_setup_gd(), which is supposed to set
>>>>>> r9, may actually have no effect, causing U-Boot to
>>>>>> use a bad address to access GD.
>>>>>>
>>>>>> Fix this by never calling arch_setup_gd() for ARM,
>>>>>> and instead setting r9 in arch/arm/lib/crt0.S, to
>>>>>> the value returned by board_init_f_alloc_reserve().
>>>>>>
>>>>>> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>
>>>>> Applied to u-boot/master, thanks!
>>>>
>>>> FYI, this commit causes U-Boot to fail (crash or hang during very
>>>> early startup with zero UART output) on at least an NVIDIA Jetson
>>>> TX1 (p2371-2180) board. Reverting just this in u-boot/master solves
>>>> the issue. I have not tested other boards or looked at the code
>>>> itself yet.
>>>
>>> Is that one of the systems where we have an ARM9 and then a Cortex-A?
>>> FWIW, my pandaboard is up in Fedora currently.  I'm trying to do some
>>> boot testing on what I have more often and then a bigger round of
>>> unboxing and testing at -rc1/release time.
>>
>> This board is AArch64. There's no SPL or dual-architecture U-Boot on
>> this system; a boot CPU runs the boot ROM and and NVIDIA binary
>> bootloader which loads U-Boot from disk and sets up the main CPU,
>> then the main ARM CPU essentially jumps straight into the main
>> U-Boot binary.
>
> Oh, it's one of the aarch64 ones, OK.  Yeah, I need to get some for that
> architecture in my setup.  It's annoyingly complex to get hikey flashed
> but I can get my hands on a dragonboard soon, so once that's in I'll be
> using that.

FWIW, I've also now validated that the issue doesn't affect at least one 
of the 32-bit boards I have (Jetson TK1, which does have the 
dual-architecture SPL-vs-main U-Boot build).