[PATCH 0/2] Enable compressed files in EXTRA_FIRMWARE

[PATCH 0/2] Enable compressed files in EXTRA_FIRMWARE

[Kevin Martin]

The linux-firmware packages on Gentoo, Fedora, Arch, and others compress
the firmware files. This works well with CONFIG_FW_LOADER_COMPRESS, but
does not work with CONFIG_EXTRA_FIRMWARE. This patch allows the build
system to decompress firmware files specified by CONFIG_EXTRA_FIRMWARE.

PATCH 1/2 adds decompression routines next to the compression routines
in scripts/Makefile.lib. That patch is then used by PATCH 2/2 to
decompress files before compiling them into the kernel.

The patch works by copying or decompressing the specified firmware files
into the build directory, then compiling them in from there. I would
prefer to not copy any uncompressed files, but I have not found a clean
way to do that.

Kevin Martin (2):
  kbuild: Enable decompression for use by EXTRA_FIRMWARE The build
    system can currently only compress files. This patch adds the
    functionality to decompress files. Decompression is needed for
    building firmware files into the kernel if those files are
    compressed on the filesystem. Compressed firmware files are in use
    by Gentoo, Fedora, Arch, and others.
  firmware_loader: Enable compressed files with EXTRA_FIRMWARE The
    linux-firmware packages on Gentoo, Fedora, Arch, and others compress
    the firmware files. This works well with CONFIG_FW_LOADER_COMPRESS
    but does not work with CONFIG_EXTRA_FIRMWARE. This patch allows the
    build system to decompress firmware files specified by
    CONFIG_EXTRA_FIRMWARE. Uncompressed files are used first, then the
    compressed files are used.

 drivers/base/firmware_loader/Kconfig          |  5 ++++-
 drivers/base/firmware_loader/builtin/Makefile | 16 ++++++++++++----
 scripts/Makefile.lib                          |  6 ++++++
 3 files changed, 22 insertions(+), 5 deletions(-)

-- 
2.41.0

[PATCH 1/2] kbuild: Enable decompression for use by EXTRA_FIRMWARE The build system can currently only compress files. This patch adds the functionality to decompress files. Decompression is needed for building firmware files into the kernel if those files are compressed on the filesystem. Compressed firmware files are in use by Gentoo, Fedora, Arch, and others.

[Kevin Martin]

Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
---
 scripts/Makefile.lib | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 1a965fe68..d043be3dc 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -523,6 +523,9 @@ quiet_cmd_xzkern_with_size = XZKERN  $@
 quiet_cmd_xzmisc = XZMISC  $@
       cmd_xzmisc = cat $(real-prereqs) | $(XZ) --check=crc32 --lzma2=dict=1MiB > $@
 
+quiet_cmd_xzdec = XZDEC   $@
+      cmd_xzdec = cat $(real-prereqs) | $(XZ) --decompress > $@
+
 # ZSTD
 # ---------------------------------------------------------------------------
 # Appends the uncompressed size of the data using size_append. The .zst
@@ -548,6 +551,9 @@ quiet_cmd_zstd22 = ZSTD22  $@
 quiet_cmd_zstd22_with_size = ZSTD22  $@
       cmd_zstd22_with_size = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
 
+quiet_cmd_zstddec = ZSTDDEC $@
+      cmd_zstddec = cat $(real-prereqs) | $(ZSTD) --decompress > $@
+
 # ASM offsets
 # ---------------------------------------------------------------------------
 
-- 
2.41.0

Re: [PATCH 1/2] kbuild: Enable decompression for use by EXTRA_FIRMWARE The build system can currently only compress files. This patch adds the functionality to decompress files. Decompression is needed for building firmware files into the kernel if those files are compressed on the filesystem. Compressed firmware files are in use by Gentoo, Fedora, Arch, and others.

[Nicolas Schier]

Hi Kevin,

> Subject: Re: [PATCH 1/2] kbuild: Enable decompression for use by
>  EXTRA_FIRMWARE The build system can currently only compress files. This
>  patch adds the functionality to decompress files. Decompression is needed
>  for building firmware files into the kernel if those files are compressed on
>  the filesystem. Compressed firmware files are in use by Gentoo, Fedora,
>  Arch, and others.

patch description is squashed into the subject.  Did your tooling
accidentially remove the empty line between?

The patch itself looks good to me.

