[PATCH] xz: make XZ_DEC_BCJ filters non-optional

[PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Kyle McMartin]

From: Kyle McMartin <kyle@redhat.com>

Having these optional is more trouble than is justified by the
negligible increase in code size to lib/xz/ if they're all compiled in.
Their optional status ends up necessitating rebuilds of the kernel
in order to be able to decompress XZ-compressed squashfs images which
use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on
x86_64 in order to loop-mount a ppc64 squashfs that uses it.)

So save ourselves the trouble and build them all into xz_dec_bcj.o to
begin with. (I could conceivably see a case where CONFIG_EXPERT made
these selectable, but again, even on embedded platforms, the .text
increase we're talking about is noise...)

text	data	bss	dec	hex	filename
1239	0	0	1239	4d7	xz_dec_bcj.o
2263	0	0	2263	8d7	xz_dec_bcj.o.2

regards, Kyle

Signed-off-by: Kyle McMartin <kyle@redhat.com>

--- a/lib/xz/Kconfig
+++ b/lib/xz/Kconfig
@@ -6,44 +6,6 @@ config XZ_DEC
 	  the .xz file format as the container. For integrity checking,
 	  CRC32 is supported. See Documentation/xz.txt for more information.
 
-if XZ_DEC
-
-config XZ_DEC_X86
-	bool "x86 BCJ filter decoder"
-	default y if X86
-	select XZ_DEC_BCJ
-
-config XZ_DEC_POWERPC
-	bool "PowerPC BCJ filter decoder"
-	default y if PPC
-	select XZ_DEC_BCJ
-
-config XZ_DEC_IA64
-	bool "IA-64 BCJ filter decoder"
-	default y if IA64
-	select XZ_DEC_BCJ
-
-config XZ_DEC_ARM
-	bool "ARM BCJ filter decoder"
-	default y if ARM
-	select XZ_DEC_BCJ
-
-config XZ_DEC_ARMTHUMB
-	bool "ARM-Thumb BCJ filter decoder"
-	default y if (ARM && ARM_THUMB)
-	select XZ_DEC_BCJ
-
-config XZ_DEC_SPARC
-	bool "SPARC BCJ filter decoder"
-	default y if SPARC
-	select XZ_DEC_BCJ
-
-endif
-
-config XZ_DEC_BCJ
-	bool
-	default n
-
 config XZ_DEC_TEST
 	tristate "XZ decompressor tester"
 	default n
diff --git a/lib/xz/Makefile b/lib/xz/Makefile
index a7fa769..4f209f7 100644
--- a/lib/xz/Makefile
+++ b/lib/xz/Makefile
@@ -1,5 +1,4 @@
 obj-$(CONFIG_XZ_DEC) += xz_dec.o
-xz_dec-y := xz_dec_syms.o xz_dec_stream.o xz_dec_lzma2.o
-xz_dec-$(CONFIG_XZ_DEC_BCJ) += xz_dec_bcj.o
+xz_dec-y := xz_dec_syms.o xz_dec_stream.o xz_dec_lzma2.o xz_dec_bcj.o
 
 obj-$(CONFIG_XZ_DEC_TEST) += xz_dec_test.o
diff --git a/lib/xz/xz_dec_bcj.c b/lib/xz/xz_dec_bcj.c
index a768e6d..e2ac55c 100644
--- a/lib/xz/xz_dec_bcj.c
+++ b/lib/xz/xz_dec_bcj.c
@@ -10,12 +10,6 @@
 
 #include "xz_private.h"
 
-/*
- * The rest of the file is inside this ifdef. It makes things a little more
- * convenient when building without support for any BCJ filters.
- */
-#ifdef XZ_DEC_BCJ
-
 struct xz_dec_bcj {
 	/* Type of the BCJ filter being used */
 	enum {
@@ -75,7 +69,6 @@ struct xz_dec_bcj {
 	} temp;
 };
 
-#ifdef XZ_DEC_X86
 /*
  * This is used to test the most significant byte of a memory address
  * in an x86 instruction.
@@ -154,9 +147,7 @@ static size_t bcj_x86(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 	s->x86_prev_mask = prev_pos > 3 ? 0 : prev_mask << (prev_pos - 1);
 	return i;
 }
-#endif
 
-#ifdef XZ_DEC_POWERPC
 static size_t bcj_powerpc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 {
 	size_t i;
@@ -175,9 +166,7 @@ static size_t bcj_powerpc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 
 	return i;
 }
-#endif
 
-#ifdef XZ_DEC_IA64
 static size_t bcj_ia64(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 {
 	static const uint8_t branch_table[32] = {
@@ -259,9 +248,7 @@ static size_t bcj_ia64(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 
 	return i;
 }
-#endif
 
-#ifdef XZ_DEC_ARM
 static size_t bcj_arm(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 {
 	size_t i;
@@ -282,9 +269,7 @@ static size_t bcj_arm(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 
 	return i;
 }
-#endif
 
-#ifdef XZ_DEC_ARMTHUMB
 static size_t bcj_armthumb(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 {
 	size_t i;
@@ -310,9 +295,7 @@ static size_t bcj_armthumb(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 
 	return i;
 }
-#endif
 
-#ifdef XZ_DEC_SPARC
 static size_t bcj_sparc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 {
 	size_t i;
@@ -332,7 +315,6 @@ static size_t bcj_sparc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 
 	return i;
 }
-#endif
 
 /*
  * Apply the selected BCJ filter. Update *pos and s->pos to match the amount
@@ -351,36 +333,24 @@ static void bcj_apply(struct xz_dec_bcj *s,
 	size -= *pos;
 
 	switch (s->type) {
-#ifdef XZ_DEC_X86
 	case BCJ_X86:
 		filtered = bcj_x86(s, buf, size);
 		break;
-#endif
-#ifdef XZ_DEC_POWERPC
 	case BCJ_POWERPC:
 		filtered = bcj_powerpc(s, buf, size);
 		break;
-#endif
-#ifdef XZ_DEC_IA64
 	case BCJ_IA64:
 		filtered = bcj_ia64(s, buf, size);
 		break;
-#endif
-#ifdef XZ_DEC_ARM
 	case BCJ_ARM:
 		filtered = bcj_arm(s, buf, size);
 		break;
-#endif
-#ifdef XZ_DEC_ARMTHUMB
 	case BCJ_ARMTHUMB:
 		filtered = bcj_armthumb(s, buf, size);
 		break;
-#endif
-#ifdef XZ_DEC_SPARC
 	case BCJ_SPARC:
 		filtered = bcj_sparc(s, buf, size);
 		break;
-#endif
 	default:
 		/* Never reached but silence compiler warnings. */
 		filtered = 0;
@@ -536,24 +506,12 @@ XZ_EXTERN struct xz_dec_bcj *xz_dec_bcj_create(bool single_call)
 XZ_EXTERN enum xz_ret xz_dec_bcj_reset(struct xz_dec_bcj *s, uint8_t id)
 {
 	switch (id) {
-#ifdef XZ_DEC_X86
 	case BCJ_X86:
-#endif
-#ifdef XZ_DEC_POWERPC
 	case BCJ_POWERPC:
-#endif
-#ifdef XZ_DEC_IA64
 	case BCJ_IA64:
-#endif
-#ifdef XZ_DEC_ARM
 	case BCJ_ARM:
-#endif
-#ifdef XZ_DEC_ARMTHUMB
 	case BCJ_ARMTHUMB:
-#endif
-#ifdef XZ_DEC_SPARC
 	case BCJ_SPARC:
-#endif
 		break;
 
 	default:
@@ -571,4 +529,3 @@ XZ_EXTERN enum xz_ret xz_dec_bcj_reset(struct xz_dec_bcj *s, uint8_t id)
 	return XZ_OK;
 }
 
-#endif
diff --git a/lib/xz/xz_dec_stream.c b/lib/xz/xz_dec_stream.c
index ac809b1..a1ce0f7 100644
--- a/lib/xz/xz_dec_stream.c
+++ b/lib/xz/xz_dec_stream.c
@@ -130,10 +130,8 @@ struct xz_dec {
 
 	struct xz_dec_lzma2 *lzma2;
 
-#ifdef XZ_DEC_BCJ
 	struct xz_dec_bcj *bcj;
 	bool bcj_active;
-#endif
 };
 
 #ifdef XZ_DEC_ANY_CHECK
@@ -222,11 +220,9 @@ static enum xz_ret dec_block(struct xz_dec *s, struct xz_buf *b)
 	s->in_start = b->in_pos;
 	s->out_start = b->out_pos;
 
-#ifdef XZ_DEC_BCJ
 	if (s->bcj_active)
 		ret = xz_dec_bcj_run(s->bcj, s->lzma2, b);
 	else
-#endif
 		ret = xz_dec_lzma2_run(s->lzma2, b);
 
 	s->block.compressed += b->in_pos - s->in_start;
@@ -465,11 +461,7 @@ static enum xz_ret dec_block_header(struct xz_dec *s)
 	 * Catch unsupported Block Flags. We support only one or two filters
 	 * in the chain, so we catch that with the same test.
 	 */
-#ifdef XZ_DEC_BCJ
 	if (s->temp.buf[1] & 0x3E)
-#else
-	if (s->temp.buf[1] & 0x3F)
-#endif
 		return XZ_OPTIONS_ERROR;
 
 	/* Compressed Size */
@@ -494,7 +486,6 @@ static enum xz_ret dec_block_header(struct xz_dec *s)
 		s->block_header.uncompressed = VLI_UNKNOWN;
 	}
 
-#ifdef XZ_DEC_BCJ
 	/* If there are two filters, the first one must be a BCJ filter. */
 	s->bcj_active = s->temp.buf[1] & 0x01;
 	if (s->bcj_active) {
@@ -512,7 +503,6 @@ static enum xz_ret dec_block_header(struct xz_dec *s)
 		if (s->temp.buf[s->temp.pos++] != 0x00)
 			return XZ_OPTIONS_ERROR;
 	}
-#endif
 
 	/* Valid Filter Flags always take at least two bytes. */
 	if (s->temp.size - s->temp.pos < 2)
@@ -775,11 +765,9 @@ XZ_EXTERN struct xz_dec *xz_dec_init(enum xz_mode mode, uint32_t dict_max)
 
 	s->mode = mode;
 
-#ifdef XZ_DEC_BCJ
 	s->bcj = xz_dec_bcj_create(DEC_IS_SINGLE(mode));
 	if (s->bcj == NULL)
 		goto error_bcj;
-#endif
 
 	s->lzma2 = xz_dec_lzma2_create(mode, dict_max);
 	if (s->lzma2 == NULL)
@@ -789,10 +777,8 @@ XZ_EXTERN struct xz_dec *xz_dec_init(enum xz_mode mode, uint32_t dict_max)
 	return s;
 
 error_lzma2:
-#ifdef XZ_DEC_BCJ
 	xz_dec_bcj_end(s->bcj);
 error_bcj:
-#endif
 	kfree(s);
 	return NULL;
 }
@@ -813,9 +799,7 @@ XZ_EXTERN void xz_dec_end(struct xz_dec *s)
 {
 	if (s != NULL) {
 		xz_dec_lzma2_end(s->lzma2);
-#ifdef XZ_DEC_BCJ
 		xz_dec_bcj_end(s->bcj);
-#endif
 		kfree(s);
 	}
 }
diff --git a/lib/xz/xz_private.h b/lib/xz/xz_private.h
index 482b90f..72334af 100644
--- a/lib/xz/xz_private.h
+++ b/lib/xz/xz_private.h
@@ -19,24 +19,6 @@
 #		include <linux/slab.h>
 #		include <linux/vmalloc.h>
 #		include <linux/string.h>
-#		ifdef CONFIG_XZ_DEC_X86
-#			define XZ_DEC_X86
-#		endif
-#		ifdef CONFIG_XZ_DEC_POWERPC
-#			define XZ_DEC_POWERPC
-#		endif
-#		ifdef CONFIG_XZ_DEC_IA64
-#			define XZ_DEC_IA64
-#		endif
-#		ifdef CONFIG_XZ_DEC_ARM
-#			define XZ_DEC_ARM
-#		endif
-#		ifdef CONFIG_XZ_DEC_ARMTHUMB
-#			define XZ_DEC_ARMTHUMB
-#		endif
-#		ifdef CONFIG_XZ_DEC_SPARC
-#			define XZ_DEC_SPARC
-#		endif
 #		define memeq(a, b, size) (memcmp(a, b, size) == 0)
 #		define memzero(buf, size) memset(buf, 0, size)
 #	endif
@@ -90,19 +72,6 @@
 #endif
 
 /*
- * If any of the BCJ filter decoders are wanted, define XZ_DEC_BCJ.
- * XZ_DEC_BCJ is used to enable generic support for BCJ decoders.
- */
-#ifndef XZ_DEC_BCJ
-#	if defined(XZ_DEC_X86) || defined(XZ_DEC_POWERPC) \
-			|| defined(XZ_DEC_IA64) || defined(XZ_DEC_ARM) \
-			|| defined(XZ_DEC_ARM) || defined(XZ_DEC_ARMTHUMB) \
-			|| defined(XZ_DEC_SPARC)
-#		define XZ_DEC_BCJ
-#	endif
-#endif
-
-/*
  * Allocate memory for LZMA2 decoder. xz_dec_lzma2_reset() must be used
  * before calling xz_dec_lzma2_run().
  */
@@ -125,7 +94,6 @@ XZ_EXTERN enum xz_ret xz_dec_lzma2_run(struct xz_dec_lzma2 *s,
 /* Free the memory allocated for the LZMA2 decoder. */
 XZ_EXTERN void xz_dec_lzma2_end(struct xz_dec_lzma2 *s);
 
-#ifdef XZ_DEC_BCJ
 /*
  * Allocate memory for BCJ decoders. xz_dec_bcj_reset() must be used before
  * calling xz_dec_bcj_run().
@@ -151,6 +119,5 @@ XZ_EXTERN enum xz_ret xz_dec_bcj_run(struct xz_dec_bcj *s,
 
 /* Free the memory allocated for the BCJ filters. */
 #define xz_dec_bcj_end(s) kfree(s)
-#endif
 
 #endif

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Lasse Collin]

On 2014-02-28 Kyle McMartin wrote:
> Having these optional is more trouble than is justified by the
> negligible increase in code size to lib/xz/ if they're all compiled
> in. Their optional status ends up necessitating rebuilds of the kernel
> in order to be able to decompress XZ-compressed squashfs images which
> use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on
> x86_64 in order to loop-mount a ppc64 squashfs that uses it.)

