[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Alex Hornung]

Hi,

I've run into some memory corruption due to an error in the logic used
to allocate the bd (and gd) during board_init of the nios2.


#define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
                                         GENERATED_GBL_DATA_SIZE)
[...]

        gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
[...]
        gd->bd = (bd_t *)(gd+1);        /* At end of global data */
[...]
        mem_malloc_init(CONFIG_SYS_MALLOC_BASE, CONFIG_SYS_MALLOC_LEN);

The relevant points here are that CONFIG_SYS_GBL_DATA_OFFSET is
GENERATED_GBL_DATA_SIZE (80) bytes below the CONFIG_SYS_MALLOC_BASE.

Given that gd is 68 bytes big, now the start of bd is only 12 bytes from
the beginning of the malloc base - but the size of bd is 36 bytes!

In other words, bd and the malloc base overlap, causing memory
corruption in some of the malloc'd memory when some bd elements are
populated. In my case in particular some content of the flash mtd
eraseregions is getting corrupted by the write to bd->bi_ip_addr after
initializing the flash stuff.

I'm not sure how this should be dealt with - I'd think the best approach
here is to change the CONFIG_SYS_GBL_DATA_OFFSET to include some space
for the bd, or malloc'ing the bd.

If you let me know which one is the preferred approach, I'll gladly
provide a patch.

Cheers,
Alex Hornung

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Albert ARIBAUD]

Hi Alex,

Le 21/02/2012 00:24, Alex Hornung a ?crit :
> Hi,
>
> I've run into some memory corruption due to an error in the logic used
> to allocate the bd (and gd) during board_init of the nios2.
>
>
> #define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
>                                           GENERATED_GBL_DATA_SIZE)
> [...]
>
>          gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
> [...]
>          gd->bd = (bd_t *)(gd+1);        /* At end of global data */
> [...]
>          mem_malloc_init(CONFIG_SYS_MALLOC_BASE, CONFIG_SYS_MALLOC_LEN);
>
> The relevant points here are that CONFIG_SYS_GBL_DATA_OFFSET is
> GENERATED_GBL_DATA_SIZE (80) bytes below the CONFIG_SYS_MALLOC_BASE.
>
> Given that gd is 68 bytes big, now the start of bd is only 12 bytes from
> the beginning of the malloc base - but the size of bd is 36 bytes!

So GENERATED_GBL_DATA_SIZE is wrong if it was supposed to contain both 
gd and bd, which I suspect is not the case; but if it is supposed to 
only contain a gd, then the definition of CONFIG_SYS_GBL_DATA_OFFSET is 
wrong in that it does not account for gd and bd as it should.

(BTW, what makes GENERATED_GBL_DATA_SIZE different from sizeof(gd_t)?)

> In other words, bd and the malloc base overlap, causing memory
> corruption in some of the malloc'd memory when some bd elements are
> populated. In my case in particular some content of the flash mtd
> eraseregions is getting corrupted by the write to bd->bi_ip_addr after
> initializing the flash stuff.
>
> I'm not sure how this should be dealt with - I'd think the best approach
> here is to change the CONFIG_SYS_GBL_DATA_OFFSET to include some space
> for the bd, or malloc'ing the bd.
>
> If you let me know which one is the preferred approach, I'll gladly
> provide a patch.

Hmm... You have sizeof(bd_t) available, so you could do something like

#define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
                                             sizeof(bd_t) - \
 >                                           GENERATED_GBL_DATA_SIZE)

That would ensure you have space available for a gd and bd.

Amicalement,
-- 
Albert.

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Graeme Russ]

Hi Albert,

On Wed, Feb 29, 2012 at 9:29 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Alex,
>
> Le 21/02/2012 00:24, Alex Hornung a ?crit :
>>
>> Hi,
>>
>> I've run into some memory corruption due to an error in the logic used
>> to allocate the bd (and gd) during board_init of the nios2.
>>
>>
>> #define CONFIG_SYS_GBL_DATA_OFFSET ? ? ?(CONFIG_SYS_MALLOC_BASE - \
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?GENERATED_GBL_DATA_SIZE)
>> [...]
>>
>> ? ? ? ? gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
>> [...]
>> ? ? ? ? gd->bd = (bd_t *)(gd+1); ? ? ? ?/* At end of global data */
>> [...]
>> ? ? ? ? mem_malloc_init(CONFIG_SYS_MALLOC_BASE, CONFIG_SYS_MALLOC_LEN);
>>
>> The relevant points here are that CONFIG_SYS_GBL_DATA_OFFSET is
>> GENERATED_GBL_DATA_SIZE (80) bytes below the CONFIG_SYS_MALLOC_BASE.
>>
>> Given that gd is 68 bytes big, now the start of bd is only 12 bytes from
>> the beginning of the malloc base - but the size of bd is 36 bytes!
>
>
> So GENERATED_GBL_DATA_SIZE is wrong if it was supposed to contain both gd
> and bd, which I suspect is not the case; but if it is supposed to only
> contain a gd, then the definition of CONFIG_SYS_GBL_DATA_OFFSET is wrong in
> that it does not account for gd and bd as it should.

