Hi Valerii,
thanks for your patch. I know several non-upstream kernel developers
that would like to see support for out-of-source builds of external
kmods (and we developed several work-arounds at AVM as well); but please
be aware that patches that don't fix or enhance the in-tree kernel/kmod
build but only add feature for out-of-tree stuff, are rarely accepted.
(Rational: better upstream your kmods to have them in-tree, especially
if they are so complex that they need subdirs.)
That said, I'll try to give some more concrete feedback below.
On Fri, Apr 05, 2024 at 09:56:08AM -0700, Valerii Chernous wrote:
> The change allow to build external modules with nested makefiles.
better use imperative statements to the code itself: "Allow to build..."
Does "nested makefile" mean that you want to support external kernel
modules with subdirs and Makefiles within?
> With current unofficial way(using "src" variable) it is possible to build
> external(out of tree) kernel module with separate source and build
> artifacts dirs but with nested makefiles it doesn't work properly.
> Build system trap to recursion inside makefiles, artifacts output dir
> path grow with each iteration until exceed max path len and build failed.
I think I know what you want to say with this sentence, but I am not
able to parse it correctly. Might you want to rephrase it a bit?
> Providing "MO" variable and using "override" directive with declaring
> "src" variable solves the problem
> Usage example:
> make -C KERNEL_SOURCE_TREE MO=BUILD_OUT_DIR M=EXT_MOD_SRC_DIR modules
>
> Cc: xe-linux-external@xxxxxxxxx
> Cc: Valerii Chernous <vchernou@xxxxxxxxx>
> Signed-off-by: Valerii Chernous <vchernou@xxxxxxxxx>
> ---
> Documentation/kbuild/kbuild.rst | 14 +++++++++++++-
> Documentation/kbuild/modules.rst | 16 +++++++++++++++-
> Makefile | 17 +++++++++++++++++
> scripts/Makefile.build | 7 +++++++
> 4 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
> index 9c8d1d046ea5..81413ddba2ad 100644
> --- a/Documentation/kbuild/kbuild.rst
> +++ b/Documentation/kbuild/kbuild.rst
> @@ -121,10 +121,22 @@ Setting "V=..." takes precedence over KBUILD_VERBOSE.
> KBUILD_EXTMOD
> -------------
> Set the directory to look for the kernel source when building external
> -modules.
> +modules. In case of using separate sources and module artifacts directories
> +(KBUILD_EXTMOD + KBUILD_EXTMOD_SRC), KBUILD_EXTMOD working as output
> +artifacts directory.
That means, iff you use KBUILD_EXTMOD_SRC and let KBUILD_EXTMOD point to
some other directory, you _have_ to be sure that your kernel supported
KBUILD_EXTMOD_SRC properly or your module will not be build at all,
right?
>
> Setting "M=..." takes precedence over KBUILD_EXTMOD.
>
> +KBUILD_EXTMOD_SRC
> +-----------------
> +Set the external module source directory in case when separate module
> +sources and build artifacts directories are used. Working in pair with
> +KBUILD_EXTMOD. If KBUILD_EXTMOD_SRC is set then KBUILD_EXTMOD is used as
> +module build artifacts directory.
> +
> +Setting "MO=..." takes precedence over KBUILD_EXTMOD.
> +Setting "M=..." takes precedence over KBUILD_EXTMOD_SRC.
(Just a note, not a complaint: This confused me while reading it the
first time, but I think it is correct for your patch. Personally, I do
not like the semantical change of KBUILD_EXTMOD/M and would rather
prefer the introduction of some more explicit names like
KBUILD_EXTMOD_SOURCE and KBUILD_EXTMOD_BUILD instead.)
> +
> KBUILD_OUTPUT
> -------------
> Specify the output directory when building the kernel.
> diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> index a1f3eb7a43e2..b6c30e76b314 100644
> --- a/Documentation/kbuild/modules.rst
> +++ b/Documentation/kbuild/modules.rst
> @@ -79,6 +79,14 @@ executed to make module versioning work.
> The kbuild system knows that an external module is being built
> due to the "M=<dir>" option given in the command.
>
> + To build an external module with separate src and artifacts dirs use::
> +
> + $ make -C <path_to_kernel_src> M=$PWD MO=<output_dir>
Is it really good to evaluate MO relative to the kernel source or build tree?
make -C /lib/modules/$(uname -r)/build M=/somewhere/source MO=../build
might accidentially lead to ugly inconsistencies in the kernel build
tree (permissions presumed).
> +
> + The kbuild system knows that an external module with separate sources
> + and build artifacts dirs is being built due to the "M=<dir>" and
> + "MO=<output_dir>" options given in the command.
> +
> To build against the running kernel use::
>
> $ make -C /lib/modules/`uname -r`/build M=$PWD
> @@ -93,7 +101,7 @@ executed to make module versioning work.
>
> ($KDIR refers to the path of the kernel source directory.)
>
> - make -C $KDIR M=$PWD
> + make -C $KDIR M=$PWD MO=<module_output_dir>
>
> -C $KDIR
> The directory where the kernel source is located.
> @@ -106,6 +114,12 @@ executed to make module versioning work.
> directory where the external module (kbuild file) is
> located.
>
> + MO=<module_output_dir>
> + Informs kbuild that external module build artifacts
> + should be placed into specific dir(<module_output_dir>).
What about "should be placed into the specified directory <module_output_dir>." ?
> + Parameter is optional. Without it "M" works as both
> + source provider and build output location.
> +
> 2.3 Targets
> ===========
>
> diff --git a/Makefile b/Makefile
> index 4bef6323c47d..3d45a41737a6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -142,6 +142,7 @@ ifeq ("$(origin M)", "command line")
> KBUILD_EXTMOD := $(M)
> endif
>
> +define kbuild_extmod_check_TEMPLATE
> $(if $(word 2, $(KBUILD_EXTMOD)), \
> $(error building multiple external modules is not supported))
>
> @@ -152,9 +153,25 @@ $(foreach x, % :, $(if $(findstring $x, $(KBUILD_EXTMOD)), \
> ifneq ($(filter %/, $(KBUILD_EXTMOD)),)
> KBUILD_EXTMOD := $(shell dirname $(KBUILD_EXTMOD).)
> endif
> +endef
> +$(eval $(call kbuild_extmod_check_TEMPLATE))
Same checks make sense for KBUILD_EXTMOD_SRC, also if MO is not set.
>
> export KBUILD_EXTMOD
>
> +# Use make M=src_dir MO=ko_dir or set the environment variables:
> +# KBUILD_EXTMOD_SRC, KBUILD_EXTMOD to specify separate directories of
> +# external module sources and build artifacts.
> +ifeq ("$(origin MO)", "command line")
> +ifeq ($(KBUILD_EXTMOD),)
> + $(error Ext module objects without module sources is not supported))
> +endif
> +KBUILD_EXTMOD_SRC := $(KBUILD_EXTMOD)
> +KBUILD_EXTMOD := $(MO)
> +$(eval $(call kbuild_extmod_check_TEMPLATE))
> +endif
> +
> +export KBUILD_EXTMOD_SRC
> +
> # backward compatibility
> KBUILD_EXTRA_WARN ?= $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index baf86c0880b6..a293950e2e07 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -3,7 +3,14 @@
> # Building
> # ==========================================================================
>
> +ifeq ($(KBUILD_EXTMOD_SRC),)
> src := $(obj)
I would leave the 'src := $(obj)' stand alone unconditionally, to
clearly separate the oot-oos-kmod support from default in-tree kernel or
kmod builds and in-source but out-of-tree kmod builds.
> +else ifeq ($(KBUILD_EXTMOD),$(obj))
> +override src := $(KBUILD_EXTMOD_SRC)
> +else
> +src_subdir := $(patsubst $(KBUILD_EXTMOD)/%,%,$(obj))
> +override src := $(KBUILD_EXTMOD_SRC)/$(src_subdir)
bike-shedding: see below
> +endif
>
> PHONY := $(obj)/
> $(obj)/:
> --
> 2.35.6
>
Testing with a simple module w/ subdir, compilation fails for me:
$ make M=../stupid-multidir-kmod/source V=2 MO=/tmp/build
CC [M] /tmp/build/subdir/module.o - due to target missing
CC [M] /tmp/build/hello.o - due to target missing
scripts/Makefile.modpost:118: /tmp/build/Makefile: No such file or directory
make[2]: *** No rule to make target '/tmp/build/Makefile'. Stop.
make[1]: *** [/data/linux/oot-kmods-MO/Makefile:1897: modpost] Error 2
make: *** [Makefile:257: __sub-make] Error 2
(similar for 'make clean'.)
The test kbuild files were as simple as:
.../source/Kbuild:
obj-m += subdir/
obj-m += hello.o
.../source/subdir/Kbuild:
obj-m += module.o
Does something like this work for you? If I understand your proposed
commit message correctly, you have some working example with subdirs?
---
Bike-shedding for inspiration:
While experimenting with similar solutions at AVM, we did the same src
override but also auto-generated dummy Makefiles in $(obj) with something
like:
ifneq ($(if $(KBUILD_EXTMOD_SRC),$(filter $(KBUILD_EXTMOD)%,$(obj))),)
# If KBUILD_EXTMOD_SOURCE is set, enable out-of-source kmod build
# support, in general. But do not disturb the kbuild preparing targets
# that need the original behaviour.
#
# Thus, iff also $(obj) is set and points to some path at $(KBUILD_EXTMOD) or
# below, we really want to set (override) $(src).
override src := $(obj:$(KBUILD_EXTMOD)%=$(KBUILD_EXTMOD_SRC)%)
# Generate or update $(obj)/Makefile to include the original $(src)/Kbuild or
# $(src)/Makefile.
_kbuild_extmod_src_makefile = $(firstword $(wildcard $(src)/Kbuild $(src)/Makefile))
$(lastword $(MAKEFILE_LIST)): $(obj)/Makefile
$(obj)/Makefile: $(_kbuild_extmod_src_makefile) FORCE
$(call filechk,build_makefile)
$(eval filechk_build_makefile = echo include $(_kbuild_extmod_src_makefile))
# Clean-up the variable space
undefine $(filter _kbuild_extmod_%,$(.VARIABLES))
endif
but we still had to add a dependency on $(subdir-ym) for all object
files in any kmod subdir to enforce proper dependency handling.
Fortunately, we almost stopped using such "enhanced" oot-oos kmod build
things.
HTH,
kind regards,
Nicolas
Attachment:
signature.asc
Description: PGP signature
