Hi Tom,
On Mon, 2 Dec 2024 at 14:59, Tom Rini trini@konsulko.com wrote:
On Sun, Dec 01, 2024 at 08:28:05AM -0700, Simon Glass wrote:
The reserve_stack_aligned() function already ensures that the resulting address is aligned to a 16-byte boundary. The comment seems to suggest that 16 is passed reserve_stack_aligned() to make it aligned.
Change the value to 0, since the stack can start at the current address, if it is suitably aligned already.
Signed-off-by: Simon Glass sjg@chromium.org
Er: /*
- reserve after start_addr_sp the requested size and make the stack pointer
- 16-byte aligned, this alignment is needed for cast on the reserved memory
- ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
= ARMv8 Instruction Set Overview: quad word, 16 bytes*/ static unsigned long reserve_stack_aligned(size_t size) { return ALIGN_DOWN(gd->start_addr_sp - size, 16); }
is what the code is today.
(no changes since v1)
common/board_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d..677e37d93c0 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) static int reserve_stacks(void) { /* make stack pointer 16-byte aligned */
gd->start_addr_sp = reserve_stack_aligned(16);
gd->start_addr_sp = reserve_stack_aligned(0); /* * let the architecture-specific code tailor gd->start_addr_sp andAnd this is the last call to reserve_stack_aligned(). So, this is going to save us between 16 and 0 bytes of memory. If we're going to make a change here we need to comment in the code more what we're up to at this point, and the commit message should be more authoritatively worded too.
So, other than that, what do you think? Should I respin it? Note that this is part of a series which I doubt will be applied to your tree.
It would be nice if other archs could weigh in, but I see that I have not copied them. Still, the current code is confusing (in that the comment doesn't match the code), so we can likely figure this out in -next
Heinrich expressed some concerns with RISC-V, but there doesn't seem to be any breakage in CI. meminfo reports that the devicetree is above the stack and it semes intact:
$ qemu-system-riscv32 -nographic -machine virt -bios /tmp/b/qemu-riscv32/u-boot.bin
U-Boot 2025.01-rc3-00068-g6f392929ea11-dirty (Dec 03 2024 - 06:51:44 -0700)
CPU: riscv Model: riscv-virtio,qemu DRAM: 128 MiB Core: 26 devices, 13 uclasses, devicetree: board Flash: 32 MiB Loading Environment from nowhere... OK In: serial,usbkbd Out: serial,vidconsole Err: serial,vidconsole No USB controllers found Net: No ethernet found.
Hit any key to stop autoboot: 0 => meminfo DRAM: 128 MiB
Region Base Size End Gap ------------------------------------------------ video 87800000 800000 88000000 code 87740000 bf2d8 877ff2d8 d28 malloc 86f20000 820000 87740000 0 board_info 86f1ffc0 40 86f20000 0 global_data 86f1fe50 170 86f1ffc0 0 devicetree 86f1ef30 f02 86f1fe32 1e stack 85ede000 1000000 86ede000 40f30 lmb 85ede000 0 85ede000 0 lmb 85edb000 3000 85ede000 0 free 80000000 85edb000 5edb000 80000000 => md 86f1ef30 86f1ef30: edfe0dd0 020f0000 38000000 980d0000 ...........8.... 86f1ef40: 28000000 11000000 10000000 00000000 ...(............ 86f1ef50: 6a010000 600d0000 00000000 00000000 ...j...`........
Regards, Simon