[PATCH] lib: xz: add support for bcj filters

[PATCH] lib: xz: add support for bcj filters

[yegorslists]

From: Yegor Yefremov <yegorslists@googlemail.com>

Add missing configuration options for various bcj filters. Without
these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
filters will be disabled.

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
 lib/Kconfig | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/lib/Kconfig b/lib/Kconfig
index f9f25bdef..83dd8e0a4 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -22,6 +22,34 @@ config XZ_DECOMPRESS
 	bool "include xz uncompression support"
 	select UNCOMPRESS
 
+if XZ_DECOMPRESS
+
+config XZ_DEC_X86
+        bool "x86 BCJ filter decoder"
+        default y
+
+config XZ_DEC_POWERPC
+        bool "PowerPC BCJ filter decoder"
+        default y
+
+config XZ_DEC_IA64
+        bool "IA-64 BCJ filter decoder"
+        default y
+
+config XZ_DEC_ARM
+        bool "ARM BCJ filter decoder"
+        default y
+
+config XZ_DEC_ARMTHUMB
+        bool "ARM-Thumb BCJ filter decoder"
+        default y
+
+config XZ_DEC_SPARC
+        bool "SPARC BCJ filter decoder"
+        default y
+
+endif
+
 config REED_SOLOMON
 	bool
 
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: xz: add support for bcj filters

[Jean-Christophe PLAGNIOL-VILLARD]

> On Feb 21, 2017, at 10:47 PM, yegorslists@googlemail.com wrote:
> 
> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> Add missing configuration options for various bcj filters. Without
> these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
> filters will be disabled.
> 
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
> lib/Kconfig | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
> 
> diff --git a/lib/Kconfig b/lib/Kconfig
> index f9f25bdef..83dd8e0a4 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -22,6 +22,34 @@ config XZ_DECOMPRESS
> 	bool "include xz uncompression support"
> 	select UNCOMPRESS
> 
> +if XZ_DECOMPRESS
> +
> +config XZ_DEC_X86
> +        bool "x86 BCJ filter decoder"
> +        default y
this need to be ARCH dependant 
> +
> +config XZ_DEC_POWERPC
> +        bool "PowerPC BCJ filter decoder"
> +        default y
> +
ditto
> +config XZ_DEC_IA64
> +        bool "IA-64 BCJ filter decoder"
> +        default y
ditto
> +config XZ_DEC_ARM
> +        bool "ARM BCJ filter decoder"
> +        default y
> +
ditto
> +config XZ_DEC_ARMTHUMB
> +        bool "ARM-Thumb BCJ filter decoder"
> +        default y
> +
ditto
> +config XZ_DEC_SPARC
> +        bool "SPARC BCJ filter decoder"
> +        default y
> +
> +endif
> +
> config REED_SOLOMON
> 	bool
> 
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: xz: add support for bcj filters

[Yegor Yefremov]

On Wed, Feb 22, 2017 at 5:29 AM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
>
>> On Feb 21, 2017, at 10:47 PM, yegorslists@googlemail.com wrote:
>>
>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>
>> Add missing configuration options for various bcj filters. Without
>> these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
>> filters will be disabled.
>>
>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> ---
>> lib/Kconfig | 28 ++++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index f9f25bdef..83dd8e0a4 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -22,6 +22,34 @@ config XZ_DECOMPRESS
>>       bool "include xz uncompression support"
>>       select UNCOMPRESS
>>
>> +if XZ_DECOMPRESS
>> +
>> +config XZ_DEC_X86
>> +        bool "x86 BCJ filter decoder"
>> +        default y
> this need to be ARCH dependant

Why? In Linux kernel it is not arch dependent, because AFAIK it
describes only compression particularities. This way you can extract
sqaushfs images on your ARM machine, that were compressed on SPARK
etc.

>> +
>> +config XZ_DEC_POWERPC
>> +        bool "PowerPC BCJ filter decoder"
>> +        default y
>> +
> ditto
>> +config XZ_DEC_IA64
>> +        bool "IA-64 BCJ filter decoder"
>> +        default y
> ditto
>> +config XZ_DEC_ARM
>> +        bool "ARM BCJ filter decoder"
>> +        default y
>> +
> ditto
>> +config XZ_DEC_ARMTHUMB
>> +        bool "ARM-Thumb BCJ filter decoder"
>> +        default y
>> +
> ditto
>> +config XZ_DEC_SPARC
>> +        bool "SPARC BCJ filter decoder"
>> +        default y
>> +
>> +endif
>> +
>> config REED_SOLOMON
>>       bool
>>
>> --
>> 2.11.0
>>
>>
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: xz: add support for bcj filters

[Sascha Hauer]

