* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
@ 2015-11-10 18:30 Albert ARIBAUD
2015-11-11 8:52 ` Albert ARIBAUD
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Albert ARIBAUD @ 2015-11-10 18:30 UTC (permalink / raw)
To: u-boot
board_init_f_mem() alters the C runtime environment's
stack it ls actually already using. This is not a valid
C runtime environment.
Split board_init_f_mem into C functions which do not
alter their own stack and therefore run in a valid C
runtime environment.
Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
For NIOS2, this patch hopefully contains all manual
fixes by Thomas.
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 | 20 +++++++++++++---
arch/arm/lib/crt0.S | 10 ++++++--
arch/arm/lib/crt0_64.S | 10 ++++++--
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 | 4 ++--
common/init/board_init.c | 31 ++++++++++++++----------
include/common.h | 52 +++++++++++++++++++++++++----------------
10 files changed, 125 insertions(+), 51 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
+
/* 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..00cef36 100644
--- a/arch/arm/lib/crt0_64.S
+++ b/arch/arm/lib/crt0_64.S
@@ -75,8 +75,14 @@ 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, sp, x0
+ mov x0, sp
+ bl board_init_f_gd
+ bl board_init_f_malloc_size
+ sub sp, 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..c163ce1 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 at h)
+ ori r2, r2, %lo(board_init_f_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_f_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 */
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..5249455 100644
--- a/arch/x86/lib/fsp/fsp_common.c
+++ b/arch/x86/lib/fsp/fsp_common.c
@@ -95,8 +95,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_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..8839a4a 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;
+ 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..a2d81d9 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.
*
- * 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.
+ * @return: GD size
+ */
+ulong board_init_f_gd_size(void);
+
+/**
+ * board_init_f_gd() - Initialize (zero) the global data
+ *
+ * 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
+ *
+ * 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
*
- * [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 malloc arena is allocated on the
+ * stack. It marks the malloc arena base in the global data.
*
- * @top: Top of available memory, also normally the top of the stack
- * @return: New stack location
+ * @malloc_base: the base of the malloc arena
*/
-ulong board_init_f_mem(ulong top);
+void board_init_f_malloc(ulong malloc_base);
/**
* arch_setup_gd() - Set up the global_data pointer
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-10 18:30 [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment Albert ARIBAUD
@ 2015-11-11 8:52 ` Albert ARIBAUD
2015-11-12 5:59 ` Thomas Chou
2015-11-13 9:08 ` Bin Meng
2 siblings, 0 replies; 10+ messages in thread
From: Albert ARIBAUD @ 2015-11-11 8:52 UTC (permalink / raw)
To: u-boot
On Tue, 10 Nov 2015 19:30:47 +0100, 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.
>
> Split board_init_f_mem into C functions which do not
> alter their own stack and therefore run in a valid C
> runtime environment.
Additional note: tested with current u-boot/master (commit cad04990) on
an OpenRD-Client board (which builds Thumb-1 code) using gcc 5.2.1 (which
saves and restores r9 in just about any C function, causing
arch_setup_gd to fail setting GD):
- without this patch, the board fails to boot with an undefined
instruction exception.
- with this patch, the board boots and functions normally (at least
dhcp, tftp, go are working as expected).
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-10 18:30 [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment Albert ARIBAUD
2015-11-11 8:52 ` Albert ARIBAUD
@ 2015-11-12 5:59 ` Thomas Chou
2015-11-12 7:17 ` Albert ARIBAUD
2015-11-13 6:41 ` Albert ARIBAUD
2015-11-13 9:08 ` Bin Meng
2 siblings, 2 replies; 10+ messages in thread
From: Thomas Chou @ 2015-11-12 5:59 UTC (permalink / raw)
To: u-boot
Hi Albert,
On 2015?11?11? 02:30, Albert ARIBAUD 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.
>
> Split board_init_f_mem into C functions which do not
> alter their own stack and therefore run in a valid C
> runtime environment.
>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> For NIOS2, this patch hopefully contains all manual
> fixes by Thomas.
>
> 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 | 20 +++++++++++++---
> arch/arm/lib/crt0.S | 10 ++++++--
> arch/arm/lib/crt0_64.S | 10 ++++++--
> 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 | 4 ++--
> common/init/board_init.c | 31 ++++++++++++++----------
> include/common.h | 52 +++++++++++++++++++++++++----------------
> 10 files changed, 125 insertions(+), 51 deletions(-)
>
Additional fixes,
------------------------------------------------------------------------
diff --git a/common/init/board_init.c b/common/init/board_init.c
index 8839a4a..703e6d8 100644
--- a/common/init/board_init.c
+++ b/common/init/board_init.c
@@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr)
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)
--------------------------------------------------------------------------
diff --git a/arch/nios2/cpu/start.S b/arch/nios2/cpu/start.S
index c163ce1..0adff46 100644
--- a/arch/nios2/cpu/start.S
+++ b/arch/nios2/cpu/start.S
@@ -110,7 +110,7 @@ _reloc:
movhi r2, %hi(board_init_f_gd_size at h)
ori r2, r2, %lo(board_init_f_gd_size at h)
callr r2
- sub sp, sp, r4
+ sub sp, sp, r2
mov r4, sp
movhi r2, %hi(board_init_f_gd at h)
ori r2, r2, %lo(board_init_f_gd at h)
@@ -119,16 +119,12 @@ _reloc:
movhi r2, %hi(board_init_f_malloc_size at h)
ori r2, r2, %lo(board_init_f_malloc_size at h)
callr r2
- sub sp, sp, r4
+ sub sp, sp, r2
mov r4, sp
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 */
- mov sp, r2
- mov fp, sp
-
/* Call board_init_f -- never returns */
mov r4, r0
movhi r2, %hi(board_init_f at h)
----------------------------------------------------------------------------
Otherwise,
Tested-by: Thomas Chou <thomas@wytron.com.tw>
Acked-by: Thomas Chou <thomas@wytron.com.tw>
Thanks.
Best regards,
Thomas
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-12 5:59 ` Thomas Chou
@ 2015-11-12 7:17 ` Albert ARIBAUD
2015-11-12 8:28 ` Thomas Chou
2015-11-13 6:41 ` Albert ARIBAUD
1 sibling, 1 reply; 10+ messages in thread
From: Albert ARIBAUD @ 2015-11-12 7:17 UTC (permalink / raw)
To: u-boot
Hello Thomas,
On Thu, 12 Nov 2015 13:59:28 +0800, Thomas Chou <thomas@wytron.com.tw>
wrote:
> Hi Albert,
>
> On 2015?11?11? 02:30, Albert ARIBAUD 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.
> >
> > Split board_init_f_mem into C functions which do not
> > alter their own stack and therefore run in a valid C
> > runtime environment.
> >
> > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > ---
> > For NIOS2, this patch hopefully contains all manual
> > fixes by Thomas.
> >
> > 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 | 20 +++++++++++++---
> > arch/arm/lib/crt0.S | 10 ++++++--
> > arch/arm/lib/crt0_64.S | 10 ++++++--
> > 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 | 4 ++--
> > common/init/board_init.c | 31 ++++++++++++++----------
> > include/common.h | 52 +++++++++++++++++++++++++----------------
> > 10 files changed, 125 insertions(+), 51 deletions(-)
> >
>
> Additional fixes,
> ------------------------------------------------------------------------
> diff --git a/common/init/board_init.c b/common/init/board_init.c
> index 8839a4a..703e6d8 100644
> --- a/common/init/board_init.c
> +++ b/common/init/board_init.c
> @@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr)
> for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); )
> *ptr++ = 0;
> #endif
> + arch_setup_gd(gd_ptr);
Correct -- in ARM (Thumb-1 at least) we cannot use arch_setup_gd() so
we set GD (in r9) from within arch/arm/lib/crt0.S, but for NIOS2 it
might (and apparently does) work. Where is GD stored in NIOS2?
> }
>
> ulong board_init_f_malloc_size(void)
> --------------------------------------------------------------------------
> diff --git a/arch/nios2/cpu/start.S b/arch/nios2/cpu/start.S
> index c163ce1..0adff46 100644
> --- a/arch/nios2/cpu/start.S
> +++ b/arch/nios2/cpu/start.S
> @@ -110,7 +110,7 @@ _reloc:
> movhi r2, %hi(board_init_f_gd_size at h)
> ori r2, r2, %lo(board_init_f_gd_size at h)
> callr r2
> - sub sp, sp, r4
> + sub sp, sp, r2
Sorry, didn't know / realize the NIOS2 ABI has r2 iass the function
value return register, not r4.
> mov r4, sp
> movhi r2, %hi(board_init_f_gd at h)
> ori r2, r2, %lo(board_init_f_gd at h)
> @@ -119,16 +119,12 @@ _reloc:
> movhi r2, %hi(board_init_f_malloc_size at h)
> ori r2, r2, %lo(board_init_f_malloc_size at h)
> callr r2
> - sub sp, sp, r4
> + sub sp, sp, r2
Ditto.
> mov r4, sp
> 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 */
> - mov sp, r2
> - mov fp, sp
> -
Oops.
> /* Call board_init_f -- never returns */
> mov r4, r0
> movhi r2, %hi(board_init_f at h)
> ----------------------------------------------------------------------------
> Otherwise,
>
> Tested-by: Thomas Chou <thomas@wytron.com.tw>
> Acked-by: Thomas Chou <thomas@wytron.com.tw>
>
> Thanks.
Thanks to you; Will send a v3 to account for your and Simon's comments.
> Best regards,
> Thomas
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-12 7:17 ` Albert ARIBAUD
@ 2015-11-12 8:28 ` Thomas Chou
2015-11-12 10:23 ` Albert ARIBAUD
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Chou @ 2015-11-12 8:28 UTC (permalink / raw)
To: u-boot
Hi Albert,
On 2015?11?12? 15:17, Albert ARIBAUD wrote:
>> ------------------------------------------------------------------------
>> diff --git a/common/init/board_init.c b/common/init/board_init.c
>> index 8839a4a..703e6d8 100644
>> --- a/common/init/board_init.c
>> +++ b/common/init/board_init.c
>> @@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr)
>> for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); )
>> *ptr++ = 0;
>> #endif
>> + arch_setup_gd(gd_ptr);
>
> Correct -- in ARM (Thumb-1 at least) we cannot use arch_setup_gd() so
> we set GD (in r9) from within arch/arm/lib/crt0.S, but for NIOS2 it
> might (and apparently does) work. Where is GD stored in NIOS2?
>
It is a register, r26 "gp".
I have another question. Will it be simpler to have two calls instead of
four?
1. get size of gd plus malloc.
2. init gd and malloc.
Best regards,
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-12 8:28 ` Thomas Chou
@ 2015-11-12 10:23 ` Albert ARIBAUD
0 siblings, 0 replies; 10+ messages in thread
From: Albert ARIBAUD @ 2015-11-12 10:23 UTC (permalink / raw)
To: u-boot
Hello Thomas,
On Thu, 12 Nov 2015 16:28:38 +0800, Thomas Chou <thomas@wytron.com.tw>
wrote:
> Hi Albert,
>
> On 2015?11?12? 15:17, Albert ARIBAUD wrote:
> >> ------------------------------------------------------------------------
> >> diff --git a/common/init/board_init.c b/common/init/board_init.c
> >> index 8839a4a..703e6d8 100644
> >> --- a/common/init/board_init.c
> >> +++ b/common/init/board_init.c
> >> @@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr)
> >> for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); )
> >> *ptr++ = 0;
> >> #endif
> >> + arch_setup_gd(gd_ptr);
> >
> > Correct -- in ARM (Thumb-1 at least) we cannot use arch_setup_gd() so
> > we set GD (in r9) from within arch/arm/lib/crt0.S, but for NIOS2 it
> > might (and apparently does) work. Where is GD stored in NIOS2?
> >
>
> It is a register, r26 "gp".
Ok. In ARM it is r9, and gcc is prevented from ever using r9 by adding
the compiler option -ffixed-r9 to the compiler command lines. I guess
the same goes for NIOS2 -- maybe the NIOS2 gcc does not even need an
option, and always leaves gp/r26 alone.
> I have another question. Will it be simpler to have two calls instead of
> four?
>
> 1. get size of gd plus malloc.
>
> 2. init gd and malloc.
I'd hinted at reducing the number of functions, but not the number of
calls, in my reply to Simon Glass in this thread, see here:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/240520
Your proposal might indeed help reducing the number of calls as well.
The first function would receive the current top of the stack and would
subtract the (aligned) gd then the (aligned) malloc arena size, and
return the new top-of-stack, which the C runtime code would enforce
in sp (or whatever the equivalent is in each arch) -- BUT it would not
write anything in that space as it would not be reserved at that point.
The second function would receive this new top-of-stack again, this
time as a base of the reserved space (or it could receive the old
top-of-stack and work its way downward) and would be able to safely
write whatever it wants inside this space.
The only caveat is, we need to be sure that the second function can
reconstruct the base addresses of all allocated chunks (gd, malloc,
whatever they'll be wanting to add later) just like the first function
computed them (it could still be a single function called twice as I'd
suggested, BTW).
Thanks for the suggestion! I'll consider it for v3.
> Best regards,
> Thomas
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-12 5:59 ` Thomas Chou
2015-11-12 7:17 ` Albert ARIBAUD
@ 2015-11-13 6:41 ` Albert ARIBAUD
2015-11-13 8:20 ` Thomas Chou
1 sibling, 1 reply; 10+ messages in thread
From: Albert ARIBAUD @ 2015-11-13 6:41 UTC (permalink / raw)
To: u-boot
Hello Thomas,
On Thu, 12 Nov 2015 13:59:28 +0800, Thomas Chou <thomas@wytron.com.tw>
wrote:
> Hi Albert,
> - /* Update stack- and frame-pointers */
> - mov sp, r2
> - mov fp, sp
Just a sec here on second thought.
I understand the 'mov sp, r2' must be removed as it is replaced by a
'sub sp, sp, r2' a few lines earlier.
But doesn't the 'mov fp, sp' need to remain? Without it, fp is not set
any more, is it?
> Best regards,
> Thomas
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-13 6:41 ` Albert ARIBAUD
@ 2015-11-13 8:20 ` Thomas Chou
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Chou @ 2015-11-13 8:20 UTC (permalink / raw)
To: u-boot
Hi Albert,
On 2015?11?13? 14:41, Albert ARIBAUD wrote:
> Hello Thomas,
>
> On Thu, 12 Nov 2015 13:59:28 +0800, Thomas Chou <thomas@wytron.com.tw>
> wrote:
>> Hi Albert,
>
>> - /* Update stack- and frame-pointers */
>> - mov sp, r2
>> - mov fp, sp
>
> Just a sec here on second thought.
>
> I understand the 'mov sp, r2' must be removed as it is replaced by a
> 'sub sp, sp, r2' a few lines earlier.
>
> But doesn't the 'mov fp, sp' need to remain? Without it, fp is not set
> any more, is it?
>
Yes, the 'mov fp, sp' need to remain. I forgot to add them.
Best regards,
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-10 18:30 [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment Albert ARIBAUD
2015-11-11 8:52 ` Albert ARIBAUD
2015-11-12 5:59 ` Thomas Chou
@ 2015-11-13 9:08 ` Bin Meng
2015-11-13 12:22 ` Albert ARIBAUD
2 siblings, 1 reply; 10+ messages in thread
From: Bin Meng @ 2015-11-13 9:08 UTC (permalink / raw)
To: u-boot
Hi Albert,
On Wed, Nov 11, 2015 at 2:30 AM, 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.
>
> Split board_init_f_mem into C functions which do not
> alter their own stack and therefore run in a valid C
> runtime environment.
>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
Thank you for the other long email thread explaining the details. It's
really helpful to understand the problem we are trying to resolve
here.
> For NIOS2, this patch hopefully contains all manual
> fixes by Thomas.
I've tested the patch on x86 boards, but it does not boot :( Please
check my fixes (3 places) below.
>
> 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 | 20 +++++++++++++---
> arch/arm/lib/crt0.S | 10 ++++++--
> arch/arm/lib/crt0_64.S | 10 ++++++--
> 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 | 4 ++--
> common/init/board_init.c | 31 ++++++++++++++----------
> include/common.h | 52 +++++++++++++++++++++++++----------------
> 10 files changed, 125 insertions(+), 51 deletions(-)
>
[snip]
> 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
I guess you are confused by the AT&T assembly syntax. This should be:
subl %eax, %esp.
> 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
Ditto.
> + 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..5249455 100644
> --- a/arch/x86/lib/fsp/fsp_common.c
> +++ b/arch/x86/lib/fsp/fsp_common.c
> @@ -95,8 +95,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_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..8839a4a 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);
This arch_setup_gd(gd_ptr) should not be deleted.
> +}
>
> +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;
> + 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..a2d81d9 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.
> *
> - * 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.
> + * @return: GD size
> + */
> +ulong board_init_f_gd_size(void);
> +
> +/**
> + * board_init_f_gd() - Initialize (zero) the global data
> + *
> + * 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
> + *
> + * 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
> *
> - * [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 malloc arena is allocated on the
> + * stack. It marks the malloc arena base in the global data.
> *
> - * @top: Top of available memory, also normally the top of the stack
> - * @return: New stack location
> + * @malloc_base: the base of the malloc arena
> */
> -ulong board_init_f_mem(ulong top);
> +void board_init_f_malloc(ulong malloc_base);
>
> /**
> * arch_setup_gd() - Set up the global_data pointer
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment
2015-11-13 9:08 ` Bin Meng
@ 2015-11-13 12:22 ` Albert ARIBAUD
0 siblings, 0 replies; 10+ messages in thread
From: Albert ARIBAUD @ 2015-11-13 12:22 UTC (permalink / raw)
To: u-boot
Hello Bin,
On Fri, 13 Nov 2015 17:08:22 +0800, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Albert,
>
> > + subl %esp, %eax
>
> I guess you are confused by the AT&T assembly syntax. This should be:
> subl %eax, %esp.
> > + subl %esp, %eax
>
> Ditto.
Thanks -- last time I touched Intel assembly was more than a decade
ago...
> > - arch_setup_gd(gd_ptr);
>
> This arch_setup_gd(gd_ptr) should not be deleted.
Correct.
> Regards,
> Bin
Thanks! These fixes will go in v3.
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-11-13 12:22 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-10 18:30 [U-Boot] [PATCH v2] Fix board init code to use a valid C runtime environment Albert ARIBAUD
2015-11-11 8:52 ` Albert ARIBAUD
2015-11-12 5:59 ` Thomas Chou
2015-11-12 7:17 ` Albert ARIBAUD
2015-11-12 8:28 ` Thomas Chou
2015-11-12 10:23 ` Albert ARIBAUD
2015-11-13 6:41 ` Albert ARIBAUD
2015-11-13 8:20 ` Thomas Chou
2015-11-13 9:08 ` Bin Meng
2015-11-13 12:22 ` Albert ARIBAUD
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox