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

[Marek Vasut]

On 5/11/20 9:36 PM, Tom Rini wrote:
> On Sun, May 10, 2020 at 09:24:19PM +0200, Marek Vasut wrote:
>> On 5/8/20 9:21 PM, Tom Rini wrote:
>>> On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
>>>> 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.
>>>
>>> There's two distinct areas here, and they keep being conflated.
>>>
>>> The case of SPL and a FIT image for U-Boot+DTB.  We've always aligned
>>> this to 4 bytes and it's worked.  I think if someone looked at the ARM
>>> ARM for aarch64 you could reason out that "4-but-not-8-byte aligned
>>> pointers are slow but work" as why this wasn't a hard fail on aarch64.
>>
>> But we had hard-fault on arm64, see
>> [PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses
> 
> You're mixing the issues again.  That's an example of "device tree
> properties are only 4 byte aligned" and we can't do what we were doing.
> I'm not even sure reverting e8c2d25845c7 would have helped in that case.
> It's also not the case we're talking about with respect to padding the
> start of embedded data.

No, I am not mixing any issues again. These issues are all
interconnected, which is why this is a valid example of the problems we
have with the padding.

>>> We should adjust our current alignment up to cover that and move on.
>>
>> Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ?
> 
> 8 bytes is the defined requirement for everything today that defines a
> required _start_ alignment.

Except DT properties are 4 byte aligned, so that's not true.

>> You will fail here either way, since if you build the fitImage with
>> embedded data, the embedded data will be aligned to 4 bytes, because DT
>> properties are aligned to 4 bytes.
> 
> Yes, there's the difference between "the device tree itself must be
> aligned to 8 bytes" and "device tree internally is 4 byte aligned".  The
> latter means that some operations are simply not possible and a feature
> of the format.

So fitImage with embedded data should not be possible at all ?

>>> The case of FIT images and "kernel_noload" / fdt_high=-1 /
>>> initrd_high=-1 and aarch64.  If you load a FIT image in to memory and
>>> try and use it as-is, it will not work.  It's not even possible in the
>>> general case as you would have to inspect the kernel, see what the
>>> text_offset is and build a FIT image that took that in to account, to
>>> not have to move the Image around.  The device tree will almost
>>> certainly be misaligned and still need to be relocated.  This is why a
>>> while back I sent out an email asking every maintainer of a board that
>>> disabled device tree relocation to stop that.  Perhaps a run-time patch
>>> to scream about this rather than note it as we do today would help (see
>>> common/image-fdt.c::boot_relocate_fdt()).
>>
>> I have a feeling we should do the relocation either way. And if there is
>> some special limited case (like the SPL), we should warn about it and
>> push mkimage with e.g. -B 8 flag to enforce the alignment.
> 
> Please send patches so we can see what that looks like, thanks!

Maybe we should remove support for embedded data from fitImage and then
enforce padding ?