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

[Jules Maselbas <jmaselbas@zdiv.net>]

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

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.

Thanks for pointing this out!

Cheers,