On Thu, Dec 15, 2022 at 07:00:51AM -0800, Simon Glass wrote:
> Hi Eugen,
>
> On Thu, 15 Dec 2022 at 06:37, <eugen.hris...@microchip.com> wrote:
> >
> > On 12/15/22 16:24, Simon Glass wrote:
> > > Hi Eugen,
> > >
> > > On Thu, 15 Dec 2022 at 03:58, Eugen Hristev <eugen.hris...@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.hris...@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. But it would be better if we could have the linker scripts
> > > fill bytes out to the required alignment. Is that possible?
> > >
> > > Reviewed-by: Simon Glass <s...@chromium.org>
> > >
> > > Regards,
> > > Simon
> >
> >
> > Hi Simon,
> >
> > I tried to check the objcopy option --pad-to , to use it at the time of
> > objcopy, but this requires a real number to be passed to it.
> > And this number could only be found by inspecting the u-boot.map file,
> > since u-boot-nodtb.bin still does not exist.
> > And if we pad to the size specified in u-boot.map, then
> > binary_size_check does not make much sense anymore, as we will basically
> > use the same information to fit the file, and it will always pass with a
> > success. (even if we would pad many more bytes than 4 )
> > Hence it would lose it's purpose ( binary_size_check ), which I think
> > was created to make sure no objects were lost when doing objcopy and
> > creating the u-boot-nodtb.bin file.
>
> Yes, I was more thinking of something like:
>
> fill {
> . = ALIGN(8);
> QUAD(0)
> };
>
> in the link script, or something that actually writes the padding bytes.
>
> >
> > On a side note, do you think I covered all the implied lds files ? I
> > would hate to break someone's boards.
>
> If CI passes you should be able to rely on the binary size check.
>
> >
> > And also, P.S. : I would require to have the same change when building a
> > FIT image with mkimage... all subimages inside a FIT must be aligned to
> > 8 bytes. However mkimage only aligns the start address and header of the
> > FIT (-B option). Out of your knowledge, is this possible and where could
> > I have a look to do this change ?
>
> I lean towards the view that this 8-byte alignment is a bad idea.I'm still working my way through this thread, but, the 8-byte requirement is hard, real, and has really been a forever-and-ever requirement. It wasn't enforced for a very long time and so various other problems cropped up over the years (the whole "dtb isn't aligned, so we need to do unaligned safe slow reads on everything" thread may ring a bell). So, please, this has been on my TODO-again list for a while, so I'm glad it was picked up by Eugen here, and whatever our final form here takes, it does need to start with "8-byte alignment is required". -- Tom
signature.asc
Description: PGP signature
