[U-Boot] [PATCH V7] POST cleanup.
Wolfgang Denk
wd at denx.de
Tue Oct 26 23:09:59 CEST 2010
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
--
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
I wish Captain Vimes were here. He wouldn't have known what to do
either, but he's got a much better vocabulary to be baffled in.
- Terry Pratchett, _Guards! Guards!_
More information about the U-Boot
mailing list