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

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

[Thomas Petazzoni]

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

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

[Lokesh Vutla]

Hi Thomas,

On Wednesday 16 August 2017 02:37 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. 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 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

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

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

[Thomas Petazzoni]

Hello,

On Wed, 16 Aug 2017 08:09:06 +0530, Lokesh Vutla wrote:

> > Should we put global_data within the .bss section, so that it gets
> > zero-initialized automatically? Should we zero-initialize it explicitly?  
> 
> 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.

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

[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

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

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com