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

[Simon Goldschmidt]

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?

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

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

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


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