[U-Boot] [PATCH] fix get_ram_size memory corruption
base[0] is saved, but never restored.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- common/memsize.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/memsize.c b/common/memsize.c index 589400d..8bda2ba 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -48,7 +48,9 @@ long get_ram_size(long *base, long maxsize) *addr = 0;
sync (); - if ((val = *addr) != 0) { + val = *addr; + *addr = save[i]; + if ((val = 0) != 0) { /* Restore the original data before leaving the function. */ sync ();
Dear Gerd,
In message 1413874945-16560-1-git-send-email-kraxel@redhat.com you wrote:
base[0] is saved, but never restored.
Patches for this have been submitted before, but were rejected. You may want to dig the archives for these.
What exactly is your test case where you see any memory corruption?
- if ((val = *addr) != 0) {
- val = *addr;
- *addr = save[i];
- if ((val = 0) != 0) {
--------^^^^^^^^^^^^^^^^^^^^^^
This code looks pretty much wrong to me; the condition "0 != 0" can never become true...
NAK.
Best regards,
Wolfgang Denk
Check if the fdt is there (happens only when passed via -dtb cmd line switch to qemu), then setup env accordingly. If present use it. Otherwise try to load from disk.
Also tweak CONFIG_SYS_LOAD_ADDR and LINUX_BOOT_PARAM_ADDR a bit to avoid them overriding the fdt. --- board/armltd/vexpress/vexpress_common.c | 18 ++++++++++++++++++ include/configs/vexpress_common.h | 9 ++++----- 2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/board/armltd/vexpress/vexpress_common.c b/board/armltd/vexpress/vexpress_common.c index cb2de2f..dafa4b2 100644 --- a/board/armltd/vexpress/vexpress_common.c +++ b/board/armltd/vexpress/vexpress_common.c @@ -20,6 +20,7 @@ #include <malloc.h> #include <errno.h> #include <netdev.h> +#include <libfdt.h> #include <asm/io.h> #include <asm/arch/systimer.h> #include <asm/arch/sysctrl.h> @@ -60,6 +61,23 @@ int board_init(void) return 0; }
+int board_late_init(void) +{ + struct fdt_header *fdt = (void*)V2M_BASE; + + if (fdt_magic(fdt) == FDT_MAGIC) { + setenv_addr("fdt_addr", (void*)(ulong)V2M_BASE); + printf("QEMU: fdt found at %s, using it.\n", getenv("fdt_addr")); + } else { + setenv_addr("fdt_addr_r", (void*)((ulong)V2M_BASE + 0x27f00000)); + setenv("fdtfile", CONFIG_DEFAULT_FDT_FILE); + printf("QEMU: will try to load %s from disk.\n", + getenv("fdtfile")); + } + + return 0; +} + int board_eth_init(bd_t *bis) { int rc = 0; diff --git a/include/configs/vexpress_common.h b/include/configs/vexpress_common.h index e378c10..e490804 100644 --- a/include/configs/vexpress_common.h +++ b/include/configs/vexpress_common.h @@ -184,8 +184,8 @@
/* Miscellaneous configurable options */ #undef CONFIG_SYS_CLKS_IN_HZ -#define CONFIG_SYS_LOAD_ADDR (V2M_BASE + 0x8000) -#define LINUX_BOOT_PARAM_ADDR (V2M_BASE + 0x2000) +#define CONFIG_SYS_LOAD_ADDR (V2M_BASE + 0x100000) +#define LINUX_BOOT_PARAM_ADDR (V2M_BASE + 0x200000)
/* Physical Memory Map */ #define CONFIG_NR_DRAM_BANKS 2 @@ -207,6 +207,8 @@ #include <config_distro_defaults.h> #include <config_distro_bootcmd.h>
+#define CONFIG_BOARD_LATE_INIT + /* Basic environment settings */ #define CONFIG_BOOTCOMMAND "run bootflash;" #ifdef CONFIG_VEXPRESS_ORIGINAL_MEMORY_MAP @@ -215,7 +217,6 @@ "kernel_addr_r=" "0x80008000\0" \ "scriptaddr=" "0x87d00000\0" \ "pxefile_addr_r=" "0x87e00000\0" \ - "fdt_addr_r=" "0x87f00000\0" \ "ramdisk_addr_r=" "0x88000000\0" #elif defined(CONFIG_VEXPRESS_EXTENDED_MEMORY_MAP) #define CONFIG_PLATFORM_ENV_SETTINGS \ @@ -223,7 +224,6 @@ "kernel_addr_r=" "0xa0008000\0" \ "scriptaddr=" "0xa7d00000\0" \ "pxefile_addr_r=" "0xa7e00000\0" \ - "fdt_addr_r=" "0xa7f00000\0" \ "ramdisk_addr_r=" "0xa8000000\0" #endif
@@ -235,7 +235,6 @@ #define CONFIG_EXTRA_ENV_SETTINGS \ CONFIG_PLATFORM_ENV_SETTINGS \ "console=ttyAMA0,115200n8\0" \ - "fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \ BOOTENV
/* FLASH and environment organization */
On Di, 2014-10-21 at 12:05 +0200, Gerd Hoffmann wrote:
Check if the fdt is there (happens only when passed via -dtb cmd line switch to qemu), then setup env accordingly. If present use it. Otherwise try to load from disk.
Oops, wrong patch, correct one just send. It shows what I'm doing though to automatically use the qemu fdt:
+int board_late_init(void) +{
- struct fdt_header *fdt = (void*)V2M_BASE;
- if (fdt_magic(fdt) == FDT_MAGIC) {
setenv_addr("fdt_addr", (void*)(ulong)V2M_BASE);printf("QEMU: fdt found at %s, using it.\n", getenv("fdt_addr"));- } else {
setenv_addr("fdt_addr_r", (void*)((ulong)V2M_BASE + 0x27f00000));setenv("fdtfile", CONFIG_DEFAULT_FDT_FILE);printf("QEMU: will try to load %s from disk.\n",getenv("fdtfile"));- }
- return 0;
+}
[ This one depends on other non-upstream patches so no plans to submit this upstream atm ]
cheers, Gerd
base[0] is saved, but never restored.
Test case: Start u-boot in qemu, using vexpress-a9 emulation. qemu places the fdt at the start of ram, as a service for the guest. Trying to pick it up there by setting fdt_addr accordingly fails because the fdt magic cookie is gone (zeroed out) after calling get_ram_size.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- common/memsize.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/memsize.c b/common/memsize.c index 589400d..ad79ff1 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -48,7 +48,9 @@ long get_ram_size(long *base, long maxsize) *addr = 0;
sync (); - if ((val = *addr) != 0) { + val = *addr; + *addr = save[i]; + if (val != 0) { /* Restore the original data before leaving the function. */ sync ();
Dear Gerd Hoffmann,
In message 1413887467-18799-1-git-send-email-kraxel@redhat.com you wrote:
base[0] is saved, but never restored.
Test case: Start u-boot in qemu, using vexpress-a9 emulation. qemu places the fdt at the start of ram, as a service for the guest. Trying to pick it up there by setting fdt_addr accordingly fails because the fdt magic cookie is gone (zeroed out) after calling get_ram_size.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
common/memsize.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
When submitting an updated version of the patch, you are supposed to mark it as such in the Subject, and to provide a change log; please see [1] for details.
[1] http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions
It seems I cannot reproduce the problem here.
Test 1: Board = TQM5200S running U-Boot 2014.10-rc2-00274-gf9860cf
=> md 0 4 00000000: bc193d2c 0783d3c7 18688dbf c3c7c30f ..=,.....h...... => mw 0 27051956 => reset ... => md 0 4 00000000: 27051956 0783d3c7 18688dbf c3c7c30f '..V.....h...... => mm 0 00000000: 27051956 ? deadbeef 00000004: 0783d3c7 ? affeaffe 00000008: 18688dbf ? 27051956 0000000c: c3c7c30f ? deadc0de 00000010: 3d6b3f3d ? . => md 0 4 00000000: deadbeef affeaffe 27051956 deadc0de ........'..V.... => md 0 4 00000000: deadbeef affeaffe 27051956 deadc0de ........'..V....
Test 2: Board = lite5200b running U-Boot 2011.09-00282-gd8fffa0
=> md 0 4 00000000: b75d351d 77fe2cf5 4ba0ebeb 896baaae .]5.w.,.K....k.. => mm 0 00000000: b75d351d ? 12345678 00000004: 77fe2cf5 ? 87654321 00000008: 4ba0ebeb ? 24681357 0000000c: 896baaae ? 86427531 00000010: 53e7f1fe ? . => md 0 4 00000000: 12345678 87654321 24681357 86427531 .4Vx.eC!$h.W.Bu1 => reset ... => md 0 4 00000000: 12345678 87654321 24681357 86427531 .4Vx.eC!$h.W.Bu1
I cannot see any memory corruption. If I understand you correctly, the contents of address 0x00000000 should be overwritten here?
Best regards,
Wolfgang Denk
Hi,
I cannot see any memory corruption. If I understand you correctly, the contents of address 0x00000000 should be overwritten here?
Depends on the address space layout. vexpress-a9 has flash mapped at 0x00000000. RAM starts at 0x60000000, and four bytes at 0x60000000 get corrupted.
Possibly it also depends on whenever the board calls get_ram_size() in the first place. For the vexpress the calls are in board-specific code (board/armltd/vexpress/vexpress_common.c).
cheers, Gerd
base[0] is saved, but only restored in case the ram test failed.
Test case: Start u-boot in qemu, using vexpress-a9 emulation. qemu places the fdt at the start of ram, as a service for the guest. Trying to pick it up there by setting fdt_addr accordingly fails because the fdt magic cookie is gone (zeroed out) after calling get_ram_size.
Cc: Wolfgang Denk wd@denx.de Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- common/memsize.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/memsize.c b/common/memsize.c index 589400d..dac1368 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -48,11 +48,12 @@ long get_ram_size(long *base, long maxsize) *addr = 0;
sync (); - if ((val = *addr) != 0) { + val = *addr; + *addr = save[i]; + if (val != 0) { /* Restore the original data before leaving the function. */ sync (); - *addr = save[i]; for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) { addr = base + cnt; sync ();
Dear Gerd,
In message 1413894504-26255-1-git-send-email-kraxel@redhat.com you wrote:
base[0] is saved, but only restored in case the ram test failed.
When posting a new version of a patch please always make sure to add a change log!
I'm afraid your patch breaks at least some boards; tested on TQM5200 board...
Before (v2014.10):
U-Boot 2014.10 (Oct 21 2014 - 15:59:47)
CPU: MPC5200B v2.2, Core v1.4 at 396 MHz Bus 132 MHz, IPB 132 MHz, PCI 66 MHz Board: TQM5200S (TQ-Components GmbH) on a STK52xx carrier board I2C: 85 kHz, ready DRAM: 64 MiB Flash: 32 MiB In: serial Out: serial Err: serial Net: FEC IDE: Bus 0: OK ...
With your patch:
U-Boot 2014.10-00001-g27030e7 (Oct 21 2014 - 15:45:12)
CPU: MPC5200B v2.2, Core v1.4 at 396 MHz Bus 132 MHz, IPB 132 MHz, PCI 66 MHz Board: TQM5200S (TQ-Components GmbH) on a STK52xx carrier board I2C: 85 kHz, ready DRAM: 1 GiB [board hangs here]
Note also the incorrect RAM size.
Sorry, but this is not a correct fix. NAK.
Best regards,
Wolfgang Denk
participants (2)
-
Gerd Hoffmann -
Wolfgang Denk