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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]

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@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,
> 
> spl_image->load_addr is always a multiple of 8.
> 
> Adding
> 
>    . = ALIGN(8)"
> 
> in arch/riscv/cpu/u-boot-spl.lds before
> 
>     _end = .;
>     _image_binary_end = .;
> 
> is all it takes.

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]