[PATCH v5 3/9] board_f: Correct stack reservation

[Tom Rini]

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 at 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 at 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 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241203/f7c393d7/attachment.sig>