Re: [PATCH v1 1/3] riscv: make image compression configurable

[Masahiro Yamada]

On Thu, May 2, 2024 at 10:05 PM Björn Töpel <bjorn@kernelorg> wrote:
>
> Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx> writes:
>
> > Previously the build process would always set KBUILD_IMAGE to the
> > uncompressed Image file (unless XIP_KERNEL or EFI_ZBOOT was enabled) and
> > unconditionally compress it into Image.gz. However there are already
> > build targets for Image.bz2, Image.lz4, Image.lzma, Image.lzo and
> > Image.zstd, so let's make use of those, make the compression method
> > configurable and set KBUILD_IMAGE accordingly so that targets like
> > 'make install' and 'make bindeb-pkg' will use the chosen image.
> >
> > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx>
> > ---
> >  arch/riscv/Kconfig         |  7 +++++++
> >  arch/riscv/Makefile        | 43 ++++++++++++++++++++------------------
> >  arch/riscv/boot/install.sh |  9 +++++---
> >  3 files changed, 36 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index be09c8836d56..6c092d1ea7db 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -138,6 +138,13 @@ config RISCV
> >       select HAVE_GCC_PLUGINS
> >       select HAVE_GENERIC_VDSO if MMU && 64BIT
> >       select HAVE_IRQ_TIME_ACCOUNTING
> > +     select HAVE_KERNEL_BZIP2 if !XIP_KERNEL && !EFI_ZBOOT
> > +     select HAVE_KERNEL_GZIP if !XIP_KERNEL && !EFI_ZBOOT
> > +     select HAVE_KERNEL_LZ4 if !XIP_KERNEL && !EFI_ZBOOT
> > +     select HAVE_KERNEL_LZMA if !XIP_KERNEL && !EFI_ZBOOT
> > +     select HAVE_KERNEL_LZO if !XIP_KERNEL && !EFI_ZBOOT
> > +     select HAVE_KERNEL_UNCOMPRESSED if !XIP_KERNEL && !EFI_ZBOOT
> > +     select HAVE_KERNEL_ZSTD if !XIP_KERNEL && !EFI_ZBOOT
> >       select HAVE_KPROBES if !XIP_KERNEL
> >       select HAVE_KPROBES_ON_FTRACE if !XIP_KERNEL
> >       select HAVE_KRETPROBES if !XIP_KERNEL
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index 5b3115a19852..29be676415d6 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -129,11 +129,27 @@ endif
> >  CHECKFLAGS += -D__riscv -D__riscv_xlen=$(BITS)
> >
> >  # Default target when executing plain make
> > -boot         := arch/riscv/boot
> > +boot := arch/riscv/boot
> >  ifeq ($(CONFIG_XIP_KERNEL),y)
> >  KBUILD_IMAGE := $(boot)/xipImage
> > +else ifeq ($(CONFIG_RISCV_M_MODE)$(CONFIG_ARCH_CANAAN),yy)
> > +KBUILD_IMAGE := $(boot)/loader.bin
> > +else ifeq ($(CONFIG_EFI_ZBOOT),y)
> > +KBUILD_IMAGE := $(boot)/vmlinuz.efi
> > +else ifeq ($(CONFIG_KERNEL_GZIP),y)
> > +KBUILD_IMAGE := $(boot)/Image.gz
> > +else ifeq ($(CONFIG_KERNEL_BZIP2),y)
> > +KBUILD_IMAGE := $(boot)/Image.bz2
> > +else ifeq ($(CONFIG_KERNEL_LZ4),y)
> > +KBUILD_IMAGE := $(boot)/Image.lz4
> > +else ifeq ($(CONFIG_KERNEL_LZMA),y)
> > +KBUILD_IMAGE := $(boot)/Image.lzma
> > +else ifeq ($(CONFIG_KERNEL_LZO),y)
> > +KBUILD_IMAGE := $(boot)/Image.lzo
> > +else ifeq ($(CONFIG_KERNEL_ZSTD),y)
> > +KBUILD_IMAGE := $(boot)/Image.zst
> >  else
> > -KBUILD_IMAGE := $(boot)/Image.gz
> > +KBUILD_IMAGE := $(boot)/Image
> >  endif
>
> Really a nit/change if you want, but maybe doing something like
> arch/s390/boot/Makefile does is easier to read:
>
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 024482c68835..70f08e9999b4 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -128,6 +128,14 @@ endif
>  # arch specific predefines for sparse
>  CHECKFLAGS += -D__riscv -D__riscv_xlen=$(BITS)
>
> +suffix-$(CONFIG_KERNEL_GZIP)  := .gz
> +suffix-$(CONFIG_KERNEL_BZIP2) := .bz2
> +suffix-$(CONFIG_KERNEL_LZ4)  := .lz4
> +suffix-$(CONFIG_KERNEL_LZMA)  := .lzma
> +suffix-$(CONFIG_KERNEL_LZO)  := .lzo
> +suffix-$(CONFIG_KERNEL_XZ)  := .xz
> +suffix-$(CONFIG_KERNEL_ZSTD)  := .zst
> +
>  # Default target when executing plain make
>  boot := arch/riscv/boot
>  ifeq ($(CONFIG_XIP_KERNEL),y)
> @@ -136,20 +144,8 @@ else ifeq ($(CONFIG_RISCV_M_MODE)$(CONFIG_ARCH_CANAAN),yy)
>  KBUILD_IMAGE := $(boot)/loader.bin
>  else ifeq ($(CONFIG_EFI_ZBOOT),y)
>  KBUILD_IMAGE := $(boot)/vmlinuz.efi
> -else ifeq ($(CONFIG_KERNEL_GZIP),y)
> -KBUILD_IMAGE := $(boot)/Image.gz
> -else ifeq ($(CONFIG_KERNEL_BZIP2),y)
> -KBUILD_IMAGE := $(boot)/Image.bz2
> -else ifeq ($(CONFIG_KERNEL_LZ4),y)
> -KBUILD_IMAGE := $(boot)/Image.lz4
> -else ifeq ($(CONFIG_KERNEL_LZMA),y)
> -KBUILD_IMAGE := $(boot)/Image.lzma
> -else ifeq ($(CONFIG_KERNEL_LZO),y)
> -KBUILD_IMAGE := $(boot)/Image.lzo
> -else ifeq ($(CONFIG_KERNEL_ZSTD),y)
> -KBUILD_IMAGE := $(boot)/Image.zst
>  else
> -KBUILD_IMAGE := $(boot)/Image
> +KBUILD_IMAGE := $(boot)/Image$(suffix-y)
>  endif




Good idea.


If you avoid the 'else ifeq' chain completely,
you also could do like this:



boot-image-$(CONFIG_KERNEL_GZIP)         := Image.gz
   ...
boot-image-$(CONFIG_KERNEL_ZSTD)         := Image.zst
boot-image-$(CONFIG_KERNEL_UNCOMPRESSED) := Image
boot-image-$(CONFIG_RISCV_M_MODE)        := loader.bin
boot-image-$(CONFIG_ARCH_CANAAN)         := loader.bin
boot-image-$(CONFIG_EFI_ZBOOT)           := vmlinuz.efi
boot-image-$(CONFIG_XIP_KERNEL)          := xipImage

KBUILD_IMAGE := $(boot)/$(boot-image-y)



Emil's current patch will work, of course.




BTW, this patch will conflict with
3b938e231b660a278de2988ee77b832d665c5326
It lands in riscv subsystem.


-- 
Best Regards
Masahiro Yamada