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

[Simon Glass]

Hi Tom,

On Tue, 3 Dec 2024 at 07:00, Tom Rini <trini at konsulko.com> 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 <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.

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