binman: Why is the first word the compressed size in compressed data

[Alper Nebi Yasak]

On 03/06/2022 18:07, Stefan Herbrechtsmeier wrote:
> Am 02.06.2022 um 20:10 schrieb Alper Nebi Yasak:
>> On 01/06/2022 11:29, Stefan Herbrechtsmeier wrote:
>>> Hi Simon,
>>>
>>> I want to compress a FPGA Image on the fly via binman but this doesn't
>>> work. I have add a bintool implementation for gzip, add gzip support to
>>> comp_util.py and set `compress` and `compression` property in the binman
>>> node of the u-boot dtsi:
>>>
>>> fpga-2cg {
>>> 	compatible = "u-boot,fpga-legacy";
>>> 	description = "FPGA";
>>> 	type = "fpga";
>>> 	compression = "gzip";
>>> 	load = <(CONFIG_SYS_TEXT_BASE + 0x4000000)>;
>>>
>>> 	blob-ext {
>>> 		compress = "gzip";
>>> 		filename = "2cg.bit.bin";
>>> 	};
>>> };
>>>
>>> It works if I remove the `compress` property and use a `2cg.bit.bin.gz`
>>> or remove the header for gzip [1].
>>>
>>> Regarding the code binman add the compressed size as first word to the
>>> data [2]. The code in spl-fit.c doesn't remove the size [3]. Why is this
>>> size added?
>>
>> I looked into this a tiny bit, git blame eventually ends up at:
>>
>>> commit eb0f4a4cb40264a90a91e10e3ec00d1e0da7fb66
>>> Author: Simon Glass <sjg at chromium.org>
>>> Date:   2019-07-20 12:24:06 -0600
>>>
>>> binman: Support replacing data in a cbfs
>>>
>>> At present binman cannot replace data within a CBFS since it does not
>>> allow rewriting of the files in that CBFS. Implement this by using the
>>> new WriteData() method to handle the case.
>>>
>>> Add a header to compressed data so that the amount of compressed data can
>>> be determined without reference to the size of the containing entry. This
>>> allows the entry to be larger that the contents, without causing errors in
>>> decompression. 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). It is not used with CBFS since it has its
>>> own metadata for this. Increase the number of passes allowed to resolve
>>> the position of entries, to handle this case.
>>>
>>> Add a test for this new logic.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
> 
> Thanks. It looks like the header was added for internal compress and 
> decompress. It shouldn't be part of the blob.
> 
>> For the record, I dislike the size header, since as you found out it
>> turns the standard formats into custom formats that nothing else knows.
> 
> Maybe I overlook something in the code but I don't find a support for 
> the header inside the bootm or spl_fit code.
> 
>> But I'm not familiar with binman's compression parts, I don't know what
>> would go wrong if the header is removed and how. I'm also not sure what
>> happens in the 'obscure case' the commit message mentions.
> 
> The header should be removed or the fit image should point to the offset 
> behind the header. The compression test passed only because it supports 
> the header.

I agree that the header should be removed completely. I'll try to do it
later on, but I don't know when I'll be able to.