[U-Boot] [PATCH] board_f: save "malloc_base" from zeroing in case of CONFIG_SYS_MALLOC_F_LEN

[U-Boot] [PATCH] board_f: save "malloc_base" from zeroing in case of CONFIG_SYS_MALLOC_F_LEN

[Alexey Brodkin]

In case of CONFIG_SYS_MALLOC_F_LEN "malloc_base" is used for early
start-up code and is set very early, typically in "start.S" or "crt1.S".

In current implementation in case of CONFIG_SYS_GENERIC_GLOBAL_DATA all
global data gets zeroed on "board_init_f" entry. But by that time
"malloc_base" could have been set already, which means it will be zeroed
and subsequent C-code will be executed improperly (if executed at all -
if there's no memory mapped to 0 or it is read-only then on some arches
there will be an exception and others will quetly die).

To work-around described situation we just need to make sure
"malloc_base" is saved prior zeroing global data and recovered
afterwards.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@ti.com>
Cc: Masahiro Yamada <yamada.m@jp.panasonic.com>
---
 common/board_f.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/common/board_f.c b/common/board_f.c
index 3a4b32c..ebdba0e 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -999,6 +999,9 @@ static init_fnc_t init_sequence_f[] = {
 void board_init_f(ulong boot_flags)
 {
 #ifdef CONFIG_SYS_GENERIC_GLOBAL_DATA
+#ifdef CONFIG_SYS_MALLOC_F_LEN
+	int malloc_base;
+#endif
 	/*
 	 * For some archtectures, global data is initialized and used before
 	 * calling this function. The data should be preserved. For others,
@@ -1009,12 +1012,25 @@ void board_init_f(ulong boot_flags)
 
 	gd = &data;
 
+#ifdef CONFIG_SYS_MALLOC_F_LEN
+	/*
+	 * "malloc_base" is supposed to be set in the very beginning of start-up
+	 * code (start.S or crt0.S), now we need to preserve it from zeroing.
+	 */
+	malloc_base = gd->malloc_base;
+#endif
+
 	/*
 	 * Clear global data before it is accessed at debug print
 	 * in initcall_run_list. Otherwise the debug print probably
 	 * get the wrong vaule of gd->have_console.
 	 */
 	zero_global_data();
+
+#ifdef CONFIG_SYS_MALLOC_F_LEN
+	/* Restore "malloc_base" value */
+	gd->malloc_base = malloc_base;
+#endif
 #endif
 
 	gd->flags = boot_flags;
-- 
2.1.0

[U-Boot] [PATCH] board_f: save "malloc_base" from zeroing in case of CONFIG_SYS_MALLOC_F_LEN

[Simon Glass]

Hi Alexey,

On 19 January 2015 at 10:55, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
>
> In case of CONFIG_SYS_MALLOC_F_LEN "malloc_base" is used for early
> start-up code and is set very early, typically in "start.S" or "crt1.S".
>
> In current implementation in case of CONFIG_SYS_GENERIC_GLOBAL_DATA all
> global data gets zeroed on "board_init_f" entry. But by that time
> "malloc_base" could have been set already, which means it will be zeroed
> and subsequent C-code will be executed improperly (if executed at all -
> if there's no memory mapped to 0 or it is read-only then on some arches
> there will be an exception and others will quetly die).
>
> To work-around described situation we just need to make sure
> "malloc_base" is saved prior zeroing global data and recovered
> afterwards.
>
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@ti.com>
> Cc: Masahiro Yamada <yamada.m@jp.panasonic.com>
> ---
>  common/board_f.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index 3a4b32c..ebdba0e 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -999,6 +999,9 @@ static init_fnc_t init_sequence_f[] = {
>  void board_init_f(ulong boot_flags)
>  {
>  #ifdef CONFIG_SYS_GENERIC_GLOBAL_DATA
> +#ifdef CONFIG_SYS_MALLOC_F_LEN
> +       int malloc_base;
> +#endif
>         /*
>          * For some archtectures, global data is initialized and used before
>          * calling this function. The data should be preserved. For others,
> @@ -1009,12 +1012,25 @@ void board_init_f(ulong boot_flags)
>
>         gd = &data;
>
> +#ifdef CONFIG_SYS_MALLOC_F_LEN
> +       /*
> +        * "malloc_base" is supposed to be set in the very beginning of start-up
> +        * code (start.S or crt0.S), now we need to preserve it from zeroing.
> +        */
> +       malloc_base = gd->malloc_base;
> +#endif
> +
>         /*
>          * Clear global data before it is accessed at debug print
>          * in initcall_run_list. Otherwise the debug print probably
>          * get the wrong vaule of gd->have_console.
>          */
>         zero_global_data();
> +
> +#ifdef CONFIG_SYS_MALLOC_F_LEN
> +       /* Restore "malloc_base" value */
> +       gd->malloc_base = malloc_base;
> +#endif
>  #endif
>
>         gd->flags = boot_flags;
> --
> 2.1.0
>

CONFIG_SYS_GENERIC_GLOBAL_DATA seems to be mis-named, but in any case
it must not be used with CONFIG_SYS_MALLOC_F_LEN. We are trying to get
rid of all these cases of multiple global_data structures. It should
be set up in start.S and not anywhere else.

Better would be:

 #ifdef CONFIG_SYS_GENERIC_GLOBAL_DATA
+#ifdef CONFIG_SYS_MALLOC_F_LEN
+#error "CONFIG_SYS_GENERIC_GLOBAL_DATA is deprecated - please remove
use of this if you want to use early malloc()
+#endif

Regards,
Simon

[U-Boot] [PATCH] board_f: save "malloc_base" from zeroing in case of CONFIG_SYS_MALLOC_F_LEN

[Albert ARIBAUD]

Hello Alexey,

On Mon, 19 Jan 2015 20:55:03 +0300, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> In case of CONFIG_SYS_MALLOC_F_LEN "malloc_base" is used for early
> start-up code and is set very early, typically in "start.S" or "crt1.S".

There is no "crt1.S" in U-Boot. Did you mean "crt0.S"?

> In current implementation in case of CONFIG_SYS_GENERIC_GLOBAL_DATA all
> global data gets zeroed on "board_init_f" entry. But by that time
> "malloc_base" could have been set already, which means it will be zeroed
> and subsequent C-code will be executed improperly (if executed at all -
> if there's no memory mapped to 0 or it is read-only then on some arches
> there will be an exception and others will quetly die).
> 
> To work-around described situation we just need to make sure
> "malloc_base" is saved prior zeroing global data and recovered
> afterwards.

Keeping data from being zeroed etc is usually done through GD. Could
malloc_base be placed there instead of creating a specific exemption
for it?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] board_f: save "malloc_base" from zeroing in case of CONFIG_SYS_MALLOC_F_LEN

[Alexey Brodkin]

Hi Albert,

On Tue, 2015-01-20 at 08:07 +0100, Albert ARIBAUD wrote:
> Hello Alexey,
> 
> On Mon, 19 Jan 2015 20:55:03 +0300, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
> > In case of CONFIG_SYS_MALLOC_F_LEN "malloc_base" is used for early
> > start-up code and is set very early, typically in "start.S" or "crt1.S".
> 
> There is no "crt1.S" in U-Boot. Did you mean "crt0.S"?

Indeed I meant "crt0.S"

> > In current implementation in case of CONFIG_SYS_GENERIC_GLOBAL_DATA all
> > global data gets zeroed on "board_init_f" entry. But by that time
> > "malloc_base" could have been set already, which means it will be zeroed
> > and subsequent C-code will be executed improperly (if executed at all -
> > if there's no memory mapped to 0 or it is read-only then on some arches
> > there will be an exception and others will quetly die).
> > 
> > To work-around described situation we just need to make sure
> > "malloc_base" is saved prior zeroing global data and recovered
> > afterwards.
> 
> Keeping data from being zeroed etc is usually done through GD. Could
> malloc_base be placed there instead of creating a specific exemption
> for it?

Unfortunately I didn't understand your suggestion here.
"malloc_base" is already in global data structure.

But the point is global data structure also requires zeroing sometime on
early start-up. This is required to make sure we don't have any garbage
in GD (for example left-overs from lower-level bootloader or previously
executed kernel etc).

So other option is to zero GD earlier in start-up code. This is
essentially doable but it will be done on per-architecture or even
per-CPU basis in their "start.S" - which means we'll have duplication of
the same functionality and maintenance will be difficult then.

Probably I just didn't get you point so then could you please clarify
what did you mean.

-Alexey

[U-Boot] [PATCH] board_f: save "malloc_base" from zeroing in case of CONFIG_SYS_MALLOC_F_LEN

[Albert ARIBAUD]

Hello Alexey,

On Tue, 20 Jan 2015 13:06:57 +0000, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> Hi Albert,
> 
> On Tue, 2015-01-20 at 08:07 +0100, Albert ARIBAUD wrote:
> > Hello Alexey,
> > 
> > On Mon, 19 Jan 2015 20:55:03 +0300, Alexey Brodkin
> > <Alexey.Brodkin@synopsys.com> wrote:
> > > In case of CONFIG_SYS_MALLOC_F_LEN "malloc_base" is used for early
> > > start-up code and is set very early, typically in "start.S" or "crt1.S".
> > 
> > There is no "crt1.S" in U-Boot. Did you mean "crt0.S"?
> 
> Indeed I meant "crt0.S"
> 
> > > In current implementation in case of CONFIG_SYS_GENERIC_GLOBAL_DATA all
> > > global data gets zeroed on "board_init_f" entry. But by that time
> > > "malloc_base" could have been set already, which means it will be zeroed
> > > and subsequent C-code will be executed improperly (if executed at all -
> > > if there's no memory mapped to 0 or it is read-only then on some arches
> > > there will be an exception and others will quetly die).
> > > 
> > > To work-around described situation we just need to make sure
> > > "malloc_base" is saved prior zeroing global data and recovered
> > > afterwards.
> > 
> > Keeping data from being zeroed etc is usually done through GD. Could
> > malloc_base be placed there instead of creating a specific exemption
> > for it?
> 
> Unfortunately I didn't understand your suggestion here.
> "malloc_base" is already in global data structure.

My bad; as you had not mentioned GD (or I'd missed it), I thought you
were referring to a standalone variable. I should have grepped, which I
have done now.

> But the point is global data structure also requires zeroing sometime on
> early start-up. This is required to make sure we don't have any garbage
> in GD (for example left-overs from lower-level bootloader or previously
> executed kernel etc).
> 
> So other option is to zero GD earlier in start-up code. This is
> essentially doable but it will be done on per-architecture or even
> per-CPU basis in their "start.S" - which means we'll have duplication of
> the same functionality and maintenance will be difficult then.

Right now, yes, we'd have to duplicate this a bit, but we can minimize
that by doing the GD init in a single routine called from each start.S;
this would only add one "bl" line per start.S (and hopefully, it can be
added the same way in each start.S and we can still eventually merge
these start.S files together).

> Probably I just didn't get you point so then could you please clarify
> what did you mean.

No, I just hadn't completely done my homework.

> -Alexey

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] board_f: save "malloc_base" from zeroing in case of CONFIG_SYS_MALLOC_F_LEN

[Simon Glass]

Hi Alexey,

On 20 January 2015 at 06:06, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
> Hi Albert,
>
> On Tue, 2015-01-20 at 08:07 +0100, Albert ARIBAUD wrote:
>> Hello Alexey,
>>
>> On Mon, 19 Jan 2015 20:55:03 +0300, Alexey Brodkin
>> <Alexey.Brodkin@synopsys.com> wrote:
>> > In case of CONFIG_SYS_MALLOC_F_LEN "malloc_base" is used for early
>> > start-up code and is set very early, typically in "start.S" or "crt1.S".
>>
>> There is no "crt1.S" in U-Boot. Did you mean "crt0.S"?
>
> Indeed I meant "crt0.S"
>
>> > In current implementation in case of CONFIG_SYS_GENERIC_GLOBAL_DATA all
>> > global data gets zeroed on "board_init_f" entry. But by that time
>> > "malloc_base" could have been set already, which means it will be zeroed
>> > and subsequent C-code will be executed improperly (if executed at all -
>> > if there's no memory mapped to 0 or it is read-only then on some arches
>> > there will be an exception and others will quetly die).
>> >
>> > To work-around described situation we just need to make sure
>> > "malloc_base" is saved prior zeroing global data and recovered
>> > afterwards.
>>
>> Keeping data from being zeroed etc is usually done through GD. Could
>> malloc_base be placed there instead of creating a specific exemption
>> for it?
>
> Unfortunately I didn't understand your suggestion here.
> "malloc_base" is already in global data structure.
>
> But the point is global data structure also requires zeroing sometime on
> early start-up. This is required to make sure we don't have any garbage
> in GD (for example left-overs from lower-level bootloader or previously
> executed kernel etc).
>
> So other option is to zero GD earlier in start-up code. This is
> essentially doable but it will be done on per-architecture or even
> per-CPU basis in their "start.S" - which means we'll have duplication of
> the same functionality and maintenance will be difficult then.

This should be done before board_init_f(). See for example this patch:

http://patchwork.ozlabs.org/patch/421210/

There is no need for it to be SOC- or even arch-specific. We can clean
this up fairly soon.

But we should not set up global_data in board_init_f(). This is a
hang-over from previous code. It needs to be removed. Perhaps in
addition to my comments above you could add a comment that the code at
the top of board_init_f() is deprecated and will soon be removed?

Regards,
Simon