The global data struct only contains a pointer to the board data struct.

IMHO I think the approach taken (but almost all arches) is very errror prone
as it relies on manually laying out gd and bd in memory with bd sitting
immediately above or below gd. In theory, this layout should never be
tampered with, but I still don't like it.

For x86, gd and bd are in BSS after relocation, so there is no need to
hack around them when calculating the heap or stack, but I have a sneaking
suspicion that this could make debugging harder as there is no way to
reliably find the relocation offset as gd is never located at a known
location in memory...

Regards,

Graeme

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Albert ARIBAUD]

Hi Graeme,

Le 28/02/2012 23:39, Graeme Russ a ?crit :
> Hi Albert,
>
> On Wed, Feb 29, 2012 at 9:29 AM, Albert ARIBAUD
> <albert.u.boot@aribaud.net>  wrote:
>> Hi Alex,
>>
>> Le 21/02/2012 00:24, Alex Hornung a ?crit :
>>>
>>> Hi,
>>>
>>> I've run into some memory corruption due to an error in the logic used
>>> to allocate the bd (and gd) during board_init of the nios2.
>>>
>>>
>>> #define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
>>>                                           GENERATED_GBL_DATA_SIZE)
>>> [...]
>>>
>>>          gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
>>> [...]
>>>          gd->bd = (bd_t *)(gd+1);        /* At end of global data */
>>> [...]
>>>          mem_malloc_init(CONFIG_SYS_MALLOC_BASE, CONFIG_SYS_MALLOC_LEN);
>>>
>>> The relevant points here are that CONFIG_SYS_GBL_DATA_OFFSET is
>>> GENERATED_GBL_DATA_SIZE (80) bytes below the CONFIG_SYS_MALLOC_BASE.
>>>
>>> Given that gd is 68 bytes big, now the start of bd is only 12 bytes from
>>> the beginning of the malloc base - but the size of bd is 36 bytes!
>>
>>
>> So GENERATED_GBL_DATA_SIZE is wrong if it was supposed to contain both gd
>> and bd, which I suspect is not the case; but if it is supposed to only
>> contain a gd, then the definition of CONFIG_SYS_GBL_DATA_OFFSET is wrong in
>> that it does not account for gd and bd as it should.
>
> The global data struct only contains a pointer to the board data struct.
>
> IMHO I think the approach taken (but almost all arches) is very errror prone
> as it relies on manually laying out gd and bd in memory with bd sitting
> immediately above or below gd. In theory, this layout should never be
> tampered with, but I still don't like it.
>
> For x86, gd and bd are in BSS after relocation, so there is no need to
> hack around them when calculating the heap or stack, but I have a sneaking
> suspicion that this could make debugging harder as there is no way to
> reliably find the relocation offset as gd is never located at a known
> location in memory...

Duh. I had misread the code... Time for me to go to sleep. :/

For ARM we have gd in r8, which makes things simpler.

Anyway -- this does not affect the fact that GENERATED_GBL_DATA_SIZE 
should be equal to or greater than sizeof(gd_t)+sizeof(bd-t), right?

> Regards,
>
> Graeme

Amicalement,
-- 
Albert.

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Graeme Russ]

Hi Albert,