Tested-by: Nicolas Schier <n.schier@avm.de>

Kind regards,
Nicolas

On Wed, Dec 20, 2023 at 05:22:50AM -0500, Kevin Martin wrote:
> Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
> ---
>  scripts/Makefile.lib | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 1a965fe68..d043be3dc 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -523,6 +523,9 @@ quiet_cmd_xzkern_with_size = XZKERN  $@
>  quiet_cmd_xzmisc = XZMISC  $@
>        cmd_xzmisc = cat $(real-prereqs) | $(XZ) --check=crc32 --lzma2=dict=1MiB > $@
>  
> +quiet_cmd_xzdec = XZDEC   $@
> +      cmd_xzdec = cat $(real-prereqs) | $(XZ) --decompress > $@
> +
>  # ZSTD
>  # ---------------------------------------------------------------------------
>  # Appends the uncompressed size of the data using size_append. The .zst
> @@ -548,6 +551,9 @@ quiet_cmd_zstd22 = ZSTD22  $@
>  quiet_cmd_zstd22_with_size = ZSTD22  $@
>        cmd_zstd22_with_size = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
>  
> +quiet_cmd_zstddec = ZSTDDEC $@
> +      cmd_zstddec = cat $(real-prereqs) | $(ZSTD) --decompress > $@
> +
>  # ASM offsets
>  # ---------------------------------------------------------------------------
>  
> -- 
> 2.41.0
> 

Re: [PATCH 1/2] kbuild: Enable decompression for use by EXTRA_FIRMWARE The build system can currently only compress files. This patch adds the functionality to decompress files. Decompression is needed for building firmware files into the kernel if those files are compressed on the filesystem. Compressed firmware files are in use by Gentoo, Fedora, Arch, and others.

[Masahiro Yamada]

On Wed, Dec 20, 2023 at 7:26 PM Kevin Martin <kevinmbecause@gmail.com> wrote:
>
> Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
> ---
>  scripts/Makefile.lib | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 1a965fe68..d043be3dc 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -523,6 +523,9 @@ quiet_cmd_xzkern_with_size = XZKERN  $@
>  quiet_cmd_xzmisc = XZMISC  $@
>        cmd_xzmisc = cat $(real-prereqs) | $(XZ) --check=crc32 --lzma2=dict=1MiB > $@
>
> +quiet_cmd_xzdec = XZDEC   $@
> +      cmd_xzdec = cat $(real-prereqs) | $(XZ) --decompress > $@
> +



Please do not fork the meaningless 'cat' process.

This should be a single process to take just one input file.

    cmd_xzdec = $(XZ) --decompress --stdout $< > $@




Commit d3dd3b5a29bb9582957451531fed461628dfc834
was a very bad commit.

The 'cat' and compression/decompression must be
separate rules.

We should not repeat the mistake in the past.



>  # ZSTD
>  # ---------------------------------------------------------------------------
>  # Appends the uncompressed size of the data using size_append. The .zst
> @@ -548,6 +551,9 @@ quiet_cmd_zstd22 = ZSTD22  $@
>  quiet_cmd_zstd22_with_size = ZSTD22  $@
>        cmd_zstd22_with_size = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
>
> +quiet_cmd_zstddec = ZSTDDEC $@
> +      cmd_zstddec = cat $(real-prereqs) | $(ZSTD) --decompress > $@
> +


Same here.
Please make this a single process:

   cmd_zstddec = $(ZSTD) --decompress --force --output=$@ $<






One small concern in the future is, if we end up with adding
quiet_cmd_bzip2dec, we will run out of the 7-column of the short log.

 quiet_cmd_bzip2dec = BZIP2DEC$@

We can increase the column size if needed, so I do not think
it is a big issue.










>  # ASM offsets
>  # ---------------------------------------------------------------------------
>
> --
> 2.41.0
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] kbuild: Enable decompression for use by EXTRA_FIRMWARE The build system can currently only compress files. This patch adds the functionality to decompress files. Decompression is needed for building firmware files into the kernel if those files are compressed on the filesystem. Compressed firmware files are in use by Gentoo, Fedora, Arch, and others.

[Kevin Martin]

[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]


On Wed, 3 Jan 2024, Masahiro Yamada wrote:

> On Wed, Dec 20, 2023 at 7:26 PM Kevin Martin <kevinmbecause@gmail.com> wrote:
> >
> > Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
> > ---
> >  scripts/Makefile.lib | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 1a965fe68..d043be3dc 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -523,6 +523,9 @@ quiet_cmd_xzkern_with_size = XZKERN  $@
> >  quiet_cmd_xzmisc = XZMISC  $@
> >        cmd_xzmisc = cat $(real-prereqs) | $(XZ) --check=crc32 --lzma2=dict=1MiB > $@
> >
> > +quiet_cmd_xzdec = XZDEC   $@
> > +      cmd_xzdec = cat $(real-prereqs) | $(XZ) --decompress > $@
> > +
> 
> 
> 
> Please do not fork the meaningless 'cat' process.
> 
> This should be a single process to take just one input file.
> 
>     cmd_xzdec = $(XZ) --decompress --stdout $< > $@
> 
> 
> 
> 
> Commit d3dd3b5a29bb9582957451531fed461628dfc834
> was a very bad commit.
> 
> The 'cat' and compression/decompression must be
> separate rules.
> 
> We should not repeat the mistake in the past.
> 

Would it be preferable to change all of the compression rules or just the 
new decompression rules?

I could change just the new ones and then begin working on a different 
patch to clean up the 'cat' processes in the compression rules.

> 
> 
> >  # ZSTD
> >  # ---------------------------------------------------------------------------
> >  # Appends the uncompressed size of the data using size_append. The .zst
> > @@ -548,6 +551,9 @@ quiet_cmd_zstd22 = ZSTD22  $@
> >  quiet_cmd_zstd22_with_size = ZSTD22  $@
> >        cmd_zstd22_with_size = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
> >
> > +quiet_cmd_zstddec = ZSTDDEC $@
> > +      cmd_zstddec = cat $(real-prereqs) | $(ZSTD) --decompress > $@
> > +
> 
> 
> Same here.
> Please make this a single process:
> 
>    cmd_zstddec = $(ZSTD) --decompress --force --output=$@ $<
> 
> 
> 
> 
> 
> 
> One small concern in the future is, if we end up with adding
> quiet_cmd_bzip2dec, we will run out of the 7-column of the short log.
> 
>  quiet_cmd_bzip2dec = BZIP2DEC$@
> 
> We can increase the column size if needed, so I do not think
> it is a big issue.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> >  # ASM offsets
> >  # ---------------------------------------------------------------------------
> >
> > --
> > 2.41.0
> >
> 
> 
> -- 
> Best Regards
> Masahiro Yamada
> 

Re: [PATCH 1/2] kbuild: Enable decompression for use by EXTRA_FIRMWARE The build system can currently only compress files. This patch adds the functionality to decompress files. Decompression is needed for building firmware files into the kernel if those files are compressed on the filesystem. Compressed firmware files are in use by Gentoo, Fedora, Arch, and others.

[Masahiro Yamada]

On Thu, Jan 4, 2024 at 4:04 PM Kevin Martin <kevinmbecause@gmail.com> wrote:
>
>
> On Wed, 3 Jan 2024, Masahiro Yamada wrote:
>
> > On Wed, Dec 20, 2023 at 7:26 PM Kevin Martin <kevinmbecause@gmail.com> wrote:
> > >
> > > Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
> > > ---
> > >  scripts/Makefile.lib | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index 1a965fe68..d043be3dc 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -523,6 +523,9 @@ quiet_cmd_xzkern_with_size = XZKERN  $@
> > >  quiet_cmd_xzmisc = XZMISC  $@
> > >        cmd_xzmisc = cat $(real-prereqs) | $(XZ) --check=crc32 --lzma2=dict=1MiB > $@
> > >
> > > +quiet_cmd_xzdec = XZDEC   $@
> > > +      cmd_xzdec = cat $(real-prereqs) | $(XZ) --decompress > $@
> > > +
> >
> >
> >
> > Please do not fork the meaningless 'cat' process.
> >
> > This should be a single process to take just one input file.
> >
> >     cmd_xzdec = $(XZ) --decompress --stdout $< > $@
> >
> >
> >
> >
> > Commit d3dd3b5a29bb9582957451531fed461628dfc834
> > was a very bad commit.
> >
> > The 'cat' and compression/decompression must be
> > separate rules.
> >
> > We should not repeat the mistake in the past.
> >
>
> Would it be preferable to change all of the compression rules or just the
> new decompression rules?