On Wed, Feb 22, 2017 at 09:15:36AM +0100, Yegor Yefremov wrote:
> On Wed, Feb 22, 2017 at 5:29 AM, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj@jcrosoft.com> wrote:
> >
> >> On Feb 21, 2017, at 10:47 PM, yegorslists@googlemail.com wrote:
> >>
> >> From: Yegor Yefremov <yegorslists@googlemail.com>
> >>
> >> Add missing configuration options for various bcj filters. Without
> >> these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
> >> filters will be disabled.
> >>
> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >> ---
> >> lib/Kconfig | 28 ++++++++++++++++++++++++++++
> >> 1 file changed, 28 insertions(+)
> >>
> >> diff --git a/lib/Kconfig b/lib/Kconfig
> >> index f9f25bdef..83dd8e0a4 100644
> >> --- a/lib/Kconfig
> >> +++ b/lib/Kconfig
> >> @@ -22,6 +22,34 @@ config XZ_DECOMPRESS
> >>       bool "include xz uncompression support"
> >>       select UNCOMPRESS
> >>
> >> +if XZ_DECOMPRESS
> >> +
> >> +config XZ_DEC_X86
> >> +        bool "x86 BCJ filter decoder"
> >> +        default y
> > this need to be ARCH dependant
> 
> Why? In Linux kernel it is not arch dependent, because AFAIK it
> describes only compression particularities. This way you can extract
> sqaushfs images on your ARM machine, that were compressed on SPARK
> etc.

So xz uses the Sparc bcj filter even when compressing ARM binaries? That
doesn't sound very mature to me.

It is quite surprising to me that I have to enable XZ_DEC_X86 in barebox
since this is the architecture the xz binary was most likely compressed
on. Also it's surprising that XZ_DEC_X86 won't bring us any gain since
the xz binary will most likely contain ARM instructions on a ARM build,
except that without XZ_DEC_X86 enabled it will not work.

Then in bcj_apply() we have:

#ifdef XZ_DEC_X86
	case BCJ_X86:
		filtered = bcj_x86(s, buf, size);
		break;
#endif

...

	default:
		/* Never reached but silence compiler warnings. */
		filtered = 0;

This is simply not true when any of the ifdefs is not true.

I think we should enable all the XZ_DEC_* options but without making them
visible since if someone ever stumbles upon these options and changes them
he will most likely do it wrong.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: xz: add support for bcj filters

[Yegor Yefremov]

On Wed, Feb 22, 2017 at 11:03 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Feb 22, 2017 at 09:15:36AM +0100, Yegor Yefremov wrote:
>> On Wed, Feb 22, 2017 at 5:29 AM, Jean-Christophe PLAGNIOL-VILLARD
>> <plagnioj@jcrosoft.com> wrote:
>> >
>> >> On Feb 21, 2017, at 10:47 PM, yegorslists@googlemail.com wrote:
>> >>
>> >> From: Yegor Yefremov <yegorslists@googlemail.com>
>> >>
>> >> Add missing configuration options for various bcj filters. Without
>> >> these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
>> >> filters will be disabled.
>> >>
>> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> >> ---
>> >> lib/Kconfig | 28 ++++++++++++++++++++++++++++
>> >> 1 file changed, 28 insertions(+)
>> >>
>> >> diff --git a/lib/Kconfig b/lib/Kconfig
>> >> index f9f25bdef..83dd8e0a4 100644
>> >> --- a/lib/Kconfig
>> >> +++ b/lib/Kconfig
>> >> @@ -22,6 +22,34 @@ config XZ_DECOMPRESS
>> >>       bool "include xz uncompression support"
>> >>       select UNCOMPRESS
>> >>
>> >> +if XZ_DECOMPRESS
>> >> +
>> >> +config XZ_DEC_X86
>> >> +        bool "x86 BCJ filter decoder"
>> >> +        default y
>> > this need to be ARCH dependant
>>
>> Why? In Linux kernel it is not arch dependent, because AFAIK it
>> describes only compression particularities. This way you can extract
>> sqaushfs images on your ARM machine, that were compressed on SPARK
>> etc.
>
> So xz uses the Sparc bcj filter even when compressing ARM binaries? That
> doesn't sound very mature to me.

This is just an xz command line option. So it is completely in the
user responsibility.

A passage from man page:

" A BCJ filter converts relative addresses in the machine code to
their absolute counterparts.  This doesn't change the size of the
data, but it increases redundancy, which can help LZMA2 to produce
0-15 %  smaller .xz file.  The BCJ filters are always reversible, so
using a BCJ filter for wrong type of data doesn't cause any data loss,
although it may make the compression ratio slightly worse."