On Wed, Feb 29, 2012 at 9:55 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Graeme,
>
> Le 28/02/2012 23:39, Graeme Russ a ?crit :
>
>> Hi Albert,
>>
>> On Wed, Feb 29, 2012 at 9:29 AM, Albert ARIBAUD
>> <albert.u.boot@aribaud.net> ?wrote:
>>>
>>> Hi Alex,
>>>
>>> Le 21/02/2012 00:24, Alex Hornung a ?crit :
>>>>
>>>>
>>>> Hi,
>>>>
>>>> I've run into some memory corruption due to an error in the logic used
>>>> to allocate the bd (and gd) during board_init of the nios2.
>>>>
>>>>
>>>> #define CONFIG_SYS_GBL_DATA_OFFSET ? ? ?(CONFIG_SYS_MALLOC_BASE - \
>>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?GENERATED_GBL_DATA_SIZE)
>>>> [...]
>>>>
>>>> ? ? ? ? gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
>>>> [...]
>>>> ? ? ? ? gd->bd = (bd_t *)(gd+1); ? ? ? ?/* At end of global data */
>>>> [...]
>>>> ? ? ? ? mem_malloc_init(CONFIG_SYS_MALLOC_BASE, CONFIG_SYS_MALLOC_LEN);
>>>>
>>>> The relevant points here are that CONFIG_SYS_GBL_DATA_OFFSET is
>>>> GENERATED_GBL_DATA_SIZE (80) bytes below the CONFIG_SYS_MALLOC_BASE.
>>>>
>>>> Given that gd is 68 bytes big, now the start of bd is only 12 bytes from
>>>> the beginning of the malloc base - but the size of bd is 36 bytes!
>>>
>>>
>>>
>>> So GENERATED_GBL_DATA_SIZE is wrong if it was supposed to contain both gd
>>> and bd, which I suspect is not the case; but if it is supposed to only
>>> contain a gd, then the definition of CONFIG_SYS_GBL_DATA_OFFSET is wrong
>>> in
>>> that it does not account for gd and bd as it should.
>>
>>
>> The global data struct only contains a pointer to the board data struct.
>>
>> IMHO I think the approach taken (but almost all arches) is very errror
>> prone
>> as it relies on manually laying out gd and bd in memory with bd sitting
>> immediately above or below gd. In theory, this layout should never be
>> tampered with, but I still don't like it.
>>
>> For x86, gd and bd are in BSS after relocation, so there is no need to
>> hack around them when calculating the heap or stack, but I have a sneaking
>> suspicion that this could make debugging harder as there is no way to
>> reliably find the relocation offset as gd is never located at a known
>> location in memory...
>
>
> Duh. I had misread the code... Time for me to go to sleep. :/
>
> For ARM we have gd in r8, which makes things simpler.

Of course :) - x86 now has it in FS so it should be easy to find

> Anyway -- this does not affect the fact that GENERATED_GBL_DATA_SIZE should
> be equal to or greater than sizeof(gd_t)+sizeof(bd-t), right?

No - GENERATED_GBL_DATA_SIZE should be sizeof(gd_t)

The space reserved between U-Boot and the heap needs to be sizeof(gd_t) +
sizeof(bd-t) (on the delicate proviso that only gd and bd live there, and
that gd and bd are immediately next to each other)

Regards,

Graeme

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Albert ARIBAUD]

Le 29/02/2012 00:20, Graeme Russ a ?crit :
> Hi Albert,
>
> On Wed, Feb 29, 2012 at 9:55 AM, Albert ARIBAUD
> <albert.u.boot@aribaud.net>  wrote:
>> Hi Graeme,
>>
>> Le 28/02/2012 23:39, Graeme Russ a ?crit :
>>
>>> Hi Albert,
>>>
>>> On Wed, Feb 29, 2012 at 9:29 AM, Albert ARIBAUD
>>> <albert.u.boot@aribaud.net>    wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> Le 21/02/2012 00:24, Alex Hornung a ?crit :
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> I've run into some memory corruption due to an error in the logic used
>>>>> to allocate the bd (and gd) during board_init of the nios2.
>>>>>
>>>>>
>>>>> #define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
>>>>>                                           GENERATED_GBL_DATA_SIZE)
>>>>> [...]
>>>>>
>>>>>          gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
>>>>> [...]
>>>>>          gd->bd = (bd_t *)(gd+1);        /* At end of global data */
>>>>> [...]
>>>>>          mem_malloc_init(CONFIG_SYS_MALLOC_BASE, CONFIG_SYS_MALLOC_LEN);
>>>>>
>>>>> The relevant points here are that CONFIG_SYS_GBL_DATA_OFFSET is
>>>>> GENERATED_GBL_DATA_SIZE (80) bytes below the CONFIG_SYS_MALLOC_BASE.
>>>>>
>>>>> Given that gd is 68 bytes big, now the start of bd is only 12 bytes from
>>>>> the beginning of the malloc base - but the size of bd is 36 bytes!
>>>>
>>>>
>>>>
>>>> So GENERATED_GBL_DATA_SIZE is wrong if it was supposed to contain both gd
>>>> and bd, which I suspect is not the case; but if it is supposed to only
>>>> contain a gd, then the definition of CONFIG_SYS_GBL_DATA_OFFSET is wrong
>>>> in
>>>> that it does not account for gd and bd as it should.
>>>
>>>
>>> The global data struct only contains a pointer to the board data struct.
>>>
>>> IMHO I think the approach taken (but almost all arches) is very errror
>>> prone
>>> as it relies on manually laying out gd and bd in memory with bd sitting
>>> immediately above or below gd. In theory, this layout should never be
>>> tampered with, but I still don't like it.
>>>
>>> For x86, gd and bd are in BSS after relocation, so there is no need to
>>> hack around them when calculating the heap or stack, but I have a sneaking
>>> suspicion that this could make debugging harder as there is no way to
>>> reliably find the relocation offset as gd is never located at a known
>>> location in memory...
>>
>>
>> Duh. I had misread the code... Time for me to go to sleep. :/
>>
>> For ARM we have gd in r8, which makes things simpler.
>
> Of course :) - x86 now has it in FS so it should be easy to find
>
>> Anyway -- this does not affect the fact that GENERATED_GBL_DATA_SIZE should
>> be equal to or greater than sizeof(gd_t)+sizeof(bd-t), right?
>
> No - GENERATED_GBL_DATA_SIZE should be sizeof(gd_t)
>
> The space reserved between U-Boot and the heap needs to be sizeof(gd_t) +
> sizeof(bd-t) (on the delicate proviso that only gd and bd live there, and
> that gd and bd are immediately next to each other)

