On Tue, Dec 03, 2024 at 06:53:55AM -0700, Simon Glass wrote:
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.
I mean, yes, I've given you review comments. If you want this applied to mainline someday then you need to address them.
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
Yes, my comment was that you need to make the comment match the code and expand upon the common since I expect it's been that way for a decade+ at this point.
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:
Yes, I too was concerned about why exactly we're changing something here with a commit message that sounds unsure if it's correct to make these changes, only probably correct.