[U-Boot] [RFC] fit: include uncompression into fit_image_load

Tue Nov 20 21:03:20 UTC 2018

On Fri, Nov 16, 2018 at 3:05 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
> On 22 October 2018 at 11:55, Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com> wrote:
> > On 22.10.2018 19:49, Simon Glass wrote:
> >>
> >> Hi Simon,
> >>
> >> On 19 October 2018 at 00:33, Simon Goldschmidt
> >> <simon.k.r.goldschmidt at gmail.com> wrote:
> >>>
> >>> On 19.10.2018 05:25, Simon Glass wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 17 October 2018 at 03:41, Simon Goldschmidt
> >>>> <simon.k.r.goldschmidt at gmail.com> wrote:
> >>>>>
> >>>>> On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf <agraf at suse.de> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 16.10.18 21:33, Simon Goldschmidt wrote:
> >>>>>>>
> >>>>>>> Currently, only the kernel can be compressed in a FIT image.
> >>>>>>> By moving the uncompression logic to 'fit_image_load()', all types
> >>>>>>> of images can be compressed.
> >>>>>>>
> >>>>>>> This works perfectly for me when using a gzip'ed FPGA image in
> >>>>>>> a FIT image on a cyclone5 board (socrates). Also, a gzip'ed initrd
> >>>>>>> being unzipped by U-Boot (not the kernel) worked.
> >>>>>>>
> >>>>>>> To clean this up, the uncompression function would have to be moved
> >>>>>>> from bootm.c ('bootm_decomp_image()') to a more generic location,
> >>>>>>> but I decided to keep it for now to make the patch easier to read.
> >>>>>>> Because of this setup, the kernel is currently uncompressed twice.
> >>>>>>> which doesn't work...
> >>>>>>>
> >>>>>>> There are, however, some more things to discuss:
> >>>>>>> - The max. size passed to gunzip() (etc.) is not known before, so
> >>>>>>>     we currently configure this to 8 MByte in U-Boot (via
> >>>>>>>     CONFIG_SYS_BOOTM_LEN), which proved too small for my initrd...
> >>>>
> >>>> We need the uncompressed size. If the legacy header doesn't have, stop
> >>>> using it and use FIT?
> >>>>
> >>>> Some compression formats include that in a header I think. But we
> >>>> should record it in the U-Boot header.
> >>>
> >>>
> >>> OK, we could embed this information into the FIT by extracting in
> >>> 'mkimage'?
> >>> That way, we know the uncompressed size...
> >>
> >> Yes. In fact I don't like the way it works at present. We have to
> >> compress the data before putting it in the FIT, since the .its file
> >> refers to the compressed data.
> >>
> >> I think it would be better for the ,its to refer to the original file,
> >> and then mkimage do the compression as it generates the FIT. That way
> >> it knows the size of the original data, and it is simpler for people
> >> to build images, since they don't need to compress everything
> >> beforehand.
> >
> >
> > Hmm, OK, I think it should not be a problem to add this to mkimage.
> > Only I don't know if this workflow change would be accepted by everyone or
> > if the old style of using pre-compressed files would have to be somehow kept
> > working?
>
> I suggest supporting the old way with a flag. Also is it possible to
> detect an already-compressed file and print an warning?

I'm working on this and have it partly running, but I had to add this
ugly "#ifndef USE_HOSTCC" thing to many files in lib/ to get the
compression algorithms to compile for the tools.

Is this acceptable? Or should we find a more generic approach, i.e.
fixing the central include files (common.h, etc.) to handle
USE_HOSTCC?

Simon