Ok, so :

1. do you know why here gd = 68 bytes and GENERATED_GBL_DATA_SIZE is 80?

2. luckily for my ego, my proposal was actually correct when I suggested 
the following, right?

#define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
                                             sizeof(bd_t) - \
                                             GENERATED_GBL_DATA_SIZE)

> Regards,
>
> Graeme

Amicalement,
-- 
Albert.

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Graeme Russ]

Hi Albert,

On Wed, Feb 29, 2012 at 10:24 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Le 29/02/2012 00:20, Graeme Russ a ?crit :
>
>> Hi Albert,
>>

>> No - GENERATED_GBL_DATA_SIZE should be sizeof(gd_t)
>>
>> The space reserved between U-Boot and the heap needs to be sizeof(gd_t) +
>> sizeof(bd-t) (on the delicate proviso that only gd and bd live there, and
>> that gd and bd are immediately next to each other)
>
>
> Ok, so :
>
> 1. do you know why here gd = 68 bytes and GENERATED_GBL_DATA_SIZE is 80?

It gets padded:

	/* Round up to make sure size gives nice stack alignment */
	DEFINE(GENERATED_GBL_DATA_SIZE,
		(sizeof(struct global_data) + 15) & ~15);

	DEFINE(GENERATED_BD_INFO_SIZE,
		(sizeof(struct bd_info) + 15) & ~15);


> 2. luckily for my ego, my proposal was actually correct when I suggested the
> following, right?
>
>
> #define CONFIG_SYS_GBL_DATA_OFFSET ? ? ?(CONFIG_SYS_MALLOC_BASE - \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?sizeof(bd_t) - \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?GENERATED_GBL_DATA_SIZE)

Probably, but I'm really not sure...

And this is why I dislike the implementation - You have to do all sorts of
weird calucations to put things in the right place when, in fact, the
location of gd and bd in memory is totally irrelavent.

Ow, ouch! - And that padding makes things more fun - The memory layout is

U-Boot | gd | pad | bd | pad | heap

So no, your calculation is not right - It should be:

#define CONFIG_SYS_GBL_DATA_OFFSET ? ? ?(CONFIG_SYS_MALLOC_BASE - \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?GENERATED_BD_INFO_SIZE - \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?GENERATED_GBL_DATA_SIZE)

Regards,

Graeme

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Mike Frysinger]

On Tuesday 28 February 2012 18:32:57 Graeme Russ wrote:
> And this is why I dislike the implementation - You have to do all sorts of
> weird calucations to put things in the right place when, in fact, the
> location of gd and bd in memory is totally irrelavent.

right, that's why i minimized the pain for Blackfin users -- this is all 
handled in the arch's config-pre.h header.  board porters only need to declare 
the size of regions they care about (monitor and heap sizes).

> Ow, ouch! - And that padding makes things more fun - The memory layout is
> 
> U-Boot | gd | pad | bd | pad | heap

fwiw, i documented the Blackfin memory layout:
http://docs.blackfin.uclinux.org/doku.php?id=bootloaders:u-boot:memory-layout
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120229/481c9218/attachment.pgp>

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Graeme Russ]

Hi Mike,