Originally all BCJ filters were enabled by default and CONFIG_EXPERT
allowed disabling them one by one. It was changed a year ago to the
current state because I wasn't against it when it was suggested:

    http://lkml.org/lkml/2013/1/15/449

It seems that I should have been against it. I suggest things are
rolled back like they were: all BCJ filters enabled by default and
CONFIG_EXPERT makes them deselectable. This time I have a somewhat
strong opinion that this is the best way to go, even if it means that
the embedded people must remember to deselect the unnneeded filters.

An alternative could be that with CONFIG_EXPERT only the BCJ filter for
the current arch would be selected by default. Without CONFIG_EXPERT
all filters would be forced on. I guess this could be confusing since
the level of XZ support they get by default when they enable Squashfs'
XZ support would depend on CONFIG_EXPERT. So I think my first suggestion
is better.

> So save ourselves the trouble and build them all into xz_dec_bcj.o to
> begin with. (I could conceivably see a case where CONFIG_EXPERT made
> these selectable, but again, even on embedded platforms, the .text
> increase we're talking about is noise...)
> 
> text	data	bss	dec	hex	filename
> 1239	0	0	1239	4d7	xz_dec_bcj.o
> 2263	0	0	2263	8d7	xz_dec_bcj.o.2

No, let's not unconditionally build them all:

  - 1 KiB might be noise, but since on embedded systems that 1 KiB
    really is completely useless code and it's very easy to omit it,
    the option to omit such code should be kept.

  - If the kernel image is compressed with XZ, a separate copy of
    the decompressor is built for the pre-boot environment.
    Conditional compilation of the BCJ filters is also used for
    pre-boot environments which your patch doesn't touch. The
    1 KiB increase would affect the copy in the pre-boot code too.

  - This XZ decompressor was written with the Linux kernel in mind,
    but it's used elsewhere too. It would be nice to minimize the
    differences between the code in Linux and the out-of-kernel tree.
    Outside Linux I will keep the BCJ filters optional anyway.

Below are two alternative patches matching my two suggestions. I prefer
the first patch and I will submit it in a day or two unless people
have better ideas.

diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig
--- linux-3.14-rc5.orig/lib/xz/Kconfig	2014-03-03 04:56:16.000000000 +0200
+++ linux-3.14-rc5/lib/xz/Kconfig	2014-03-03 13:26:58.402872951 +0200
@@ -9,33 +9,33 @@
 if XZ_DEC
 
 config XZ_DEC_X86
-	bool "x86 BCJ filter decoder"
-	default y if X86
+	bool "x86 BCJ filter decoder" if EXPERT
+	default y
 	select XZ_DEC_BCJ
 
 config XZ_DEC_POWERPC
-	bool "PowerPC BCJ filter decoder"
-	default y if PPC
+	bool "PowerPC BCJ filter decoder" if EXPERT
+	default y
 	select XZ_DEC_BCJ
 
 config XZ_DEC_IA64
-	bool "IA-64 BCJ filter decoder"
-	default y if IA64
+	bool "IA-64 BCJ filter decoder" if EXPERT
+	default y
 	select XZ_DEC_BCJ
 
 config XZ_DEC_ARM
-	bool "ARM BCJ filter decoder"
-	default y if ARM
+	bool "ARM BCJ filter decoder" if EXPERT
+	default y
 	select XZ_DEC_BCJ
 
 config XZ_DEC_ARMTHUMB
-	bool "ARM-Thumb BCJ filter decoder"
-	default y if (ARM && ARM_THUMB)
+	bool "ARM-Thumb BCJ filter decoder" if EXPERT
+	default y
 	select XZ_DEC_BCJ
 
 config XZ_DEC_SPARC
-	bool "SPARC BCJ filter decoder"
-	default y if SPARC
+	bool "SPARC BCJ filter decoder" if EXPERT
+	default y
 	select XZ_DEC_BCJ
 
 endif

The second version enables the BCJ filter only for the current arch by
default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced
on:

diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig
--- linux-3.14-rc5.orig/lib/xz/Kconfig	2014-03-03 04:56:16.000000000 +0200
+++ linux-3.14-rc5/lib/xz/Kconfig	2014-03-03 14:15:42.196209325 +0200
@@ -9,33 +9,33 @@
 if XZ_DEC
 
 config XZ_DEC_X86
-	bool "x86 BCJ filter decoder"
-	default y if X86
+	bool "x86 BCJ filter decoder" if EXPERT
+	default y if !EXPERT || X86
 	select XZ_DEC_BCJ
 
 config XZ_DEC_POWERPC
-	bool "PowerPC BCJ filter decoder"
-	default y if PPC
+	bool "PowerPC BCJ filter decoder" if EXPERT
+	default y if !EXPERT || PPC
 	select XZ_DEC_BCJ
 
 config XZ_DEC_IA64
-	bool "IA-64 BCJ filter decoder"
-	default y if IA64
+	bool "IA-64 BCJ filter decoder" if EXPERT
+	default y if !EXPERT || IA64
 	select XZ_DEC_BCJ
 
 config XZ_DEC_ARM
-	bool "ARM BCJ filter decoder"
-	default y if ARM
+	bool "ARM BCJ filter decoder" if EXPERT
+	default y if !EXPERT || ARM
 	select XZ_DEC_BCJ
 
 config XZ_DEC_ARMTHUMB
-	bool "ARM-Thumb BCJ filter decoder"
-	default y if (ARM && ARM_THUMB)
+	bool "ARM-Thumb BCJ filter decoder" if EXPERT
+	default y if !EXPERT || (ARM && ARM_THUMB)
 	select XZ_DEC_BCJ
 
 config XZ_DEC_SPARC
-	bool "SPARC BCJ filter decoder"
-	default y if SPARC
+	bool "SPARC BCJ filter decoder" if EXPERT
+	default y if !EXPERT || SPARC
 	select XZ_DEC_BCJ
 
 endif

-- 
Lasse Collin  |  IRC: Larhzu @ IRCnet & Freenode

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Florian Fainelli]

