Re: [PATCH] lds: align u-boot-nodtb with 8 bytes boundary

["Pali Rohár" <pali@kernel.org>]

On Saturday 17 December 2022 23:54:40 Pali Rohár wrote:
> On Saturday 17 December 2022 23:04:08 Pali Rohár wrote:
> > On Saturday 17 December 2022 14:40:44 Simon Glass wrote:
> > > Hi Pali,
> > > 
> > > On Thu, 15 Dec 2022 at 16:13, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Thursday 15 December 2022 06:24:16 Simon Glass wrote:
> > > > > Hi Eugen,
> > > > >
> > > > > On Thu, 15 Dec 2022 at 03:58, Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > > > >
> > > > > > Newer DTC require that the DTB start address is aligned at 8 bytes.
> > > > > > In the u-boot.bin case, the u-boot-nodtb.bin is concatenated with the
> > > > > > DTB, but there is no alignment/padding to the next 8byte aligned address.
> > > > > > This causes the board to fail booting, because the FDT will claim
> > > > > > that the DTB inside u-boot.bin is not a valid DTB, it will fail with
> > > > > > -FDT_ERR_ALIGNMENT.
> > > > > > To solve this, have the u-boot binary `_end` aligned with 8 bytes.
> > > > > > The objcopy in the Makefile will create the u-boot-nodtb.bin and it has to
> > > > > > be truncated to 8 bytes to correspond to the u-boot.map file which will
> > > > > > have the `_end` aligned to 8 bytes.
> > > > > > The lds files which use devicetrees have been changed to align the `_end`
> > > > > > tag with 8 bytes.
> > > > > >
> > > > > > This patch is also a prerequisite to have the possibility to update the
> > > > > > dtc inside u-boot to newer versions (1.6.1+)
> > > > > >
> > > > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > > ---
> > > > > > Hi,
> > > > > >
> > > > > > I could not test all affected boards, it's an impossible task.
> > > > > > I tried this on at91 boards which I have, and ran the CI on denx.
> > > > > > I cannot guarantee that no other boards are affected, so this patch is a bit
> > > > > > of an RFC.
> > > > > > If the u-boot-nodtb.bin does not have the size equal with the corresponding
> > > > > > one in u-boot.map, the binary_size_check will fail at build time with
> > > > > > something like this:
> > > > > >
> > > > > > u-boot.map shows a binary size of 502684
> > > > > > but u-boot-nodtb.bin shows 502688
> > > > > >
> > > > > > Thanks,
> > > > > > Eugen
> > > > > >
> > > > > >  Makefile                                    | 2 ++
> > > > > >  arch/arm/cpu/armv8/u-boot.lds               | 4 ++--
> > > > > >  arch/arm/cpu/u-boot-spl.lds                 | 1 +
> > > > > >  arch/arm/cpu/u-boot.lds                     | 1 +
> > > > > >  arch/arm/lib/elf_arm_efi.lds                | 5 +++++
> > > > > >  arch/arm/mach-at91/arm926ejs/u-boot-spl.lds | 2 +-
> > > > > >  arch/arm/mach-at91/armv7/u-boot-spl.lds     | 2 +-
> > > > > >  arch/arm/mach-zynq/u-boot-spl.lds           | 2 +-
> > > > > >  arch/mips/cpu/u-boot.lds                    | 2 +-
> > > > > >  arch/sandbox/cpu/u-boot.lds                 | 6 ++++++
> > > > > >  arch/sh/cpu/u-boot.lds                      | 2 ++
> > > > > >  board/ti/am335x/u-boot.lds                  | 1 +
> > > > > >  tools/binman/test/u_boot_binman_embed.lds   | 2 +-
> > > > > >  13 files changed, 25 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/Makefile b/Makefile
> > > > > > index 9d84f96481..b4d387bcce 100644
> > > > > > --- a/Makefile
> > > > > > +++ b/Makefile
> > > > > > @@ -1317,6 +1317,8 @@ endif
> > > > > >
> > > > > >  u-boot-nodtb.bin: u-boot FORCE
> > > > > >         $(call if_changed,objcopy_uboot)
> > > > > > +# Make sure the size is 8 byte-aligned.
> > > > > > +       @truncate -s %8 $@
> > > > > >         $(BOARD_SIZE_CHECK)
> > > > >
> > > > > I agree this line is needed, since otherwise we will only get 4-byte
> > > > > alignment.
> > > >
> > > > Hello! I do not fully agree that this line is needed.
> > > >
> > > > The whole issue is about DTB binary and its offset. So code/Makefile
> > > > which construct u-boot.bin should be fixed and not u-boot-nodtb.bin.
> > > 
> > > While I agree that is true in theory, im practice we do actually need
> > > the _image_binary_end symbol to point to the right place. It is
> > > hopelessly confusing if u-boot-nodtb.bin is shorter than indicated by
> > > that symbol, and this is why we have binary_size_check
> > > 
> > > So I believe that padding u-boot-nodtb.bin is the best solution.
> > > 
> > > With binman we can fairly easily pad to solve the problem, e.g. by
> > > always adding 'align = <8>' to dtb entries, but when using 'cat' to
> > > put images together, it is much easier if the original image has an
> > > aligned size.
> > 
> > I think that we should write rules to produce u-boot*.bin binaries of
> > correct size (as it is written in ELF metadata) and not "workarounding"
> > it by "truncate -s %8" command or "align = <8>" binman directive.
> > 
> > Either by reading correct size from ELF or MAP file and then manually
> > calling "truncate -s" or by issuing objcopy or "fixing" linker / script
> > to do it.
> > 
> > And it is because position where DTB file starts is already defined in
> > linker script. And this should be source of the truth.
> > 
> > So I'm fine with fixing also u-boot-nodtb.bin target but not by
> > "@truncate -s %8 $@" rule.
> > 
> > > >
> > > > > But it would be better if we could have the linker scripts
> > > > > fill bytes out to the required alignment. Is that possible?
> > > >
> > > > I already investigated this. LD linker and objcopy (at least older
> > > > version in Debian 10) drops trailing zero bytes in raw binary output.
> > > >
> > > > You can specify zero bytes in the linker script and they are filled in
> > > > ELF or COFF output. But not in raw binary, which u-boot.bin is.
> > > 
> > > Is that really what is happening? If you use BYTE(0) does it actually
> > > get dropped but with BYTE(1) it doesn't? That sounds very broken.
> > > 
> > > I thought it was actually because updating the current location
> > > doesn't actually change what is there, e.g.:
> > > 
> > >    . = . + 4
> > > 
> > > just adds to the location pointer, whereas
> > > 
> > >    . = . + 4
> > >    BYTE(0)
> > > 
> > > actually adds a byte there. But I do find it confusing so it would be
> > > great to know how to do this properly.
> > 
> > I was playing with mpc85xx linker scripts and I was not able to instruct
> > linker to put padding into output raw binary file. If I manually changed
> > padding filler from default 0x00 to 0xff then 0xff bytes were added.
> > 
> > And some linker scripts already contains directive to change padding
> > byte to 0xff just to ensure that some section is not truncated.
> > 
> > I'm not sure if problem is in u-boot build system, linker script or in
> > linker itself (I used older version of GNU LD for powerpc). And I got
> > into impression that issue is in the linker. Hence I changed alignment
> > to just 4 bytes (in commits mentioned below) and it fixes these issues.
> > 
> > I do not remember all details. So this issue should be re-investigated
> > again and ideally fixed (or workarounded) that raw binary can be
> > correctly padded with any filler, including zero byte.
> 
> Ok, the explanation is already in commit message e8c0e0064c8a.
> 
> BYTE(0) works but it is useless as it cannot be used directly in
> SECTIONS { ... } part of linker script.
> 
> So common pattern
> 
>   . = ALIGN(256);
>   _end = .;
> 
> does not cause that RAW output binary would have trailing zero bytes.
> So symbol _end does not have to be immediately after the RAW binary and
> there can be a gap.
> 
> So it would be needed to read address of "_end" symbol from u-boot.map
> and put DTB file at this position when generating final u-boot.bin
> binary. I think that objcopy should do it, but cat obviously not.