I do not require you to change the existing code.

For decompression, it is unlikely that the recipe
takes multiple input files.
So, 'cat' is unneeded.




> I could change just the new ones and then begin working on a different
> patch to clean up the 'cat' processes in the compression rules.



If you get rid of the 'cat', you need to refactor the user code.
arch/x86/boot/compressed/Makefile relies on 'cat' and 'compress',
but please double-check no other Makefile uses it.


Also, you might need some research about the potential
impact onto the reproducible builds.
Without 'cat |', the compressed archive might encode
the timestamp of the original file.
GZIP records the timestamp in the header.




>
> >
> >
> > >  # ZSTD
> > >  # ---------------------------------------------------------------------------
> > >  # Appends the uncompressed size of the data using size_append. The .zst
> > > @@ -548,6 +551,9 @@ quiet_cmd_zstd22 = ZSTD22  $@
> > >  quiet_cmd_zstd22_with_size = ZSTD22  $@
> > >        cmd_zstd22_with_size = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
> > >
> > > +quiet_cmd_zstddec = ZSTDDEC $@
> > > +      cmd_zstddec = cat $(real-prereqs) | $(ZSTD) --decompress > $@
> > > +
> >
> >
> > Same here.
> > Please make this a single process:
> >
> >    cmd_zstddec = $(ZSTD) --decompress --force --output=$@ $<
> >
> >
> >
> >
> >
> >
> > One small concern in the future is, if we end up with adding
> > quiet_cmd_bzip2dec, we will run out of the 7-column of the short log.
> >
> >  quiet_cmd_bzip2dec = BZIP2DEC$@
> >
> > We can increase the column size if needed, so I do not think
> > it is a big issue.
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > >  # ASM offsets
> > >  # ---------------------------------------------------------------------------
> > >
> > > --
> > > 2.41.0
> > >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
> >



-- 
Best Regards
Masahiro Yamada

[PATCH 2/2] firmware_loader: Enable compressed files in EXTRA_FIRMWARE

[Kevin Martin]

The linux-firmware packages on Gentoo, Fedora, Arch, and others compress
the firmware files. This works well with CONFIG_FW_LOADER_COMPRESS, but
does not work with CONFIG_EXTRA_FIRMWARE. This patch allows the build
system to decompress firmware files specified by CONFIG_EXTRA_FIRMWARE.
Uncompressed files are used first, then the compressed files are used.

Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
---
 drivers/base/firmware_loader/Kconfig          |  5 ++++-
 drivers/base/firmware_loader/builtin/Makefile | 16 ++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
index 5ca00e02f..b7a908bff 100644
--- a/drivers/base/firmware_loader/Kconfig
+++ b/drivers/base/firmware_loader/Kconfig
@@ -76,7 +76,10 @@ config EXTRA_FIRMWARE
 	  image since it combines both GPL and non-GPL work. You should
 	  consult a lawyer of your own before distributing such an image.
 
-	  NOTE: Compressed files are not supported in EXTRA_FIRMWARE.
+	  NOTE: Compressed files are supported by EXTRA_FIRMWARE. The build
+	  system will look for uncompressed files first then fall back to
+	  searching for compressed files in a similar way to
+	  CONFIG_FW_LOADER_COMPRESS.
 
 config EXTRA_FIRMWARE_DIR
 	string "Firmware blobs root directory"
diff --git a/drivers/base/firmware_loader/builtin/Makefile b/drivers/base/firmware_loader/builtin/Makefile
index 6c067dedc..cc60eb441 100644
--- a/drivers/base/firmware_loader/builtin/Makefile
+++ b/drivers/base/firmware_loader/builtin/Makefile
@@ -20,7 +20,7 @@ filechk_fwbin = \
 	echo "    .section .rodata"				;\
 	echo "    .p2align 4"					;\
 	echo "_fw_$(FWSTR)_bin:"				;\
-	echo "    .incbin \"$(fwdir)/$(FWNAME)\""		;\
+	echo "    .incbin \"$(obj)/$(FWNAME)\""		;\
 	echo "_fw_end:"						;\
 	echo "    .section .rodata.str,\"aMS\",$(PROGBITS),1"	;\
 	echo "    .p2align $(ASM_ALIGN)"			;\
@@ -36,7 +36,15 @@ $(obj)/%.gen.S: FORCE
 	$(call filechk,fwbin)
 
 # The .o files depend on the binaries directly; the .S files don't.
