| Message ID | 1421690103-29091-1-git-send-email-abrodkin@synopsys.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
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
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,
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
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,
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
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;
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(+)