[PATCH 2/3] binman: Remove header from compressed data

[Stefan Herbrechtsmeier]

Hi Simon,

Am 03.08.2022 um 20:14 schrieb Simon Glass:
> Hi Stefan,
> 
> On Tue, 2 Aug 2022 at 07:45, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>
>> Hi Simon,
>>
>> Am 02.08.2022 um 14:41 schrieb Simon Glass:
>>> Hi Stefan,
>>>
>>> On Tue, 2 Aug 2022 at 06:29, Stefan Herbrechtsmeier
>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>
>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>>>>
>>>> Remove header from compressed data because this is uncommon, not
>>>> supported by U-Boot and incompatible with external compressed artifacts.
>>>>
>>>> The header was introduced as part of commit eb0f4a4cb402 ("binman:
>>>> Support replacing data in a cbfs") to allow device tree entries to be
>>>> larger that the compressed contents. Regarding the commit "this is
>>>> necessary to cope with a compressed device tree being updated in such a
>>>> way that it shrinks after the entry size is already set (an obscure
>>>> case)". This case need to be fixed without influence the compressed data
>>>> by itself.
>>>
>>> I was not able to find a way around this due to the chicken-and egg
>>> problem. Compressed data has an unpredictable size and adding an extra
>>> uncompressed byte may increase or decrease the compressed size.
>>
>> Is it possible to use the `pad-after` attribute to record the unused
>> space. In this case it is possible to calculate the size of the
>> compressed data.
> 
> Well if you update that attribute it can change the size of the DTB
> which is the chicken-and-egg problem again. As far as I know, if
> people set the size of the region to something a bit larger than
> needed, the problem is avoided. But the image generation does need to
> be deterministic.

Does this means the size is only needed for the creation of the fitImage 
and not for decompression in u-boot?

> 
>>
>> Do you have a test for this use case?
> 
> There are various ones, e.g. testCompressDtb()

Thanks. Now I understand the problem and why you call it a 
chicken-and-egg problem. It wasn't clear to me that the attributes are 
inside the DTB.

But I wonder how the decompression of the DTB works if the fitImage 
implementation doesn't support the header.

>>> So my solution was to add the header.
>>
>> Is the header used outside of binman? I don't spot it in the decompress
>> fitImage implementation.
> 
> It is used in the Chromium OS verified boot implementation, but not elsewhere.
> 
>>
>>
>>> It is optional though, so can we perhaps have a property in the
>>> description to enable it?
>>
>> Is this header needed and supported outside of binman?
>>
>> At the moment the header is incompatible and not well documented. It
>> took me some time to find out why my gzip compression via binman doesn't
>> work as expected because I assume a compatibility between binman
>> compress and fitImage decompress.
> 
> Yes I understand that, however you can pass a parameter to not include
> the size value.

Do we need the header outside of the DTB? Otherwise we could handle this 
special or we could add a special compression type.

> It would also be possible to add a property to the image (top-level
> section) that indicates whether this field is present, such property
> to apply to the whole image. We could have it default to off, if you
> like.

Do we really need the header outside of the DTB entry?

Regards
   Stefan