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

[Bin Meng]

Hi Albert,

On Wed, Nov 11, 2015 at 2:30 AM, Albert ARIBAUD
<albert.u.boot at 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 at 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