[U-Boot] [PATCH v5] Fix board init code to respect the C runtime environment

[Simon Glass]

Hi Albert,

On 17 November 2015 at 05:59, Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:
> Hello Simon,
>
> On Mon, 16 Nov 2015 21:11:38 -0700, Simon Glass <sjg at chromium.org>
> wrote:
>
>> > +++ b/arch/arm/lib/crt0.S
>> > @@ -82,9 +82,11 @@ ENTRY(_main)
>> >  #else
>> >         bic     sp, sp, #7      /* 8-byte alignment for ABI compliance */
>> >  #endif
>> > +       bl      board_init_f_get_reserve_size
>> > +       sub     sp, sp, r0
>> > +       mov     r9, sp
>>
>> Why do you need that?
>
> To set gd in ARM architecture, as arch_setup_gd() may not work for
> ARM, see my 'Note about the ARM case' in my answer dated Sun, 15 Nov
> 2015 14:51:04 +0100 for details:
>
> https://www.mail-archive.com/u-boot@lists.denx.de/msg192494.html
>

OK I understand that now.

> I shall post a 2-patch v6 with this fix standalone as 1/2 and the rest
> of the changes as 2/2.
>
>> > +++ b/common/init/board_init.c
>> > @@ -21,39 +21,121 @@ 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_SYS_THUMB_BUILD */
>> >
>> > -ulong board_init_f_mem(ulong top)
>> > +/*
>> > + * Compute size of space to reserve on stack for use as 'globals'
>> > + * and return size request for reserved area.
>> > + *
>> > + * 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:
>> > + *
>> > + * The return value of this function has two uses:
>> > + * - it determines the amount of stack reserved for global data and
>> > + *   the malloc arena;
>>
>> early malloc() area
>
> "Arena", and "malloc arena", are established designations for the
> memory space used by malloc implementations; and I prefer to use this
> more specific term, as people may use it as a search keyword when
> looking for malloc related stuff.

Arena is OK, but can you please mention 'early' each time? It's
confusing otherwise. I think we should have a clear distinction
between the early malloc() and full malloc().

>
>> Another option would be to pass the address and have this function
>> return the new address. That would simplify the assembly code slightly
>> for all archs I think. It would also allow you to align the address
>> for sure, whereas at present it only works if the original address was
>> aligned, and CONFIG_SYS_MALLOC_F_LEN is aligned.
>
> Good point, with a few caveats though.
>
> Regarding alignment of CONFIG_SYS_MALLOC_F_LEN:
>
> Indeed, no attempt is made to check that it is aligned (and no attempt
> is made in the original code either) -- all the more since there is no
> strict definition of what it should be aligned to. There is, actually,
> no indication that it should be aligned, although it will probably only
> be used up until the last full 4-byte-word (or 8-byte word for 64-bit
> architectures). In fact, the alignment of CONFIG_SYS_MALLOC_F_LEN does
> not matter much, see (*) below.
>
> Regarding alignment of the original/'top' address:
>
> This top address is the SP for all architectures, and at least for ARM,
> it is aligned to an 8-byte boundary, as this is an ARM architecture
> EABI requirement. For other architectures, I cannot claim it is, but I
> suspect all initial values of SP, which generally are CONFIG_SPL_STACK
> or CONFIG_SYS_INIT_SP_ADDR, are (sufficiently) aligned.
>
> And if CONFIG_SPL_STACK and CONFIG_SYS_INIT_SP_ADDR are not
> (sufficiently) aligned in their definition, then either we can fix
> these definitions (and that of CONFIG_SYS_MALLOC_F_LEN too, while we
> are at it), or, if we can only fix this at run time, then this should be
> done where the stack pointer is altered, in the crt0.S file or whatever
> its equivalent is for any given architecture, outside the C environment.
>
> But that will have to go in another patch dealing with alignment.

If you can have it so that the stack top equals the global_data
bottom, then we should be OK.

>
> (*)
>
> Indeed board_init_f_get_reserve_size may have to respect some location
> or size alignment for each of the chunks it reserves. Right now, for
> instance, GD is aligned to a 16-byte boundary, and the malloc arena is
> aligned by virtue of the rounded value of CONFIG_SYS_MALLOC_F_LEN.
>
> And yes, should some new reservation be made beside GD and the malloc
> arena, it might require some specific alignment not dealt with so far;
> for instance, we may need to reserve some 4KB-aligned chunk for memory
> management purposes, or whatever, and there is no guarantee that the
> original stack is 4KB-aligned.
>
> In that light, ulong board_init_f_get_reserve_size(void) does not
> permit controlled alignment, whereas ulong board_init_f_reserve(ulong
> top) does, since we can round 'top' down to any value we like.
>
> Note, however, that it will not simplify assembly code: it will turn a
> subtraction from sp into an assignment to sp, which is not simpler, and
> it will add an assignment to whatever register represents the first
> argument, since we'll turn a (void) function into a (ulong top)
> function, so all in all, it will add one assembly instruction with
> respect to the 'ulong board_init_f_get_reserve_size(void)' approach.

True, but now we are just passing values around rather than doing
arithmetic in assembler.

>
>> > +       /* set GD unless architecture did it already */
>> > +#if !defined(CONFIG_X86) && !defined(CONFIG_ARM)
>> >         arch_setup_gd(gd_ptr);
>>
>> Why does this not work for ARM?
>
> See above, but note that whatever the architecture, gd will be useable
> after this call, so :
>
>> > +#endif
>> > +       /* next alloc will be higher by one GD plus 16-byte alignment */
>> > +       base += roundup(sizeof(struct global_data), 16);
>> > +
>> > +       /*
>> > +        * record malloc arena start
>> > +        */
>> >
>> >  #if defined(CONFIG_SYS_MALLOC_F)
>> > -       top -= CONFIG_SYS_MALLOC_F_LEN;
>> > -       gd->malloc_base = top;
>> > +       /* go down one malloc arena */
>> > +       gd->malloc_base = base;
>>
>> gd_ptr?
>
> gd works at this point, and I want to avoid any aliasing issue.

I don't really understand that, but if you want to use gd I think it
would be worth a one-line comment.

>
>> Regards,
>> Simon
>
> Thanks for your comments! I'll turn
>
>         ulong board_init_f_get_reserve_size(void)
>
> into
>
>         ulong board_init_f_get_reserve(ulong top)
>
> So that arbitrary alignments will be possible, and I will move the r9
> fix into its own patch.

OK sounds good.

>
> Amicalement,
> --
> Albert.