[U-Boot] [PATCH v2 00/10] bootstage: TPL and SPL improvements
At present bootstage cannot be fully used on x86 since it violates a few U-Boot rules, mostly accessing pre-relocation memory after relocation. This series corrects this and adds better support for using bootstage in TPL.
It also includes a few improvements to tiny-printf.
Changes in v2: - Add a new patch to support %p without DEBUG - Adjust SPL logic to avoid failing if TPL does not provide bootstage data - Add a new patch to support %p without DEBUG in tiny-printf
Simon Glass (10): tiny-printf: Reduce size by removing ctype tiny-printf: Add print_grouped_ull() tiny-printf: Reorder code to support %p bloblist: Reserve an aligned base bootstage: Store the next ID in the stash bootstage: Fix counting of entries in stash bootstage: Avoid conflicts between stash/unstash bootstage: Correct relocation algorithm bootstage: Mark the start/end of TPL and SPL separately bootstage: Allow SPL to obtain bootstage info from TPL
common/board_f.c | 2 ++ common/board_r.c | 1 - common/bootstage.c | 53 ++++++++++++++++++++++++++++++--------------- common/spl/spl.c | 23 ++++++++++++++++---- include/bootstage.h | 2 ++ lib/tiny-printf.c | 29 ++++++++++++++++++++----- 6 files changed, 81 insertions(+), 29 deletions(-)
The ctype array is brought into the image, adding 256 bytes, when it is unlikely to be needed. The extra code for %p is only present when DEBUG is defined, so let's drop ctype as well unless DEBUG is defined.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
lib/tiny-printf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index ebef92fc9f6..632b4249142 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -289,8 +289,15 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) break; case 'p': pointer(info, fmt, va_arg(va, void *)); + /* + * Skip this because it pulls in _ctype which is + * 256 bytes, and we don't generally implement + * pointer anyway + */ +#ifdef DEBUG while (isalnum(fmt[0])) fmt++; +#endif break; case '%': out(info, '%');
The ctype array is brought into the image, adding 256 bytes, when it is unlikely to be needed. The extra code for %p is only present when DEBUG is defined, so let's drop ctype as well unless DEBUG is defined.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
lib/tiny-printf.c | 7 +++++++ 1 file changed, 7 insertions(+)
Applied to u-boot-dm, thanks!
This function is used in the bootstage report which may be trigged in TPL or TPL. Add a very basic implication of this function so that it builds. There is no attempt to get the formatting right, since this would add too much code size.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
lib/tiny-printf.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 632b4249142..430c3255bc5 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -389,3 +389,9 @@ int snprintf(char *buf, size_t size, const char *fmt, ...)
return ret; } + +void print_grouped_ull(unsigned long long int_val, int digits) +{ + /* Don't try to ptint the upper 32-bits */ + printf("%ld ", (ulong)int_val); +}
On 22.10.19 01:26, Simon Glass wrote:
This function is used in the bootstage report which may be trigged in TPL or TPL. Add a very basic implication of this function so that it builds. There is no attempt to get the formatting right, since this would add too much code size.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
lib/tiny-printf.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 632b4249142..430c3255bc5 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -389,3 +389,9 @@ int snprintf(char *buf, size_t size, const char *fmt, ...)
return ret; }
+void print_grouped_ull(unsigned long long int_val, int digits) +{
- /* Don't try to ptint the upper 32-bits */
s/ptint/print ?
- printf("%ld ", (ulong)int_val);
+}
Other than that:
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
On 22.10.19 01:26, Simon Glass wrote:
This function is used in the bootstage report which may be trigged in TPL or TPL. Add a very basic implication of this function so that it builds. There is no attempt to get the formatting right, since this would add too much code size.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
lib/tiny-printf.c | 6 ++++++ 1 file changed, 6 insertions(+)
Applied to u-boot-dm, thanks!
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add a new patch to support %p without DEBUG
lib/tiny-printf.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 430c3255bc5..c80c4f95ae0 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -157,7 +157,8 @@ static void ip4_addr_string(struct printf_info *info, u8 *addr) * decimal). */
-static void pointer(struct printf_info *info, const char *fmt, void *ptr) +static void __maybe_unused pointer(struct printf_info *info, const char *fmt, + void *ptr) { #ifdef DEBUG unsigned long num = (uintptr_t)ptr; @@ -266,6 +267,21 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) div_out(info, &num, div); } break; + case 'p': +#ifdef DEBUG + pointer(info, fmt, va_arg(va, void *)); + /* + * Skip this because it pulls in _ctype which is + * 256 bytes, and we don't generally implement + * pointer anyway + */ + while (isalnum(fmt[0])) + fmt++; + break; +#else + islong = true; + /* no break */ +#endif case 'x': if (islong) { num = va_arg(va, unsigned long); @@ -287,18 +303,6 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) case 's': p = va_arg(va, char*); break; - case 'p': - pointer(info, fmt, va_arg(va, void *)); - /* - * Skip this because it pulls in _ctype which is - * 256 bytes, and we don't generally implement - * pointer anyway - */ -#ifdef DEBUG - while (isalnum(fmt[0])) - fmt++; -#endif - break; case '%': out(info, '%'); default:
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add a new patch to support %p without DEBUG
lib/tiny-printf.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
Applied to u-boot-dm, thanks!
Hi Simon,
On 22/10/19 4:56 am, Simon Glass wrote:
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
Signed-off-by: Simon Glass sjg@chromium.org
This patch seems to have broken Ethernet boot in am335x-evm for me. It seems to be caused by SPL not being able to set ethaddr variable but its not obvious to me why this would cause it to happen. Here's a log:
Trying to boot from USB eth ## Error: flags type check failure for "ethaddr" <= "40309f20M" (type: m) ## Error inserting "ethaddr" variable, errno=1
Warning: eth_cpsw using MAC address from ROM eth0: eth_cpsw## Error: flags type check failure for "eth1addr" <= "81f01098M" (type: m) ## Error inserting "eth1addr" variable, errno=1
Warning: usb_ether using MAC address from ROM , eth1: usb_ether eth_cpsw Waiting for PHY auto negotiation to complete...... done link up on port 0, speed 1000, full duplex BOOTP broadcast 1 BOOTP broadcast 2 BOOTP broadcast 3 BOOTP broadcast 4 BOOTP broadcast 5 BOOTP broadcast 6 BOOTP broadcast 7 BOOTP broadcast 8 BOOTP broadcast 9 BOOTP broadcast 10 BOOTP broadcast 11 BOOTP broadcast 12 BOOTP broadcast 13 BOOTP broadcast 14 BOOTP broadcast 15 BOOTP broadcast 16 BOOTP broadcast 17
Retry time exceeded; starting again Problem booting with BOOTP SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ###
Reverting this patch on the latest U-boot master fixes the issue for me.
I'll look into this more deeply tomorrow. Let me know if you see something obviously wrong with the patch.
Thanks, Faiz
Hi Faiz,
On Thu, 30 Jan 2020 at 08:22, Faiz Abbas faiz_abbas@ti.com wrote:
Hi Simon,
On 22/10/19 4:56 am, Simon Glass wrote:
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
Signed-off-by: Simon Glass sjg@chromium.org
This patch seems to have broken Ethernet boot in am335x-evm for me. It seems to be caused by SPL not being able to set ethaddr variable but its not obvious to me why this would cause it to happen. Here's a log:
Trying to boot from USB eth ## Error: flags type check failure for "ethaddr" <= "40309f20M" (type: m) ## Error inserting "ethaddr" variable, errno=1
Warning: eth_cpsw using MAC address from ROM eth0: eth_cpsw## Error: flags type check failure for "eth1addr" <= "81f01098M" (type: m) ## Error inserting "eth1addr" variable, errno=1
Warning: usb_ether using MAC address from ROM , eth1: usb_ether eth_cpsw Waiting for PHY auto negotiation to complete...... done link up on port 0, speed 1000, full duplex BOOTP broadcast 1 BOOTP broadcast 2 BOOTP broadcast 3 BOOTP broadcast 4 BOOTP broadcast 5 BOOTP broadcast 6 BOOTP broadcast 7 BOOTP broadcast 8 BOOTP broadcast 9 BOOTP broadcast 10 BOOTP broadcast 11 BOOTP broadcast 12 BOOTP broadcast 13 BOOTP broadcast 14 BOOTP broadcast 15 BOOTP broadcast 16 BOOTP broadcast 17
Retry time exceeded; starting again Problem booting with BOOTP SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ###
Reverting this patch on the latest U-boot master fixes the issue for me.
I'll look into this more deeply tomorrow. Let me know if you see something obviously wrong with the patch.
Well one thing is that eth_env_set_enetaddr() called from the board's board.c has this:
sprintf(buf, "%pM", enetaddr);
which is not supported with tiny-printf.
Before my patch it would probably produce an empty string, but now it will produce garbage. Perhaps need an SPL check there.
Regards, Simon
Hi Simon,
On 31/01/20 7:57 am, Simon Glass wrote:
Hi Faiz,
On Thu, 30 Jan 2020 at 08:22, Faiz Abbas faiz_abbas@ti.com wrote:
Hi Simon,
On 22/10/19 4:56 am, Simon Glass wrote:
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
[...]
Retry time exceeded; starting again Problem booting with BOOTP SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ###
Reverting this patch on the latest U-boot master fixes the issue for me.
I'll look into this more deeply tomorrow. Let me know if you see something obviously wrong with the patch.
Well one thing is that eth_env_set_enetaddr() called from the board's board.c has this:
sprintf(buf, "%pM", enetaddr);
which is not supported with tiny-printf.
That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See:
https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183
I added this specifically to support Ethernet Boot usecases on TI platforms
But above commit seems to move pointer() function that formats the output under #ifdef DEBUG which definitely breaks %pM
Before my patch it would probably produce an empty string, but now it will produce garbage. Perhaps need an SPL check there.
Regards, Simon
Hi Vignesh,
On Thu, 30 Jan 2020 at 22:12, Vignesh Raghavendra vigneshr@ti.com wrote:
Hi Simon,
On 31/01/20 7:57 am, Simon Glass wrote:
Hi Faiz,
On Thu, 30 Jan 2020 at 08:22, Faiz Abbas faiz_abbas@ti.com wrote:
Hi Simon,
On 22/10/19 4:56 am, Simon Glass wrote:
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
[...]
Retry time exceeded; starting again Problem booting with BOOTP SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ###
Reverting this patch on the latest U-boot master fixes the issue for me.
I'll look into this more deeply tomorrow. Let me know if you see something obviously wrong with the patch.
Well one thing is that eth_env_set_enetaddr() called from the board's board.c has this:
sprintf(buf, "%pM", enetaddr);
which is not supported with tiny-printf.
That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See:
https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183
I added this specifically to support Ethernet Boot usecases on TI platforms
But above commit seems to move pointer() function that formats the output under #ifdef DEBUG which definitely breaks %pM
OK I see. I think it is too confusing to use #ifdef DEBUG in this code.
One fix would be to change pointer() to return true if it actually does something. I'll take a look.
This code needs tests also. Vignesh, do you feel like writing something?
Before my patch it would probably produce an empty string, but now it will produce garbage. Perhaps need an SPL check there.
Regards, Simon
Faiz,
On 31/01/20 11:44 pm, Simon Glass wrote:
Hi Vignesh,
On Thu, 30 Jan 2020 at 22:12, Vignesh Raghavendra vigneshr@ti.com wrote:
Hi Simon,
On 31/01/20 7:57 am, Simon Glass wrote:
Hi Faiz,
On Thu, 30 Jan 2020 at 08:22, Faiz Abbas faiz_abbas@ti.com wrote:
Hi Simon,
On 22/10/19 4:56 am, Simon Glass wrote:
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
[...]
Retry time exceeded; starting again Problem booting with BOOTP SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ###
Reverting this patch on the latest U-boot master fixes the issue for me.
I'll look into this more deeply tomorrow. Let me know if you see something obviously wrong with the patch.
Well one thing is that eth_env_set_enetaddr() called from the board's board.c has this:
sprintf(buf, "%pM", enetaddr);
which is not supported with tiny-printf.
That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See:
https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183
I added this specifically to support Ethernet Boot usecases on TI platforms
But above commit seems to move pointer() function that formats the output under #ifdef DEBUG which definitely breaks %pM
OK I see. I think it is too confusing to use #ifdef DEBUG in this code.
One fix would be to change pointer() to return true if it actually does something. I'll take a look.
This code needs tests also. Vignesh, do you feel like writing something?
Is there a testcase for full printf()? I am not sure where to look for. Is test/print_ut.c the right place to add new test?
Hi Vignesh,
On Tue, 4 Feb 2020 at 00:58, Vignesh Raghavendra vigneshr@ti.com wrote:
Faiz,
On 31/01/20 11:44 pm, Simon Glass wrote:
Hi Vignesh,
On Thu, 30 Jan 2020 at 22:12, Vignesh Raghavendra vigneshr@ti.com wrote:
Hi Simon,
On 31/01/20 7:57 am, Simon Glass wrote:
Hi Faiz,
On Thu, 30 Jan 2020 at 08:22, Faiz Abbas faiz_abbas@ti.com wrote:
Hi Simon,
On 22/10/19 4:56 am, Simon Glass wrote:
With a bit of code reordering we can support %p using the existing code for ulong.
Move the %p code up and adjust the logic accordingly.
[...]
Retry time exceeded; starting again Problem booting with BOOTP SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ###
Reverting this patch on the latest U-boot master fixes the issue for me.
I'll look into this more deeply tomorrow. Let me know if you see something obviously wrong with the patch.
Well one thing is that eth_env_set_enetaddr() called from the board's board.c has this:
sprintf(buf, "%pM", enetaddr);
which is not supported with tiny-printf.
That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See:
https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183
I added this specifically to support Ethernet Boot usecases on TI platforms
But above commit seems to move pointer() function that formats the output under #ifdef DEBUG which definitely breaks %pM
OK I see. I think it is too confusing to use #ifdef DEBUG in this code.
One fix would be to change pointer() to return true if it actually does something. I'll take a look.
This code needs tests also. Vignesh, do you feel like writing something?
Is there a testcase for full printf()? I am not sure where to look for. Is test/print_ut.c the right place to add new test?
Yes.
See also this commit in u-boot-dm/testing
test: Add a way to check each line of console output
Regards, Simon
Make sure that the bloblist starts on an aligned boundary. This protects against one of the early allocating causing the alignment to be lost.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/board_f.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/common/board_f.c b/common/board_f.c index 591f18f391e..4852a3b0d84 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -588,6 +588,7 @@ static int reserve_stacks(void) static int reserve_bloblist(void) { #ifdef CONFIG_BLOBLIST + gd->start_addr_sp &= ~0xf; gd->start_addr_sp -= CONFIG_BLOBLIST_SIZE; gd->new_bloblist = map_sysmem(gd->start_addr_sp, CONFIG_BLOBLIST_SIZE); #endif
Make sure that the bloblist starts on an aligned boundary. This protects against one of the early allocating causing the alignment to be lost.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/board_f.c | 1 + 1 file changed, 1 insertion(+)
Applied to u-boot-dm, thanks!
When stashing bootstage info, store the next ID so that it can be used when the stash is restored. This avoids the ID starting at zero and potentially overwriting existing entries.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/bootstage.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/common/bootstage.c b/common/bootstage.c index 56ef91ad859..257dc5d402f 100644 --- a/common/bootstage.c +++ b/common/bootstage.c @@ -41,10 +41,11 @@ enum { };
struct bootstage_hdr { - uint32_t version; /* BOOTSTAGE_VERSION */ - uint32_t count; /* Number of records */ - uint32_t size; /* Total data size (non-zero if valid) */ - uint32_t magic; /* Unused */ + u32 version; /* BOOTSTAGE_VERSION */ + u32 count; /* Number of records */ + u32 size; /* Total data size (non-zero if valid) */ + u32 magic; /* Magic number */ + u32 next_id; /* Next ID to use for bootstage */ };
int bootstage_relocate(void) @@ -392,6 +393,7 @@ int bootstage_stash(void *base, int size) hdr->count = count; hdr->size = 0; hdr->magic = BOOTSTAGE_MAGIC; + hdr->next_id = data->next_id; ptr += sizeof(*hdr);
/* Write the records, silently stopping when we run out of space */ @@ -485,6 +487,7 @@ int bootstage_unstash(const void *base, int size)
/* Mark the records as read */ data->rec_count += hdr->count; + data->next_id = hdr->next_id; debug("Unstashed %d records\n", hdr->count);
return 0;
When stashing bootstage info, store the next ID so that it can be used when the stash is restored. This avoids the ID starting at zero and potentially overwriting existing entries.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/bootstage.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
Applied to u-boot-dm, thanks!
The current code searches for empty records but these not existing with bootstage now. This used to be needed when bootstage records were stored in a spare array.
Drop the unnecessary code and fix a code-style nit at the same time.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/bootstage.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/common/bootstage.c b/common/bootstage.c index 257dc5d402f..fe36bac0474 100644 --- a/common/bootstage.c +++ b/common/bootstage.c @@ -373,7 +373,6 @@ int bootstage_stash(void *base, int size) const struct bootstage_record *rec; char buf[20]; char *ptr = base, *end = ptr + size; - uint32_t count; int i;
if (hdr + 1 > (struct bootstage_hdr *)end) { @@ -384,22 +383,15 @@ int bootstage_stash(void *base, int size) /* Write an arbitrary version number */ hdr->version = BOOTSTAGE_VERSION;
- /* Count the number of records, and write that value first */ - for (rec = data->record, i = count = 0; i < data->rec_count; - i++, rec++) { - if (rec->id != 0) - count++; - } - hdr->count = count; + hdr->count = data->rec_count; hdr->size = 0; hdr->magic = BOOTSTAGE_MAGIC; hdr->next_id = data->next_id; ptr += sizeof(*hdr);
/* Write the records, silently stopping when we run out of space */ - for (rec = data->record, i = 0; i < data->rec_count; i++, rec++) { + for (rec = data->record, i = 0; i < data->rec_count; i++, rec++) append_data(&ptr, end, rec, sizeof(*rec)); - }
/* Write the name strings */ for (rec = data->record, i = 0; i < data->rec_count; i++, rec++) {
The current code searches for empty records but these not existing with bootstage now. This used to be needed when bootstage records were stored in a spare array.
Drop the unnecessary code and fix a code-style nit at the same time.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/bootstage.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
Applied to u-boot-dm, thanks!
At present there is a single shared address for bootstage data in both TPL and SPL. If SPL unstashs TPL bootstage info and then stashes it again to pass it to U-Boot, the new stash overwrites the strings of the old stash.
Fix this by duplicating the strings into the malloc() region. This should be a small code. Fix the header-file order at the same time.
This problem doesn't happen at the next stage (SPL->U-Boot) since U-Boot relocates the boostage data.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/bootstage.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/common/bootstage.c b/common/bootstage.c index fe36bac0474..4557ed4508c 100644 --- a/common/bootstage.c +++ b/common/bootstage.c @@ -10,9 +10,10 @@ */
#include <common.h> -#include <linux/libfdt.h> #include <malloc.h> +#include <spl.h> #include <linux/compiler.h> +#include <linux/libfdt.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -472,6 +473,8 @@ int bootstage_unstash(const void *base, int size) for (rec = data->record + data->next_id, i = 0; i < hdr->count; i++, rec++) { rec->name = ptr; + if (spl_phase() == PHASE_SPL) + rec->name = strdup(ptr);
/* Assume no data corruption here */ ptr += strlen(ptr) + 1;
At present there is a single shared address for bootstage data in both TPL and SPL. If SPL unstashs TPL bootstage info and then stashes it again to pass it to U-Boot, the new stash overwrites the strings of the old stash.
Fix this by duplicating the strings into the malloc() region. This should be a small code. Fix the header-file order at the same time.
This problem doesn't happen at the next stage (SPL->U-Boot) since U-Boot relocates the boostage data.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/bootstage.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Applied to u-boot-dm, thanks!
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 ---
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(); } #endif
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)
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 ---
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(-)
Applied to u-boot-dm, thanks!
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)
Hi Heinrich,
On Sat, 25 Jan 2020 at 01:59, Heinrich Schuchardt xypron.debian@gmx.de wrote:
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.
Yes that's right. This is a bit tricky.
The original malloc() does not include space for strings, since the caller passes them in and we just use pointers.
When we relocate we copy the structure but then also write out the strings after it.
The only obvious solution is to store the total size of the bootstage record in the bootstage_data record, probably adding a version number as well.
Regards, Simon
At present bootstage in TPL and SPL use the same ID so it is not possible to see the timing of each. Separate out the IDs and use the correct one depending on which phase we are at.
Example output:
Timer summary in microseconds (14 records): Mark Elapsed Stage 0 0 reset 224,787 224,787 TPL 282,248 57,461 end TPL 341,067 58,819 SPL 925,436 584,369 end SPL 931,710 6,274 board_init_f 1,035,482 103,772 board_init_r 1,387,852 352,370 main_loop 1,387,911 59 id=175
Accumulated time: 196 dm_r 8,300 dm_spl 14,139 dm_f 229,121 fsp-m 262,992 fsp-s
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/spl/spl.c | 9 ++++++--- include/bootstage.h | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/common/spl/spl.c b/common/spl/spl.c index a9d3e847af5..eabb1fbc138 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -402,7 +402,8 @@ static int spl_common_init(bool setup_malloc) ret); return ret; } - bootstage_mark_name(BOOTSTAGE_ID_START_SPL, "spl"); + bootstage_mark_name(spl_phase() == PHASE_TPL ? BOOTSTAGE_ID_START_TPL : + BOOTSTAGE_ID_START_SPL, SPL_TPL_NAME); #if CONFIG_IS_ENABLED(LOG) ret = log_init(); if (ret) { @@ -418,7 +419,8 @@ static int spl_common_init(bool setup_malloc) } } if (CONFIG_IS_ENABLED(DM)) { - bootstage_start(BOOTSTATE_ID_ACCUM_DM_SPL, "dm_spl"); + bootstage_start(BOOTSTATE_ID_ACCUM_DM_SPL, + spl_phase() == PHASE_TPL ? "dm tpl" : "dm_spl"); /* With CONFIG_SPL_OF_PLATDATA, bring in all devices */ ret = dm_init_and_scan(!CONFIG_IS_ENABLED(OF_PLATDATA)); bootstage_accum(BOOTSTATE_ID_ACCUM_DM_SPL); @@ -704,8 +706,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2) debug("SPL malloc() used 0x%lx bytes (%ld KB)\n", gd->malloc_ptr, gd->malloc_ptr / 1024); #endif + bootstage_mark_name(spl_phase() == PHASE_TPL ? BOOTSTAGE_ID_END_TPL : + BOOTSTAGE_ID_END_SPL, "end " SPL_TPL_NAME); #ifdef CONFIG_BOOTSTAGE_STASH - bootstage_mark_name(BOOTSTAGE_ID_END_SPL, "end_spl"); ret = bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR, CONFIG_BOOTSTAGE_STASH_SIZE); if (ret) diff --git a/include/bootstage.h b/include/bootstage.h index 5e7e242b834..d105ae01813 100644 --- a/include/bootstage.h +++ b/include/bootstage.h @@ -170,6 +170,8 @@ enum bootstage_id { * rough boot timing information. */ BOOTSTAGE_ID_AWAKE, + BOOTSTAGE_ID_START_TPL, + BOOTSTAGE_ID_END_TPL, BOOTSTAGE_ID_START_SPL, BOOTSTAGE_ID_END_SPL, BOOTSTAGE_ID_START_UBOOT_F,
At present bootstage in TPL and SPL use the same ID so it is not possible to see the timing of each. Separate out the IDs and use the correct one depending on which phase we are at.
Example output:
Timer summary in microseconds (14 records): Mark Elapsed Stage 0 0 reset 224,787 224,787 TPL 282,248 57,461 end TPL 341,067 58,819 SPL 925,436 584,369 end SPL 931,710 6,274 board_init_f 1,035,482 103,772 board_init_r 1,387,852 352,370 main_loop 1,387,911 59 id=175
Accumulated time: 196 dm_r 8,300 dm_spl 14,139 dm_f 229,121 fsp-m 262,992 fsp-s
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/spl/spl.c | 9 ++++++--- include/bootstage.h | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-)
Applied to u-boot-dm, thanks!
It is possible to enable bootstage in TPL. TPL can stash the info for SPL. But at present this information is then lost because SPL does not read from the stash.
Add support for SPL not being the first phase to enable bootstage. Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Adjust SPL logic to avoid failing if TPL does not provide bootstage data - Add a new patch to support %p without DEBUG in tiny-printf
common/spl/spl.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/common/spl/spl.c b/common/spl/spl.c index eabb1fbc138..f1ad8dc9dab 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -18,6 +18,7 @@ #include <version.h> #include <image.h> #include <malloc.h> +#include <mapmem.h> #include <dm/root.h> #include <linux/compiler.h> #include <fdt_support.h> @@ -396,12 +397,23 @@ static int spl_common_init(bool setup_malloc) gd->malloc_ptr = 0; } #endif - ret = bootstage_init(true); + ret = bootstage_init(u_boot_first_phase()); if (ret) { debug("%s: Failed to set up bootstage: ret=%d\n", __func__, ret); return ret; } +#ifdef CONFIG_BOOTSTAGE_STASH + if (!u_boot_first_phase()) { + const void *stash = map_sysmem(CONFIG_BOOTSTAGE_STASH_ADDR, + CONFIG_BOOTSTAGE_STASH_SIZE); + + ret = bootstage_unstash(stash, CONFIG_BOOTSTAGE_STASH_SIZE); + if (ret) + debug("%s: Failed to unstash bootstage: ret=%d\n", + __func__, ret); + } +#endif /* CONFIG_BOOTSTAGE_STASH */ bootstage_mark_name(spl_phase() == PHASE_TPL ? BOOTSTAGE_ID_START_TPL : BOOTSTAGE_ID_START_SPL, SPL_TPL_NAME); #if CONFIG_IS_ENABLED(LOG)
It is possible to enable bootstage in TPL. TPL can stash the info for SPL. But at present this information is then lost because SPL does not read from the stash.
Add support for SPL not being the first phase to enable bootstage. Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Adjust SPL logic to avoid failing if TPL does not provide bootstage data - Add a new patch to support %p without DEBUG in tiny-printf
common/spl/spl.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
Applied to u-boot-dm, thanks!
participants (6)
-
Faiz Abbas -
Heinrich Schuchardt -
Simon Glass -
sjg＠google.com -
Stefan Roese -
Vignesh Raghavendra