Initializing global_data on SuperH before board_init_f() ?

Hello,

As you probably noticed with the few patches I sent late July, I am porting U-Boot to an old SH7786 platform. 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().

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). 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? Should we zero-initialize it explicitly?

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.

Thanks!

Thomas Petazzoni

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Hi Thomas,

I am not sure how SuperH allocates the space for global data but typically the following two function takes care of allocating and zeroing global data(at least for arm):

In file common/init/board_init.c
board_init_f_alloc_reserve()
board_init_f_init_reserve()

May be, using these two functions might solve your problem.

Thanks and regards,
Lokesh

Hello,

Seems like a good idea indeed, I hadn't noticed those functions. If I understand correctly, and after looking at how a few architectures do, they:

- Initialize the stack pointer just below the U-Boot entry point

- Call board_init_f_alloc_reserve(), to allocate enough space for the global data on the stack

- Call board_init_f_init_reserve() to zero initialize it

I guess the SH code can be adapted to use this logic, I'll have a look into that. Could take a while though, since I'm not fluent in SH assembly.

Thanks for the hint!

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Hello Thomas,

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?

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)?

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

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.

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?

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

index 37d38d5fb849..e126a39761ca 100644
@@ -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

Hello,

He, yes :-)

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

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

See above for two examples.

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.

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com