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

[Tom Rini]

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.

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

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

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

-- 
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/20200511/e0bc77eb/attachment.sig>