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

[Tom Rini]

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.
We should adjust our current alignment up to cover that and move on.

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()).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/3fd9e3f8/attachment.sig>