[U-Boot] Initializing global_data on SuperH before board_init_f() ?

Hello Thomas,

On 08/16/2017 12:07 AM, Thomas Petazzoni wrote:
> Hello,
> 
> As you probably noticed with the few patches I sent late July, I am
> porting U-Boot to an old SH7786 platform.

nice, as time passes by, more and more U-Boot/SH users appear, now
there are two of us :)

Are you going to upstream the board support?

> As part of this effort, I stumbled across a bug: the global_data
> structure is not initialized to zero by the SuperH architecture
> code before calling board_init_f().

Right, there is such a problem. Can you reconfirm that you point
out a problem of garbage in global data _before_ relocation (from
board_init_f() call to relocate_code() call)?

> The SuperH architecture code defines the global data in
> arch/sh/lib/start.S:
> 
>         mov.l   ._gd_init, r13          /* global data */
> [...]
>         mov.l   ._sh_generic_init, r0
>         jsr     @r0
> [...]
> ._gd_init:              .long   (_start - GENERATED_GBL_DATA_SIZE)
> ._sh_generic_init:      .long   board_init_f
> 
> So basically, it makes r13 points to the global data (which is expected
> on this architecture), and then calls board_init_f().
> 
> Hence, we enter board_init_f() with global_data uninitialized, which
> have caused me quite some troubles, as I was seeing semi-random
> behavior: in various places, we test if a pointer in global_data is
> NULL or not to decide to do something (or not).

It'd be good to get a list of all such cases.

> This obviously goes really bad when global_data contains garbage.
> Should we put global_data within the .bss section, so that it gets
> zero-initialized automatically?

It is possible, but it will result in wasted space (140 bytes) in
.bss section after relocation, because gd will be reassigned to
point to another area outside of the relocated data, and .bss is
relocated.

By the way IMHO generally it's a good idea to have global data in
.bss, I wonder why none of supported arch does it, if I'm not mistaken
blackfin followed that approach before the arch was removed.

It will require to spend some time on development and testing to 
implement this approach in a generic and shareable with other archs
way.

> Should we zero-initialize it explicitly?

From my point of view this is the simplest and the most preferable
change, please find a change below, can you test that it works?

> I've currently worked-around the problem by adding a memset() to zero
> of the global_data at the beginning of board_init_f(), but I'd prefer
> to find an upstreamable fix.
> 

Other options:

1) board_init_f_alloc_reserve() / board_init_f_init_reserve() called
   from start.S, the functions are too heavyweight IMHO, because it is
   possible to avoid them, and the functions require and do unnecessary 
   things on SH; I can share with you a (fragile) implementation, if
   you ask,

2) introduce a SH specific startup function instead of board_init_f(),
   do board_init_f_alloc_reserve() / board_init_f_init_reserve()
   from it, again it is too heavy, but it's a movement from asm to C,

3) leave a zero_global_data() hook in board_init_f() for SH, I've
   noticed your CONFIG_SYS_GENERIC_GLOBAL_DATA removal, it makes sense,
   and I found that on my SH4 board (IODATA Landisk which is still
   found on second hand markets, board support is not sent to U-Boot
   inclusion, but I can do it, if anybody) I enable the option, that's
   why I've missed the bug reported by you,

4) wipe pre-relocated global data space, that's the optimal change
   among other ones, please test my implementation:

----8<----
----8<----

--
With best wishes,
Vladimir

Hello,

On Thu, 17 Aug 2017 21:30:34 +0300, Vladimir Zapolskiy wrote:

> > As you probably noticed with the few patches I sent late July, I am
> > porting U-Boot to an old SH7786 platform.  
> 
> nice, as time passes by, more and more U-Boot/SH users appear, now
> there are two of us :)

He, yes :-)

> Are you going to upstream the board support?

No, because it's a custom, vendor-specific/internal board. But I intend
to upstream everything I can except the board specific bits.

> > As part of this effort, I stumbled across a bug: the global_data
> > structure is not initialized to zero by the SuperH architecture
> > code before calling board_init_f().  
> 
> Right, there is such a problem. Can you reconfirm that you point
> out a problem of garbage in global data _before_ relocation (from
> board_init_f() call to relocate_code() call)?

I'm not sure what you mean here. One problem I had for example was that:

static int reserve_board(void)
{
        if (!gd->bd) {

gd->bd was not NULL, so reserve_board() assumed that gd->bd was a
correct pointer... except it was pointing to garbage.

I also had a similar problem earlier in fdtdec_setup().

> > Hence, we enter board_init_f() with global_data uninitialized, which
> > have caused me quite some troubles, as I was seeing semi-random
> > behavior: in various places, we test if a pointer in global_data is
> > NULL or not to decide to do something (or not).  
> 
> It'd be good to get a list of all such cases.

See above for two examples.

> 1) board_init_f_alloc_reserve() / board_init_f_init_reserve() called
>    from start.S, the functions are too heavyweight IMHO, because it is
>    possible to avoid them, and the functions require and do unnecessary 
>    things on SH; I can share with you a (fragile) implementation, if
>    you ask,

I really think this approach is the right one. I'm not sure why you say
they do unnecessary things on SH: board_init_f_alloc_reserve() only
adjusts the "top" value, i.e 2 calculations, and
board_init_f_init_reserve() only zero-initialize it.

These two functions are used on many other architectures, and having SH
be more similar to other architectures (ARM, x86) is good for
maintenance.

Those functions are only executed once at boot time, so it's not like
we need to micro-optimize this code sequence.

> 2) introduce a SH specific startup function instead of board_init_f(),
>    do board_init_f_alloc_reserve() / board_init_f_init_reserve()
>    from it, again it is too heavy, but it's a movement from asm to C,
> 
> 3) leave a zero_global_data() hook in board_init_f() for SH, I've
>    noticed your CONFIG_SYS_GENERIC_GLOBAL_DATA removal, it makes sense,
>    and I found that on my SH4 board (IODATA Landisk which is still
>    found on second hand markets, board support is not sent to U-Boot
>    inclusion, but I can do it, if anybody) I enable the option, that's
>    why I've missed the bug reported by you,
> 
> 4) wipe pre-relocated global data space, that's the optimal change
>    among other ones, please test my implementation:
> 
> ----8<----
> diff --git a/arch/sh/lib/start.S b/arch/sh/lib/start.S
> index 37d38d5fb849..e126a39761ca 100644
> --- a/arch/sh/lib/start.S
> +++ b/arch/sh/lib/start.S
> @@ -46,6 +46,11 @@ _start:
>  	bf	3b
>  
>  	mov.l	._gd_init, r13		/* global data */
> +	mov.l	._reloc_dst, r4
> +4:	mov.l	r1, @-r4
> +	cmp/hs	r4, r13
> +	bf	4b
> +

This indeed should solve the problem, though I'm not able to test right
now, as I don't have access to the board and JTAG right now. I'll get
back to you when I've been able to test.

Thanks,

Thomas