arm: fix bug on relocation address

PATCH New Luca Ellero 3 participants 7 messages Jan 31, 2013 – Feb 7, 2013 Arm (32-bit)

Share & tools

Luca Ellero

author

2013-01-31 14:29 UTC #0 lore raw

PATCH 1/1 arm: fix bug on relocation address

If (N. SDRAM banks > 1) and they are not contiguous, don't relocate u-boot at (CONFIG_SYS_SDRAM_BASE + gd->ram_size), which is a bug. Instead use the end of 2nd bank (even if there are more than 2 banks)

Signed-off-by Luca Ellero <[email protected]>

Cc Albert Aribaud <[email protected]>

Cc Heiko Schocher <[email protected]>

---

On ARM architectures there is a bug getting top of SDRAM (where u-boot will be relocated). Top of SDRAM will always be:

CONFIG_SYS_SDRAM_BASE + gd->ram_size

anyway this can be wrong since SDRAM can be composed by more that one bank in not-contiguous address space.
(CONFIG_SYS_SDRAM_BASE + gd->ram_size) can land to not existent SDRAM addresses and can be very dangerous since it can potentially corrupt real SDRAM (in most cases SDRAM is aliased so writing to some not-existent address can write to real address).
Some arch use more than 2 banks but implementing all macros checks to PHYS_SDRAM_* leads to very ugly code, so I think using 2nd bank is good and does not generates bloated code

 arch/arm/lib/board.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

arch/arm/lib/board.c +12 -0

index 9f861cc..98634ab 100644
@@ -333,7 +333,19 @@ void board_init_f(ulong bootflag)
 	gd->ram_size -= CONFIG_SYS_MEM_TOP_HIDE;
 #endif
 
+#if defined(PHYS_SDRAM_2) && defined(PHYS_SDRAM_2_SIZE)
+	if (CONFIG_NR_DRAM_BANKS > 1 &&
+		(PHYS_SDRAM_1 + PHYS_SDRAM_1_SIZE) != PHYS_SDRAM_2)
+		addr =  PHYS_SDRAM_2 + PHYS_SDRAM_2_SIZE;
+	else
+		addr = CONFIG_SYS_SDRAM_BASE + gd->ram_size;
+#else
 	addr = CONFIG_SYS_SDRAM_BASE + gd->ram_size;
+#endif
+
+
+
+
 
 #ifdef CONFIG_LOGBUFFER
 #ifndef CONFIG_ALT_LB_ADDR

Signature · 1 line

1.7.0.4

Jeroen Hofstee

2013-01-31 19:08 UTC #1 lore raw

PATCH 1/1 arm: fix bug on relocation address

Hello Luca,

Quoted 9 lines · Luca Ellero

If (N. SDRAM banks > 1) and they are not contiguous, don't relocate u-boot at (CONFIG_SYS_SDRAM_BASE + gd->ram_size), which is a bug. Instead use the end of 2nd bank (even if there are more than 2 banks)
Signed-off-by Luca Ellero <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Heiko Schocher <[email protected]>
---

Quoted 7 lines

On ARM architectures there is a bug getting top of SDRAM (where u-boot will be relocated). Top of SDRAM will always be:
CONFIG_SYS_SDRAM_BASE + gd->ram_size
anyway this can be wrong since SDRAM can be composed by more that one bank in not-contiguous address space.

I don't think this is a valid use case since the README says:

"The available memory is mapped to fixed addresses using the memory controller. In this process, a contiguous block is formed for each memory type (Flash, SDRAM, SRAM), even when it consists of several physical memory banks."

Regards,
Jeroen

Luca Ellero

author

2013-02-01 08:50 UTC #2 lore raw

PATCH 1/1 arm: fix bug on relocation address

Hi Jeroen,
Hi Heiko,

Quoted 17 lines · Jeroen Hofstee

