Re: [PATCH 04/15] kbuild: add generic support for built-in boot DTBs

[Masahiro Yamada <masahiroy@kernel.org>]

On Thu, Sep 5, 2024 at 11:17 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 08:47:40AM +0900, Masahiro Yamada wrote:
> > Some architectures embed boot DTBs in vmlinux. A potential issue for
> > these architectures is a race condition during parallel builds because
> > Kbuild descends into arch/*/boot/dts/ twice.
> >
> > One build thread is initiated by the 'dtbs' target, which is a
> > prerequisite of the 'all' target in the top-level Makefile:
> >
> >   ifdef CONFIG_OF_EARLY_FLATTREE
> >   all: dtbs
> >   endif
> >
> > For architectures that support the embedded boot dtb, arch/*/boot/dts/
> > is visited also during the ordinary directory traversal in order to
> > build obj-y objects that wrap DTBs.
> >
> > Since these build threads are unaware of each other, they can run
> > simultaneously during parallel builds.
> >
> > This commit introduces a generic build rule to scripts/Makefile.vmlinux
> > to support embedded boot DTBs in a race-free way. Architectures that
> > want to use this rule need to select CONFIG_GENERIC_BUILTIN_DTB.
> >
> > After the migration, Makefiles under arch/*/boot/dts/ will be visited
> > only once to build only *.dtb files.
> >
> > This change also aims to unify the CONFIG options used for embedded DTBs
> > support. Currently, different architectures use different CONFIG options
> > for the same purposes.
> >
> > The CONFIG options are unified as follows:
> >
> >  - CONFIG_GENERIC_BUILTIN_DTB
> >
> >    This enables the generic rule for embedded boot DTBs. This will be
> >    renamed to CONFIG_BUILTIN_DTB after all architectures migrate to the
> >    generic rule.
> >
> >  - CONFIG_BUILTIN_DTB_NAME
> >
> >    This specifies the path to the embedded DTB.
> >    (relative to arch/*/boot/dts/)
> >
> >  - CONFIG_BUILTIN_DTB_ALL
> >
> >    If this is enabled, all DTB files compiled under arch/*/boot/dts/ are
> >    embedded into vmlinux. Only used by MIPS.
>
> I started to do this a long time ago, but then decided we didn't want to
> encourage this feature. IMO it should only be for legacy bootloaders or
> development/debug. And really, appended DTB is more flexible for the
> legacy bootloader case.


I learned CONFIG_ARM_APPENDED_DTB today.



> In hindsight, a common config would have been easier to limit new
> arches...
>
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  Makefile                 |  7 ++++++-
> >  drivers/of/Kconfig       |  6 ++++++
> >  scripts/Makefile.vmlinux | 44 ++++++++++++++++++++++++++++++++++++++++
> >  scripts/link-vmlinux.sh  |  4 ++++
> >  4 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 145112bf281a..1c765c12ab9e 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1417,6 +1417,10 @@ ifdef CONFIG_OF_EARLY_FLATTREE
> >  all: dtbs
> >  endif
> >
> > +ifdef CONFIG_GENERIC_BUILTIN_DTB
> > +vmlinux: dtbs
> > +endif
> > +
> >  endif
> >
> >  PHONY += scripts_dtc
> > @@ -1483,7 +1487,8 @@ endif # CONFIG_MODULES
> >  CLEAN_FILES += vmlinux.symvers modules-only.symvers \
> >              modules.builtin modules.builtin.modinfo modules.nsdeps \
> >              compile_commands.json rust/test \
> > -            rust-project.json .vmlinux.objs .vmlinux.export.c
> > +            rust-project.json .vmlinux.objs .vmlinux.export.c \
> > +               .builtin-dtbs-list .builtin-dtb.S
> >
> >  # Directories & files removed with 'make mrproper'
> >  MRPROPER_FILES += include/config include/generated          \
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index dd726c7056bf..5142e7d7fef8 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -2,6 +2,12 @@
> >  config DTC
> >       bool
> >
> > +config GENERIC_BUILTIN_DTB
> > +     bool
>
> So that we don't add new architectures to this, I would like something
> like:
>
> # Do not add new architectures to this list
> depends on MIPS || RISCV || MICROBLAZE ...
>
> Yes, it's kind of odd since the arch selects the option...
>
> For sure, we don't want this option on arm64. For that, I can rely on
> Will and Catalin rejecting a select, but on some new arch I can't.
>
> > +
> > +config BUILTIN_DTB_ALL
> > +     bool
>
> Can this be limited to MIPS?


I am fine with hard-coded "depends on"
if this feature is discouraged.




> > +
> >  menuconfig OF
> >       bool "Device Tree and Open Firmware support"
> >       help
> > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > index 5ceecbed31eb..4626b472da49 100644
> > --- a/scripts/Makefile.vmlinux
> > +++ b/scripts/Makefile.vmlinux
> > @@ -17,6 +17,50 @@ quiet_cmd_cc_o_c = CC      $@
> >  %.o: %.c FORCE
> >       $(call if_changed_dep,cc_o_c)
> >
> > +quiet_cmd_as_o_S = AS      $@
> > +      cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
> > +
> > +%.o: %.S FORCE
> > +     $(call if_changed_dep,as_o_S)
> > +
> > +# Built-in dtb
> > +# ---------------------------------------------------------------------------
> > +
> > +quiet_cmd_wrap_dtbs = WRAP    $@
> > +      cmd_wrap_dtbs = {                                                      \
> > +     echo '\#include <asm-generic/vmlinux.lds.h>';                   \
> > +     echo '.section .dtb.init.rodata,"a"';                           \
> > +     while read dtb; do                                              \
> > +             symbase=__dtb_$$(basename -s .dtb "$${dtb}" | tr - _);  \
> > +             echo '.balign STRUCT_ALIGNMENT';                        \
>
> Is this always guaranteed to be at least 8 bytes? That's the required
> alignment for dtbs and assumed by libfdt.


I think so.


include/asm-generic/vmlinux.lds.h defines it as 32.


We can loosen the alignment to 8, but for safety,
I just copied this from scripts/Makefile.lib
because 32-byte alignment is what we do now.








-- 
Best Regards
Masahiro Yamada