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

[Simon Glass]

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.

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