On Thu, Mar 1, 2012 at 6:04 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 28 February 2012 18:32:57 Graeme Russ wrote:
>> And this is why I dislike the implementation - You have to do all sorts of
>> weird calucations to put things in the right place when, in fact, the
>> location of gd and bd in memory is totally irrelavent.
>
> right, that's why i minimized the pain for Blackfin users -- this is all
> handled in the arch's config-pre.h header. ?board porters only need to declare
> the size of regions they care about (monitor and heap sizes).
>
>> Ow, ouch! - And that padding makes things more fun - The memory layout is
>>
>> U-Boot | gd | pad | bd | pad | heap
>
> fwiw, i documented the Blackfin memory layout:
> http://docs.blackfin.uclinux.org/doku.php?id=bootloaders:u-boot:memory-layout

I had a look at this and noticed that you statically allocate locations for
gd and bd (CONFIG_SYS_GBL_DATA_ADDR, CONFIG_SYS_BD_INFO_ADDR)

Considering that:

a) the gd pointer is in a register (P3) and thus easily locatable by a
   debugger, and;
b) the bd pointer is in gd

Is there any reason not to have gd and bd in BSS?

Regards,

Graeme

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Mike Frysinger]

On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
> On Thu, Mar 1, 2012 at 6:04 AM, Mike Frysinger wrote:
> > On Tuesday 28 February 2012 18:32:57 Graeme Russ wrote:
> >> And this is why I dislike the implementation - You have to do all sorts
> >> of weird calucations to put things in the right place when, in fact,
> >> the location of gd and bd in memory is totally irrelavent.
> > 
> > right, that's why i minimized the pain for Blackfin users -- this is all
> > handled in the arch's config-pre.h header.  board porters only need to
> > declare the size of regions they care about (monitor and heap sizes).
> > 
> >> Ow, ouch! - And that padding makes things more fun - The memory layout
> >> is
> >> 
> >> U-Boot | gd | pad | bd | pad | heap
> > 
> > fwiw, i documented the Blackfin memory layout:
> > http://docs.blackfin.uclinux.org/doku.php?id=bootloaders:u-boot:memory-la
> > yout
> 
> I had a look at this and noticed that you statically allocate locations for
> gd and bd (CONFIG_SYS_GBL_DATA_ADDR, CONFIG_SYS_BD_INFO_ADDR)
> 
> Considering that:
> 
> a) the gd pointer is in a register (P3) and thus easily locatable by a
>    debugger, and;
> b) the bd pointer is in gd
> 
> Is there any reason not to have gd and bd in BSS?

in the Blackfin case, most likely not.  we don't do relocation, and the bss is 
cleared long before board_init_f() gets called.  the only reason for allowing 
the config to override would be if someone wanted to put gd/bd into on-chip L1 
data, but i can't imagine this structure being performance critical enough to 
warrant that.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120229/8b14858d/attachment.pgp>

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Graeme Russ]

Hi Mike,

On Thu, Mar 1, 2012 at 9:29 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
>> On Thu, Mar 1, 2012 at 6:04 AM, Mike Frysinger wrote:
>> > On Tuesday 28 February 2012 18:32:57 Graeme Russ wrote:
>> >> And this is why I dislike the implementation - You have to do all sorts
>> >> of weird calucations to put things in the right place when, in fact,
>> >> the location of gd and bd in memory is totally irrelavent.
>> >
>> > right, that's why i minimized the pain for Blackfin users -- this is all
>> > handled in the arch's config-pre.h header. ?board porters only need to
>> > declare the size of regions they care about (monitor and heap sizes).
>> >
>> >> Ow, ouch! - And that padding makes things more fun - The memory layout
>> >> is
>> >>
>> >> U-Boot | gd | pad | bd | pad | heap
>> >
>> > fwiw, i documented the Blackfin memory layout:
>> > http://docs.blackfin.uclinux.org/doku.php?id=bootloaders:u-boot:memory-la
>> > yout
>>
>> I had a look at this and noticed that you statically allocate locations for
>> gd and bd (CONFIG_SYS_GBL_DATA_ADDR, CONFIG_SYS_BD_INFO_ADDR)
>>
>> Considering that:
>>
>> a) the gd pointer is in a register (P3) and thus easily locatable by a
>> ? ?debugger, and;
>> b) the bd pointer is in gd
>>
>> Is there any reason not to have gd and bd in BSS?
>
> in the Blackfin case, most likely not. ?we don't do relocation, and the bss is
> cleared long before board_init_f() gets called. ?the only reason for allowing
> the config to override would be if someone wanted to put gd/bd into on-chip L1
> data, but i can't imagine this structure being performance critical enough to
> warrant that.

I thought as much - I moved gd/bd into BSS for x86 without really thinking
about why everyone else calculates the location of these data structures
around the stack and heap. The longer I think about it, the more I think
that it was not a bad move and that maybe other arches can follow suit as
part of standardising the init sequences

Regards,

