Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
> On Jul 24, 2020, at 7:30 AM, Arvind Sankar wrote: > > On Fri, Jul 24, 2020 at 02:50:34AM +0200, Sedat Dilek wrote: >> On Thu, Jul 23, 2020 at 9:30 PM Nick Terrell wrote: >>> >>> From: Nick Terrell >>> >>> * Add support for zstd compressed kernel >>> * Define __DISABLE_EXPORTS in misc.c >>> * Bump the heap size for zstd. >>> * Update the documentation. >>> >>> Integrates the ZSTD decompression code to the x86 pre-boot code. >>> >>> Zstandard requires slightly more memory during the kernel decompression >>> on x86 (192 KB vs 64 KB), and the memory usage is independent of the >>> window size. >>> >>> __DISABLE_EXPORTS is defined in misc.c instead of the Makefile because >>> kaslr.c defines __DISABLE_EXPORTS, and defining it in the Makefile gives >>> duplicate definition warnings. >>> >> >> That was reported by Arvind - feel free to add a Reported-by: ... >> >> - Sedat - >> > > It's not necessary to add Reported-by's for problems encountered while > developing the series. Especially as it was my drive-by suggestion to > use __DISABLE_EXPORTS that introduced the issue in the first place :) > > I'd have added it to the Makefile and just dropped the definition in > kaslr.c -- should be no reason for anything in here to use EXPORT_SYMBOL. Sure, I was a bit hesitant to modify more than necessary here, but I will go ahead and do that. Thanks for the reviews!
Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
> On Jul 24, 2020, at 5:26 AM, Ingo Molnar wrote: > > > * Nick Terrell wrote: > >> --- a/arch/x86/boot/compressed/misc.c >> +++ b/arch/x86/boot/compressed/misc.c >> @@ -12,6 +12,11 @@ >> * High loaded stuff by Hans Lermen & Werner Almesberger, Feb. 1996 >> */ >> >> +/* decompressors bring in EXPORT_SYMBOL which is meaningless and will >> + * cause compiler errors in some cases. >> + */ >> +#define __DISABLE_EXPORTS >> + >> #include "misc.h" >> #include "error.h" >> #include "pgtable.h" >> @@ -77,6 +82,10 @@ static int lines, cols; >> #ifdef CONFIG_KERNEL_LZ4 >> #include "../../../../lib/decompress_unlz4.c" >> #endif >> + >> +#ifdef CONFIG_KERNEL_ZSTD >> +#include "../../../../lib/decompress_unzstd.c" >> +#endif >> /* >> * NOTE: When adding a new decompressor, please update the analysis in >> * ../header.S. >> diff --git a/arch/x86/include/asm/boot.h b/arch/x86/include/asm/boot.h >> index 680c320363db..d6dd43d25d9f 100644 >> --- a/arch/x86/include/asm/boot.h >> +++ b/arch/x86/include/asm/boot.h >> @@ -24,9 +24,11 @@ >> # error "Invalid value for CONFIG_PHYSICAL_ALIGN" >> #endif >> >> -#ifdef CONFIG_KERNEL_BZIP2 >> +#if defined(CONFIG_KERNEL_BZIP2) >> # define BOOT_HEAP_SIZE 0x40 >> -#else /* !CONFIG_KERNEL_BZIP2 */ >> +#elif defined(CONFIG_KERNEL_ZSTD) >> +# define BOOT_HEAP_SIZE 0x3 >> +#else >> # define BOOT_HEAP_SIZE 0x1 >> #endif > > So the other patches explain why the decompression buffer extra space > was increased from 64k to 128k, but is there a similar > calculation/estimate for bumping BOOT_HEAD_SIZE from 64k to 192k? > > Admittedly the BZ2 exception doesn't set a good example, but maybe we > can do this for ZSTD? Yup, the next version will include an explanation. Thanks for the review!
Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
On Fri, Jul 24, 2020 at 04:48:36PM +0200, Sedat Dilek wrote: > On Fri, Jul 24, 2020 at 4:30 PM Arvind Sankar wrote: > > > > On Fri, Jul 24, 2020 at 02:50:34AM +0200, Sedat Dilek wrote: > > > On Thu, Jul 23, 2020 at 9:30 PM Nick Terrell > > > wrote: > > > > > > > > From: Nick Terrell > > > > > > > > * Define __DISABLE_EXPORTS in misc.c > > > > > > > > __DISABLE_EXPORTS is defined in misc.c instead of the Makefile because > > > > kaslr.c defines __DISABLE_EXPORTS, and defining it in the Makefile gives > > > > duplicate definition warnings. > > > > > > I'd have added it to the Makefile and just dropped the definition in > > kaslr.c -- should be no reason for anything in here to use EXPORT_SYMBOL. > > > > I cannot follow - this is no more needed as this was due to some of > your local changes in kaslr.c? > > - Sedat - That part was meant as a comment on the patch, i.e. avoid the duplicate definition warning not by putting it in misc.c instead of the Makefile, but by dropping the definition in kaslr.c.
Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
On Fri, Jul 24, 2020 at 4:30 PM Arvind Sankar wrote: > > On Fri, Jul 24, 2020 at 02:50:34AM +0200, Sedat Dilek wrote: > > On Thu, Jul 23, 2020 at 9:30 PM Nick Terrell wrote: > > > > > > From: Nick Terrell > > > > > > * Add support for zstd compressed kernel > > > * Define __DISABLE_EXPORTS in misc.c > > > * Bump the heap size for zstd. > > > * Update the documentation. > > > > > > Integrates the ZSTD decompression code to the x86 pre-boot code. > > > > > > Zstandard requires slightly more memory during the kernel decompression > > > on x86 (192 KB vs 64 KB), and the memory usage is independent of the > > > window size. > > > > > > __DISABLE_EXPORTS is defined in misc.c instead of the Makefile because > > > kaslr.c defines __DISABLE_EXPORTS, and defining it in the Makefile gives > > > duplicate definition warnings. > > > > > > > That was reported by Arvind - feel free to add a Reported-by: ... > > > > - Sedat - > > > > It's not necessary to add Reported-by's for problems encountered while > developing the series. Especially as it was my drive-by suggestion to > use __DISABLE_EXPORTS that introduced the issue in the first place :) > It is up to you with credits. > I'd have added it to the Makefile and just dropped the definition in > kaslr.c -- should be no reason for anything in here to use EXPORT_SYMBOL. > I cannot follow - this is no more needed as this was due to some of your local changes in kaslr.c? - Sedat -
Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
On Fri, Jul 24, 2020 at 02:50:34AM +0200, Sedat Dilek wrote: > On Thu, Jul 23, 2020 at 9:30 PM Nick Terrell wrote: > > > > From: Nick Terrell > > > > * Add support for zstd compressed kernel > > * Define __DISABLE_EXPORTS in misc.c > > * Bump the heap size for zstd. > > * Update the documentation. > > > > Integrates the ZSTD decompression code to the x86 pre-boot code. > > > > Zstandard requires slightly more memory during the kernel decompression > > on x86 (192 KB vs 64 KB), and the memory usage is independent of the > > window size. > > > > __DISABLE_EXPORTS is defined in misc.c instead of the Makefile because > > kaslr.c defines __DISABLE_EXPORTS, and defining it in the Makefile gives > > duplicate definition warnings. > > > > That was reported by Arvind - feel free to add a Reported-by: ... > > - Sedat - > It's not necessary to add Reported-by's for problems encountered while developing the series. Especially as it was my drive-by suggestion to use __DISABLE_EXPORTS that introduced the issue in the first place :) I'd have added it to the Makefile and just dropped the definition in kaslr.c -- should be no reason for anything in here to use EXPORT_SYMBOL. Thanks.
Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
On Fri, Jul 24, 2020 at 02:26:40PM +0200, Ingo Molnar wrote: > > -#ifdef CONFIG_KERNEL_BZIP2 > > +#if defined(CONFIG_KERNEL_BZIP2) > > # define BOOT_HEAP_SIZE0x40 > > -#else /* !CONFIG_KERNEL_BZIP2 */ > > +#elif defined(CONFIG_KERNEL_ZSTD) > > +# define BOOT_HEAP_SIZE 0x3 > > +#else > > # define BOOT_HEAP_SIZE 0x1 > > #endif > > So the other patches explain why the decompression buffer extra space > was increased from 64k to 128k, but is there a similar > calculation/estimate for bumping BOOT_HEAD_SIZE from 64k to 192k? > > Admittedly the BZ2 exception doesn't set a good example, but maybe we > can do this for ZSTD? By the way, I have a patchset on top of this, to drop BZ2 and LZMA(1) support, that should clean up this code somewhat. And bring a lot of lines of Linus happiness, as both bzip2 and lzma code are not used by anything else in the kernel, unlike lzma2 (xz). If you draw a speed-vs-size graph, at no point bzip2 or lzma are a good choice, while zstd wins by a large margin for most of the range. Meow! -- ⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ ⢿⡄⠘⠷⠚⠋⠀ It's time to migrate your Imaginary Protocol from version 4i to 6i. ⠈⠳⣄
Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
* Nick Terrell wrote: > --- a/arch/x86/boot/compressed/misc.c > +++ b/arch/x86/boot/compressed/misc.c > @@ -12,6 +12,11 @@ > * High loaded stuff by Hans Lermen & Werner Almesberger, Feb. 1996 > */ > > +/* decompressors bring in EXPORT_SYMBOL which is meaningless and will > + * cause compiler errors in some cases. > + */ > +#define __DISABLE_EXPORTS > + > #include "misc.h" > #include "error.h" > #include "pgtable.h" > @@ -77,6 +82,10 @@ static int lines, cols; > #ifdef CONFIG_KERNEL_LZ4 > #include "../../../../lib/decompress_unlz4.c" > #endif > + > +#ifdef CONFIG_KERNEL_ZSTD > +#include "../../../../lib/decompress_unzstd.c" > +#endif > /* > * NOTE: When adding a new decompressor, please update the analysis in > * ../header.S. > diff --git a/arch/x86/include/asm/boot.h b/arch/x86/include/asm/boot.h > index 680c320363db..d6dd43d25d9f 100644 > --- a/arch/x86/include/asm/boot.h > +++ b/arch/x86/include/asm/boot.h > @@ -24,9 +24,11 @@ > # error "Invalid value for CONFIG_PHYSICAL_ALIGN" > #endif > > -#ifdef CONFIG_KERNEL_BZIP2 > +#if defined(CONFIG_KERNEL_BZIP2) > # define BOOT_HEAP_SIZE 0x40 > -#else /* !CONFIG_KERNEL_BZIP2 */ > +#elif defined(CONFIG_KERNEL_ZSTD) > +# define BOOT_HEAP_SIZE 0x3 > +#else > # define BOOT_HEAP_SIZE 0x1 > #endif So the other patches explain why the decompression buffer extra space was increased from 64k to 128k, but is there a similar calculation/estimate for bumping BOOT_HEAD_SIZE from 64k to 192k? Admittedly the BZ2 exception doesn't set a good example, but maybe we can do this for ZSTD? Thanks, Ingo
Re: [PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
On Thu, Jul 23, 2020 at 9:30 PM Nick Terrell wrote: > > From: Nick Terrell > > * Add support for zstd compressed kernel > * Define __DISABLE_EXPORTS in misc.c > * Bump the heap size for zstd. > * Update the documentation. > > Integrates the ZSTD decompression code to the x86 pre-boot code. > > Zstandard requires slightly more memory during the kernel decompression > on x86 (192 KB vs 64 KB), and the memory usage is independent of the > window size. > > __DISABLE_EXPORTS is defined in misc.c instead of the Makefile because > kaslr.c defines __DISABLE_EXPORTS, and defining it in the Makefile gives > duplicate definition warnings. > That was reported by Arvind - feel free to add a Reported-by: ... - Sedat - > This patch has been boot tested with both a zstd and gzip compressed > kernel on i386 and x86_64 using buildroot and QEMU. > > Additionally, this has been tested in production on x86_64 devices. > We saw a 2 second boot time reduction by switching kernel compression > from xz to zstd. > > Reviewed-by: Kees Cook > Tested-by: Sedat Dilek > Signed-off-by: Nick Terrell > --- > Documentation/x86/boot.rst| 6 +++--- > arch/x86/Kconfig | 1 + > arch/x86/boot/compressed/Makefile | 5 - > arch/x86/boot/compressed/misc.c | 9 + > arch/x86/include/asm/boot.h | 6 -- > 5 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst > index 5325c71ca877..7fafc7ac00d7 100644 > --- a/Documentation/x86/boot.rst > +++ b/Documentation/x86/boot.rst > @@ -782,9 +782,9 @@ Protocol: 2.08+ >uncompressed data should be determined using the standard magic >numbers. The currently supported compression formats are gzip >(magic numbers 1F 8B or 1F 9E), bzip2 (magic number 42 5A), LZMA > - (magic number 5D 00), XZ (magic number FD 37), and LZ4 (magic number > - 02 21). The uncompressed payload is currently always ELF (magic > - number 7F 45 4C 46). > + (magic number 5D 00), XZ (magic number FD 37), LZ4 (magic number > + 02 21) and ZSTD (magic number 28 B5). The uncompressed payload is > + currently always ELF (magic number 7F 45 4C 46). > > == > Field name:payload_length > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 883da0abf779..4a64395bc35d 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -188,6 +188,7 @@ config X86 > select HAVE_KERNEL_LZMA > select HAVE_KERNEL_LZO > select HAVE_KERNEL_XZ > + select HAVE_KERNEL_ZSTD > select HAVE_KPROBES > select HAVE_KPROBES_ON_FTRACE > select HAVE_FUNCTION_ERROR_INJECTION > diff --git a/arch/x86/boot/compressed/Makefile > b/arch/x86/boot/compressed/Makefile > index 7619742f91c9..3498cd990869 100644 > --- a/arch/x86/boot/compressed/Makefile > +++ b/arch/x86/boot/compressed/Makefile > @@ -26,7 +26,7 @@ OBJECT_FILES_NON_STANDARD := y > KCOV_INSTRUMENT:= n > > targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 > vmlinux.bin.lzma \ > - vmlinux.bin.xz vmlinux.bin.lzo vmlinux.bin.lz4 > + vmlinux.bin.xz vmlinux.bin.lzo vmlinux.bin.lz4 vmlinux.bin.zst > > KBUILD_CFLAGS := -m$(BITS) -O2 > KBUILD_CFLAGS += -fno-strict-aliasing $(call cc-option, -fPIE, -fPIC) > @@ -145,6 +145,8 @@ $(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE > $(call if_changed,lzo) > $(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE > $(call if_changed,lz4) > +$(obj)/vmlinux.bin.zst: $(vmlinux.bin.all-y) FORCE > + $(call if_changed,zstd22) > > suffix-$(CONFIG_KERNEL_GZIP) := gz > suffix-$(CONFIG_KERNEL_BZIP2) := bz2 > @@ -152,6 +154,7 @@ suffix-$(CONFIG_KERNEL_LZMA):= lzma > suffix-$(CONFIG_KERNEL_XZ) := xz > suffix-$(CONFIG_KERNEL_LZO):= lzo > suffix-$(CONFIG_KERNEL_LZ4):= lz4 > +suffix-$(CONFIG_KERNEL_ZSTD) := zst > > quiet_cmd_mkpiggy = MKPIGGY $@ >cmd_mkpiggy = $(obj)/mkpiggy $< > $@ > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > index 9652d5c2afda..885dc20680c2 100644 > --- a/arch/x86/boot/compressed/misc.c > +++ b/arch/x86/boot/compressed/misc.c > @@ -12,6 +12,11 @@ > * High loaded stuff by Hans Lermen & Werner Almesberger, Feb. 1996 > */ > > +/* decompressors bring in EXPORT_SYMBOL which is meaningless and will > + * cause compiler errors in some cases. > + */ > +#define __DISABLE_EXPORTS > + > #include "misc.h" > #include "error.h" > #include "pgtable.h" > @@ -77,6 +82,10 @@ static int lines, cols; > #ifdef CONFIG_KERNEL_LZ4 > #include "../../../../lib/decompress_unlz4.c" > #endif > + > +#ifdef CONFIG_KERNEL_ZSTD > +#include "../../../../lib/decompress_unzstd.c" > +#endif > /* > * NOTE: When adding a new decompressor, please update the analysis in > * ../header.S. > diff --git a/arch/x86/include/asm/boot.h b/arch/x86/include/asm/boot.h > index 680c320363db..d6dd43d25d9f 100644 > --- a/arch/x86/include/asm/boot.h >
[PATCH v8 6/7] x86: Add support for ZSTD compressed kernel
From: Nick Terrell
* Add support for zstd compressed kernel
* Define __DISABLE_EXPORTS in misc.c
* Bump the heap size for zstd.
* Update the documentation.
Integrates the ZSTD decompression code to the x86 pre-boot code.
Zstandard requires slightly more memory during the kernel decompression
on x86 (192 KB vs 64 KB), and the memory usage is independent of the
window size.
__DISABLE_EXPORTS is defined in misc.c instead of the Makefile because
kaslr.c defines __DISABLE_EXPORTS, and defining it in the Makefile gives
duplicate definition warnings.
This patch has been boot tested with both a zstd and gzip compressed
kernel on i386 and x86_64 using buildroot and QEMU.
Additionally, this has been tested in production on x86_64 devices.
We saw a 2 second boot time reduction by switching kernel compression
from xz to zstd.
Reviewed-by: Kees Cook
Tested-by: Sedat Dilek
Signed-off-by: Nick Terrell
---
Documentation/x86/boot.rst| 6 +++---
arch/x86/Kconfig | 1 +
arch/x86/boot/compressed/Makefile | 5 -
arch/x86/boot/compressed/misc.c | 9 +
arch/x86/include/asm/boot.h | 6 --
5 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index 5325c71ca877..7fafc7ac00d7 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -782,9 +782,9 @@ Protocol: 2.08+
uncompressed data should be determined using the standard magic
numbers. The currently supported compression formats are gzip
(magic numbers 1F 8B or 1F 9E), bzip2 (magic number 42 5A), LZMA
- (magic number 5D 00), XZ (magic number FD 37), and LZ4 (magic number
- 02 21). The uncompressed payload is currently always ELF (magic
- number 7F 45 4C 46).
+ (magic number 5D 00), XZ (magic number FD 37), LZ4 (magic number
+ 02 21) and ZSTD (magic number 28 B5). The uncompressed payload is
+ currently always ELF (magic number 7F 45 4C 46).
==
Field name:payload_length
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 883da0abf779..4a64395bc35d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -188,6 +188,7 @@ config X86
select HAVE_KERNEL_LZMA
select HAVE_KERNEL_LZO
select HAVE_KERNEL_XZ
+ select HAVE_KERNEL_ZSTD
select HAVE_KPROBES
select HAVE_KPROBES_ON_FTRACE
select HAVE_FUNCTION_ERROR_INJECTION
diff --git a/arch/x86/boot/compressed/Makefile
b/arch/x86/boot/compressed/Makefile
index 7619742f91c9..3498cd990869 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -26,7 +26,7 @@ OBJECT_FILES_NON_STANDARD := y
KCOV_INSTRUMENT:= n
targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma
\
- vmlinux.bin.xz vmlinux.bin.lzo vmlinux.bin.lz4
+ vmlinux.bin.xz vmlinux.bin.lzo vmlinux.bin.lz4 vmlinux.bin.zst
KBUILD_CFLAGS := -m$(BITS) -O2
KBUILD_CFLAGS += -fno-strict-aliasing $(call cc-option, -fPIE, -fPIC)
@@ -145,6 +145,8 @@ $(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
$(call if_changed,lzo)
$(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE
$(call if_changed,lz4)
+$(obj)/vmlinux.bin.zst: $(vmlinux.bin.all-y) FORCE
+ $(call if_changed,zstd22)
suffix-$(CONFIG_KERNEL_GZIP) := gz
suffix-$(CONFIG_KERNEL_BZIP2) := bz2
@@ -152,6 +154,7 @@ suffix-$(CONFIG_KERNEL_LZMA):= lzma
suffix-$(CONFIG_KERNEL_XZ) := xz
suffix-$(CONFIG_KERNEL_LZO):= lzo
suffix-$(CONFIG_KERNEL_LZ4):= lz4
+suffix-$(CONFIG_KERNEL_ZSTD) := zst
quiet_cmd_mkpiggy = MKPIGGY $@
cmd_mkpiggy = $(obj)/mkpiggy $< > $@
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 9652d5c2afda..885dc20680c2 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -12,6 +12,11 @@
* High loaded stuff by Hans Lermen & Werner Almesberger, Feb. 1996
*/
+/* decompressors bring in EXPORT_SYMBOL which is meaningless and will
+ * cause compiler errors in some cases.
+ */
+#define __DISABLE_EXPORTS
+
#include "misc.h"
#include "error.h"
#include "pgtable.h"
@@ -77,6 +82,10 @@ static int lines, cols;
#ifdef CONFIG_KERNEL_LZ4
#include "../../../../lib/decompress_unlz4.c"
#endif
+
+#ifdef CONFIG_KERNEL_ZSTD
+#include "../../../../lib/decompress_unzstd.c"
+#endif
/*
* NOTE: When adding a new decompressor, please update the analysis in
* ../header.S.
diff --git a/arch/x86/include/asm/boot.h b/arch/x86/include/asm/boot.h
index 680c320363db..d6dd43d25d9f 100644
--- a/arch/x86/include/asm/boot.h
+++ b/arch/x86/include/asm/boot.h
@@ -24,9 +24,11 @@
# error "Invalid value for CONFIG_PHYSICAL_ALIGN"
#endif
-#ifdef CONFIG_KERNEL_BZIP2
+#if defined(CONFIG_KERNEL_BZIP2)
# define BOOT_HEAP_SIZE0x40
-#else /* !CONFIG_KERNEL_BZIP2 */
+#elif defined(CONFIG_KERNEL_ZSTD)
+# define BOOT_HEAP_SIZE 0x3
+#else
# define