>
> >
> > But what would that mean for CONFIG_SYS_BOOTM_LEN? As I already wrote
> > before, this constant is currently used to trim copy-only, too. Now if the
> > FIT would embed "uncompressed size", would we limit that to
> > CONFIG_SYS_BOOTM_LEN, too? I think it would then make more sense to dump
> > this constant and rely on lmb allocation to detect overlapping. (That
> > assumes the load address is within lmb, of course.)
>
> Yes that should be the limit I think.
>
> >
> >>
> >>>>>>> - CONFIG_SYS_BOOTM_LEN is set to 64 MByte default in SPL, so it's
> >>>>>>>     a different default for SPL than for U-Boot !?!
> >>>>
> >>>> Ick
> >>>>
> >>>>>>> - CONFIG_SYS_BOOTM_LEN seemed to initially be used for kernel
> >>>>>>>     uncompression but is now used as a copy-only limit, too (no
> >>>>>>> unzip)
> >>>>
> >>>> Yes.
> >>>
> >>>
> >>> Why do we need an extra limit for uncompressed copy-only? Both load
> >>> address
> >>> and size are known from the FIT.
> >>
> >> I'm not suggesting separate limit. We don't need that.
> >
> >
> > But bootm_decomp_image() *does* use CONFIG_SYS_BOOTM_LEN already with
> > IH_COMP_NONE to limit the size of an uncompressed kernel image.
> > Is that a bug then?
>
> I suppose it is just that we have no information about the size of the
> kernel, so use this fixed limit?
>
> >
> > I don't understand why we need this limit. It seems arbitrary to me given we
> > only limit the size but don't know which address we limit...
>
> Perhaps for security reasons, to avoid memory overflow?
>
> >
> >>>
> >>>>>>> - Uncompression only works if a load address is given, what should
> >>>>>>>     happen if the FIT image does not contain a load address?
> >>>>
> >>>> Fail.
> >
> >
> > Given correct lmb integration, wouldn't it make more sense to use lmb to
> > allocate a block? Especially if we know the uncompressed size?
>
> Yes I think so.
>
> >
> >
> > Simon
> >
> >
> >>>>
> >>>>>>> - The whole memory management around FIT images is a bit messed up
> >>>>>>>     in that memory allocation is a mix of where U-Boot relocates
> >>>>>>> itself
> >>>>>>>     (and which address ranges it used), the load addresses of the FIT
> >>>>>>>     image and the load addresses of the FIT image contents (and
> >>>>>>> sizes).
> >>>>>>>     What's the point here to check CONFIG_SYS_BOOTM_LEN? Maybe it
> >>>>>>> would
> >>>>>>>     be better to keep a memory map of allowed and already used data
> >>>>>>> to
> >>>>>>>     check if we're overwriting things or to get the maximum size
> >>>>>>> passed
> >>>>>>>     to gunzip etc.?
> >>>>
> >>>> See lmb
> >>>>
> >>>>>> So I can at least give input on the memory map part :).
> >>>>>>
> >>>>>> In efi_loader, we actually do maintain a full system memory map
> >>>>>> already,
> >>>>>> including allocation functions that give you "safe" allocation
> >>>>>> functionality (allocate somewhere in memory where you know nothing
> >>>>>> overlaps).
> >>>>>>
> >>>>>> Maybe we should move this into a more generic system and reuse it for
> >>>>>> big memory allocations that really don't need to be in the U-Boot
> >>>>>> preallocated regions?
> >>>>>
> >>>>> Hmm, inspecting this further, it seems that there is such an allocator
> >>>>> for bootm (using lmb_*() functions and struct lmb). Maybe this should
> >>>>> be
> >>>>> better integrated into the fit loading function. I don't know if the
> >>>>> lmb functions
> >>>>> correctly detect overlapping of regions allocated by known addresses
> >>>>> though.
> >>>>>
> >>>>> Thanks for your thoughts!
> >>>>
> >>>> Yes lmb is the right mechanism and I think it checks for overlap.
> >>>
> >>>
> >>> That didn't work for all overlap checks for me. I'll dig into it to see
> >>> what's missing for me.
> >>
> >> Sounds good.
> >>
> >> Also off this stuff could do with more tests. Our current bootm tests
> >> do not check lmb as far as I know.
> >>
> >> Regards,
> >> Simon
> >
> >
> >
>
> Regards,
> Simon