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

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

[Stefan Herbrechtsmeier]

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?

[1] 
https://source.denx.de/u-boot/u-boot/-/blob/master/tools/binman/comp_util.py#L44
[2] 
https://source.denx.de/u-boot/u-boot/-/blob/master/tools/binman/entry.py#L1065
[3] 
https://source.denx.de/u-boot/u-boot/-/blob/master/common/spl/spl_fit.c#L334

Regards
   Stefan

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

[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@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@chromium.org>

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.

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.

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

[Stefan Herbrechtsmeier]

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

Re: 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@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@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.