Hi Tom, On Tue, 3 Dec 2024 at 07:00, Tom Rini <[email protected]> wrote: > > 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 <[email protected]> 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 <[email protected]> > > > > > > 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 and > > > > > > And 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.
OK, it is true I was hoping for some comments. Unfortunately the code has been there forever. But I'll reword it to be more assertive. Regards, Simon