2014-03-03 4:51 GMT-08:00 Lasse Collin <lasse.collin@tukaani.org>:
> On 2014-02-28 Kyle McMartin wrote:
>> Having these optional is more trouble than is justified by the
>> negligible increase in code size to lib/xz/ if they're all compiled
>> in. Their optional status ends up necessitating rebuilds of the kernel
>> in order to be able to decompress XZ-compressed squashfs images which
>> use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on
>> x86_64 in order to loop-mount a ppc64 squashfs that uses it.)
>
> Originally all BCJ filters were enabled by default and CONFIG_EXPERT
> allowed disabling them one by one. It was changed a year ago to the
> current state because I wasn't against it when it was suggested:
>
>     http://lkml.org/lkml/2013/1/15/449
>
> It seems that I should have been against it. I suggest things are
> rolled back like they were: all BCJ filters enabled by default and
> CONFIG_EXPERT makes them deselectable. This time I have a somewhat
> strong opinion that this is the best way to go, even if it means that
> the embedded people must remember to deselect the unnneeded filters.
>
> An alternative could be that with CONFIG_EXPERT only the BCJ filter for
> the current arch would be selected by default. Without CONFIG_EXPERT
> all filters would be forced on. I guess this could be confusing since
> the level of XZ support they get by default when they enable Squashfs'
> XZ support would depend on CONFIG_EXPERT. So I think my first suggestion
> is better.
>
>> So save ourselves the trouble and build them all into xz_dec_bcj.o to
>> begin with. (I could conceivably see a case where CONFIG_EXPERT made
>> these selectable, but again, even on embedded platforms, the .text
>> increase we're talking about is noise...)
>>
>> text  data    bss     dec     hex     filename
>> 1239  0       0       1239    4d7     xz_dec_bcj.o
>> 2263  0       0       2263    8d7     xz_dec_bcj.o.2
>
> No, let's not unconditionally build them all:
>
>   - 1 KiB might be noise, but since on embedded systems that 1 KiB
>     really is completely useless code and it's very easy to omit it,
>     the option to omit such code should be kept.
>
>   - If the kernel image is compressed with XZ, a separate copy of
>     the decompressor is built for the pre-boot environment.
>     Conditional compilation of the BCJ filters is also used for
>     pre-boot environments which your patch doesn't touch. The
>     1 KiB increase would affect the copy in the pre-boot code too.
>
>   - This XZ decompressor was written with the Linux kernel in mind,
>     but it's used elsewhere too. It would be nice to minimize the
>     differences between the code in Linux and the out-of-kernel tree.
>     Outside Linux I will keep the BCJ filters optional anyway.

Absolutely, this was actually the primary concern for these patches.
1KiB might not sound like a lot, but when you take than into account
in the bigger picture of saving kernel size, this ends up making a
difference.

>
> Below are two alternative patches matching my two suggestions. I prefer
> the first patch and I will submit it in a day or two unless people
> have better ideas.


>
> diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig
> --- linux-3.14-rc5.orig/lib/xz/Kconfig  2014-03-03 04:56:16.000000000 +0200
> +++ linux-3.14-rc5/lib/xz/Kconfig       2014-03-03 13:26:58.402872951 +0200
> @@ -9,33 +9,33 @@
>  if XZ_DEC
>
>  config XZ_DEC_X86
> -       bool "x86 BCJ filter decoder"
> -       default y if X86
> +       bool "x86 BCJ filter decoder" if EXPERT
> +       default y
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_POWERPC
> -       bool "PowerPC BCJ filter decoder"
> -       default y if PPC
> +       bool "PowerPC BCJ filter decoder" if EXPERT
> +       default y
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_IA64
> -       bool "IA-64 BCJ filter decoder"
> -       default y if IA64
> +       bool "IA-64 BCJ filter decoder" if EXPERT
> +       default y
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_ARM
> -       bool "ARM BCJ filter decoder"
> -       default y if ARM
> +       bool "ARM BCJ filter decoder" if EXPERT
> +       default y
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_ARMTHUMB
> -       bool "ARM-Thumb BCJ filter decoder"
> -       default y if (ARM && ARM_THUMB)
> +       bool "ARM-Thumb BCJ filter decoder" if EXPERT
> +       default y
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_SPARC
> -       bool "SPARC BCJ filter decoder"
> -       default y if SPARC
> +       bool "SPARC BCJ filter decoder" if EXPERT
> +       default y
>         select XZ_DEC_BCJ
>
>  endif
>
> The second version enables the BCJ filter only for the current arch by
> default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced
> on:

I like this version better because it still provides you with accurate
defaults (i.e: enable only th X86 BCJ filter decoder where it makes
sense by default) and still provides enough flexibility.

>
> diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig
> --- linux-3.14-rc5.orig/lib/xz/Kconfig  2014-03-03 04:56:16.000000000 +0200
> +++ linux-3.14-rc5/lib/xz/Kconfig       2014-03-03 14:15:42.196209325 +0200
> @@ -9,33 +9,33 @@
>  if XZ_DEC
>
>  config XZ_DEC_X86
> -       bool "x86 BCJ filter decoder"
> -       default y if X86
> +       bool "x86 BCJ filter decoder" if EXPERT
> +       default y if !EXPERT || X86
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_POWERPC
> -       bool "PowerPC BCJ filter decoder"
> -       default y if PPC
> +       bool "PowerPC BCJ filter decoder" if EXPERT
> +       default y if !EXPERT || PPC
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_IA64
> -       bool "IA-64 BCJ filter decoder"
> -       default y if IA64
> +       bool "IA-64 BCJ filter decoder" if EXPERT
> +       default y if !EXPERT || IA64
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_ARM
> -       bool "ARM BCJ filter decoder"
> -       default y if ARM
> +       bool "ARM BCJ filter decoder" if EXPERT
> +       default y if !EXPERT || ARM
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_ARMTHUMB
> -       bool "ARM-Thumb BCJ filter decoder"
> -       default y if (ARM && ARM_THUMB)
> +       bool "ARM-Thumb BCJ filter decoder" if EXPERT
> +       default y if !EXPERT || (ARM && ARM_THUMB)
>         select XZ_DEC_BCJ
>
>  config XZ_DEC_SPARC
> -       bool "SPARC BCJ filter decoder"
> -       default y if SPARC
> +       bool "SPARC BCJ filter decoder" if EXPERT
> +       default y if !EXPERT || SPARC
>         select XZ_DEC_BCJ
>
>  endif
>
> --
> Lasse Collin  |  IRC: Larhzu @ IRCnet & Freenode



-- 
Florian

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Lasse Collin]

