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

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

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

Yes please.

Regards,
Simon