-$(addprefix $(obj)/, $(firmware)): $(obj)/%.gen.o: $(fwdir)/%
+$(addprefix $(obj)/, $(firmware)): $(obj)/%.gen.o: $(obj)/%
 
-targets := $(patsubst $(obj)/%,%, \
-                                $(shell find $(obj) -name \*.gen.S 2>/dev/null))
+$(obj)/% : $(fwdir)/% FORCE
+	$(call if_changed,copy)
+
+$(obj)/% : $(fwdir)/%.xz FORCE
+	$(call if_changed,xzdec)
+
+$(obj)/% : $(fwdir)/%.zst FORCE
+	$(call if_changed,zstddec)
+
+targets := $(patsubst %.gen.o, %.gen.S, $(firmware)) $(CONFIG_EXTRA_FIRMWARE)
-- 
2.41.0

Re: [PATCH 2/2] firmware_loader: Enable compressed files in EXTRA_FIRMWARE

[Nicolas Schier]

On Wed, Dec 20, 2023 at 05:29:33AM -0500, Kevin Martin wrote:
> The linux-firmware packages on Gentoo, Fedora, Arch, and others compress
> the firmware files. This works well with CONFIG_FW_LOADER_COMPRESS, but
> does not work with CONFIG_EXTRA_FIRMWARE. This patch allows the build
> system to decompress firmware files specified by CONFIG_EXTRA_FIRMWARE.
> Uncompressed files are used first, then the compressed files are used.
> 
> Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
> ---

Tested-by: Nicolas Schier <n.schier@avm.de>

Re: [PATCH 2/2] firmware_loader: Enable compressed files in EXTRA_FIRMWARE

[Masahiro Yamada]

On Wed, Dec 20, 2023 at 7:33 PM Kevin Martin <kevinmbecause@gmail.com> wrote:
>
> The linux-firmware packages on Gentoo, Fedora, Arch, and others compress
> the firmware files. This works well with CONFIG_FW_LOADER_COMPRESS, but
> does not work with CONFIG_EXTRA_FIRMWARE. This patch allows the build
> system to decompress firmware files specified by CONFIG_EXTRA_FIRMWARE.
> Uncompressed files are used first, then the compressed files are used.







>
> Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
> ---
>  drivers/base/firmware_loader/Kconfig          |  5 ++++-
>  drivers/base/firmware_loader/builtin/Makefile | 16 ++++++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
> index 5ca00e02f..b7a908bff 100644
> --- a/drivers/base/firmware_loader/Kconfig
> +++ b/drivers/base/firmware_loader/Kconfig
> @@ -76,7 +76,10 @@ config EXTRA_FIRMWARE
>           image since it combines both GPL and non-GPL work. You should
>           consult a lawyer of your own before distributing such an image.
>
> -         NOTE: Compressed files are not supported in EXTRA_FIRMWARE.
> +         NOTE: Compressed files are supported by EXTRA_FIRMWARE. The build
> +         system will look for uncompressed files first then fall back to
> +         searching for compressed files in a similar way to
> +         CONFIG_FW_LOADER_COMPRESS.
>
>  config EXTRA_FIRMWARE_DIR
>         string "Firmware blobs root directory"
> diff --git a/drivers/base/firmware_loader/builtin/Makefile b/drivers/base/firmware_loader/builtin/Makefile
> index 6c067dedc..cc60eb441 100644
> --- a/drivers/base/firmware_loader/builtin/Makefile
> +++ b/drivers/base/firmware_loader/builtin/Makefile
> @@ -20,7 +20,7 @@ filechk_fwbin = \
>         echo "    .section .rodata"                             ;\
>         echo "    .p2align 4"                                   ;\
>         echo "_fw_$(FWSTR)_bin:"                                ;\
> -       echo "    .incbin \"$(fwdir)/$(FWNAME)\""               ;\
> +       echo "    .incbin \"$(obj)/$(FWNAME)\""         ;\




Insert a tab to keep the alignment of ";\"




Also, please update the .gitignore because
any arbitrary firmware file might be copied to
this directory.
All files except the 3 check-in files must be
ignored.



@@ -1,2 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-*.gen.S
+*
+!.gitignore
+!Makefile
+!main.c









--
Best Regards
Masahiro Yamada