On Mon, Feb 24, 2025 at 8:03 AM Tom Rini <tr...@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 <cfswo...@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