Hello Luca,
Quoted 14 lines · Luca Ellero
If (N. SDRAM banks > 1) and they are not contiguous, don't relocate u-boot at (CONFIG_SYS_SDRAM_BASE + gd->ram_size), which is a bug. Instead use the end of 2nd bank (even if there are more than 2 banks)
Signed-off-by Luca Ellero <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Heiko Schocher <[email protected]>
---
On ARM architectures there is a bug getting top of SDRAM (where u-boot will be relocated). Top of SDRAM will always be:
CONFIG_SYS_SDRAM_BASE + gd->ram_size

Quoted 9 lines

Quoted 2 lines
anyway this can be wrong since SDRAM can be composed by more that one bank in not-contiguous address space.
I don't think this is a valid use case since the README says:
"The available memory is mapped to fixed addresses using the memory controller. In this process, a contiguous block is formed for each memory type (Flash, SDRAM, SRAM), even when it consists of several physical memory banks."

Thank for your comments.
You are saying more or less the same thing but I'm afraid I didn't really catch what you mean.
I know how is the U-Boot memory map and I summarize it here (more or less, depending on board configuration):

TOP of RAM ------------------------

              Kernel Log Buffer
            ---------------------------
              Protected RAM
            ---------------------------
              TLB Table (32kB to 64 kB)
            ---------------------------
              LCD Frame Buffer
            ---------------------------
              Relocated U-Boot Code
            ---------------------------
              malloc area
            ---------------------------
              Board Info (struct bd_t)
            ---------------------------
              Global Data (struct gd_t)
            ---------------------------
              IRQ Stack
            ---------------------------
              Abort Stack
            ---------------------------
	     ...

Now, I have a Freescale iMX53 LOCO board which have 2 banks of 512 MB SDRAM, for total of 1GB. One bank is at phys 0x70000000-0x8fffffff, the other is at 0xb0000000-0xcfffffff.
If I stop U-Boot execution after relocation (with a JTAG debugger) I see that it is running at physical address 0xaff6D000 (more or less). As far as I can see this address is not existent. And the dangerous part is that I can see the same data (U-Boot code) at address 0x8ff6D000. This clearly states that U-Boot is relocated at 0xAff6D000 but in reality it is at 0x8ff6D000 an the relocation can potentially override data already existing there.
Don't you think this is a wrong behaviour?
Thanks
Regards
Luca

Heiko Schocher

2013-02-01 10:07 UTC #3 lore raw

PATCH 1/1 arm: fix bug on relocation address

Hello Luce,

Am 01.02.2013 09:50, schrieb Luca Ellero:

Quoted 31 lines

Hi Jeroen,
Hi Heiko,
Quoted 17 lines · Jeroen Hofstee
Hello Luca,
Quoted 14 lines · Luca Ellero
If (N. SDRAM banks > 1) and they are not contiguous, don't relocate u-boot at (CONFIG_SYS_SDRAM_BASE + gd->ram_size), which is a bug. Instead use the end of 2nd bank (even if there are more than 2 banks)
Signed-off-by Luca Ellero <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Heiko Schocher <[email protected]>
---
On ARM architectures there is a bug getting top of SDRAM (where u-boot will be relocated). Top of SDRAM will always be:
CONFIG_SYS_SDRAM_BASE + gd->ram_size
Quoted 9 lines
Quoted 2 lines
anyway this can be wrong since SDRAM can be composed by more that one bank in not-contiguous address space.
I don't think this is a valid use case since the README says:
"The available memory is mapped to fixed addresses using the memory controller. In this process, a contiguous block is formed for each memory type (Flash, SDRAM, SRAM), even when it consists of several physical memory banks."

Quoted 3 lines

Thank for your comments.
You are saying more or less the same thing but I'm afraid I didn't really catch what you mean.

You have 2 memory banks which are not contiguos.

[...]

Quoted 3 lines

Now, I have a Freescale iMX53 LOCO board which have 2 banks of 512 MB SDRAM, for total of 1GB. One bank is at phys 0x70000000-0x8fffffff, the other is at 0xb0000000-0xcfffffff.

Here you have a gap from 0x90000000 - 0xafffffff between the two banks, which U-Boot currently not supports ...

Quoted 8 lines

