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

[Eugen.Hristev at microchip.com]

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 at 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 at 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 at 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