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

[Vladimir Zapolskiy]

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<----
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
+
 	mov.l	._stack_init, r15	/* stack */
 
 	mov.l	._sh_generic_init, r0
----8<----

--
With best wishes,
Vladimir