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

[Simon Glass]

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?

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