On 2014-03-03 Florian Fainelli wrote:
> 2014-03-03 4:51 GMT-08:00 Lasse Collin <lasse.collin@tukaani.org>:
> > The second version enables the BCJ filter only for the current arch
> > by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are
> > forced on:
> 
> I like this version better because it still provides you with accurate
> defaults (i.e: enable only th X86 BCJ filter decoder where it makes
> sense by default) and still provides enough flexibility.

I understand your viewpoint, but different people probably consider
different options as "accurate defaults". The current behavior certainly
isn't very accurate; otherwise Kyle McMartin wouldn't have had a reason
to send a patch. The patch you prefer would give both of you the
defaults you want *if* embedded devs always use CONFIG_EXPERT and
non-embedded people never do. If a non-embedded dev uses CONFIG_EXPERT,
then Kyle's problem can reappear.

I still think it is clearest to enable all by default and allow
deselecting filters if CONFIG_EXPERT. That way the default selection
doesn't change behind anyone's back when CONFIG_EXPERT is selected.
I guess embedded devs go through the config carefully looking for
things to deselect anyway, but maybe it was easy to miss these options
just like some desktop users have missed them now.

I'd like to make the defaults such that minimum number of people get
annoyed in the long term. There will always be people who will need to
change the options (otherwise options wouldn't be needed). At the same
time this is so very tiny issue that I don't expect many people to care
at all.

I'll submit some patch this week but I'll wait for more opinions for a
day or two. Thanks.

-- 
Lasse Collin  |  IRC: Larhzu @ IRCnet & Freenode

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Florian Fainelli]