If I stop U-Boot execution after relocation (with a JTAG debugger) I see that it is running at physical address 0xaff6D000 (more or less). As far as I can see this address is not existent. And the dangerous part is that I can see the same data (U-Boot code) at address 0x8ff6D000. This clearly states that U-Boot is relocated at 0xAff6D000 but in reality it is at 0x8ff6D000 an the relocation can potentially override data already existing there.
Don't you think this is a wrong behaviour?

This wrong behaviour results because you use U-Boot in a configuration, which U-Boot currently not handle correct resp. does not support ...

Why you need such a memory configuration?

I suggest to move the second bank (if possible) to 0x90000000 and you have a contiguos memory, and U-Boot should work fine.

bye
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Luca Ellero

author

2013-02-01 09:49 UTC #4 lore raw

PATCH 1/1 arm: fix bug on relocation address

Hi,

Quoted 57 lines · Heiko Schocher

Hello Luce,
Am 01.02.2013 09:50, schrieb Luca Ellero:
Quoted 31 lines
Hi Jeroen,
Hi Heiko,
Quoted 17 lines · Jeroen Hofstee
Hello Luca,
Quoted 14 lines · Luca Ellero
If (N. SDRAM banks > 1) and they are not contiguous, don't relocate u-boot at (CONFIG_SYS_SDRAM_BASE + gd->ram_size), which is a bug. Instead use the end of 2nd bank (even if there are more than 2 banks)
Signed-off-by Luca Ellero <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Heiko Schocher <[email protected]>
---
On ARM architectures there is a bug getting top of SDRAM (where u-boot will be relocated). Top of SDRAM will always be:
CONFIG_SYS_SDRAM_BASE + gd->ram_size
Quoted 9 lines
Quoted 2 lines
anyway this can be wrong since SDRAM can be composed by more that one bank in not-contiguous address space.
I don't think this is a valid use case since the README says:
"The available memory is mapped to fixed addresses using the memory controller. In this process, a contiguous block is formed for each memory type (Flash, SDRAM, SRAM), even when it consists of several physical memory banks."
Quoted 3 lines
Thank for your comments.
You are saying more or less the same thing but I'm afraid I didn't really catch what you mean.
You have 2 memory banks which are not contiguos.
[...]
Quoted 3 lines
Now, I have a Freescale iMX53 LOCO board which have 2 banks of 512 MB SDRAM, for total of 1GB. One bank is at phys 0x70000000-0x8fffffff, the other is at 0xb0000000-0xcfffffff.
Here you have a gap from 0x90000000 - 0xafffffff between the two banks, which U-Boot currently not supports ...
Quoted 8 lines
If I stop U-Boot execution after relocation (with a JTAG debugger) I see that it is running at physical address 0xaff6D000 (more or less). As far as I can see this address is not existent. And the dangerous part is that I can see the same data (U-Boot code) at address 0x8ff6D000. This clearly states that U-Boot is relocated at 0xAff6D000 but in reality it is at 0x8ff6D000 an the relocation can potentially override data already existing there.
Don't you think this is a wrong behaviour?

Quoted 9 lines

This wrong behaviour results because you use U-Boot in a configuration, which U-Boot currently not handle correct resp. does not support ...
Why you need such a memory configuration?
I suggest to move the second bank (if possible) to 0x90000000 and you have a contiguos memory, and U-Boot should work fine.

That's exactly the point!!!
As far as I know iMX53 _can't_ physically move banks to other addresses. And likely there are some other architectures that have the same behaviour. Maybe someone on the list can confirm this.
Bye
Luca

Luca Ellero

author

2013-02-07 09:07 UTC #5 lore raw

PATCH 1/1 arm: fix bug on relocation address

Hi all,

Quoted 70 lines · Luca Ellero