> It is quite surprising to me that I have to enable XZ_DEC_X86 in barebox
> since this is the architecture the xz binary was most likely compressed
> on. Also it's surprising that XZ_DEC_X86 won't bring us any gain since
> the xz binary will most likely contain ARM instructions on a ARM build,
> except that without XZ_DEC_X86 enabled it will not work.
>
> Then in bcj_apply() we have:
>
> #ifdef XZ_DEC_X86
>         case BCJ_X86:
>                 filtered = bcj_x86(s, buf, size);
>                 break;
> #endif
>
> ...
>
>         default:
>                 /* Never reached but silence compiler warnings. */
>                 filtered = 0;
>
> This is simply not true when any of the ifdefs is not true.
>
> I think we should enable all the XZ_DEC_* options but without making them
> visible since if someone ever stumbles upon these options and changes them
> he will most likely do it wrong.

OK. I'll send v2.

Yegor

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: xz: add support for bcj filters

[Sascha Hauer]

On Wed, Feb 22, 2017 at 11:10:36AM +0100, Yegor Yefremov wrote:
> On Wed, Feb 22, 2017 at 11:03 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Feb 22, 2017 at 09:15:36AM +0100, Yegor Yefremov wrote:
> >> On Wed, Feb 22, 2017 at 5:29 AM, Jean-Christophe PLAGNIOL-VILLARD
> >> <plagnioj@jcrosoft.com> wrote:
> >> >
> >> >> On Feb 21, 2017, at 10:47 PM, yegorslists@googlemail.com wrote:
> >> >>
> >> >> From: Yegor Yefremov <yegorslists@googlemail.com>
> >> >>
> >> >> Add missing configuration options for various bcj filters. Without
> >> >> these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
> >> >> filters will be disabled.
> >> >>
> >> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >> >> ---
> >> >> lib/Kconfig | 28 ++++++++++++++++++++++++++++
> >> >> 1 file changed, 28 insertions(+)
> >> >>
> >> >> diff --git a/lib/Kconfig b/lib/Kconfig
> >> >> index f9f25bdef..83dd8e0a4 100644
> >> >> --- a/lib/Kconfig
> >> >> +++ b/lib/Kconfig
> >> >> @@ -22,6 +22,34 @@ config XZ_DECOMPRESS
> >> >>       bool "include xz uncompression support"
> >> >>       select UNCOMPRESS
> >> >>
> >> >> +if XZ_DECOMPRESS
> >> >> +
> >> >> +config XZ_DEC_X86
> >> >> +        bool "x86 BCJ filter decoder"
> >> >> +        default y
> >> > this need to be ARCH dependant
> >>
> >> Why? In Linux kernel it is not arch dependent, because AFAIK it
> >> describes only compression particularities. This way you can extract
> >> sqaushfs images on your ARM machine, that were compressed on SPARK
> >> etc.
> >
> > So xz uses the Sparc bcj filter even when compressing ARM binaries? That
> > doesn't sound very mature to me.
> 
> This is just an xz command line option. So it is completely in the
> user responsibility.

I see. What's the default for this option? The architecture xz is
running on?

Otherwise we could do a:

config XZ_DEC_ARM
        bool "ARM BCJ filter decoder"
        default y if ARCH_ARM

On the other hand I'm not sure if the potential image size gain for
disabling the unused bcj filters is even worth thinking about the
problem...

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: xz: add support for bcj filters

[Yegor Yefremov]

