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

[Michal Simek <michal.simek@amd.com>]

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