2014-03-04 18:51 GMT-08:00 Phillip Lougher <phillip@lougher.demon.co.uk>:
> On 04/03/14 18:20, Lasse Collin wrote:
>> On 2014-03-03 Florian Fainelli wrote:
>>> 2014-03-03 4:51 GMT-08:00 Lasse Collin <lasse.collin@tukaani.org>:
>>>> The second version enables the BCJ filter only for the current arch
>>>> by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are
>>>> forced on:
>>>
>>> I like this version better because it still provides you with accurate
>>> defaults (i.e: enable only th X86 BCJ filter decoder where it makes
>>> sense by default) and still provides enough flexibility.
>>
>> I understand your viewpoint, but different people probably consider
>> different options as "accurate defaults". The current behavior certainly
>> isn't very accurate; otherwise Kyle McMartin wouldn't have had a reason
>> to send a patch. The patch you prefer would give both of you the
>> defaults you want *if* embedded devs always use CONFIG_EXPERT and
>> non-embedded people never do. If a non-embedded dev uses CONFIG_EXPERT,
>> then Kyle's problem can reappear.
>>
>
> CONFIG_EMBEDDED was renamed CONFIG_EXPERT a couple of years
> ago because it was being used by more than embedded people.  As
> such I'd be wary adding an option which relied on the original
> meaning.
>
> http://lwn.net/Articles/421304/
>
>
>> I still think it is clearest to enable all by default and allow
>> deselecting filters if CONFIG_EXPERT. That way the default selection
>> doesn't change behind anyone's back when CONFIG_EXPERT is selected.
>> I guess embedded devs go through the config carefully looking for
>> things to deselect anyway, but maybe it was easy to miss these options
>> just like some desktop users have missed them now.
>>
>> I'd like to make the defaults such that minimum number of people get
>> annoyed in the long term. There will always be people who will need to
>> change the options (otherwise options wouldn't be needed). At the same
>> time this is so very tiny issue that I don't expect many people to care
>> at all.
>>
>> I'll submit some patch this week but I'll wait for more opinions for a
>> day or two. Thanks.
>>
>
> As Kyle should know Redhat hit this problem with Squashfs a while back,
> and it was I who identified the config changes as the culprit
> (BTW Kyle you should have CC'd me on the patch as a courtesy).
> The bug is only viewable by Redhat employees (disclaimer, I'm
> also a Redhat employee), and so I'll say no more.
>
> But speaking as the Squashfs author, the lack of BCJ support for
> an architecture creates a subtle failure mode in Squashfs, this is
> because not all blocks in a Squashfs filesystem get compressed
> with a BCJ filter.  At compression time each block is compressed
> without any BCJ filter, and then with the BCJ filter(s) selected on
> the command line, and the best compression for *that* block is
> chosen.  What this means is kernels without a particular
> BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
> read many of the files, it is only some files that mysteriously
> fail with decompression error.  As such this will be (and has been)
> invariably treated as a bug in Squashfs.
>
> Moreover, without expert knowledge of Squashfs, and the config
> options, most people will not have a clue how to fix the issue.
>
> This is why I prefer the first option, which is to reinstate
> the enabling of all filters by default, and then to allow people
> to remove the filters they don't want.
>
> BTW there is a potential additional fix for Squashfs that will
> make its handling of (lack of) BCJ filters more intelligent
> at mount time, but this of course only addresses Squashfs,
> and it relies on an additional call into XZ being added.  The
> BCJ filters specified at filesystem creation are stored in the
> compression options part of the superblock, and are known at
> mount time.  Squashfs should check that these filters are
> supported by the kernel and refuse to mount it otherwise.  This
> has not been done because AFAIK there is no way to query XZ to
> determine which BCJ filters are supported (beyond passing it a
> test stream which is too messy).
>
> If this call was added, I would be happy to add a patch to Squashfs
> to do this.

I am okay with enabling all filters by default as long as there is a
knob to allow turning achitecture specific BCJ filters off, as it
barely makes sense for embedded systems to have a BCJ filter
implementation for anything but the architecture you are running on.

It does make sense for a desktop use-case like what Kyle describes in
his initial patch. What you describes sounds like there is room for
improvements though instead of silently failing...

Thanks
-- 
Florian

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Geert Uytterhoeven]

On Wed, Mar 5, 2014 at 4:50 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> I am okay with enabling all filters by default as long as there is a
> knob to allow turning achitecture specific BCJ filters off, as it
> barely makes sense for embedded systems to have a BCJ filter
> implementation for anything but the architecture you are running on.

And for x86? (oh no, I started promoting x86 ;-)

I once tried xz with an initrd on ARM. The kernel complained it couldn't
decompress the initrd, oops. I didn't investigate it at that time, but probably
I didn't have the x86 BCJ filter enabled, while I compressed the initrd on\
amd64.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Lasse Collin]

On 2014-03-06 Geert Uytterhoeven wrote:
> I once tried xz with an initrd on ARM. The kernel complained it
> couldn't decompress the initrd, oops. I didn't investigate it at that
> time, but probably I didn't have the x86 BCJ filter enabled, while I
> compressed the initrd on\ amd64.

With so little information I can only guess, but it sounds unlikely
that you would have enabled the x86 BCJ filter without knowing it
unless you used some wrapper script that does it. It's more likely that
the ARM kernel didn't support XZ at all, you forgot --check=crc32,
or it ran out of RAM due to too big LZMA2 dictionary (if you used -9,
the decompressor allocates 64 MiB of memory, but I cannot guess how
much RAM the target system had).

In Documentation/xz.txt under "Notes on compression options" there are
some tips about compressing files for the in-kernel XZ decompressor.

-- 
Lasse Collin  |  IRC: Larhzu @ IRCnet & Freenode

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Geert Uytterhoeven]