Graeme

[U-Boot] [PATCH] nios2: move gd and bd into BSS

[Thomas Chou]

As suggested by Graeme Russ, move gd and bd data structrures
to BSS instead of calculating the locations around the stack
and heap.

CC: Graeme Russ <graeme.russ@gmail.com>
CC: Mike Frysinger <vapier@gentoo.org>
CC: Alex Hornung <alex@alexhornung.com>
Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
For u-boot.

 arch/nios2/lib/board.c          |   13 ++++++-------
 include/configs/nios2-generic.h |    8 ++------
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/arch/nios2/lib/board.c b/arch/nios2/lib/board.c
index 65de26e..19e688a 100644
--- a/arch/nios2/lib/board.c
+++ b/arch/nios2/lib/board.c
@@ -83,21 +83,20 @@ init_fnc_t *init_sequence[] = {
 
 
 /***********************************************************************/
+gd_t gd_data;
+bd_t bd_data;
+
 void board_init (void)
 {
 	bd_t *bd;
 	init_fnc_t **init_fnc_ptr;
 
-	/* Pointer is writable since we allocated a register for it.
-	 * Nios treats CONFIG_SYS_GBL_DATA_OFFSET as an address.
-	 */
-	gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
+	/* Pointer is writable since we allocated a register for it. */
+	gd = &gd_data;
 	/* compiler optimization barrier needed for GCC >= 3.4 */
 	__asm__ __volatile__("": : :"memory");
 
-	memset( gd, 0, GENERATED_GBL_DATA_SIZE );
-
-	gd->bd = (bd_t *)(gd+1);	/* At end of global data */
+	gd->bd = &bd_data;
 	gd->baudrate = CONFIG_BAUDRATE;
 	gd->cpu_clk = CONFIG_SYS_CLK_FREQ;
 
diff --git a/include/configs/nios2-generic.h b/include/configs/nios2-generic.h
index 17017a5..dccb66c 100644
--- a/include/configs/nios2-generic.h
+++ b/include/configs/nios2-generic.h
@@ -119,8 +119,7 @@
  * MEMORY ORGANIZATION
  *	-Monitor at top of sdram.
  *	-The heap is placed below the monitor
- *	-Global data is placed below the heap.
- *	-The stack is placed below global data (&grows down).
+ *	-The stack is placed below the heap (&grows down).
  */
 #define CONFIG_MONITOR_IS_IN_RAM
 #define CONFIG_SYS_MONITOR_LEN		0x40000	/* Reserve 256k */
@@ -130,10 +129,7 @@
 #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE + 0x20000)
 #define CONFIG_SYS_MALLOC_BASE		(CONFIG_SYS_MONITOR_BASE - \
 					 CONFIG_SYS_MALLOC_LEN)
-#define CONFIG_SYS_GBL_DATA_OFFSET	(CONFIG_SYS_MALLOC_BASE - \
-					 GENERATED_GBL_DATA_SIZE - \
-					 GENERATED_BD_INFO_SIZE)
-#define CONFIG_SYS_INIT_SP		CONFIG_SYS_GBL_DATA_OFFSET
+#define CONFIG_SYS_INIT_SP		CONFIG_SYS_MALLOC_BASE
 
 /*
  * MISC
-- 
1.7.7.6

[U-Boot] [PATCH] nios2: move gd and bd into BSS

[Mike Frysinger]

On Thursday 01 March 2012 02:09:05 Thomas Chou wrote:
> --- a/arch/nios2/lib/board.c
> +++ b/arch/nios2/lib/board.c
>
> +gd_t gd_data;
> +bd_t bd_data;

mark both static

> --- a/include/configs/nios2-generic.h
> +++ b/include/configs/nios2-generic.h
>
>   * MEMORY ORGANIZATION
>   *	-Monitor at top of sdram.
>   *	-The heap is placed below the monitor
> - *	-Global data is placed below the heap.
> - *	-The stack is placed below global data (&grows down).
> + *	-The stack is placed below the heap (&grows down).

you've got a tab after ther "*" but none of the previous lines do ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120301/554f6ead/attachment.pgp>

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Albert ARIBAUD]

Hi Graeme,

Le 29/02/2012 23:41, Graeme Russ a ?crit :
> Hi Mike,
>
> On Thu, Mar 1, 2012 at 9:29 AM, Mike Frysinger<vapier@gentoo.org>  wrote:
>> On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
>>> On Thu, Mar 1, 2012 at 6:04 AM, Mike Frysinger wrote:
>>>> On Tuesday 28 February 2012 18:32:57 Graeme Russ wrote:
>>>>> And this is why I dislike the implementation - You have to do all sorts
>>>>> of weird calucations to put things in the right place when, in fact,
>>>>> the location of gd and bd in memory is totally irrelavent.
>>>>
>>>> right, that's why i minimized the pain for Blackfin users -- this is all
>>>> handled in the arch's config-pre.h header.  board porters only need to
>>>> declare the size of regions they care about (monitor and heap sizes).
>>>>
>>>>> Ow, ouch! - And that padding makes things more fun - The memory layout
>>>>> is
>>>>>
>>>>> U-Boot | gd | pad | bd | pad | heap
>>>>
>>>> fwiw, i documented the Blackfin memory layout:
>>>> http://docs.blackfin.uclinux.org/doku.php?id=bootloaders:u-boot:memory-la
>>>> yout
>>>
>>> I had a look at this and noticed that you statically allocate locations for
>>> gd and bd (CONFIG_SYS_GBL_DATA_ADDR, CONFIG_SYS_BD_INFO_ADDR)
>>>
>>> Considering that:
>>>
>>> a) the gd pointer is in a register (P3) and thus easily locatable by a
>>>     debugger, and;
>>> b) the bd pointer is in gd
>>>
>>> Is there any reason not to have gd and bd in BSS?
>>
>> in the Blackfin case, most likely not.  we don't do relocation, and the bss is
>> cleared long before board_init_f() gets called.  the only reason for allowing
>> the config to override would be if someone wanted to put gd/bd into on-chip L1
>> data, but i can't imagine this structure being performance critical enough to
>> warrant that.
>
> I thought as much - I moved gd/bd into BSS for x86 without really thinking
> about why everyone else calculates the location of these data structures
> around the stack and heap. The longer I think about it, the more I think
> that it was not a bad move and that maybe other arches can follow suit as
> part of standardising the init sequences

ARMs relocate and don't have a valid BSS until board_init_r() but 
require gd as early as board_init_f().

> Regards,
>
> Graeme

Amicalement,
-- 
Albert.

[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Graeme Russ]

Hi Albert,

On Fri, Mar 2, 2012 at 8:57 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Graeme,
>
> Le 29/02/2012 23:41, Graeme Russ a ?crit :
>>
>> Hi Mike,
>>
>> On Thu, Mar 1, 2012 at 9:29 AM, Mike Frysinger<vapier@gentoo.org> ?wrote:
>>>
>>> On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
>>>>
>>>> On Thu, Mar 1, 2012 at 6:04 AM, Mike Frysinger wrote:
>>>>>
>>>>> On Tuesday 28 February 2012 18:32:57 Graeme Russ wrote:
>>>>>>
>>>>>> And this is why I dislike the implementation - You have to do all
>>>>>> sorts
>>>>>> of weird calucations to put things in the right place when, in fact,
>>>>>> the location of gd and bd in memory is totally irrelavent.
>>>>>
>>>>>
>>>>> right, that's why i minimized the pain for Blackfin users -- this is
>>>>> all
>>>>> handled in the arch's config-pre.h header. ?board porters only need to
>>>>> declare the size of regions they care about (monitor and heap sizes).
>>>>>
>>>>>> Ow, ouch! - And that padding makes things more fun - The memory layout
>>>>>> is
>>>>>>
>>>>>> U-Boot | gd | pad | bd | pad | heap
>>>>>
>>>>>
>>>>> fwiw, i documented the Blackfin memory layout:
>>>>>
>>>>> http://docs.blackfin.uclinux.org/doku.php?id=bootloaders:u-boot:memory-la
>>>>> yout
>>>>
>>>>
>>>> I had a look at this and noticed that you statically allocate locations
>>>> for
>>>> gd and bd (CONFIG_SYS_GBL_DATA_ADDR, CONFIG_SYS_BD_INFO_ADDR)
>>>>
>>>> Considering that:
>>>>
>>>> a) the gd pointer is in a register (P3) and thus easily locatable by a
>>>> ? ?debugger, and;
>>>> b) the bd pointer is in gd
>>>>
>>>> Is there any reason not to have gd and bd in BSS?
>>>
>>>
>>> in the Blackfin case, most likely not. ?we don't do relocation, and the
>>> bss is
>>> cleared long before board_init_f() gets called. ?the only reason for
>>> allowing
>>> the config to override would be if someone wanted to put gd/bd into
>>> on-chip L1
>>> data, but i can't imagine this structure being performance critical
>>> enough to
>>> warrant that.
>>
>>
>> I thought as much - I moved gd/bd into BSS for x86 without really thinking
>> about why everyone else calculates the location of these data structures
>> around the stack and heap. The longer I think about it, the more I think
>> that it was not a bad move and that maybe other arches can follow suit as
>> part of standardising the init sequences
>
>
> ARMs relocate and don't have a valid BSS until board_init_r() but require gd
> as early as board_init_f().

So does x86 - A temporary gd is kept in Cache-As-RAM until SDRAM is
initialised.

I just noticed that for x86, only bd is in bss - I still calculate a
permanent resting place for gd around the relocated U-Boot, heap and stack
but I plan to change this so the init sequence will be:

 - Create temporary gd and stack in CAR for board_init_f
 - After SDRAM is initialised and the new stack created, 'pivot' U-Boot so
   it is running from flash, but using SDRAM for stack
 - Copy gd from CAR to stack
 - Copy U-Boot to SDDRAM, clear BSS, do relocation fixups
 - Copy gd from stack to BSS
 - 'pivot' execution from flash to SDRAM (this may involve resetting the
   stack pointer, just to save a few bytes used by the no longer needed
   call stack and temporary gd, but this is not a neccessity)
 - Create heap below U-Boot

So the end memory layout is:

----------- Top Of SDRAM -----------
          .bss   \
          .data  + - U-Boot.bin
          .text  /
------------------------------------

          heap

------------------------------------

          stack (grows down)

....................................

          Free Memory

--- Bottom of SDRAM (0x00000000) ---


Simple :)

Regards,

Graeme

[U-Boot] [PATCH v2] nios2: move gd and bd into BSS

[Thomas Chou]

As suggested by Graeme Russ, move gd and bd data structrures
to BSS instead of calculating the locations around the stack
and heap.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
For u-boot.
v2, mark gd/bd static

 arch/nios2/lib/board.c          |   12 +++++-------
 include/configs/nios2-generic.h |   12 ++++--------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/nios2/lib/board.c b/arch/nios2/lib/board.c
index 65de26e..6aa6b9c 100644
--- a/arch/nios2/lib/board.c
+++ b/arch/nios2/lib/board.c
@@ -87,17 +87,15 @@ void board_init (void)
 {
 	bd_t *bd;
 	init_fnc_t **init_fnc_ptr;
+	static gd_t gd_data;
+	static bd_t bd_data;
 
-	/* Pointer is writable since we allocated a register for it.
-	 * Nios treats CONFIG_SYS_GBL_DATA_OFFSET as an address.
-	 */
-	gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
+	/* Pointer is writable since we allocated a register for it. */
+	gd = &gd_data;
 	/* compiler optimization barrier needed for GCC >= 3.4 */
 	__asm__ __volatile__("": : :"memory");
 
