| Message ID | 20221215115825.827111-1-eugen.hristev@microchip.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | lds: align u-boot-nodtb with 8 bytes boundary | expand |
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. 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 <sjg@chromium.org> Regards, Simon
On 12/15/22 16:24, 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. 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 <sjg@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. On a side note, do you think I covered all the implied lds files ? I would hate to break someone's boards. 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 ? Thanks ! Eugen
Hi Eugen, On Thu, 15 Dec 2022 at 06:37, <Eugen.Hristev@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.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. 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 <sjg@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. Does libfdt itself support this sort of thing? We'll need to be able to tell it to align the value of a property to 8 bytes, both in the sw interface and the normal one. For binman to support it this is pretty easy once the sw interface is updated - see _add_node() in fit.py. For mkimage to support it, see fit_import_data(). Perhaps need a libfdt function like fdt_setprop_align()...? Regards, Simon
On 12/15/22 16:00, Simon Glass wrote: > Hi Eugen, > > On Thu, 15 Dec 2022 at 06:37, <Eugen.Hristev@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.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. 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 <sjg@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. FYI: I dealt with this 8byte alignment issue in past too. And here are patches. f28a22d55a5d ("arm64: dts: Make sure that all DTBs are 64bit aligned") 570c4636808e ("Makefile: Align fit-dtb.blob and u-boot.itb by 64bits") 5bd5ee02b23b ("xilinx: zynqmp: Check that DT is 64bit aligned") The issue was related to reserved memory node and functions called there. Thanks, Michal
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. > 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. 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.
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. > > > 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. > > 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
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. > > > > 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
> From: Eugen Hristev <eugen.hristev@microchip.com> > Date: Thu, 15 Dec 2022 13:58:25 +0200 > > 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 $@ $ truncate ksh: truncate: not found In other words: truncate(1) isn't a standard UNIX utility and not present on OpenBSD for example. It isn't part of POSIX and therefore its usage is unportable. Please find a different solution. > $(BOARD_SIZE_CHECK) > > u-boot.ldr: u-boot > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds > index 8fe4682dd2..e5fa4ef95c 100644 > --- a/arch/arm/cpu/armv8/u-boot.lds > +++ b/arch/arm/cpu/armv8/u-boot.lds > @@ -145,10 +145,10 @@ SECTIONS > *(.__rel_dyn_end) > } > > - _end = .; > - > . = ALIGN(8); > > + _end = .; > + > .bss_start : { > KEEP(*(.__bss_start)); > } > diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds > index fb2189d50d..732ed42db1 100644 > --- a/arch/arm/cpu/u-boot-spl.lds > +++ b/arch/arm/cpu/u-boot-spl.lds > @@ -55,6 +55,7 @@ SECTIONS > > .end : > { > + . = ALIGN(8); > *(.__end) > } > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > index f25f72b2e0..274e1a7d30 100644 > --- a/arch/arm/cpu/u-boot.lds > +++ b/arch/arm/cpu/u-boot.lds > @@ -193,6 +193,7 @@ SECTIONS > > .end : > { > + . = ALIGN(8); > *(.__end) > } > > diff --git a/arch/arm/lib/elf_arm_efi.lds b/arch/arm/lib/elf_arm_efi.lds > index 767ebda635..f3867021d3 100644 > --- a/arch/arm/lib/elf_arm_efi.lds > +++ b/arch/arm/lib/elf_arm_efi.lds > @@ -54,6 +54,11 @@ SECTIONS > .rel.data : { *(.rel.data) *(.rel.data*) } > _data_size = . - _etext; > > + .end : { > + . = ALIGN(8); > + *(.__end) > + } > + > /DISCARD/ : { > *(.rel.reloc) > *(.eh_frame) > diff --git a/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds b/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds > index 1a8bf94dee..d7e5d81ecc 100644 > --- a/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds > +++ b/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds > @@ -31,7 +31,7 @@ SECTIONS > . = ALIGN(4); > __u_boot_list : { KEEP(*(SORT(__u_boot_list*))) } > .sram > > - . = ALIGN(4); > + . = ALIGN(8); > __image_copy_end = .; > > .end : > diff --git a/arch/arm/mach-at91/armv7/u-boot-spl.lds b/arch/arm/mach-at91/armv7/u-boot-spl.lds > index 6ca725fc4c..2a0a1b4b86 100644 > --- a/arch/arm/mach-at91/armv7/u-boot-spl.lds > +++ b/arch/arm/mach-at91/armv7/u-boot-spl.lds > @@ -38,7 +38,7 @@ SECTIONS > . = ALIGN(4); > __u_boot_list : { KEEP(*(SORT(__u_boot_list*))) } > .sram > > - . = ALIGN(4); > + . = ALIGN(8); > __image_copy_end = .; > > .end : > diff --git a/arch/arm/mach-zynq/u-boot-spl.lds b/arch/arm/mach-zynq/u-boot-spl.lds > index 8c18d3f91f..c0320dbe1e 100644 > --- a/arch/arm/mach-zynq/u-boot-spl.lds > +++ b/arch/arm/mach-zynq/u-boot-spl.lds > @@ -41,7 +41,7 @@ SECTIONS > KEEP(*(SORT(__u_boot_list*))); > } > .sram > > - . = ALIGN(4); > + . = ALIGN(8); > > _image_binary_end = .; > > diff --git a/arch/mips/cpu/u-boot.lds b/arch/mips/cpu/u-boot.lds > index 9a4ebcd151..182e9bc6e5 100644 > --- a/arch/mips/cpu/u-boot.lds > +++ b/arch/mips/cpu/u-boot.lds > @@ -56,7 +56,7 @@ SECTIONS > . += CONFIG_MIPS_RELOCATION_TABLE_SIZE - 4; > } > > - . = ALIGN(4); > + . = ALIGN(8); > _end = .; > > .bss __rel_start (OVERLAY) : { > diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds > index ba8dee50c7..3fbca6d248 100644 > --- a/arch/sandbox/cpu/u-boot.lds > +++ b/arch/sandbox/cpu/u-boot.lds > @@ -51,6 +51,12 @@ SECTIONS > *(.dynsym) > __dyn_sym_end = .; > } > + > + .end : > + { > + . = ALIGN(8); > + *(.__end) > + } > } > > INSERT BEFORE .data; > diff --git a/arch/sh/cpu/u-boot.lds b/arch/sh/cpu/u-boot.lds > index d360eea7eb..226f50b19c 100644 > --- a/arch/sh/cpu/u-boot.lds > +++ b/arch/sh/cpu/u-boot.lds > @@ -74,6 +74,8 @@ SECTIONS > KEEP(*(SORT(__u_boot_list*))); > } >ram > > + . = ALIGN(8); > + > PROVIDE (__init_end = .); > PROVIDE (reloc_dst_end = .); > PROVIDE (_end = .); > diff --git a/board/ti/am335x/u-boot.lds b/board/ti/am335x/u-boot.lds > index 087dee8bb2..deb9098a90 100644 > --- a/board/ti/am335x/u-boot.lds > +++ b/board/ti/am335x/u-boot.lds > @@ -118,6 +118,7 @@ SECTIONS > > .end : > { > + . = ALIGN(8); > *(.__end) > } > > diff --git a/tools/binman/test/u_boot_binman_embed.lds b/tools/binman/test/u_boot_binman_embed.lds > index e213fa8a84..93c0ae6aed 100644 > --- a/tools/binman/test/u_boot_binman_embed.lds > +++ b/tools/binman/test/u_boot_binman_embed.lds > @@ -18,7 +18,7 @@ SECTIONS > *(.text*) > } > > - . = ALIGN(4); > + . = ALIGN(8); > .data : { > dtb_embed_begin = .; > KEEP(*(.mydtb)); > -- > 2.25.1 > >
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. 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
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
On 12/18/22 01:59, Pali Rohár wrote: > 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. >> Hello Simon, Pali, At the moment I am unable to work more on this issue. If you think you have a better solution (or someone else that is ), feel free to pick my patch and improve it (or discard it that is) I somehow agree that u-boot-nodtb.bin should not really be truncated, and we should have the dtb starting at an aligned address by its own. The problem might be the fact that objcopy does not take into account the _end from the linker script, and just copies the objects, resulting in a different size. Or, we could stop caring about the size of u-boot-nodtb.bin, and we would stop using 'cat' to simply concatenate and do some intermediary steps to have the dtb.bin aligned next to u-boot-nodtb.bin. On the other hand , what you are saying Pali, that objcopy trims trailing zeros, and your platform is broken, hence you are aligning to 4 bytes, I do not think that alignment to 4 bytes is the solution. So I would be against what you did , to align to 4 bytes. If the DTB is appended at the wrong position (somehow this is happening in my case as well), we should have U-boot (and your platform) look up the DTB at the right position , where it's supposed to be placed. And not move the DTB to a 4 byte alignment just because Uboot searches the DTB there. In the future if the DTC requires all DTBs to be aligned to 8 bytes as a hard rule, your solution is again not right. We would have to have the DTB aligned to 8 and the code to lookup the DTB in the right position. So looking up the _end and placing the DTB directly there might be a better way to solve all the problems we are facing (as you actually suggested ) Eugen >>>>> >>>>> 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
On Thu, Dec 15, 2022 at 07:00:51AM -0800, Simon Glass wrote: > Hi Eugen, > > On Thu, 15 Dec 2022 at 06:37, <Eugen.Hristev@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.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. 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 <sjg@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".
On Sat, Dec 17, 2022 at 11:14:41PM +0100, Mark Kettenis wrote: > > From: Eugen Hristev <eugen.hristev@microchip.com> > > Date: Thu, 15 Dec 2022 13:58:25 +0200 > > > > 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 $@ > > $ truncate > ksh: truncate: not found > > In other words: truncate(1) isn't a standard UNIX utility and not > present on OpenBSD for example. It isn't part of POSIX and therefore > its usage is unportable. > > Please find a different solution. Ah yes. Can this perhaps be done with dd? A bit of looking around suggests that this might be a portable way to truncate a file to say 512 bytes: dd if=/dev/urandom of=testfile bs=1 count=500 dd if=/dev/null of=testfile bs=1 count=512 And then hexdump or whatever to see that the last 12 bytes are zero. Can you please test this on OpenBSD? We'll need some shell work to get current byte size and make it 8-byte aligned, but that should be portable at least. Thanks!
On Wednesday 21 December 2022 14:47:48 Eugen.Hristev@microchip.com wrote: > On the other hand , what you are saying Pali, that objcopy trims > trailing zeros, and your platform is broken, hence you are aligning to 4 > bytes, I do not think that alignment to 4 bytes is the solution. Yes, it is not the proper solution but at that time binary was already broken and I did not find at that time better fix for it to have again working u-boot build. > So I would be against what you did , to align to 4 bytes. If the DTB is > appended at the wrong position (somehow this is happening in my case as > well), we should have U-boot (and your platform) look up the DTB at the > right position , where it's supposed to be placed. And not move the DTB > to a 4 byte alignment just because Uboot searches the DTB there. > In the future if the DTC requires all DTBs to be aligned to 8 bytes as a > hard rule, your solution is again not right. We would have to have the > DTB aligned to 8 and the code to lookup the DTB in the right position. > > So looking up the _end and placing the DTB directly there might be a > better way to solve all the problems we are facing (as you actually > suggested ) Yes, for me it looks like a better solution which fixes both problems. And allows to put DTB at any position or alignment. Then alignment could be changed also for mpc85xx to 8 or 128 bytes...
> Date: Wed, 21 Dec 2022 16:56:37 -0500 > From: Tom Rini <trini@konsulko.com> > > On Sat, Dec 17, 2022 at 11:14:41PM +0100, Mark Kettenis wrote: > > > From: Eugen Hristev <eugen.hristev@microchip.com> > > > Date: Thu, 15 Dec 2022 13:58:25 +0200 > > > > > > 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 $@ > > > > $ truncate > > ksh: truncate: not found > > > > In other words: truncate(1) isn't a standard UNIX utility and not > > present on OpenBSD for example. It isn't part of POSIX and therefore > > its usage is unportable. > > > > Please find a different solution. > > Ah yes. Can this perhaps be done with dd? A bit of looking around > suggests that this might be a portable way to truncate a file to say 512 > bytes: > dd if=/dev/urandom of=testfile bs=1 count=500 > dd if=/dev/null of=testfile bs=1 count=512 > > And then hexdump or whatever to see that the last 12 bytes are zero. Can > you please test this on OpenBSD? We'll need some shell work to get > current byte size and make it 8-byte aligned, but that should be > portable at least. Thanks! Hi Tom, That results on a file with zero bytes on OpenBSD... but also on Linux. Did you intend to write something different? I can't immediately come up with a way to do this with dd(1). Cheers, Mark
On Wed, Dec 21, 2022 at 11:42:56PM +0100, Mark Kettenis wrote: > > Date: Wed, 21 Dec 2022 16:56:37 -0500 > > From: Tom Rini <trini@konsulko.com> > > > > On Sat, Dec 17, 2022 at 11:14:41PM +0100, Mark Kettenis wrote: > > > > From: Eugen Hristev <eugen.hristev@microchip.com> > > > > Date: Thu, 15 Dec 2022 13:58:25 +0200 > > > > > > > > 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 $@ > > > > > > $ truncate > > > ksh: truncate: not found > > > > > > In other words: truncate(1) isn't a standard UNIX utility and not > > > present on OpenBSD for example. It isn't part of POSIX and therefore > > > its usage is unportable. > > > > > > Please find a different solution. > > > > Ah yes. Can this perhaps be done with dd? A bit of looking around > > suggests that this might be a portable way to truncate a file to say 512 > > bytes: > > dd if=/dev/urandom of=testfile bs=1 count=500 > > dd if=/dev/null of=testfile bs=1 count=512 > > > > And then hexdump or whatever to see that the last 12 bytes are zero. Can > > you please test this on OpenBSD? We'll need some shell work to get > > current byte size and make it 8-byte aligned, but that should be > > portable at least. Thanks! > > Hi Tom, > > That results on a file with zero bytes on OpenBSD... but also on > Linux. Did you intend to write something different? I can't > immediately come up with a way to do this with dd(1). Ah, I typod that, sorry! I meant (and just re-checked): dd if=/dev/urandom of=testfile bs=1 count=500 dd if=/dev/null of=testfile bs=1 seek=512 Which here does: $ dd if=/dev/urandom of=testfile bs=1 count=500 500+0 records in 500+0 records out 500 bytes copied, 0.00641125 s, 78.0 kB/s $ dd if=/dev/null of=testfile bs=1 seek=512 0+0 records in 0+0 records out 0 bytes copied, 0.000358246 s, 0.0 kB/s $ ls -l testfile -rw-rw-r-- 1 trini trini 512 Dec 21 18:07 testfile $ hexdump testfile | tail -n2 00001f0 8cdd 300e 0000 0000 0000 0000 0000 0000 0000200
> Date: Wed, 21 Dec 2022 18:09:10 -0500 > From: Tom Rini <trini@konsulko.com> > > On Wed, Dec 21, 2022 at 11:42:56PM +0100, Mark Kettenis wrote: > > > Date: Wed, 21 Dec 2022 16:56:37 -0500 > > > From: Tom Rini <trini@konsulko.com> > > > > > > On Sat, Dec 17, 2022 at 11:14:41PM +0100, Mark Kettenis wrote: > > > > > From: Eugen Hristev <eugen.hristev@microchip.com> > > > > > Date: Thu, 15 Dec 2022 13:58:25 +0200 > > > > > > > > > > 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 $@ > > > > > > > > $ truncate > > > > ksh: truncate: not found > > > > > > > > In other words: truncate(1) isn't a standard UNIX utility and not > > > > present on OpenBSD for example. It isn't part of POSIX and therefore > > > > its usage is unportable. > > > > > > > > Please find a different solution. > > > > > > Ah yes. Can this perhaps be done with dd? A bit of looking around > > > suggests that this might be a portable way to truncate a file to say 512 > > > bytes: > > > dd if=/dev/urandom of=testfile bs=1 count=500 > > > dd if=/dev/null of=testfile bs=1 count=512 > > > > > > And then hexdump or whatever to see that the last 12 bytes are zero. Can > > > you please test this on OpenBSD? We'll need some shell work to get > > > current byte size and make it 8-byte aligned, but that should be > > > portable at least. Thanks! > > > > Hi Tom, > > > > That results on a file with zero bytes on OpenBSD... but also on > > Linux. Did you intend to write something different? I can't > > immediately come up with a way to do this with dd(1). > > Ah, I typod that, sorry! I meant (and just re-checked): > dd if=/dev/urandom of=testfile bs=1 count=500 > dd if=/dev/null of=testfile bs=1 seek=512 > > Which here does: > $ dd if=/dev/urandom of=testfile bs=1 count=500 > 500+0 records in > 500+0 records out > 500 bytes copied, 0.00641125 s, 78.0 kB/s > $ dd if=/dev/null of=testfile bs=1 seek=512 > 0+0 records in > 0+0 records out > 0 bytes copied, 0.000358246 s, 0.0 kB/s > $ ls -l testfile > -rw-rw-r-- 1 trini trini 512 Dec 21 18:07 testfile > $ hexdump testfile | tail -n2 > 00001f0 8cdd 300e 0000 0000 0000 0000 0000 0000 > 0000200 That works here as well.
On Thu, Dec 22, 2022 at 12:20:01AM +0100, Mark Kettenis wrote: > > Date: Wed, 21 Dec 2022 18:09:10 -0500 > > From: Tom Rini <trini@konsulko.com> > > > > On Wed, Dec 21, 2022 at 11:42:56PM +0100, Mark Kettenis wrote: > > > > Date: Wed, 21 Dec 2022 16:56:37 -0500 > > > > From: Tom Rini <trini@konsulko.com> > > > > > > > > On Sat, Dec 17, 2022 at 11:14:41PM +0100, Mark Kettenis wrote: > > > > > > From: Eugen Hristev <eugen.hristev@microchip.com> > > > > > > Date: Thu, 15 Dec 2022 13:58:25 +0200 > > > > > > > > > > > > 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 $@ > > > > > > > > > > $ truncate > > > > > ksh: truncate: not found > > > > > > > > > > In other words: truncate(1) isn't a standard UNIX utility and not > > > > > present on OpenBSD for example. It isn't part of POSIX and therefore > > > > > its usage is unportable. > > > > > > > > > > Please find a different solution. > > > > > > > > Ah yes. Can this perhaps be done with dd? A bit of looking around > > > > suggests that this might be a portable way to truncate a file to say 512 > > > > bytes: > > > > dd if=/dev/urandom of=testfile bs=1 count=500 > > > > dd if=/dev/null of=testfile bs=1 count=512 > > > > > > > > And then hexdump or whatever to see that the last 12 bytes are zero. Can > > > > you please test this on OpenBSD? We'll need some shell work to get > > > > current byte size and make it 8-byte aligned, but that should be > > > > portable at least. Thanks! > > > > > > Hi Tom, > > > > > > That results on a file with zero bytes on OpenBSD... but also on > > > Linux. Did you intend to write something different? I can't > > > immediately come up with a way to do this with dd(1). > > > > Ah, I typod that, sorry! I meant (and just re-checked): > > dd if=/dev/urandom of=testfile bs=1 count=500 > > dd if=/dev/null of=testfile bs=1 seek=512 > > > > Which here does: > > $ dd if=/dev/urandom of=testfile bs=1 count=500 > > 500+0 records in > > 500+0 records out > > 500 bytes copied, 0.00641125 s, 78.0 kB/s > > $ dd if=/dev/null of=testfile bs=1 seek=512 > > 0+0 records in > > 0+0 records out > > 0 bytes copied, 0.000358246 s, 0.0 kB/s > > $ ls -l testfile > > -rw-rw-r-- 1 trini trini 512 Dec 21 18:07 testfile > > $ hexdump testfile | tail -n2 > > 00001f0 8cdd 300e 0000 0000 0000 0000 0000 0000 > > 0000200 > > That works here as well. Great, thanks!
On Fri, Dec 16, 2022 at 01:13:07AM +0100, Pali Rohár 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. We should indeed not force u-boot-nodtb.bin itself to be an 8 byte aligned size, yes. > > 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. > > 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. This however needs a follow-up. By spec, device tree binaries must start at an 8 byte aligned address, not 4. Yes, 4 is clearly functional in this case, but when we update to a newer libdtc that finally enforces this check we'll have a problem.
On Thursday 22 December 2022 10:49:58 Tom Rini wrote: > On Fri, Dec 16, 2022 at 01:13:07AM +0100, Pali Rohár 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. > > We should indeed not force u-boot-nodtb.bin itself to be an 8 byte > aligned size, yes. > > > > 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. > > > > 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. > > This however needs a follow-up. By spec, device tree binaries must > start at an 8 byte aligned address, not 4. Yes, 4 is clearly functional > in this case, but when we update to a newer libdtc that finally enforces > this check we'll have a problem. In https://lore.kernel.org/u-boot/20221217235913.w7ihsktbplbp2j7z@pali/ I proposed how to extend Makefile rule to generate u-boot-nodtb.bin with all required paddings. However this (enforcing u-boot-nodtb.bin to be 8 byte aligned) is not we think that is the correct way. But I do not have any other option yet how to solve this problem. I think that the source of this issue is that we are not putting DTB binary into ELF binary (so objcopy would be able to easily generate final u-boot.bin). And instead trying to simulate joining more binary files (u-boot-nodtb.bin and DTB binary) into one _without_ information how to do it correctly. But we already have this information in linker script and also in ELF binary.
Hi Pali, On Fri, 30 Dec 2022 at 09:32, Pali Rohár <pali@kernel.org> wrote: > > On Thursday 22 December 2022 10:49:58 Tom Rini wrote: > > On Fri, Dec 16, 2022 at 01:13:07AM +0100, Pali Rohár 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. > > > > We should indeed not force u-boot-nodtb.bin itself to be an 8 byte > > aligned size, yes. > > > > > > 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. > > > > > > 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. > > > > This however needs a follow-up. By spec, device tree binaries must > > start at an 8 byte aligned address, not 4. Yes, 4 is clearly functional > > in this case, but when we update to a newer libdtc that finally enforces > > this check we'll have a problem. > > In https://lore.kernel.org/u-boot/20221217235913.w7ihsktbplbp2j7z@pali/ > I proposed how to extend Makefile rule to generate u-boot-nodtb.bin with > all required paddings. However this (enforcing u-boot-nodtb.bin to be 8 > byte aligned) is not we think that is the correct way. But I do not have > any other option yet how to solve this problem. See the other thread [1] on this > > I think that the source of this issue is that we are not putting DTB > binary into ELF binary (so objcopy would be able to easily generate > final u-boot.bin). And instead trying to simulate joining more binary > files (u-boot-nodtb.bin and DTB binary) into one _without_ information > how to do it correctly. But we already have this information in linker > script and also in ELF binary. The DTB can come later, during packaging, or may be updated in that step. Or there may multiple DTBs and SPL can decide which to use. Overall the DTB is considered separate from the code by design, so that it is possible to build one U-Boot that supports multiple boards. That is why we don't allow DTB in the ELF. Regards, Simon [1] https://lore.kernel.org/u-boot/CAPnjgZ1Vho5-sNMmPeU_9hCsGHK9pSGbYYnrQVxkB5D5R=biXw@mail.gmail.com/ > > I think that the source of this issue is that we are not putting DTB > binary into ELF binary (so objcopy would be able to easily generate > final u-boot.bin). And instead trying to simulate joining more binary > files (u-boot-nodtb.bin and DTB binary) into one _without_ information > how to do it correctly. But we already have this information in linker > script and also in ELF binary.
> From: Simon Glass <sjg@chromium.org> > Date: Fri, 30 Dec 2022 11:51:05 -0600 > > Hi Pali, > > On Fri, 30 Dec 2022 at 09:32, Pali Rohár <pali@kernel.org> wrote: > > > > On Thursday 22 December 2022 10:49:58 Tom Rini wrote: > > > On Fri, Dec 16, 2022 at 01:13:07AM +0100, Pali Rohár 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. > > > > > > We should indeed not force u-boot-nodtb.bin itself to be an 8 byte > > > aligned size, yes. > > > > > > > > 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. > > > > > > > > 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. > > > > > > This however needs a follow-up. By spec, device tree binaries must > > > start at an 8 byte aligned address, not 4. Yes, 4 is clearly functional > > > in this case, but when we update to a newer libdtc that finally enforces > > > this check we'll have a problem. > > > > In https://lore.kernel.org/u-boot/20221217235913.w7ihsktbplbp2j7z@pali/ > > I proposed how to extend Makefile rule to generate u-boot-nodtb.bin with > > all required paddings. However this (enforcing u-boot-nodtb.bin to be 8 > > byte aligned) is not we think that is the correct way. But I do not have > > any other option yet how to solve this problem. > > See the other thread [1] on this > > > > > I think that the source of this issue is that we are not putting DTB > > binary into ELF binary (so objcopy would be able to easily generate > > final u-boot.bin). And instead trying to simulate joining more binary > > files (u-boot-nodtb.bin and DTB binary) into one _without_ information > > how to do it correctly. But we already have this information in linker > > script and also in ELF binary. > > The DTB can come later, during packaging, or may be updated in that > step. Or there may multiple DTBs and SPL can decide which to use. > Overall the DTB is considered separate from the code by design, so > that it is possible to build one U-Boot that supports multiple boards. > > That is why we don't allow DTB in the ELF. Repeating that statement doesn't make it true. We have an option for this (OF_EMBED) and that option is legitimately used by several "boards" in cases where U-Boot is used as in combination with "firmware" that expects a valid ELF binary as its payload. That said, I do think that OF_SEPARATE should be used whenever possible. And in that case it should be the tool that combines the various components into a single binary that makes sure the DTB is properly aligned.
Hi Mark, On Fri, 30 Dec 2022 at 12:11, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Simon Glass <sjg@chromium.org> > > Date: Fri, 30 Dec 2022 11:51:05 -0600 > > > > Hi Pali, > > > > On Fri, 30 Dec 2022 at 09:32, Pali Rohár <pali@kernel.org> wrote: > > > > > > On Thursday 22 December 2022 10:49:58 Tom Rini wrote: > > > > On Fri, Dec 16, 2022 at 01:13:07AM +0100, Pali Rohár 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. > > > > > > > > We should indeed not force u-boot-nodtb.bin itself to be an 8 byte > > > > aligned size, yes. > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > This however needs a follow-up. By spec, device tree binaries must > > > > start at an 8 byte aligned address, not 4. Yes, 4 is clearly functional > > > > in this case, but when we update to a newer libdtc that finally enforces > > > > this check we'll have a problem. > > > > > > In https://lore.kernel.org/u-boot/20221217235913.w7ihsktbplbp2j7z@pali/ > > > I proposed how to extend Makefile rule to generate u-boot-nodtb.bin with > > > all required paddings. However this (enforcing u-boot-nodtb.bin to be 8 > > > byte aligned) is not we think that is the correct way. But I do not have > > > any other option yet how to solve this problem. > > > > See the other thread [1] on this > > > > > > > > I think that the source of this issue is that we are not putting DTB > > > binary into ELF binary (so objcopy would be able to easily generate > > > final u-boot.bin). And instead trying to simulate joining more binary > > > files (u-boot-nodtb.bin and DTB binary) into one _without_ information > > > how to do it correctly. But we already have this information in linker > > > script and also in ELF binary. > > > > The DTB can come later, during packaging, or may be updated in that > > step. Or there may multiple DTBs and SPL can decide which to use. > > Overall the DTB is considered separate from the code by design, so > > that it is possible to build one U-Boot that supports multiple boards. > > > > That is why we don't allow DTB in the ELF. > > Repeating that statement doesn't make it true. We have an option for > this (OF_EMBED) and that option is legitimately used by several > "boards" in cases where U-Boot is used as in combination with > "firmware" that expects a valid ELF binary as its payload. We have this problem with EFI, and the approach is to embed an ELF 'stub' with a u-boot.bin payload. But a better way would be to update that 'firmware' to pass the DTB to U-Boot using standard passage. > > That said, I do think that OF_SEPARATE should be used whenever > possible. And in that case it should be the tool that combines the > various components into a single binary that makes sure the DTB is > properly aligned. OK good. That seems to be where we are heading. Regards, Simon
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) u-boot.ldr: u-boot diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index 8fe4682dd2..e5fa4ef95c 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -145,10 +145,10 @@ SECTIONS *(.__rel_dyn_end) } - _end = .; - . = ALIGN(8); + _end = .; + .bss_start : { KEEP(*(.__bss_start)); } diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds index fb2189d50d..732ed42db1 100644 --- a/arch/arm/cpu/u-boot-spl.lds +++ b/arch/arm/cpu/u-boot-spl.lds @@ -55,6 +55,7 @@ SECTIONS .end : { + . = ALIGN(8); *(.__end) } diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index f25f72b2e0..274e1a7d30 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -193,6 +193,7 @@ SECTIONS .end : { + . = ALIGN(8); *(.__end) } diff --git a/arch/arm/lib/elf_arm_efi.lds b/arch/arm/lib/elf_arm_efi.lds index 767ebda635..f3867021d3 100644 --- a/arch/arm/lib/elf_arm_efi.lds +++ b/arch/arm/lib/elf_arm_efi.lds @@ -54,6 +54,11 @@ SECTIONS .rel.data : { *(.rel.data) *(.rel.data*) } _data_size = . - _etext; + .end : { + . = ALIGN(8); + *(.__end) + } + /DISCARD/ : { *(.rel.reloc) *(.eh_frame) diff --git a/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds b/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds index 1a8bf94dee..d7e5d81ecc 100644 --- a/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds +++ b/arch/arm/mach-at91/arm926ejs/u-boot-spl.lds @@ -31,7 +31,7 @@ SECTIONS . = ALIGN(4); __u_boot_list : { KEEP(*(SORT(__u_boot_list*))) } > .sram - . = ALIGN(4); + . = ALIGN(8); __image_copy_end = .; .end : diff --git a/arch/arm/mach-at91/armv7/u-boot-spl.lds b/arch/arm/mach-at91/armv7/u-boot-spl.lds index 6ca725fc4c..2a0a1b4b86 100644 --- a/arch/arm/mach-at91/armv7/u-boot-spl.lds +++ b/arch/arm/mach-at91/armv7/u-boot-spl.lds @@ -38,7 +38,7 @@ SECTIONS . = ALIGN(4); __u_boot_list : { KEEP(*(SORT(__u_boot_list*))) } > .sram - . = ALIGN(4); + . = ALIGN(8); __image_copy_end = .; .end : diff --git a/arch/arm/mach-zynq/u-boot-spl.lds b/arch/arm/mach-zynq/u-boot-spl.lds index 8c18d3f91f..c0320dbe1e 100644 --- a/arch/arm/mach-zynq/u-boot-spl.lds +++ b/arch/arm/mach-zynq/u-boot-spl.lds @@ -41,7 +41,7 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); } > .sram - . = ALIGN(4); + . = ALIGN(8); _image_binary_end = .; diff --git a/arch/mips/cpu/u-boot.lds b/arch/mips/cpu/u-boot.lds index 9a4ebcd151..182e9bc6e5 100644 --- a/arch/mips/cpu/u-boot.lds +++ b/arch/mips/cpu/u-boot.lds @@ -56,7 +56,7 @@ SECTIONS . += CONFIG_MIPS_RELOCATION_TABLE_SIZE - 4; } - . = ALIGN(4); + . = ALIGN(8); _end = .; .bss __rel_start (OVERLAY) : { diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds index ba8dee50c7..3fbca6d248 100644 --- a/arch/sandbox/cpu/u-boot.lds +++ b/arch/sandbox/cpu/u-boot.lds @@ -51,6 +51,12 @@ SECTIONS *(.dynsym) __dyn_sym_end = .; } + + .end : + { + . = ALIGN(8); + *(.__end) + } } INSERT BEFORE .data; diff --git a/arch/sh/cpu/u-boot.lds b/arch/sh/cpu/u-boot.lds index d360eea7eb..226f50b19c 100644 --- a/arch/sh/cpu/u-boot.lds +++ b/arch/sh/cpu/u-boot.lds @@ -74,6 +74,8 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); } >ram + . = ALIGN(8); + PROVIDE (__init_end = .); PROVIDE (reloc_dst_end = .); PROVIDE (_end = .); diff --git a/board/ti/am335x/u-boot.lds b/board/ti/am335x/u-boot.lds index 087dee8bb2..deb9098a90 100644 --- a/board/ti/am335x/u-boot.lds +++ b/board/ti/am335x/u-boot.lds @@ -118,6 +118,7 @@ SECTIONS .end : { + . = ALIGN(8); *(.__end) } diff --git a/tools/binman/test/u_boot_binman_embed.lds b/tools/binman/test/u_boot_binman_embed.lds index e213fa8a84..93c0ae6aed 100644 --- a/tools/binman/test/u_boot_binman_embed.lds +++ b/tools/binman/test/u_boot_binman_embed.lds @@ -18,7 +18,7 @@ SECTIONS *(.text*) } - . = ALIGN(4); + . = ALIGN(8); .data : { dtb_embed_begin = .; KEEP(*(.mydtb));
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(-)