On Sat, Mar 8, 2014 at 11:11 AM, Lasse Collin <lasse.collin@tukaani.org> wrote:
> On 2014-03-06 Geert Uytterhoeven wrote:
>> I once tried xz with an initrd on ARM. The kernel complained it
>> couldn't decompress the initrd, oops. I didn't investigate it at that
>> time, but probably I didn't have the x86 BCJ filter enabled, while I
>> compressed the initrd on\ amd64.
>
> With so little information I can only guess, but it sounds unlikely

I understand. Will see if I find time to retry...

> that you would have enabled the x86 BCJ filter without knowing it
> unless you used some wrapper script that does it. It's more likely that
> the ARM kernel didn't support XZ at all, you forgot --check=crc32,

I definitely didn't pass --check=crc32, just plain xz (like I'm used to plain
gzip ;-) So that's the most likely culprit.

> or it ran out of RAM due to too big LZMA2 dictionary (if you used -9,
> the decompressor allocates 64 MiB of memory, but I cannot guess how
> much RAM the target system had).

2 GiB, so that shouldn't be an issue.

> In Documentation/xz.txt under "Notes on compression options" there are
> some tips about compressing files for the in-kernel XZ decompressor.

Thanks, the --check=crc32 is indeed mentioned there.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Lasse Collin]

