Received-SPF: pass (google.com: domain of linux-kernel+bounces-168695-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
In-Reply-To: <CAK7LNASqwN1gd8TovcR3RDkxkE_M=BSDD5GoOoLOkDQnuNDAcA@mail.gmail.com>
References: <20240502111613.1380453-1-emil.renner.berthing@canonical.com>
 <20240502111613.1380453-2-emil.renner.berthing@canonical.com>
 <87ttjgcqjv.fsf@all.your.base.are.belong.to.us> <CAK7LNASqwN1gd8TovcR3RDkxkE_M=BSDD5GoOoLOkDQnuNDAcA@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
Date: Sat, 4 May 2024 13:54:37 +0000
Message-ID: <CAJM55Z-dorP1MgiCu=+VV_D+b6XXKWacUjT04QAsKvdmQtNv3g@mail.gmail.com>
Subject: Re: [PATCH v1 1/3] riscv: make image compression configurable
To: Masahiro Yamada <masahiroy@kernel.org>, =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
Cc: Emil Renner Berthing <emil.renner.berthing@canonical.com>, linux-riscv@lists.infradead.org, 
	linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org, 
	Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Nathan Chancellor <nathan@kernel.org>, Nicolas Schier <nicolas@fjasle.eu>, Nick Terrell <terrelln@fb.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Masahiro Yamada wrote:
> On Thu, May 2, 2024 at 10:05=E2=80=AFPM Bj=C3=B6rn T=C3=B6pel <bjorn@kern=
el.org> wrote:
> >
> > Emil Renner Berthing <emil.renner.berthing@canonical.com> 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@canonical.c=
om>
> > > ---
> > >  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 +=3D -D__riscv -D__riscv_xlen=3D$(BITS)
> > >
> > >  # Default target when executing plain make
> > > -boot         :=3D arch/riscv/boot
> > > +boot :=3D arch/riscv/boot
> > >  ifeq ($(CONFIG_XIP_KERNEL),y)
> > >  KBUILD_IMAGE :=3D $(boot)/xipImage
> > > +else ifeq ($(CONFIG_RISCV_M_MODE)$(CONFIG_ARCH_CANAAN),yy)
> > > +KBUILD_IMAGE :=3D $(boot)/loader.bin
> > > +else ifeq ($(CONFIG_EFI_ZBOOT),y)
> > > +KBUILD_IMAGE :=3D $(boot)/vmlinuz.efi
> > > +else ifeq ($(CONFIG_KERNEL_GZIP),y)
> > > +KBUILD_IMAGE :=3D $(boot)/Image.gz
> > > +else ifeq ($(CONFIG_KERNEL_BZIP2),y)
> > > +KBUILD_IMAGE :=3D $(boot)/Image.bz2
> > > +else ifeq ($(CONFIG_KERNEL_LZ4),y)
> > > +KBUILD_IMAGE :=3D $(boot)/Image.lz4
> > > +else ifeq ($(CONFIG_KERNEL_LZMA),y)
> > > +KBUILD_IMAGE :=3D $(boot)/Image.lzma
> > > +else ifeq ($(CONFIG_KERNEL_LZO),y)
> > > +KBUILD_IMAGE :=3D $(boot)/Image.lzo
> > > +else ifeq ($(CONFIG_KERNEL_ZSTD),y)
> > > +KBUILD_IMAGE :=3D $(boot)/Image.zst
> > >  else
> > > -KBUILD_IMAGE :=3D $(boot)/Image.gz
> > > +KBUILD_IMAGE :=3D $(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 +=3D -D__riscv -D__riscv_xlen=3D$(BITS)
> >
> > +suffix-$(CONFIG_KERNEL_GZIP)  :=3D .gz
> > +suffix-$(CONFIG_KERNEL_BZIP2) :=3D .bz2
> > +suffix-$(CONFIG_KERNEL_LZ4)  :=3D .lz4
> > +suffix-$(CONFIG_KERNEL_LZMA)  :=3D .lzma
> > +suffix-$(CONFIG_KERNEL_LZO)  :=3D .lzo
> > +suffix-$(CONFIG_KERNEL_XZ)  :=3D .xz
> > +suffix-$(CONFIG_KERNEL_ZSTD)  :=3D .zst
> > +
> >  # Default target when executing plain make
> >  boot :=3D arch/riscv/boot
> >  ifeq ($(CONFIG_XIP_KERNEL),y)
> > @@ -136,20 +144,8 @@ else ifeq ($(CONFIG_RISCV_M_MODE)$(CONFIG_ARCH_CAN=
AAN),yy)
> >  KBUILD_IMAGE :=3D $(boot)/loader.bin
> >  else ifeq ($(CONFIG_EFI_ZBOOT),y)
> >  KBUILD_IMAGE :=3D $(boot)/vmlinuz.efi
> > -else ifeq ($(CONFIG_KERNEL_GZIP),y)
> > -KBUILD_IMAGE :=3D $(boot)/Image.gz
> > -else ifeq ($(CONFIG_KERNEL_BZIP2),y)
> > -KBUILD_IMAGE :=3D $(boot)/Image.bz2
> > -else ifeq ($(CONFIG_KERNEL_LZ4),y)
> > -KBUILD_IMAGE :=3D $(boot)/Image.lz4
> > -else ifeq ($(CONFIG_KERNEL_LZMA),y)
> > -KBUILD_IMAGE :=3D $(boot)/Image.lzma
> > -else ifeq ($(CONFIG_KERNEL_LZO),y)
> > -KBUILD_IMAGE :=3D $(boot)/Image.lzo
> > -else ifeq ($(CONFIG_KERNEL_ZSTD),y)
> > -KBUILD_IMAGE :=3D $(boot)/Image.zst
> >  else
> > -KBUILD_IMAGE :=3D $(boot)/Image
> > +KBUILD_IMAGE :=3D $(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)         :=3D Image.gz
>    ...
> boot-image-$(CONFIG_KERNEL_ZSTD)         :=3D Image.zst
> boot-image-$(CONFIG_KERNEL_UNCOMPRESSED) :=3D Image
> boot-image-$(CONFIG_RISCV_M_MODE)        :=3D loader.bin
> boot-image-$(CONFIG_ARCH_CANAAN)         :=3D loader.bin
> boot-image-$(CONFIG_EFI_ZBOOT)           :=3D vmlinuz.efi
> boot-image-$(CONFIG_XIP_KERNEL)          :=3D xipImage
>
> KBUILD_IMAGE :=3D $(boot)/$(boot-image-y)

Hi Masahiro and Bj=C3=B6rn.

I like this approach. But I think it doesn't quite do the same when fx.
CONFIG_RISCV_M_MODE=3Dn but CONFIG_ARCH_CANAAN=3Dy which I think is a valid
configuration for the new Kendryte K230 support. In this case boot-image-y
would be overwritten with the loader.bin value even for S-mode kernels.

To keep the previous behaviour we'd need something like

  boot-image-$(CONFIG_RISCV_M_MODE *and* CONFIG_ARCH_CANAAN) :=3D loader.bi=
n

Maybe just

  ifeq ($(CONFIG_RISCV_M_MODE),y)
  boot-image-$(CONFIG_ARCH_CANAAN) :=3D loader.bin
  endif

Do you guys have a better solution?

/Emil

>
>
>
> Emil's current patch will work, of course.
>
>
>
>
> BTW, this patch will conflict with
> 3b938e231b660a278de2988ee77b832d665c5326
> It lands in riscv subsystem.

Oh, you're right. I'll rebase on that for v2, thanks.

/Emil