[PATCH 10/17] spl: Align FDT load address

[Sam Edwards]

On Mon, Feb 24, 2025 at 8:03 AM Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Feb 24, 2025 at 04:54:09PM +0100, Heinrich Schuchardt wrote:
> > On 24.02.25 15:56, Tom Rini wrote:
> > > On Mon, Feb 24, 2025 at 09:59:42AM +0100, Heinrich Schuchardt wrote:
> > > > On 2/24/25 06:55, Sam Edwards wrote:
> > > > > While the image size is generally a multiple of 8 bytes, this is not
> > > > > actually guaranteed; some linkers (like LLD) may shave a few bytes off
> > > > > of the end of output sections if there are no content bytes there. Since
> > > > > libfdt imposes a hard rule of 8-byte alignment, make the SPL also be
> > > > > explicit about the alignment when loading the FDT.
> > > > >
> > > > > Signed-off-by: Sam Edwards <CFSworks at gmail.com>
> > > > > ---
> > > > >    common/spl/spl_fit.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > > > > index 49b4df60560..86506d6905c 100644
> > > > > --- a/common/spl/spl_fit.c
> > > > > +++ b/common/spl/spl_fit.c
> > > > > @@ -397,7 +397,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
> > > > >          * Use the address following the image as target address for the
> > > > >          * device tree.
> > > > >          */
> > > > > -       image_info.load_addr = spl_image->load_addr + spl_image->size;
> > > > > +       image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
> > > >
> > > > We want to keep the SPL code size as small as possible as on many
> > > > platforms it is restricted to the cache size.
> > > >
> > > > Can't we fix this linker issue in the linker script by properly aligning
> > > > the SPL image end address?
> > >
> > > Size growth is always something to watch for, but not at the expense of
> > > correctness and saving a few bytes. We really do need to fix the places
> > > where U-Boot could but doesn't ensure the device tree is correctly
> > > aligned in memory.
> > >
> >
> > Hello Tom,
> >

Hi gents,

> > spl_image->load_addr is always a multiple of 8.

So while spl_image->load_addr is 8-aligned, spl_image->size appears to
have no such guarantees.

When loaded from FIT, the size is simply the data length of the U-Boot
image, which is itself the filesize of `u-boot-nodtb.bin`. That ends
up being determined by the whims of the toolchain (ld+objcopy).

> >
> > Adding
> >
> >    . = ALIGN(8)"
> >
> > in arch/riscv/cpu/u-boot-spl.lds before
> >
> >     _end = .;
> >     _image_binary_end = .;
> >
> > is all it takes.

While that guarantees that `_end` and `_image_binary_end` are aligned
on an 8-byte boundary, that does not necessarily mean that sufficient
padding bytes will be emitted, so the `u-boot-notdb.bin` file may
still end immediately after the last included content byte.

If there is a hard rule that the size must be a multiple of 8, we
should probably be enforcing/padding it when the FIT is generated.
(And then, a comment here in spl_fit.c explaining that it is
guaranteeing the alignment by depending on load_addr/size assumptions
would go a long way toward keeping those assumptions valid.) But if
that rule does not exist, and it's actually valid for the size to be
whatever odd length, then this patch is necessary for correctness.

> But this is generic code and I don't see how we know that in every case
> every way we could be reading the device tree that it will be at an 8
> byte aligned location. There's a number of ways today where it's not,
> which is what Patrice found as part of updating our own libfdt to one
> that enforces the alignment check.

I mentioned in my reply to patch 9/17 of this series that I would also
be fine with a common assert that catches any misaligned FDTs before
they are passed to third-party components for consumption. I suspect
the main reason that many of these cases are slipping through is
because some of those components are tolerant of misaligned FDTs, and
if a developer is only testing against those then they won't notice
the misalignment. Putting something in the unconditional code path of
the SPL that enforces the FDT alignment ought to put a stop to that.

Thoughts?

Cheers,
Sam

>
> --
> Tom