objcopy has parameter --pad-to= which can be used to pad bytes up to the
some address, e.g. address of "_end" symbol where DTB should start.

So something like this which read address of "_end" symbol from
u-boot.map file could be used to generate u-boot-nodtb.bin file with
correct size:

  addr=$(sed -n 's/^\s*\([^\s]*\)\s\s*_end\s*=\s*\.\s*$/\1/p' u-boot.map)
  objcopy -O binary --pad-to=$addr u-boot u-boot-nodtb.bin

Or it is possible to use also nm to get address of "_end" symbol:

  addr=$(nm -n u-boot | sed -n 's/ . _end$//p')

> Also binman should be teach to put DTB file at location of "_end" symbol
> and _not_ immediately after the u-boot-nodtb.bin binary.
> 
> > > >
> > > > So it could be possible to extract "correct" size from ELF binary and
> > > > call truncate on raw binary generated from objcopy.
> > > >
> > > >
> > > >
> > > > I already hit this trailing-zeros issue for powerpc and I fixed it in:
> > > > 7696b80ec5e9 powerpc: mpc85xx: Fix loading U-Boot proper from SD card in SPL
> > > > b898f6a6db76 powerpc: mpc85xx: Fix CONFIG_OF_SEPARATE support for NOR booting
> > > > e8c0e0064c8a powerpc: mpc85xx: Fix CONFIG_OF_SEPARATE support
> > > >
> > > > All those commits re-align output to just 4 bytes, not more.
> > > >
> > > > So Makefile change in this proposed patch is going to break all
> > > > powerpc/mpc85xx boards.
> > > 
> > > Regards,
> > > Simon