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

[Marek Vasut]

On 5/8/20 8:47 PM, Tom Rini wrote:
> On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
>> On 5/7/20 10:46 PM, Samuel Holland wrote:
>>> 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;
>>
>> Somehow this doesn't match the 8-byte alignment Tom was suggesting.
>> And that only leads me to believe that we can either make assumptions
>> about alignment, which would very likely fail one way or the other OR we
>> can say that for SPL as a special case, we enforce some alignment.
> 
> It's likely the case that on arm32 as there's no natural alignment
> problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure
> and is rarely if ever given 4-but-not-8-byte-aligned addresses of the
> DTB.  Which is why we should probably move the alignment here to 8 bytes
> instead of 4.
> 
>> But that in turn fails for fitImage with embedded data, where the
>> embedded data are always aligned to 4 bytes, because that's how DTC
>> aligns properties.
> 
> I think the answer is that the use-case you're talking about is simply
> going to require data to be relocated.

I have a feeling that no matter how much you try to pad when generating
fitImage from U-Boot, there will always be a case where that will fail.
I listed at least two:
- fitImage with embedded data, 4byte alignment due to DTC
- Older fitImages, 4byte alignment, fails on arm64
- Someone can generate signed fitImage with older mkimage => fail

So that relocation logic or at least warning or something should be in
there, no matter what.