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

[Stefan Herbrechtsmeier]

Hi Simon,

Am 05.08.2022 um 18:48 schrieb Simon Glass:
> Hi Stefan,
> 
> On Fri, 5 Aug 2022 at 01:51, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>
>> Hi Simon,
>>
>> Am 04.08.2022 um 15:57 schrieb Simon Glass:
>>> Hi Stefan,
>>>
>>> On Thu, 4 Aug 2022 at 01:50, Stefan Herbrechtsmeier
>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>
>>>> 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?
>>>
>>> Possibly, but of course we cannot do that. As you say, U-Boot mainline
>>> does not expect or support the header, at present.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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.
>>>
>>> OK good.
>>>
>>>>
>>>> But I wonder how the decompression of the DTB works if the fitImage
>>>> implementation doesn't support the header.
>>>
>>> It doesn't. Something needs to change here for compression to work.
>>>
>>>>
>>>>>>> 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?
>>>
>>> That's an interesting question. It is possible that we only need it if
>>> DTB is present and is compressed. It should be possible to check this
>>> by adjusting the tests and checking for failures.
>>>
>>> But I am not sure it is a good idea, since it is wildly inconsistent.
>>> I do prefer things to be deterministic - i.e.  you specify what you
>>> want and you get it. If binman starts adopting obscure conventions it
>>> could be confusing.
>>
>> I add tests for gzip, lz4 and lzma_alone and all support padding at the
>> end and don't need the header. Even the testCompressDtb works without
>> the header. Furthermore I add bzip2, lzop, xz and zstd support to
>> bintool and only zstd doesn't support padding. Do we really need the
>> header or could we add an error if DTB is used together with zstd?
> 
> OK great!
> 
> Yes, I tried pretty hard to avoid it, but could not make it work.
> Would you like me to take a look at the situations that spark it? It
> might be around replacing things, too.
> 
> I really tried to avoid banning things, since it is such a pain and
> confusing for people. But since this is error (and a corner case) I
> think it would be fine to require a particular property to enable the
> advanced functionality.

I have send a new version. Would be nice if you could check if you need 
the new attribute to append the length header.

>> Should I commit bzip2, lzop, xz and zstd support to bintool?

I have add the tools to the series. Maybe the test environment need an 
update to include all compression tools.

Regards
   Stefan