-	memset( gd, 0, GENERATED_GBL_DATA_SIZE );
-
-	gd->bd = (bd_t *)(gd+1);	/* At end of global data */
+	gd->bd = &bd_data;
 	gd->baudrate = CONFIG_BAUDRATE;
 	gd->cpu_clk = CONFIG_SYS_CLK_FREQ;
 
diff --git a/include/configs/nios2-generic.h b/include/configs/nios2-generic.h
index 17017a5..6a79d09 100644
--- a/include/configs/nios2-generic.h
+++ b/include/configs/nios2-generic.h
@@ -117,10 +117,9 @@
 
 /*
  * MEMORY ORGANIZATION
- *	-Monitor at top of sdram.
- *	-The heap is placed below the monitor
- *	-Global data is placed below the heap.
- *	-The stack is placed below global data (&grows down).
+ * -Monitor at top of sdram.
+ * -The heap is placed below the monitor
+ * -The stack is placed below the heap (&grows down).
  */
 #define CONFIG_MONITOR_IS_IN_RAM
 #define CONFIG_SYS_MONITOR_LEN		0x40000	/* Reserve 256k */
@@ -130,10 +129,7 @@
 #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE + 0x20000)
 #define CONFIG_SYS_MALLOC_BASE		(CONFIG_SYS_MONITOR_BASE - \
 					 CONFIG_SYS_MALLOC_LEN)
-#define CONFIG_SYS_GBL_DATA_OFFSET	(CONFIG_SYS_MALLOC_BASE - \
-					 GENERATED_GBL_DATA_SIZE - \
-					 GENERATED_BD_INFO_SIZE)
-#define CONFIG_SYS_INIT_SP		CONFIG_SYS_GBL_DATA_OFFSET
+#define CONFIG_SYS_INIT_SP		CONFIG_SYS_MALLOC_BASE
 
 /*
  * MISC
-- 
1.7.7.6

[U-Boot] [PATCH v2] nios2: move gd and bd into BSS

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120301/3482e6fc/attachment.pgp>