[U-Boot] [PATCH V7] POST cleanup.

[Wolfgang Denk]

Dear Michael,

Ping!

In message <20101026210959.518031365D8 at gemini.denx.de> I wrote:
> Dear Michael Zaidman,
> 
> In message <d520c6ef298416a03789ebfa4e05e257b5331693.1284965175.git.michael.zaidman at gmail.com> you wrote:
> > - Revives POST for blackfin arch;
> > - Removes redundant code:
> >      arch/blackfin/lib/post.c
> >      arch/powerpc/cpu/ppc4xx/commproc.c
> >      arch/powerpc/cpu/mpc512x/common.c
> > - fixes up the post_word_{load|store} usage.
> 
> Unfortunately it turns out that the code now contains a few nasty
> bugs...
> 
> ...
> >  #define CONFIG_SYS_GBL_DATA_OFFSET   (CONFIG_SYS_INIT_RAM_END - CONFIG_SYS_GBL_DATA_SIZE)
> > -#define CONFIG_SYS_POST_WORD_ADDR    (CONFIG_SYS_GBL_DATA_OFFSET - 0x4)
> > -#define CONFIG_SYS_INIT_SP_OFFSET    CONFIG_SYS_POST_WORD_ADDR
> > +#define CONFIG_SYS_INIT_SP_OFFSET    (CONFIG_SYS_GBL_DATA_OFFSET - 0x4)
> 
> This is a seriously broken design, as it sneaks in storage for a
> variable in a storage location where it is not expected.
> 
> The "official" layout is that we have CONFIG_SYS_INIT_RAM_BYTES
> available; the top CONFIG_SYS_GBL_DATA_SIZE bytes (now
> GENERATED_GBL_DATA_SIZE) are used for global data, and the part below
> is used for the stack.  No other room is reserved there.
> 
> Shifting down the stack by 4 bytes as it's done here causes that the
> stack is not correctly aligned any more, which may cause really nasty
> subsequent errors.
> 
> But it's even worse.
> 
> > diff --git a/include/configs/mpc5121-common.h b/include/configs/mpc5121-common.h
> > index 96fab20..afae1ab 100644
> > --- a/include/configs/mpc5121-common.h
> > +++ b/include/configs/mpc5121-common.h
> ...
> > -#define CONFIG_SYS_POST_WORD_ADDR	(CONFIG_SYS_GBL_DATA_OFFSET - 0x4)
> > -#define CONFIG_SYS_INIT_SP_OFFSET	CONFIG_SYS_POST_WORD_ADDR
> > +#define CONFIG_SYS_INIT_SP_OFFSET	(CONFIG_SYS_GBL_DATA_OFFSET - 0x4)
> 
> There the same is done, but what happens actually?
> 
> Have a look how the stack setup gets implemented in
> "arch/powerpc/cpu/mpc512x/start.S":
> 
> ...
> 244 in_flash:
> 245         lis     r1, (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET)@h
> 246         ori     r1, r1, (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET)@l
> 247
> 248         li      r0, 0           /* Make room for stack frame header and */
> 249         stwu    r0, -4(r1)      /* clear final stack frame so that      */
> 250         stwu    r0, -4(r1)      /* stack backtraces terminate cleanly   */
> ...
> 
> As you can see, the code does not use CONFIG_SYS_INIT_SP_OFFSET at
> all; instead it performs a calculation which should be redundant, but
> in the current code it means that the location of the POST_WORD is
> right in the initial stack.
> 
> I did not check if the code for other processors has similar issues.
> 
> 
> "Reserving" private storage like that is bad, as other involved
> parties probably have no knowledge of such a private reservation.
> 
> 
> Why do we not simply reserve a word in the global data structure instead?
> 
> 
> This bug needs pretty urgent fixing.
> 
> 
> Best regards,
> 
> Wolfgang Denk

Viele Grüße,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If a train station is a place where a train stops,
                                           then what's a workstation?