On 10/22/19 1:26 AM, Simon Glass wrote:
At present bootstage relocation assumes that it is possible to point back to memory available before relocation, so it does not relocate the strings. However this is not the case on some platforms, such as x86 which uses the cache as RAM and loses access to this when the cache is enabled.
Move the relocation step to before U-Boot relocates, expand the allocated region to include space for the strings and relocate the strings at the same time as the bootstage records.
This ensures that bootstage data can remain accessible from TPL through SPL to U-Boot before/after relocation.
Signed-off-by: Simon Glass sjg@chromium.org
Hello Simon,
this merged patch seems to be incorrect. I compiled sandbox_defconfig with clang and ran it with valgrind.
We allocate memory in bootstage_init() for gd->bootstage. But from bootstage_get_size() we return a size that is larger than what we have allocated and use that larger memory area in reloc_bootstage(). See output below.
sudo docker pull trini/u-boot-gitlab-ci-runner:bionic-20200112-17Jan2020 sudo docker run -it 2e501a804876 /bin/bash cd ~ git clone https://gitlab.denx.de/u-boot/u-boot.git cd u-boot sudo apt-get update sudo apt-get install clang valgrind make CC=clang HOSTCC=clang sandbox_defconfig make CC=clang HOSTCC=clang -j8 valgrind ./u-boot -d arch/sandbox/dts/test.dtb
==89423== Memcheck, a memory error detector ==89423== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==89423== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info ==89423== Command: ./u-boot -d arch/sandbox/dts/test.dtb ==89423==
U-Boot 2020.01-00812-gc98c2b07e5 (Jan 25 2020 - 07:36:12 +0000)
Model: sandbox DRAM: 128 MiB ==89423== Invalid read of size 8 ==89423== at 0x4D4E90: memcpy (string.c:543) ==89423== by 0x424F88: reloc_bootstage (board_f.c:702) ==89423== by 0x424B22: initcall_run_list (initcall.h:40) ==89423== by 0x424B22: board_init_f (board_f.c:1019) ==89423== by 0x402755: main (start.c:386) ==89423== Address 0xa66f4e8 is 0 bytes after a block of size 968 alloc'd ==89423== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==89423== by 0x42E740: bootstage_init (bootstage.c:522) ==89423== by 0x424B84: initf_bootstage (board_f.c:806) ==89423== by 0x424B22: initcall_run_list (initcall.h:40) ==89423== by 0x424B22: board_init_f (board_f.c:1019) ==89423== by 0x402755: main (start.c:386) ==89423== ==89423== Invalid read of size 8 ==89423== at 0x4D4EA4: memcpy (string.c:542) ==89423== by 0x424F88: reloc_bootstage (board_f.c:702) ==89423== by 0x424B22: initcall_run_list (initcall.h:40) ==89423== by 0x424B22: board_init_f (board_f.c:1019) ==89423== by 0x402755: main (start.c:386) ==89423== Address 0xa66f4f0 is 8 bytes after a block of size 968 alloc'd ==89423== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==89423== by 0x42E740: bootstage_init (bootstage.c:522) ==89423== by 0x424B84: initf_bootstage (board_f.c:806) ==89423== by 0x424B22: initcall_run_list (initcall.h:40) ==89423== by 0x424B22: board_init_f (board_f.c:1019) ==89423== by 0x402755: main (start.c:386) ==89423== WDT: Started with servicing (60s timeout) MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) In: serial Out: vidconsole Err: vidconsole Model: sandbox SCSI: Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth1: eth@10004000 Hit any key to stop autoboot: 0 =>
Best regards
Heinrich
Changes in v2: None
common/board_f.c | 1 + common/board_r.c | 1 - common/bootstage.c | 25 ++++++++++++++++++++++--- 3 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/common/board_f.c b/common/board_f.c index 4852a3b0d84..e3591cbaebd 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -696,6 +696,7 @@ static int reloc_bootstage(void) gd->bootstage, gd->new_bootstage, size); memcpy(gd->new_bootstage, gd->bootstage, size); gd->bootstage = gd->new_bootstage;
bootstage_relocate();diff --git a/common/board_r.c b/common/board_r.c index d6fb5047a26..c1ecb06b743 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -670,7 +670,6 @@ static init_fnc_t init_sequence_r[] = { #ifdef CONFIG_SYS_NONCACHED_MEMORY initr_noncached, #endif
- bootstage_relocate, #ifdef CONFIG_OF_LIVE initr_of_live, #endif
diff --git a/common/bootstage.c b/common/bootstage.c index 4557ed4508c..e8b7bbf81a6 100644 --- a/common/bootstage.c +++ b/common/bootstage.c @@ -53,14 +53,23 @@ int bootstage_relocate(void) { struct bootstage_data *data = gd->bootstage; int i;
char *ptr;
/* Figure out where to relocate the strings to */
ptr = (char *)(data + 1);
/*
- Duplicate all strings. They may point to an old location in the
- program .text section that can eventually get trashed.
*/ debug("Relocating %d records\n", data->rec_count);
- for (i = 0; i < data->rec_count; i++)
data->record[i].name = strdup(data->record[i].name);
for (i = 0; i < data->rec_count; i++) {
const char *from = data->record[i].name;strcpy(ptr, from);data->record[i].name = ptr;ptr += strlen(ptr) + 1;}
return 0; }
@@ -490,7 +499,17 @@ int bootstage_unstash(const void *base, int size)
int bootstage_get_size(void) {
- return sizeof(struct bootstage_data);
struct bootstage_data *data = gd->bootstage;
struct bootstage_record *rec;
int size;
int i;
size = sizeof(struct bootstage_data);
for (rec = data->record, i = 0; i < data->rec_count;
i++, rec++)size += strlen(rec->name) + 1;return size; }
int bootstage_init(bool first)