Message ID | 20200723192801.351114-7-nickrterrell@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add support for ZSTD-compressed kernel and initramfs | expand |
On Thu, Jul 23, 2020 at 9:30 PM Nick Terrell <nickrterrell@gmail.com> wrote: > > From: Nick Terrell <terrelln@fb.com> > > * 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 <keescook@chromium.org> > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> > Signed-off-by: Nick Terrell <terrelln@fb.com> > --- > 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_SIZE 0x400000 > -#else /* !CONFIG_KERNEL_BZIP2 */ > +#elif defined(CONFIG_KERNEL_ZSTD) > +# define BOOT_HEAP_SIZE 0x30000 > +#else > # define BOOT_HEAP_SIZE 0x10000 > #endif > > -- > 2.27.0 >
* Nick Terrell <nickrterrell@gmail.com> 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 0x400000 > -#else /* !CONFIG_KERNEL_BZIP2 */ > +#elif defined(CONFIG_KERNEL_ZSTD) > +# define BOOT_HEAP_SIZE 0x30000 > +#else > # define BOOT_HEAP_SIZE 0x10000 > #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
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_SIZE 0x400000 > > -#else /* !CONFIG_KERNEL_BZIP2 */ > > +#elif defined(CONFIG_KERNEL_ZSTD) > > +# define BOOT_HEAP_SIZE 0x30000 > > +#else > > # define BOOT_HEAP_SIZE 0x10000 > > #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!
On Fri, Jul 24, 2020 at 02:50:34AM +0200, Sedat Dilek wrote: > On Thu, Jul 23, 2020 at 9:30 PM Nick Terrell <nickrterrell@gmail.com> wrote: > > > > From: Nick Terrell <terrelln@fb.com> > > > > * 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.
On Fri, Jul 24, 2020 at 4:30 PM Arvind Sankar <nivedita@alum.mit.edu> 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 <nickrterrell@gmail.com> wrote: > > > > > > From: Nick Terrell <terrelln@fb.com> > > > > > > * 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 -
On Fri, Jul 24, 2020 at 04:48:36PM +0200, Sedat Dilek wrote: > On Fri, Jul 24, 2020 at 4:30 PM Arvind Sankar <nivedita@alum.mit.edu> 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 <nickrterrell@gmail.com> wrote: > > > > > > > > From: Nick Terrell <terrelln@fb.com> > > > > > > > > * 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.
> On Jul 24, 2020, at 5:26 AM, Ingo Molnar <mingo@kernel.org> wrote: > > > * Nick Terrell <nickrterrell@gmail.com> 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 0x400000 >> -#else /* !CONFIG_KERNEL_BZIP2 */ >> +#elif defined(CONFIG_KERNEL_ZSTD) >> +# define BOOT_HEAP_SIZE 0x30000 >> +#else >> # define BOOT_HEAP_SIZE 0x10000 >> #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!
> On Jul 24, 2020, at 7:30 AM, Arvind Sankar <nivedita@alum.mit.edu> 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 <nickrterrell@gmail.com> wrote: >>> >>> From: Nick Terrell <terrelln@fb.com> >>> >>> * 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!
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_SIZE 0x400000 -#else /* !CONFIG_KERNEL_BZIP2 */ +#elif defined(CONFIG_KERNEL_ZSTD) +# define BOOT_HEAP_SIZE 0x30000 +#else # define BOOT_HEAP_SIZE 0x10000 #endif