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

[Albert ARIBAUD]

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

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.

> 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.

(*)

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.

> > +       /* 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.

> 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.

Amicalement,
-- 
Albert.