Hi,
Quoted 57 lines · Heiko Schocher
Hello Luce,
Am 01.02.2013 09:50, schrieb Luca Ellero:
Quoted 31 lines
Hi Jeroen,
Hi Heiko,
Quoted 17 lines · Jeroen Hofstee
Hello Luca,
Quoted 14 lines · Luca Ellero
If (N. SDRAM banks > 1) and they are not contiguous, don't relocate u-boot at (CONFIG_SYS_SDRAM_BASE + gd->ram_size), which is a bug. Instead use the end of 2nd bank (even if there are more than 2 banks)
Signed-off-by Luca Ellero <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Heiko Schocher <[email protected]>
---
On ARM architectures there is a bug getting top of SDRAM (where u-boot will be relocated). Top of SDRAM will always be:
CONFIG_SYS_SDRAM_BASE + gd->ram_size
Quoted 9 lines
Quoted 2 lines
anyway this can be wrong since SDRAM can be composed by more that one bank in not-contiguous address space.
I don't think this is a valid use case since the README says:
"The available memory is mapped to fixed addresses using the memory controller. In this process, a contiguous block is formed for each memory type (Flash, SDRAM, SRAM), even when it consists of several physical memory banks."
Quoted 3 lines
Thank for your comments.
You are saying more or less the same thing but I'm afraid I didn't really catch what you mean.
You have 2 memory banks which are not contiguos.
[...]
Quoted 3 lines
Now, I have a Freescale iMX53 LOCO board which have 2 banks of 512 MB SDRAM, for total of 1GB. One bank is at phys 0x70000000-0x8fffffff, the other is at 0xb0000000-0xcfffffff.
Here you have a gap from 0x90000000 - 0xafffffff between the two banks, which U-Boot currently not supports ...
Quoted 8 lines
If I stop U-Boot execution after relocation (with a JTAG debugger) I see that it is running at physical address 0xaff6D000 (more or less). As far as I can see this address is not existent. And the dangerous part is that I can see the same data (U-Boot code) at address 0x8ff6D000. This clearly states that U-Boot is relocated at 0xAff6D000 but in reality it is at 0x8ff6D000 an the relocation can potentially override data already existing there.
Don't you think this is a wrong behaviour?
Quoted 9 lines
This wrong behaviour results because you use U-Boot in a configuration, which U-Boot currently not handle correct resp. does not support ...
Why you need such a memory configuration?
I suggest to move the second bank (if possible) to 0x90000000 and you have a contiguos memory, and U-Boot should work fine.

Quoted 6 lines

That's exactly the point!!!
As far as I know iMX53 _can't_ physically move banks to other addresses. And likely there are some other architectures that have the same behaviour. Maybe someone on the list can confirm this.
Bye
Luca

Any news here?
If this patch isn't the proper way to fix this misbehaviour, please suggest some other way to correct it. I'm a bit scary of viewing code running in "not-existing" addresses :-)
Thanks
Regards
Luca

Heiko Schocher

2013-02-01 08:17 UTC #6 lore raw

PATCH 1/1 arm: fix bug on relocation address

In reply to Luca Ellero

Hello Luca,

Quoted 14 lines · Luca Ellero

If (N. SDRAM banks > 1) and they are not contiguous, don't relocate u-boot at (CONFIG_SYS_SDRAM_BASE + gd->ram_size), which is a bug. Instead use the end of 2nd bank (even if there are more than 2 banks)
Signed-off-by Luca Ellero <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Heiko Schocher <[email protected]>
---
On ARM architectures there is a bug getting top of SDRAM (where u-boot will be relocated). Top of SDRAM will always be:
CONFIG_SYS_SDRAM_BASE + gd->ram_size

Quoted 9 lines

anyway this can be wrong since SDRAM can be composed by more that one bank in not-contiguous address space.
(CONFIG_SYS_SDRAM_BASE + gd->ram_size) can land to not existent SDRAM addresses and can be very dangerous since it can potentially corrupt real SDRAM (in most cases SDRAM is aliased so writing to some not-existent address can write to real address).
Some arch use more than 2 banks but implementing all macros checks to PHYS_SDRAM_* leads to very ugly code, so I think using 2nd bank is good and does not generates bloated code

This is not a valid use case for U-Boot! Look in the README section "Memory Management". This is not so trivial as it looks, as for example the stack pointer starts somewhere near the end of RAM and grows downwards. If you now have RAM with gaps, it maybe reaches an address, which is not valid ...

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany