Re: [PATCH 1/1] kbuild: install the modules.order for external modules

[Will McVicker <willmcvicker@google.com>]

On Mon, Dec 20, 2021 at 10:13 AM Will McVicker <willmcvicker@google.com> wrote:
>
> On Fri, Dec 17, 2021 at 5:01 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > (Cc: Lucas De Marchi)
> >
> > On Thu, Dec 16, 2021 at 2:23 AM Will McVicker <willmcvicker@google.com> wrote:
> > >
> > > Add support to install the modules.order file for external modules
> > > during module_install in order to retain the Makefile ordering
> > > of external modules. This helps reduce the extra steps necessary to
> > > properly order loading of external modules when there are multiple
> > > kernel modules compiled within a given KBUILD_EXTMOD directory.
> > >
> > > To handle compiling multiple external modules within the same
> > > INSTALL_MOD_DIR, kbuild will append a suffix to the installed
> > > modules.order file defined like so:
> > >
> > >   echo ${KBUILD_EXTMOD} | sed 's:[./_]:_:g'
> > >
> > > Ex:
> > >   KBUILD_EXTMOD=/mnt/a.b/c-d/my_driver results in:
> > >   modules.order._mnt_a_b_c_d_my_driver
> > >
> > > The installed module.order.$(extmod_suffix) files can then be cat'd
> > > together to create a single modules.order file which would define the
> > > order to load all of the modules during boot.
> >
> >
> > So, the user must do this manually?
> >
> > cat extra/modules.order._mnt_a_b_c_d_my_driver  \
> >    extra/modules.order._mnt_e_f_g_h_my_driver \
> >    >> modules.order
> >
> > This is so ugly, and incomplete.
> >
> > I cc'ed the kmod maintainer, who may have
> > comments or better approach.
> >
> >
> >
> >
> >
> >
> > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > ---
> > >  scripts/Makefile.modinst | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
> > > index ff9b09e4cfca..2e2e31696fd6 100644
> > > --- a/scripts/Makefile.modinst
> > > +++ b/scripts/Makefile.modinst
> > > @@ -24,6 +24,10 @@ suffix-$(CONFIG_MODULE_COMPRESS_XZ)  := .xz
> > >  suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)  := .zst
> > >
> > >  modules := $(patsubst $(extmod_prefix)%, $(dst)/%$(suffix-y), $(modules))
> > > +ifneq ($(KBUILD_EXTMOD),)
> > > +extmod_suffix := $(subst /,_,$(subst .,_,$(subst -,_,$(KBUILD_EXTMOD))))
> > > +modules += $(dst)/modules.order.$(extmod_suffix)
> > > +endif
> > >
> > >  __modinst: $(modules)
> > >         @:
> > > @@ -82,6 +86,12 @@ $(dst)/%.ko: $(extmod_prefix)%.ko FORCE
> > >         $(call cmd,strip)
> > >         $(call cmd,sign)
> > >
> > > +ifneq ($(KBUILD_EXTMOD),)
> > > +$(dst)/modules.order.$(extmod_suffix): $(MODORDER) FORCE
> > > +       $(call cmd,install)
> > > +       @sed -i "s:^$(KBUILD_EXTMOD):$(INSTALL_MOD_DIR):g" $@
> > > +endif
> > > +
> > >  else
> > >
> > >  $(dst)/%.ko: FORCE
> > > --
> > > 2.34.1.173.g76aa8bc2d0-goog
> > >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
> >
> > --
> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> >
>
> Hi Masahiro,
>
> Thanks for your response! I agree with you that this is ugly and would
> love feedback on the direction to take this. With regards to
> incompleteness, the existing kbuild implementation for external
> modules is already incomplete in the sense that it doesn't even
> attempt to install the already generated modules.order files to
> INSTALL_MOD_DIR. I believe this patch gets us a little closer to
> closing the gap, but agree it's nowhere complete. I would be happy to
> fully close the gap if I knew that it would be accepted or welcomed.
> Let me give you some insight on how we currently do this on Android
> and how this patch changes that.
>
> Currently, the Android build.sh script (which is used to compile the
> Android Common Kernel) supports the build variable EXT_MODULES which
> is a list of paths to external modules to be compiled. The build
> script loops through this list and calls "make modules" and "make
> modules_install" to compile and install the external modules. Then,
> the build script creates the modules.order file like this:
>
> cd ${MODLIB}
> find extra -type f -name "*.ko" | sort >> modules.order
>
> Sorting is used so that the modules.order file is deterministic. This
> proposed patch allows us to use the Makefile defined ordering instead
> of this sorted ordering. In Android the paths to external modules must
> be relative to the kernel repository. For example, the kernel and all
> external modules are all cloned within a root directory and the paths
> in EXT_MODULES are all relative to the root directory. So our build.sh
> script can use the relative path from the root directory as part of
> INSTALL_MOD_DIR to get rid of the suffix ugliness. For example, we can
> do this:
>
> for EXT_MOD in ${EXT_MODULES}; do
>   # make modules
>   make -C ${EXT_MOD} ...
>   # make modules_install
>   make -C ${EXT_MOD} ...  INSTALL_MOD_DIR="extra/${EXT_MOD}" modules_install
> done
>
> for EXT_MOD in ${EXT_MODULES}; do
>   modules_order_file=$(ls ${MODLIB}/extra/${EXT_MOD}/modules.order.*)
>   cat ${modules_order_file} >> ${MODLIB}/modules.order
> done
>
> Since kbuild doesn't know about how many external modules there are
> nor does it retain any information about each individual call to "make
> modules" or "make modules_install", we can't do the concatenation
> within the Makefile.modinst script. To close the gap, we could add an
> additional make target that one could call after all of the external
> modules have been installed to do this concatenation, but I wasn't
> sure how open everyone would be to accepting this. Let me know your
> thoughts on that.
>
> Thanks,
> Will

Friendly reminder on this patch now that most are back from the holiday break.

Lucas, could you comment please?

Thanks,
Will