On Wed, Feb 22, 2017 at 11:21 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Feb 22, 2017 at 11:10:36AM +0100, Yegor Yefremov wrote:
>> On Wed, Feb 22, 2017 at 11:03 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Wed, Feb 22, 2017 at 09:15:36AM +0100, Yegor Yefremov wrote:
>> >> On Wed, Feb 22, 2017 at 5:29 AM, Jean-Christophe PLAGNIOL-VILLARD
>> >> <plagnioj@jcrosoft.com> wrote:
>> >> >
>> >> >> On Feb 21, 2017, at 10:47 PM, yegorslists@googlemail.com wrote:
>> >> >>
>> >> >> From: Yegor Yefremov <yegorslists@googlemail.com>
>> >> >>
>> >> >> Add missing configuration options for various bcj filters. Without
>> >> >> these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
>> >> >> filters will be disabled.
>> >> >>
>> >> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> >> >> ---
>> >> >> lib/Kconfig | 28 ++++++++++++++++++++++++++++
>> >> >> 1 file changed, 28 insertions(+)
>> >> >>
>> >> >> diff --git a/lib/Kconfig b/lib/Kconfig
>> >> >> index f9f25bdef..83dd8e0a4 100644
>> >> >> --- a/lib/Kconfig
>> >> >> +++ b/lib/Kconfig
>> >> >> @@ -22,6 +22,34 @@ config XZ_DECOMPRESS
>> >> >>       bool "include xz uncompression support"
>> >> >>       select UNCOMPRESS
>> >> >>
>> >> >> +if XZ_DECOMPRESS
>> >> >> +
>> >> >> +config XZ_DEC_X86
>> >> >> +        bool "x86 BCJ filter decoder"
>> >> >> +        default y
>> >> > this need to be ARCH dependant
>> >>
>> >> Why? In Linux kernel it is not arch dependent, because AFAIK it
>> >> describes only compression particularities. This way you can extract
>> >> sqaushfs images on your ARM machine, that were compressed on SPARK
>> >> etc.
>> >
>> > So xz uses the Sparc bcj filter even when compressing ARM binaries? That
>> > doesn't sound very mature to me.
>>
>> This is just an xz command line option. So it is completely in the
>> user responsibility.
>
> I see. What's the default for this option? The architecture xz is
> running on?
>
> Otherwise we could do a:
>
> config XZ_DEC_ARM
>         bool "ARM BCJ filter decoder"
>         default y if ARCH_ARM
>
> On the other hand I'm not sure if the potential image size gain for
> disabling the unused bcj filters is even worth thinking about the
> problem...

My barebox images without filters:

4668516 barebox
640752 barebox.bin

with all filters:

4679592 barebox
642632 barebox.bin

Yegor

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: xz: add support for bcj filters

[Sascha Hauer]

On Wed, Feb 22, 2017 at 11:35:46AM +0100, Yegor Yefremov wrote:
> On Wed, Feb 22, 2017 at 11:21 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Feb 22, 2017 at 11:10:36AM +0100, Yegor Yefremov wrote:
> >> On Wed, Feb 22, 2017 at 11:03 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > On Wed, Feb 22, 2017 at 09:15:36AM +0100, Yegor Yefremov wrote:
> >> >> On Wed, Feb 22, 2017 at 5:29 AM, Jean-Christophe PLAGNIOL-VILLARD
> >> >> <plagnioj@jcrosoft.com> wrote:
> >> >> >
> >> >> >> On Feb 21, 2017, at 10:47 PM, yegorslists@googlemail.com wrote:
> >> >> >>
> >> >> >> From: Yegor Yefremov <yegorslists@googlemail.com>
> >> >> >>
> >> >> >> Add missing configuration options for various bcj filters. Without
> >> >> >> these options the lib/xz/xz_dec_bcj.c file will be compiled, but all
> >> >> >> filters will be disabled.
> >> >> >>
> >> >> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >> >> >> ---
> >> >> >> lib/Kconfig | 28 ++++++++++++++++++++++++++++
> >> >> >> 1 file changed, 28 insertions(+)
> >> >> >>
> >> >> >> diff --git a/lib/Kconfig b/lib/Kconfig
> >> >> >> index f9f25bdef..83dd8e0a4 100644
> >> >> >> --- a/lib/Kconfig
> >> >> >> +++ b/lib/Kconfig
> >> >> >> @@ -22,6 +22,34 @@ config XZ_DECOMPRESS
> >> >> >>       bool "include xz uncompression support"
> >> >> >>       select UNCOMPRESS
> >> >> >>
> >> >> >> +if XZ_DECOMPRESS
> >> >> >> +
> >> >> >> +config XZ_DEC_X86
> >> >> >> +        bool "x86 BCJ filter decoder"
> >> >> >> +        default y
> >> >> > this need to be ARCH dependant
> >> >>
> >> >> Why? In Linux kernel it is not arch dependent, because AFAIK it
> >> >> describes only compression particularities. This way you can extract
> >> >> sqaushfs images on your ARM machine, that were compressed on SPARK
> >> >> etc.
> >> >
> >> > So xz uses the Sparc bcj filter even when compressing ARM binaries? That
> >> > doesn't sound very mature to me.
> >>
> >> This is just an xz command line option. So it is completely in the
> >> user responsibility.
> >
> > I see. What's the default for this option? The architecture xz is
> > running on?
> >
> > Otherwise we could do a:
> >
> > config XZ_DEC_ARM
> >         bool "ARM BCJ filter decoder"
> >         default y if ARCH_ARM
> >
> > On the other hand I'm not sure if the potential image size gain for
> > disabling the unused bcj filters is even worth thinking about the
> > problem...
> 
> My barebox images without filters:
> 
> 4668516 barebox
> 640752 barebox.bin
> 
> with all filters:
> 
> 4679592 barebox
> 642632 barebox.bin

Ok, so not worth discussing about. Size concerned people should disable
xz support ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox