Messages in this thread | | | From | Masahiro Yamada <> | Date | Mon, 7 Aug 2023 04:32:09 +0900 | Subject | Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step |
| |
On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote: > > Currently Makefile.modinst does three tasks on each module built: > - Install modules > - Sign modules > - Compress modules > > All the above tasks happen from a single place. > > This patch divides this task further and uses a different makefile for > each task. > Signing module logic is completely refactored and everything happens > from a shell script now. > > Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
This patch is bad in multiple ways.
1. Break "make modules_sign" 2. Serialize the installation steps, that is, works less efficiently 3. Increase code without adding any benefits.
There is no good reason to do these changes.
NACK.
> --- > scripts/Makefile.compress | 53 ++++++++++++++++++ > scripts/Makefile.install | 66 +++++++++++++++++++++++ > scripts/Makefile.modinst | 111 +++----------------------------------- > scripts/Makefile.sign | 37 +++++++++++++ > scripts/signfile.sh | 24 +++++++++ > 5 files changed, 186 insertions(+), 105 deletions(-) > create mode 100644 scripts/Makefile.compress > create mode 100644 scripts/Makefile.install > create mode 100644 scripts/Makefile.sign > create mode 100755 scripts/signfile.sh > > diff --git a/scripts/Makefile.compress b/scripts/Makefile.compress > new file mode 100644 > index 000000000000..35d337ac9b6c > --- /dev/null > +++ b/scripts/Makefile.compress > @@ -0,0 +1,53 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# ========================================================================== > +# Compressing modules > +# ========================================================================== > + > +PHONY := __modcompress > +__modcompress: > + > +include include/config/auto.conf > +include $(srctree)/scripts/Kbuild.include > + > +modules := $(call read-file, $(MODORDER)) > + > +ifeq ($(KBUILD_EXTMOD),) > +dst := $(MODLIB)/kernel > +else > +INSTALL_MOD_DIR ?= updates > +dst := $(MODLIB)/$(INSTALL_MOD_DIR) > +endif > + > +suffix-y := > +suffix-$(CONFIG_MODULE_COMPRESS_GZIP) := .gz > +suffix-$(CONFIG_MODULE_COMPRESS_XZ) := .xz > +suffix-$(CONFIG_MODULE_COMPRESS_ZSTD) := .zst > + > +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules)) > + > +__modcompress: $(modules) > + @: > + > +# > +# Compression > +# > +quiet_cmd_gzip = GZIP $@ > + cmd_gzip = $(KGZIP) -n -f $< > +quiet_cmd_xz = XZ $@ > + cmd_xz = $(XZ) --lzma2=dict=2MiB -f $< > +quiet_cmd_zstd = ZSTD $@ > + cmd_zstd = $(ZSTD) -T0 --rm -f -q $< > + > +$(dst)/%.ko.gz: $(dst)/%.ko FORCE > + $(call cmd,gzip) > + > +$(dst)/%.ko.xz: $(dst)/%.ko FORCE > + $(call cmd,xz) > + > +$(dst)/%.ko.zst: $(dst)/%.ko FORCE > + $(call cmd,zstd) > + > +PHONY += FORCE > +FORCE: > + > +.PHONY: $(PHONY) > diff --git a/scripts/Makefile.install b/scripts/Makefile.install > new file mode 100644 > index 000000000000..40c496cb99dc > --- /dev/null > +++ b/scripts/Makefile.install > @@ -0,0 +1,66 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# ========================================================================== > +# Installing modules > +# ========================================================================== > + > +PHONY := __modinstall > +__modinstall: > + > +include include/config/auto.conf > +include $(srctree)/scripts/Kbuild.include > + > +modules := $(call read-file, $(MODORDER)) > + > +ifeq ($(KBUILD_EXTMOD),) > +dst := $(MODLIB)/kernel > +else > +INSTALL_MOD_DIR ?= updates > +dst := $(MODLIB)/$(INSTALL_MOD_DIR) > +endif > + > +$(foreach x, % :, $(if $(findstring $x, $(dst)), \ > + $(error module installation path cannot contain '$x'))) > + > +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules)) > + > +__modinstall: $(modules) > + @: > + > +# > +# Installation > +# > +quiet_cmd_install = INSTALL $@ > + cmd_install = mkdir -p $(dir $@); cp $< $@ > + > +# Strip > +# > +# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they > +# are installed. If INSTALL_MOD_STRIP is '1', then the default option > +# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used > +# as the options to the strip command. > +ifdef INSTALL_MOD_STRIP > + > +ifeq ($(INSTALL_MOD_STRIP),1) > +strip-option := --strip-debug > +else > +strip-option := $(INSTALL_MOD_STRIP) > +endif > + > +quiet_cmd_strip = STRIP $@ > + cmd_strip = $(STRIP) $(strip-option) $@ > + > +else > + > +quiet_cmd_strip = > + cmd_strip = : > + > +endif > + > +$(dst)/%.ko: $(extmod_prefix)%.ko FORCE > + $(call cmd,install) > + $(call cmd,strip) > + > +PHONY += FORCE > +FORCE: > + > +.PHONY: $(PHONY) > diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst > index ab0c5bd1a60f..d87e09e57963 100644 > --- a/scripts/Makefile.modinst > +++ b/scripts/Makefile.modinst > @@ -1,116 +1,17 @@ > # SPDX-License-Identifier: GPL-2.0 > # ========================================================================== > -# Installing modules > +# Install, Sign & Compress modules > # ========================================================================== > > -PHONY := __modinst > -__modinst: > - > include include/config/auto.conf > include $(srctree)/scripts/Kbuild.include > > -modules := $(call read-file, $(MODORDER)) > - > -ifeq ($(KBUILD_EXTMOD),) > -dst := $(MODLIB)/kernel > -else > -INSTALL_MOD_DIR ?= updates > -dst := $(MODLIB)/$(INSTALL_MOD_DIR) > -endif > - > -$(foreach x, % :, $(if $(findstring $x, $(dst)), \ > - $(error module installation path cannot contain '$x'))) > - > -suffix-y := > -suffix-$(CONFIG_MODULE_COMPRESS_GZIP) := .gz > -suffix-$(CONFIG_MODULE_COMPRESS_XZ) := .xz > -suffix-$(CONFIG_MODULE_COMPRESS_ZSTD) := .zst > - > -modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules)) > - > -__modinst: $(modules) > - @: > - > -# > -# Installation > -# > -quiet_cmd_install = INSTALL $@ > - cmd_install = mkdir -p $(dir $@); cp $< $@ > - > -# Strip > -# > -# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they > -# are installed. If INSTALL_MOD_STRIP is '1', then the default option > -# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used > -# as the options to the strip command. > -ifdef INSTALL_MOD_STRIP > - > -ifeq ($(INSTALL_MOD_STRIP),1) > -strip-option := --strip-debug > -else > -strip-option := $(INSTALL_MOD_STRIP) > -endif > - > -quiet_cmd_strip = STRIP $@ > - cmd_strip = $(STRIP) $(strip-option) $@ > - > -else > - > -quiet_cmd_strip = > - cmd_strip = : > - > -endif > - > -# > -# Signing > -# Don't stop modules_install even if we can't sign external modules. > -# > -ifeq ($(CONFIG_MODULE_SIG_ALL),y) > -ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),) > -sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY) > -else > -sig-key := $(CONFIG_MODULE_SIG_KEY) > -endif > -quiet_cmd_sign = SIGN $@ > - cmd_sign = scripts/sign-file $(CONFIG_MODULE_SIG_HASH) "$(sig-key)" certs/signing_key.x509 $@ \ > - $(if $(KBUILD_EXTMOD),|| true) > -else > -quiet_cmd_sign := > - cmd_sign := : > -endif > - > -ifeq ($(modules_sign_only),) > - > -$(dst)/%.ko: $(extmod_prefix)%.ko FORCE > - $(call cmd,install) > - $(call cmd,strip) > - $(call cmd,sign) > - > -else > - > -$(dst)/%.ko: FORCE > - $(call cmd,sign) > - > -endif > - > -# > -# Compression > -# > -quiet_cmd_gzip = GZIP $@ > - cmd_gzip = $(KGZIP) -n -f $< > -quiet_cmd_xz = XZ $@ > - cmd_xz = $(XZ) --lzma2=dict=2MiB -f $< > -quiet_cmd_zstd = ZSTD $@ > - cmd_zstd = $(ZSTD) -T0 --rm -f -q $< > - > -$(dst)/%.ko.gz: $(dst)/%.ko FORCE > - $(call cmd,gzip) > - > -$(dst)/%.ko.xz: $(dst)/%.ko FORCE > - $(call cmd,xz) > +PHONY := __modinst > > -$(dst)/%.ko.zst: $(dst)/%.ko FORCE > - $(call cmd,zstd) > +__modinst: FORCE > + $(MAKE) -f scripts/Makefile.install > + $(MAKE) -f scripts/Makefile.sign > + $(MAKE) -f scripts/Makefile.compress > > PHONY += FORCE > FORCE: > diff --git a/scripts/Makefile.sign b/scripts/Makefile.sign > new file mode 100644 > index 000000000000..d6b242b16657 > --- /dev/null > +++ b/scripts/Makefile.sign > @@ -0,0 +1,37 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# ========================================================================== > +# Signing modules > +# ========================================================================== > + > +PHONY := __modsign > +__modsign: > + > +include include/config/auto.conf > +include $(srctree)/scripts/Kbuild.include > + > +# > +# Signing > +# Don't stop modules_install even if we can't sign external modules. > +# > +ifeq ($(CONFIG_MODULE_SIG_ALL),y) > +ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),) > +sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY) > +else > +sig-key := $(CONFIG_MODULE_SIG_KEY) > +endif > +quiet_cmd_sign = SIGNING ALL MODULES ... > + cmd_sign = $(CONFIG_SHELL) $(srctree)/scripts/signfile.sh \ > + "$(CONFIG_MODULE_SIG_HASH)" \ > + "$(sig-key)" > +else > +quiet_cmd_sign := > + cmd_sign := : > +endif > + > +__modsign: FORCE > + $(call cmd,sign) > + > +PHONY += FORCE > +FORCE: > + > +.PHONY: $(PHONY) > diff --git a/scripts/signfile.sh b/scripts/signfile.sh > new file mode 100755 > index 000000000000..b2b58bfbd5ba > --- /dev/null > +++ b/scripts/signfile.sh > @@ -0,0 +1,24 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > +# > +# A sign-file wrapper used by scripts/Makefile.sign > + > +#set -x > + > +if test $# -ne 2; then > + echo "Usage: $0 <hash-algo> <sign-key>" >&2 > + exit 1 > +fi > + > +SIG_HASH="$1" > +SIG_KEY="$2" > + > +MODULES_PATH="${INSTALL_MOD_PATH}/lib/modules/${KERNELRELEASE}" > + > +find "${MODULES_PATH}" -name *.ko -type f -print0 | \ > + xargs -r -0 -P$(nproc) -x -n32 sh -c "\ > +${srctree}/scripts/sign-file \ > +-a \"${SIG_HASH}\" \ > +-i \"${SIG_KEY}\" \ > +-x ${srctree}/certs/signing_key.x509 \ > +-b \$@ \$0" > -- > 2.41.0 >
-- Best Regards Masahiro Yamada
| |