On 2014-03-05 Phillip Lougher wrote:
> (BTW Kyle you should have CC'd me on the patch as a courtesy).

I could have done that too but somehow I didn't, sorry.

> But speaking as the Squashfs author, the lack of BCJ support for
> an architecture creates a subtle failure mode in Squashfs, this is
> because not all blocks in a Squashfs filesystem get compressed
> with a BCJ filter.  At compression time each block is compressed
> without any BCJ filter, and then with the BCJ filter(s) selected on
> the command line, and the best compression for *that* block is
> chosen.  What this means is kernels without a particular
> BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
> read many of the files, it is only some files that mysteriously
> fail with decompression error.  As such this will be (and has been)
> invariably treated as a bug in Squashfs.

There is an easy way to make Squashfs give an error message in the
kernel log. xz_dec_run() gives XZ_OPTIONS_ERROR when valid-looking but
unsupported input is detected. Currently Squashfs treats all error
codes from xz_dec_run() the same way so the reason for the
decompression error is silently lost.

Below is an *untested* fix. I'm not sure about the exact wording of the
error message, so feel free to improve it.

diff -Narup linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c linux-3.14-rc5/fs/squashfs/xz_wrapper.c
--- linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c	2014-03-03 04:56:16.000000000 +0200
+++ linux-3.14-rc5/fs/squashfs/xz_wrapper.c	2014-03-05 18:08:58.729643127 +0200
@@ -170,8 +170,13 @@ static int squashfs_xz_uncompress(struct
 
 	squashfs_finish_page(output);
 
-	if (xz_err != XZ_STREAM_END || k < b)
+	if (xz_err != XZ_STREAM_END || k < b) {
+		if (xz_err == XZ_OPTIONS_ERROR)
+			ERROR("Unsupported XZ-compressed data; check the XZ "
+					"options in the kernel config\n");
+
 		goto out;
+	}
 
 	return total + stream->buf.out_pos;
 

> Moreover, without expert knowledge of Squashfs, and the config
> options, most people will not have a clue how to fix the issue.
> 
> This is why I prefer the first option, which is to reinstate
> the enabling of all filters by default, and then to allow people
> to remove the filters they don't want.

I will submit the first option. In the other email Florian Fainelli
seemed to be OK with that too.

> BTW there is a potential additional fix for Squashfs that will
> make its handling of (lack of) BCJ filters more intelligent
> at mount time, but this of course only addresses Squashfs,
> and it relies on an additional call into XZ being added.  The
> BCJ filters specified at filesystem creation are stored in the
> compression options part of the superblock, and are known at
> mount time.  Squashfs should check that these filters are
> supported by the kernel and refuse to mount it otherwise.  This
> has not been done because AFAIK there is no way to query XZ to
> determine which BCJ filters are supported (beyond passing it a
> test stream which is too messy).

You can use #ifdef CONFIG_XZ_DEC_X86 and such, although maybe that's
not convenient enough.

Adding a function to xz_dec module could be sort of OK. It's important
to keep in mind that the ability to disable filters is there to reduce
code size *slightly*. If you or we add something extra to figure out
what is supported at runtime, the size benefit may get lost.

If all filters are enabled by default, a clear error message on
XZ_OPTIONS_ERROR should be enough, I think.

-- 
Lasse Collin  |  IRC: Larhzu @ IRCnet & Freenode

Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Phillip Lougher]

On 05/03/14 16:24, Lasse Collin wrote:
> On 2014-03-05 Phillip Lougher wrote:
>> (BTW Kyle you should have CC'd me on the patch as a courtesy).
>
> I could have done that too but somehow I didn't, sorry.

np

>
>> But speaking as the Squashfs author, the lack of BCJ support for
>> an architecture creates a subtle failure mode in Squashfs, this is
>> because not all blocks in a Squashfs filesystem get compressed
>> with a BCJ filter.  At compression time each block is compressed
>> without any BCJ filter, and then with the BCJ filter(s) selected on
>> the command line, and the best compression for *that* block is
>> chosen.  What this means is kernels without a particular
>> BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
>> read many of the files, it is only some files that mysteriously
>> fail with decompression error.  As such this will be (and has been)
>> invariably treated as a bug in Squashfs.
>
> There is an easy way to make Squashfs give an error message in the
> kernel log. xz_dec_run() gives XZ_OPTIONS_ERROR when valid-looking but
> unsupported input is detected. Currently Squashfs treats all error
> codes from xz_dec_run() the same way so the reason for the
> decompression error is silently lost.

Yes, that is deliberate.  It's to give a generic easy to understand
error message for potentially novice users that may be running
Linux from LiveCDs.

When I wrote the original zlib support for Squashfs, I put in a lot
of debug information in the zlib error messages, e.g.

                        ERROR("zlib_inflate returned unexpected result"
                                " 0x%x, srclength %d, avail_in %d,"
                                " avail_out %d\n", zlib_err, srclength,
                                msblk->stream.avail_in,
                                msblk->stream.avail_out);

But after mainlining Squashfs in 2009, I started to get increasing
complaints that the error messages were too technical (full of
hex numbers) and confusing to new users - the kind of people who
maybe burn a corrupt liveCD and then get screenfulls of these
errors full of different numbers coming up on the screen.  They
would do a websearch and discover that the errors meant
"corrupted disk", and then ask why didn't it just say that, and
not give all those numbers.  Or worse they'd just silently give up
and go back to Windows.

So in March 2009 I changed it to the error message

ERROR("zlib_inflate error, data probably corrupt\n")

With *no* numbers ... and I copied the same approach for xz.

In kernel 3.13 (released earlier this year) I went further and
pulled out the error message printing from the compression
wrappers, and made it a single generic message, because I
realised there was no longer any decompressor specific error
handling (just the same message in each wrapper).

ERROR("%s decompression failed, data probably corrupt\n",
                         msblk->decompressor->name);

Putting back separate error messages into each decompressor wrapper,
and then putting back different error messages based on the
error code return seems like a retrograde step because distros don't
like them.

>
> Below is an *untested* fix. I'm not sure about the exact wording of the
> error message, so feel free to improve it.
>
> diff -Narup linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c linux-3.14-rc5/fs/squashfs/xz_wrapper.c
> --- linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c	2014-03-03 04:56:16.000000000 +0200
> +++ linux-3.14-rc5/fs/squashfs/xz_wrapper.c	2014-03-05 18:08:58.729643127 +0200
> @@ -170,8 +170,13 @@ static int squashfs_xz_uncompress(struct
>
>   	squashfs_finish_page(output);
>
> -	if (xz_err != XZ_STREAM_END || k < b)
> +	if (xz_err != XZ_STREAM_END || k < b) {
> +		if (xz_err == XZ_OPTIONS_ERROR)
> +			ERROR("Unsupported XZ-compressed data; check the XZ "
> +					"options in the kernel config\n");
> +
>   		goto out;
> +	}
>
>   	return total + stream->buf.out_pos;
>
>
>> Moreover, without expert knowledge of Squashfs, and the config
>> options, most people will not have a clue how to fix the issue.
>>
>> This is why I prefer the first option, which is to reinstate
>> the enabling of all filters by default, and then to allow people
>> to remove the filters they don't want.
>
> I will submit the first option. In the other email Florian Fainelli
> seemed to be OK with that too.
>
>> BTW there is a potential additional fix for Squashfs that will
>> make its handling of (lack of) BCJ filters more intelligent
>> at mount time, but this of course only addresses Squashfs,
>> and it relies on an additional call into XZ being added.  The
>> BCJ filters specified at filesystem creation are stored in the
>> compression options part of the superblock, and are known at
>> mount time.  Squashfs should check that these filters are
>> supported by the kernel and refuse to mount it otherwise.  This
>> has not been done because AFAIK there is no way to query XZ to
>> determine which BCJ filters are supported (beyond passing it a
>> test stream which is too messy).
>
> You can use #ifdef CONFIG_XZ_DEC_X86 and such, although maybe that's
> not convenient enough.

Yes, I will try using the XZ config definitions directly in
Squashfs.

Thanks

Phillip