[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Samuel Holland <samuel@sholland.org>]

On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>>>> aligned?  Is that really a better option in this case than ensuring
>>>>> alignment within the file?
>>>>
>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>>>> only IFF it is required ? 
>>>
>>> Perhaps.  But..
>>>
>>>> Then we can enforce it separately for 32bit
>>>> and 64bit platforms to 4 and 8 bytes respectively even.
>>>
>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
>>> you only do 4 byte alignment but the documented requirement is 8, for
>>> arm32.
>>
>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
>> for arm64 for example.
> 
> For arm64 you have to move it to where text_offset says it needs to be.
> For arm32 the common (always, practically?) case is you're firing off
> the zImage which does what's needed.  But..
> 
>> And what you usually parse in-place would be the DT then.
> 
> Yes, the practical case is that it's a DT and that needs 8 byte
> alignment.  And we should just get back to aligning that correctly.
> Going back to the v1 thread, it turns out the answer to "why do we even
> have this padding?" is "we need the DT to be aligned".

This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
alignment of external data after the FIT. In spl_load_simple_fit():

 /*
  * For FIT with external data, figure out where the external images
  * start. This is the base for the data-offset properties in each
  * image.
  */
 size = fdt_totalsize(fit);
 size = (size + 3) & ~3;
 size = board_spl_fit_size_align(size);
 base_offset = (size + 3) & ~3;

And then later on in spl_load_fit_image():

 if (!fit_image_get_data_position(fit, node, &offset)) {
         external_data = true;
 } else if (!fit_image_get_data_offset(fit, node, &offset)) {
         offset += base_offset;
         external_data = true;
 }

In my case, after this change, the FIT binary was 0x453 bytes long. But SPL
rounded it up to 0x454, so the external data offsets were off by one, and the
first byte of every loadable was cut off in RAM:

 Trying to boot from MMC1
 fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x454
 firmware: 'uboot'
 External data: dst=4a000000, offset=454, size=81208
 src = 4a000054, dest = 4a000000
 4a000000: 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00 00

If I remove both "(size + 3) & ~3" lines, I can boot successfully:

 Trying to boot from MMC1

 fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x453
 firmware: 'uboot'

 External data: dst=4a000000, offset=453, size=81208
 src = 4a000053, dest = 4a000000

 4a000000: 1f 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00

In other words, it's not just FDTs that are affected by this change, but _all_
external data loaded from SPL.

So if you change the alignment to anything but 4 (be it 1 or 8 or something
else), you must also update the assumptions made by SPL.

Regards,
Samuel