Re: [PATCH 1/3] decompressors: Update xz to include ARM64 BCJ decoder

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Jules,

On 07.09.23 12:42, Jules Maselbas wrote:
> Hi Ahmad,
> 
> On Thu, Sep 07, 2023 at 12:08:34PM +0200, Ahmad Fatoum wrote:
>> Hello Jules,
>>
>> On 07.09.23 11:57, Jules Maselbas wrote:
>>> On Thu, Sep 07, 2023 at 11:00:05AM +0200, Ahmad Fatoum wrote:
>>>> On 06.09.23 17:08, Jules Maselbas wrote:
>>>>> Update lib/xz/xz_dec_bcj.c and lib/xz/xz_private.h files from xz-embedded [1],
>>>>> which include spelling fixes and the new ARM64 BCJ decoder which was recently
>>>>> introduced into .xz file format version 1.1.0 (2022-12-11) [2].
>>>>>
>>>>> [1] https://git.tukaani.org/?p=xz-embedded.git
>>>>> [2] https://tukaani.org/xz/xz-file-format-1.1.0.txt
>>>>>
>>>>> Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>
>>>>> ---
>>>>>  lib/Kconfig           |  4 ++++
>>>>>  lib/decompress_unxz.c |  3 +++
>>>>>  lib/xz/xz_dec_bcj.c   | 54 ++++++++++++++++++++++++++++++++++++++++---
>>>>>  lib/xz/xz_private.h   |  3 +++
>>>>>  4 files changed, 61 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>>>> index 758197b608..9291e5a8ff 100644
>>>>> --- a/lib/Kconfig
>>>>> +++ b/lib/Kconfig
>>>>> @@ -42,6 +42,7 @@ config XZ_DECOMPRESS
>>>>>  	select XZ_DEC_ARM
>>>>>  	select XZ_DEC_ARMTHUMB
>>>>>  	select XZ_DEC_SPARC
>>>>> +	select XZ_DEC_ARM64
>>>>
>>>> Hmm, maybe for PBL, we should only support the XZ BCJ filter that is chosen
>>>> for barebox proper? If you are trying to get barebox binary size down,
>>>> maybe you are inclined to check? :-)
>>>
>>> indeed, i thought it was already the case. How would do that ? have two version
>>> of the unxz binary object ? one for pbl and the other for barebox proper ?
>>> Or maybe only select the relevent decoder ?
>>
>> You already have two objects (.o and .o.pbl). What you'd do is ignore the Kconfig
>> option in the PBL case. Looking into the code, that's indeed happening
>> CONFIG_XZ_DEC_ARM is only consulted when !defined(XZ_PREBOOT) and that symbol
>> is only defined for PBL. This has one small implication for your code though:
> 
> I didn't found the .o.pbl, but this seems to be compiled in pbl/decompress.o,
> which directly includes decompress_unxz.c after defining STATIC (which will makes
> XZ_PREBOOT being defined).

Ah, right. The decompressors are a bit "special" in that regard.

> 
> After digging a bit, it seems that pbl already only contains the relevent decoder.
> 
>>>
>>> snip
>>>
>>>>> diff --git a/lib/decompress_unxz.c b/lib/decompress_unxz.c
>>>>> index a7e2d331ab..5c906932f8 100644
>>>>> --- a/lib/decompress_unxz.c
>>>>> +++ b/lib/decompress_unxz.c
>>>>> @@ -132,6 +132,9 @@
>>>>>  #endif
>>>>>  #ifdef CONFIG_ARM
>>>>>  #	define XZ_DEC_ARM
>>>>> +#	ifdef CONFIG_CPU_64
>>>>> +#		define XZ_DEC_ARM64
>>
>> You define XZ_DEC_ARM in the aarch64 case, which wastes space unnecessarily.
> Yeah, i was just a bit worried about "backward" compatibility where a newer pbl
> would be used to boot an older barebox proper... compressed with the arm BCJ.
> On a second thought, this doesn't makes much sens, barebox proper and pbl are
> tightly integrated, so this should not be an issue.

Ye, booting mismatched barebox PBLs and proper binaries isn't something
that we guarantee to work. For this reason, e.g. i.MX8M enters PBL twice:
Once while running in SRAM and once after relocating whole barebox binary
(including PBL) to DRAM.

Cheers,
Ahmad

> 
> Thanks for pointing this out!
> 
> Cheers,
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |