Hi Jonas,
On 8/24/26 5:59 PM, Jonas Karlman wrote:
Hi Quentin,
On 8/24/2026 3:45 PM, Quentin Schulz wrote:
Hi Jonas,
On 8/10/26 11:14 PM, Jonas Karlman wrote:
The PX30/RK3326 has 16 KiB SRAM at [0xff0e0000, 0xff0e4000), with the
initial 4 KiB at [0xff0e0000, 0xff0e1000) reserved for BootROM at boot.
SRAM addressing only seems to use 14 bits, meaning that reading from
0xff0e4000+ wraps around and instead reads back data at 0xff0e0000+.
Using a TPL_STACK at 0xff0e4ff0 (16-bytes aligned) means TPL use BootROM
reserved region for its global data, malloc area and runtime stack.
This should be its own commit.
The main purpose of this patch is to fix the overflow of TPL into lower
4 KiB of SRAM, i.e. the area used as BootROMs stack and state. Returning
to BootROM to load next image (SPL) could possible be affected depending
on TPLs use of malloc and/or its stack usage.
Yeah but that's already the case no? It's not because there is one issue
that there's necessarily one cause to the issue, or many ways to trigger
the issue.
All changes have been kept in a single patch to make changes safe for
bisect. If we just change TPL_STACK and keep malloc size, or just change
Bisect isn't necessarily useful if we start with a broken state. It
matters if we somehow make it worse, which we shouldn't.
malloc size TPL may start to accidentally overwrite areas in the BootROM
reserved area that affects BootROMs ability to load next image or the
TPL stack may start to overwrite end of TPL image.
E.g. if we only change TPL_STACK we end up with 136 byte free for
stack usage before stack can start to overwrite TPL BSS/code:
[0xff0e0000, 0xff0e4000) 16 KiB SRAM
[0xff0e0000, 0xff0e1000) 4 KiB BROM area
[0xff0e1000, 0xff0e4000) 12 KiB TPL area
[0xff0e1000, 0xff0e3800) 10 KiB code
[0xff0e3800, 0xff0e3848) 72 bytes BSS
[0xff0e38d0] stack pointer
[0xff0e3848, 0xff0e38d0] 136 bytes stack area
[0xff0e38d0, 0xff0e3ed0) 1536 bytes malloc area (unused)
[0xff0e3ed0, 0xff0e4000) 304 bytes global data
And if we change malloc size the TPL stack likely move closer to
BootROMs own stack. Current combo of TPL_STACK, GD_SIZE and MALLOC_F_LEN
seem to not affect BootROM but that could change if any one of them
changes independently.
TPL on PX30/RK3326 does not have any use for the malloc area, the
generated GD_SIZE is typically around 304 bytes and BSS around 72 bytes.
As far as I understood, this is space/size optimization and not a bug
fix, so should therefore be its own commit.
The only part that possible safely can be moved to its own commit is the
increase to 11 KiB for code size. Mostly wanted to keep this together
to help for future references as this patch then have all details on why
such size change was possible and could be considered safe.
Once the issues are fixed and merged, you can always point at a git
commit hash in another patch.
Current memory layout possibly looks something like:
[0xff0e0000, 0xff0e4000) 16 KiB SRAM
[0xff0e0000, 0xff0e1000) 4 KiB BROM area
[0xff0e09f0, 0xff0e0ff0) 1536 bytes malloc area (unused)
[0xff0e08c0, 0xff0e09f0) 304 bytes global data
[0xff0e08c0] stack pointer
[0xff0e1000, 0xff0e4000) 12 KiB TPL area
[0xff0e1000, 0xff0e3800) 10 KiB code
[0xff0e3800, 0xff0e3848) 72 bytes BSS
Update TPL_STACK to use the top of SRAM address and reduce MALLOC_F_LEN
to 0x0 to avoid overflowing global data and stack into BootROM reserved
region.
Aren't we then missing a build-time check to make sure this is not possible?
Not sure how we could build-time check this without a guesstimate on how
deep runtime stack is needed.
Fair enough, we know the pool we've reserved for the heap (malloc) but
we cannot reserve space for the stack so we cannot know.
After those changes the memory layout looks something like:
[0xff0e0000, 0xff0e4000) 16 KiB SRAM
[0xff0e0000, 0xff0e1000) 4 KiB BROM area
[0xff0e1000, 0xff0e4000) 12 KiB TPL area
[0xff0e1000, 0xff0e3800) 10 KiB code
[0xff0e3800, 0xff0e3848) 72 bytes BSS
[0xff0e3ed0, 0xff0e4000) 304 bytes global data
[0xff0e3ed0] stack pointer
[0xff0e3848, 0xff0e3ed0) 1672 bytes stack area
With these changes the TPL_MAX_SIZE can safely be extended to 11 KiB,
leaving enough for the stack to grow down around 648 bytes before it
starts to overlap into TPL or BSS.
And with all changes the memory layout looks something like:
[0xff0e0000, 0xff0e4000) 16 KiB SRAM
[0xff0e0000, 0xff0e1000) 4 KiB BROM area
[0xff0e1000, 0xff0e4000) 12 KiB TPL area
[0xff0e1000, 0xff0e3c00) 11 KiB code
[0xff0e3c00, 0xff0e3c48) 72 bytes BSS
[0xff0e3ed0, 0xff0e4000) 304 bytes global data
[0xff0e3ed0] stack pointer
[0xff0e3c48, 0xff0e3ed0) 648 bytes stack area
Signed-off-by: Jonas Karlman <[email protected]>
Reviewed-by: Simon Glass <[email protected]>
---
The overflow/wrapping was runtime validated on a RK3326 ODROID-GO Super
by printing out content of [0xff0e0000, 0xff0e5000] and comparing the
initial 4 KiB with the 16-20 KiB and the data matched 1:1.
How did you test that? md.l on either address results in a Synchronous
Abort on my PX30 without your patch. We could also just write a random
pattern only at one address, and if it appears at the other address as
well, then you've discovered an address wrap indeed.
I used something like following to dump content as early as possible
from TPL. I used something that printed multiple columns, and compared
That was the hint I was missing. This likely means TF-A protects the
area somehow. I removed TF-A from u-boot.itb and I could compare both
addresses with:
=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same
Funnily enough, I can increase the area to check to 0x3000 words but
anything after that results in a Synchronous abort.
the output for 0x0-0x1000 and 0x4000-0x5000. Think I also tried to write
something and could see the values being reflected in both places.
=> md.l 0xff0e0000 0x4
ff0e0000: 62144471 b9d836bc 5ef2ce5b e00c1527 qD.b.6..[..^'...
=> random 0xff0e0000 0x10
=> md.l 0xff0e0000 0x4
ff0e0000: a727051d b7145d3f aac7cfa6 3557e1df ..'.?]........W5
=> md.l 0xff0e4000 0x4
ff0e4000: a727051d b7145d3f aac7cfa6 3557e1df ..'.?]........W5
=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same
=> random 0xff0e0000 0x4000
16384 bytes filled with random data
=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same
=> random 0xff0e4000 0x4000
16384 bytes filled with random data
=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same
So I think this proves what you've just said.
void tpl_board_init(void)
{
for (int i = 0; i < 0x5000 / sizeof(u32); i++) {
ulong addr = SRAM_BASE + i * sizeof(u32);
u32 val = readl(addr);
if (!val)
continue;
printascii("SRAM ");
printhex8(addr);
printascii(": ");
printhex8(val);
printascii("\n");
}
}
Changes in v2:
- Mention BSS in commit message
- Update spelling in commit message
- Collect r-b tag
With the patch "global_data: Only include driver model fields when DM is
enabled" [1], the stack area can grow down an additional 48 bytes before
it starts to overlap into TPL or BSS.
[1] https://patch.msgid.link/[email protected]/
---
arch/arm/mach-rockchip/px30/Kconfig | 4 ++--
common/spl/Kconfig.tpl | 2 +-
tools/rkcommon.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-rockchip/px30/Kconfig
b/arch/arm/mach-rockchip/px30/Kconfig
index adba1b49a52b..cc0632cf7b2d 100644
--- a/arch/arm/mach-rockchip/px30/Kconfig
+++ b/arch/arm/mach-rockchip/px30/Kconfig
@@ -81,10 +81,10 @@ config TPL_TEXT_BASE
default 0xff0e1000
config TPL_STACK
- default 0xff0e4fff
+ default 0xff0e4000
config TPL_SYS_MALLOC_F_LEN
- default 0x600
+ default 0x0
Disable malloc in TPL on PX30 instead, that's actually what you want to
be doing and not "reserve 0 bytes for malloc". That's gated by
TPL_SYS_MALLOC_F.
Ideally we should disable TPL_SYS_MALLOC_F, but I thought it was too
obtrusive to change the default value for TPL_SYS_MALLOC_F as part of
this fix, so opted to just use 0x0 as size limit as there was no
difference in code for the resulting TPL binary. The only benefit of
TPL_SYS_MALLOC_F being disabled, beside being schematically more correct,> is
that GD_SIZE could be around 24 bytes smaller.
I can set the TPL_SYS_MALLOC_F_LEN to 0x200 and fully drop the 10 to 11
KiB code change in a v3 if you prefer that over this?
If we don't malloc() today, does it matter how much TPL_SYS_MALLOC_F_LEN
is? I won't take the TPL_SYS_MALLOC_F_LEN = 0 change, now or later. If
we really can disable malloc in TPL without making our lives much harder
in the future, I still think we should aim for separate commits for
disabling it and then reclaiming its space on the SRAM for the TPL max size.
Is there any chance you can check we really do not use malloc() (and
associated, like realloc, memalign, valloc, pvalloc, calloc,
malloc_trim, ...) in TPL, I'm a bit concerned by what this will limit us
to in the future as well. The helptext only says the driver model (I'm
assuming DM/xPL_DM) needs this but it isn't actually enforced at the
Kconfig-level of the DM/xPL_DM symbols.
config DEBUG_UART_CHANNEL
int "Mux channel to use for debug UART2/UART3"
diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl
index a535b61ecd35..cdb9cb188c7c 100644
--- a/common/spl/Kconfig.tpl
+++ b/common/spl/Kconfig.tpl
@@ -129,7 +129,7 @@ config TPL_MAX_SIZE
default 0x2e000 if ROCKCHIP_RK3399
default 0x8000 if ROCKCHIP_RK3288 || ROCKCHIP_RV1126
default 0x7000 if ROCKCHIP_RK322X || ROCKCHIP_RK3328 || ROCKCHIP_RK3368
- default 0x2800 if ROCKCHIP_PX30
+ default 0x2c00 if ROCKCHIP_PX30
default 0x0
help
The maximum size (in bytes) of the TPL stage.
diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index b39777fc0607..872fc21c039d 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -137,7 +137,7 @@ struct spl_info {
};
static struct spl_info spl_infos[] = {
- { "px30", "RK33", 0x2800, false, RK_HEADER_V1 },
+ { "px30", "RK33", 0x2c00, false, RK_HEADER_V1 },
Downstream actually has 0x3000 there with the justification that SRAM is
16KiB and BootROM has 4KiB reserved for the stack, c.f. 3bbe0d2f9129
("tools: rkcommon: add rk3326 and correct spl size for px30"), why are
we still 0x400 below?
I actually tried to change to use 0x3000 value in a prior patch, but
dropped the change due to a response from Kever [1].
I understood Kever's comment as "only take what's in downstream, no
extra changes" and 0x3000 is coming from downstream. From looking at the
changes, the RK3368 one you suggested back then is different from what's
downstream (0x10000 - 0x1000 instead of 0x8000 - 0x1000), same for
RK3288 with 0x18000 - 0x1000 vs 0x8000. The rest is identical in result
although they may be written differently (e.g. 0x2000 - 0x800 instead of
0x1800).
For most RK SoCs mkimage use a hard size limit and only exclude the BROM
reserved area, and for some reason on PX30 the size limit also likely
leave room for U-Boot TPL runtime needs, e.g. BSS/malloc/stack use.
In my opinion the mkimage tool should not try to impose any limit on
what the packaged image does or require during runtime, it should just
be a simple tool that packages binary images into bootable idblock
format and only apply hard limits.
We need mkimage to validate the generated image is usable. I think
0x2800 for the usable SRAM size for PX30 was a mistake made by Rockchip
in their downstream and we just used that when upstreaming support for
it. Downstream fixed it in 3bbe0d2f9129 ("tools: rkcommon: add rk3326
and correct spl size for px30"), dated "Fri Jan 7 15:22:54 2022 +0800",
while upstream support for PX30 (in tools/rkcommon.c) got added in
c6e66b12e4cd ("rockchip: mkimage: add support for px30"), dated "Fri Jul
12 11:43:34 2019 +0200", so it's just that nobody bothered fixing this
upstream yet.
I don't know if we take into account the malloc pool size when computing
the size of the binary (or if we should), but there's a tool for that I
believe in tools/spl_size_limit.c that's triggered if
CONFIG_SPL_SIZE_LIMIT != 0x0.
Cheers,
Quentin
