Skip to contentSkip to conversation

memory corruption on nios2 due to overlap of gbl data and malloc

U-Boot Superseded Alex Hornung 5 participants 17 messages Feb 20, 2012 – Mar 2, 2012 NIOS2 architecture

lore
Share & tools
lore

Alex Hornung

author
#0 lore raw

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

#1 lore raw

Hi Alex,

Le 21/02/2012 00:24, Alex Hornung a ?crit :

Quoted 16 lines

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);
Quoted 5 lines

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)?)

Quoted 6 lines

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.

Quoted 6 lines

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) - \
Quoted 1 line

GENERATED_GBL_DATA_SIZE)

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

Amicalement,

Signature · 1 line
Albert.
#2 lore raw

Hi Albert,

Quoted 23 lines · Albert ARIBAUD

Hi Alex,

Le 21/02/2012 00:24, Alex Hornung a ?crit :

Quoted 20 lines

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.

Quoted 8 lines
Quoted 2 lines

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

#3 lore raw

Hi Graeme,

Le 28/02/2012 23:39, Graeme Russ a ?crit :

Quoted 43 lines

Hi Albert,

Quoted 23 lines · Albert ARIBAUD

Hi Alex,

Le 21/02/2012 00:24, Alex Hornung a ?crit :

Quoted 20 lines

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.

Quoted 8 lines
Quoted 2 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

Regards,

Graeme

Amicalement,

Signature · 1 line
Albert.
#4 lore raw

Hi Albert,

Quoted 60 lines · Albert ARIBAUD

Hi Graeme,

Le 28/02/2012 23:39, Graeme Russ a ?crit :

Quoted 49 lines

Hi Albert,

Quoted 30 lines · On Wed, Feb 29, 2012 at 9:29 AM, Albert ARIBAUD <[email protected]>…

Hi Alex,

Le 21/02/2012 00:24, Alex Hornung a ?crit :

Quoted 18 lines

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);

Quoted 5 lines

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!

Quoted 5 lines

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.

Quoted 5 lines

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...

Quoted 3 lines

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

Quoted 2 lines

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

#5 lore raw

Le 29/02/2012 00:20, Graeme Russ a ?crit :

Quoted 70 lines

Hi Albert,

Quoted 60 lines · Albert ARIBAUD

Hi Graeme,

Le 28/02/2012 23:39, Graeme Russ a ?crit :

Quoted 49 lines

Hi Albert,

Quoted 30 lines · Albert ARIBAUD

Hi Alex,

Le 21/02/2012 00:24, Alex Hornung a ?crit :

Quoted 18 lines

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);
Quoted 5 lines

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!

Quoted 5 lines

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.

Quoted 5 lines

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...

Quoted 3 lines

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

Quoted 8 lines
Quoted 2 lines

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)
Quoted 3 lines

Regards,

Graeme

Amicalement,

Signature · 1 line
Albert.
#6 lore raw

Hi Albert,

Quoted 7 lines · Albert ARIBAUD

Le 29/02/2012 00:20, Graeme Russ a ?crit :

Quoted 5 lines

Hi Albert,

No - GENERATED_GBL_DATA_SIZE should be sizeof(gd_t)

Quoted 8 lines
Quoted 3 lines

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);
Quoted 7 lines

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

#7 lore raw
Quoted 3 lines · 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).

Quoted 3 lines

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>

#8 lore raw

Hi Mike,

Quoted 9 lines · Mike Frysinger
Quoted 3 lines · 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).

Quoted 6 lines
Quoted 3 lines

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

#9 lore raw
Quoted 22 lines · On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
Quoted 9 lines · Mike Frysinger
Quoted 3 lines · 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).

Quoted 8 lines
Quoted 4 lines

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)

Quoted 7 lines

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>

#10 lore raw

Hi Mike,

Quoted 29 lines · Mike Frysinger
Quoted 28 lines · On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
Quoted 9 lines · Mike Frysinger
Quoted 3 lines · 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).

Quoted 8 lines
Quoted 4 lines

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

Quoted 7 lines
Quoted 1 line

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

#11 lore raw

PATCH 1/1 nios2: move gd and bd into BSS

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 <[email protected]>
CC Mike Frysinger <[email protected]>
CC Alex Hornung <[email protected]>
Signed-off-by Thomas Chou <[email protected]>

---
For u-boot.

 arch/nios2/lib/board.c          |   13 ++++++-------
 include/configs/nios2-generic.h |    8 ++------
 2 files changed, 8 insertions(+), 13 deletions(-)
arch/nios2/lib/board.c +6 -7
index 65de26e..19e688a 100644
@@ -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;
include/configs/nios2-generic.h +2 -6
index 17017a5..dccb66c 100644
@@ -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
Signature · 1 line
1.7.7.6
#12 lore raw

PATCH 1/1 nios2: move gd and bd into BSS

arch/nios2/lib/board.c init_fnc_t *init_sequence[] = { · line 87

Quoted 5 lines · On Thursday 01 March 2012 02:09:05 Thomas Chou wrote:
arch/nios2/lib/board.c +2 -0
 
+gd_t gd_data;
+bd_t bd_data;

mark both static

include/configs/nios2-generic.h line 122

Quoted 9 lines
include/configs/nios2-generic.h +1 -2
 
  * 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>

#13 lore raw

PATCH v2 1/1 nios2: move gd and bd into BSS

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 <[email protected]>

---
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(-)
arch/nios2/lib/board.c +5 -7
index 65de26e..6aa6b9c 100644
@@ -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;
include/configs/nios2-generic.h +4 -8
index 17017a5..6a79d09 100644
@@ -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
Signature · 1 line
1.7.7.6
#15 lore raw
In reply to Graeme Russ

Hi Graeme,

Le 29/02/2012 23:41, Graeme Russ a ?crit :

Quoted 40 lines

Hi Mike,

Quoted 29 lines · Mike Frysinger
Quoted 28 lines · On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
Quoted 9 lines · Mike Frysinger
Quoted 3 lines · 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).

Quoted 8 lines
Quoted 4 lines

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

Quoted 7 lines
Quoted 1 line

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.

Quoted 5 lines

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().

Quoted 3 lines

Regards,

Graeme

Amicalement,

Signature · 1 line
Albert.
#16 lore raw

Hi Albert,

Quoted 68 lines · Albert ARIBAUD

Hi Graeme,

Le 29/02/2012 23:41, Graeme Russ a ?crit :

Quoted 58 lines

Hi Mike,

Quoted 43 lines · On Thu, Mar 1, 2012 at 9:29 AM, Mike Frysinger<[email protected]> ?wrote:
Quoted 32 lines · On Wednesday 29 February 2012 17:22:26 Graeme Russ wrote:
Quoted 17 lines · Mike Frysinger
Quoted 5 lines · 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).

Quoted 3 lines

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

Quoted 7 lines
Quoted 1 line

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)

Quoted 7 lines

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?

Quoted 9 lines

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.

Quoted 5 lines

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

Quoted 2 lines

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