diff mbox series

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

Message ID 20240904234803.698424-5-masahiroy@kernel.org (mailing list archive)
State Handled Elsewhere
Headers show
Series kbuild: refactor DTB build rules, introduce a generic built-in boot DTB support | expand

Commit Message

Masahiro Yamada Sept. 4, 2024, 11:47 p.m. UTC
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.

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(-)

Comments

Rob Herring Sept. 5, 2024, 2:17 p.m. UTC | #1
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.

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?

> +
>  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.

> +		echo ".global $${symbase}_begin";			\
> +		echo "$${symbase}_begin:";				\
> +		echo '.incbin "'$$dtb'" ';				\
> +		echo ".global $${symbase}_end";				\
> +		echo "$${symbase}_end:";				\
> +	done < $<;							\
> +	} > $@
Masahiro Yamada Sept. 6, 2024, 1:56 a.m. UTC | #2
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.
Masahiro Yamada Sept. 10, 2024, 9:35 a.m. UTC | #3
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.
>
> 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 ...


This will not work after 14/15 is applied.


For example, if arch/arm/Kconfig has


config BUILTIN_DTB
      bool "enable BUILTIN_DTB"


No warning is displayed.
diff mbox series

Patch

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
+
+config BUILTIN_DTB_ALL
+	bool
+
 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';			\
+		echo ".global $${symbase}_begin";			\
+		echo "$${symbase}_begin:";				\
+		echo '.incbin "'$$dtb'" ';				\
+		echo ".global $${symbase}_end";				\
+		echo "$${symbase}_end:";				\
+	done < $<;							\
+	} > $@
+
+.builtin-dtbs.S: .builtin-dtbs-list FORCE
+	$(call if_changed,wrap_dtbs)
+
+quiet_cmd_gen_dtbs_list = GEN     $@
+      cmd_gen_dtbs_list = \
+	$(if $(CONFIG_BUILTIN_DTB_NAME), echo "arch/$(SRCARCH)/boot/dts/$(CONFIG_BUILTIN_DTB_NAME).dtb",:) > $@
+
+.builtin-dtbs-list: arch/$(SRCARCH)/boot/dts/dtbs-list FORCE
+	$(call if_changed,$(if $(CONFIG_BUILTIN_DTB_ALL),copy,gen_dtbs_list))
+
+targets += .builtin-dtbs-list
+
+ifdef CONFIG_GENERIC_BUILTIN_DTB
+targets += .builtin-dtbs.S .builtin-dtbs.o
+vmlinux: .builtin-dtbs.o
+endif
+
+# vmlinux
+# ---------------------------------------------------------------------------
+
 ifdef CONFIG_MODULES
 targets += .vmlinux.export.o
 vmlinux: .vmlinux.export.o
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index c27b4e969f20..bd196944e350 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -68,6 +68,10 @@  vmlinux_link()
 		libs="${KBUILD_VMLINUX_LIBS}"
 	fi
 
+	if is_enabled CONFIG_GENERIC_BUILTIN_DTB; then
+		objs="${objs} .builtin-dtbs.o"
+	fi
+
 	if is_enabled CONFIG_MODULES; then
 		objs="${objs} .vmlinux.export.o"
 	fi