of: move empty_root and unittest data DTBs to .init.rodata section

PATCH In review Masahiro Yamada 3 participants 5 messages Aug 26, 2024 – Aug 31, 2024

Subsystems (2)

Kbuild Device tree

lore

Share & tools

lore

Masahiro Yamada

author

2024-08-26 12:48 UTC #0 lore raw

PATCH 1/1 of: move empty_root and unittest data DTBs to .init.rodata section

Some architectures can embed DTB(s) in vmlinux. Most of them expect a single DTB in the .dtb.init.rodata section.

For example, RISC-V previously allowed embedding multiple DTBs in vmlinux, but only the first DTB in the .dtb.init.rodata section was used. Which DTB was used was unpredictable, as it depended on the link order (i.e., the order in Makefile).

Commit 2672031b20f6 ("riscv: dts: Move BUILTIN_DTB_SOURCE to common Kconfig") changed the Makefiles to ensure only one DTB is embedded.

However, commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware") introduced another DTB into the .dtb.init.rodata section.

Since then, the symbol dump (sorted by address) for ARCH=riscv nommu_k210_defconfig is as follows:

    00000000801290e0 D __dtb_k210_generic_begin
    00000000801290e0 D __dtb_start
    000000008012b571 D __dtb_k210_generic_end
    000000008012b580 D __dtb_empty_root_begin
    000000008012b5c8 D __dtb_empty_root_end
    000000008012b5e0 D __dtb_end

The .dtb.init.rodata section now contains the following two DTB files:

arch/riscv/boot/dts/canaan/k210_generic.dtb
drivers/of/empty_root.dtb

This is not an immediate problem because the boot code picks up the first DTB. The second one, empty_root.dtb is just ignored.

However, as mentioned above, it is fragile to rely on the link order, as future Makefile changes may break the behavior.

The cmd_wrap_S_dtb rule in scripts/Makefile.lib is used for embedding a DTB into the .dtb.init.rodata, so that the arch boot code can find it by the __dtb_start symbol.

empty_root.dtb is looked up by its own symbol, so it does not need to be located in the .dtb.init.rodata. It can be moved to the .init.rodata section.

When CONFIG_OF_UNITTEST is enabled, more unittest DTBOs are embedded in the .dtb.init.rodata section. These are also looked up by name and for generic purposes, so they can be moved to the .init.rodata section as well.

I added a wrapper source file, drivers/of/empty_root_dtb.S, because this is the only wrapper used in driver/of/Makefile. I moved the rule for generating *.dtbo.S to drivers/of/unittest-data/Makefile because it is not used anywhere else.

I added the __initdata annotation to the overlay_info data array to avoid section mismatch warnings. The .dtb.init.rodata section is not checked by modpost, even though it is discarded after the early boot stage.

Signed-off-by Masahiro Yamada <[email protected]>

---

 drivers/of/Makefile               |  4 +++-
 drivers/of/empty_root_dtb.S       | 11 +++++++++++
 drivers/of/fdt.c                  |  4 ----
 drivers/of/unittest-data/Makefile | 20 ++++++++++++++++++++
 drivers/of/unittest.c             |  6 +++---
 scripts/Makefile.build            |  2 --
 scripts/Makefile.lib              |  5 +----
 7 files changed, 38 insertions(+), 14 deletions(-)
 create mode 100644 drivers/of/empty_root_dtb.S

drivers/of/Makefile +3 -1

index 251d33532148..c6eb4f6df6e6 100644
@@ -2,7 +2,9 @@
 obj-y = base.o cpu.o device.o module.o platform.o property.o
 obj-$(CONFIG_OF_KOBJ) += kobj.o
 obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
-obj-$(CONFIG_OF_FLATTREE) += fdt.o empty_root.dtb.o
+obj-$(CONFIG_OF_FLATTREE) += fdt.o empty_root_dtb.o
+$(obj)/empty_root_dtb.o: $(obj)/empty_root.dtb
+targets += empty_root.dtb
 obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
 obj-$(CONFIG_OF_ADDRESS)  += address.o

drivers/of/empty_root_dtb.S new file +11 -0

new file mode 100644
index 000000000000..482d6a33f4ae
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#include <asm-generic/vmlinux.lds.h>
+
+.section .init.rodata,"a"
+.balign STRUCT_ALIGNMENT
+.global __dtb_empty_root_begin
+__dtb_empty_root_begin:
+.incbin "drivers/of/empty_root.dtb"
+.global __dtb_empty_root_end
+__dtb_empty_root_end:
+.balign STRUCT_ALIGNMENT

drivers/of/fdt.c +0 -4

index 68103ad230ee..f719256988eb 100644
@@ -32,10 +32,6 @@
 
 #include "of_private.h"
 
-/*
- * __dtb_empty_root_begin[] and __dtb_empty_root_end[] magically created by
- * cmd_dt_S_dtb in scripts/Makefile.lib
- */
 extern uint8_t __dtb_empty_root_begin[];
 extern uint8_t __dtb_empty_root_end[];

drivers/of/unittest-data/Makefile +20 -0

index 01a966e39f23..fe280a36cf88 100644
@@ -104,3 +104,23 @@ static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1)
 static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2)
 
 dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb
+
+# Generate an assembly file to wrap the output of the device tree compiler
+quiet_cmd_wrap_S_dtbo = WRAP    $@
+      cmd_wrap_S_dtbo = {					\
+		echo '\#include <asm-generic/vmlinux.lds.h>';	\
+		echo '.section .init.rodata,"a"';		\
+		echo '.balign STRUCT_ALIGNMENT';		\
+		echo '.global __dtbo_$*_begin';			\
+		echo '__dtbo_$*_begin:';			\
+		echo '.incbin "$<"';				\
+		echo '.global __dtbo_$*_end';			\
+		echo '__dtbo_$*_end:';				\
+		echo '.balign STRUCT_ALIGNMENT';		\
+	} > $@
+
+$(obj)/%.dtbo.S: $(obj)/%.dtbo FORCE
+	$(call if_changed,wrap_S_dtbo)
+
+targets += $(foreach x, dtbo.S dtbo, \
+              $(patsubst %.dtbo.o,%.$(x), $(filter %.dtbo.o, $(obj-y))))

drivers/of/unittest.c +3 -3

index c830f346df45..21ee856e9d74 100644
@@ -1861,7 +1861,7 @@ static int __init unittest_data_add(void)
 	struct device_node *unittest_data_node = NULL, *np;
 	/*
 	 * __dtbo_testcases_begin[] and __dtbo_testcases_end[] are magically
-	 * created by cmd_dt_S_dtbo in scripts/Makefile.lib
+	 * created by cmd_wrap_S_dtbo in drivers/of/unittest-data/Makefile
 	 */
 	extern uint8_t __dtbo_testcases_begin[];
 	extern uint8_t __dtbo_testcases_end[];

@@ -3525,7 +3525,7 @@ static void __init of_unittest_lifecycle(void)
 
 /*
  * __dtbo_##overlay_name##_begin[] and __dtbo_##overlay_name##_end[] are
- * created by cmd_dt_S_dtbo in scripts/Makefile.lib
+ * created by cmd_wrap_S_dtbo in drivers/of/unittest-data/Makefile
  */
 
 #define OVERLAY_INFO_EXTERN(overlay_name) \

@@ -3585,7 +3585,7 @@ OVERLAY_INFO_EXTERN(overlay_bad_symbol);
 OVERLAY_INFO_EXTERN(overlay_bad_unresolved);
 
 /* entries found by name */
-static struct overlay_info overlays[] = {
+static __initdata struct overlay_info overlays[] = {
 	OVERLAY_INFO(overlay_base, -9999, 0),
 	OVERLAY_INFO(overlay, 0, 0),
 	OVERLAY_INFO(overlay_0, 0, 0),

scripts/Makefile.build +0 -2

index a5ac8ed1936f..d0dfdf043ce2 100644
@@ -451,12 +451,10 @@ intermediate_targets = $(foreach sfx, $(2), \
 					$(filter %$(strip $(1)), $(targets))))
 # %.asn1.o <- %.asn1.[ch] <- %.asn1
 # %.dtb.o <- %.dtb.S <- %.dtb <- %.dts
-# %.dtbo.o <- %.dtbo.S <- %.dtbo <- %.dtso
 # %.lex.o <- %.lex.c <- %.l
 # %.tab.o <- %.tab.[ch] <- %.y
 targets += $(call intermediate_targets, .asn1.o, .asn1.c .asn1.h) \
 	   $(call intermediate_targets, .dtb.o, .dtb.S .dtb) \
-	   $(call intermediate_targets, .dtbo.o, .dtbo.S .dtbo) \
 	   $(call intermediate_targets, .lex.o, .lex.c) \
 	   $(call intermediate_targets, .tab.o, .tab.c .tab.h)

scripts/Makefile.lib +1 -4

index 207325eaf1d1..a5584ca72857 100644
@@ -382,7 +382,7 @@ DTC_FLAGS += $(if $(filter $(patsubst $(obj)/%,%,$@), $(base-dtb-y)), -@)
 # Generate an assembly file to wrap the output of the device tree compiler
 quiet_cmd_wrap_S_dtb = WRAP    $@
       cmd_wrap_S_dtb = {								\
-		symbase=__$(patsubst .%,%,$(suffix $<))_$(subst -,_,$(notdir $*));	\
+		symbase=__dtb_$(subst -,_,$(notdir $*));				\
 		echo '\#include <asm-generic/vmlinux.lds.h>';				\
 		echo '.section .dtb.init.rodata,"a"';					\
 		echo '.balign STRUCT_ALIGNMENT';					\

@@ -397,9 +397,6 @@ quiet_cmd_wrap_S_dtb = WRAP    $@
 $(obj)/%.dtb.S: $(obj)/%.dtb FORCE
 	$(call if_changed,wrap_S_dtb)
 
-$(obj)/%.dtbo.S: $(obj)/%.dtbo FORCE
-	$(call if_changed,wrap_S_dtb)
-
 quiet_dtb_check_tag = $(if $(dtb-check-enabled),[C],   )
 cmd_dtb_check = $(if $(dtb-check-enabled),; $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true)

Signature · 1 line

2.43.0

Rob Herring

maintainer

2024-08-27 15:52 UTC #1 lore raw

Quoted 48 lines · Masahiro Yamada

Some architectures can embed DTB(s) in vmlinux. Most of them expect a single DTB in the .dtb.init.rodata section.
For example, RISC-V previously allowed embedding multiple DTBs in vmlinux, but only the first DTB in the .dtb.init.rodata section was used. Which DTB was used was unpredictable, as it depended on the link order (i.e., the order in Makefile).
Commit 2672031b20f6 ("riscv: dts: Move BUILTIN_DTB_SOURCE to common Kconfig") changed the Makefiles to ensure only one DTB is embedded.
However, commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware") introduced another DTB into the .dtb.init.rodata section.
Since then, the symbol dump (sorted by address) for ARCH=riscv nommu_k210_defconfig is as follows:
    00000000801290e0 D __dtb_k210_generic_begin
    00000000801290e0 D __dtb_start
    000000008012b571 D __dtb_k210_generic_end
    000000008012b580 D __dtb_empty_root_begin
    000000008012b5c8 D __dtb_empty_root_end
    000000008012b5e0 D __dtb_end
The .dtb.init.rodata section now contains the following two DTB files:
arch/riscv/boot/dts/canaan/k210_generic.dtb
drivers/of/empty_root.dtb
This is not an immediate problem because the boot code picks up the first DTB. The second one, empty_root.dtb is just ignored.
However, as mentioned above, it is fragile to rely on the link order, as future Makefile changes may break the behavior.
The cmd_wrap_S_dtb rule in scripts/Makefile.lib is used for embedding a DTB into the .dtb.init.rodata, so that the arch boot code can find it by the __dtb_start symbol.
empty_root.dtb is looked up by its own symbol, so it does not need to be located in the .dtb.init.rodata. It can be moved to the .init.rodata section.
When CONFIG_OF_UNITTEST is enabled, more unittest DTBOs are embedded in the .dtb.init.rodata section. These are also looked up by name and for generic purposes, so they can be moved to the .init.rodata section as well.

Quoted 4 lines

I added a wrapper source file, drivers/of/empty_root_dtb.S, because this is the only wrapper used in driver/of/Makefile. I moved the rule for generating *.dtbo.S to drivers/of/unittest-data/Makefile because it is not used anywhere else.

That is likely to change. We've had fixup overlays (fixup an old dt to a new binding) added into the kernel from time to time. There were 2, but they've been removed. However, I just recently suggested adding some new ones[1].

It seems we need a named section when we access the dtb by variable name and an unnamed or boot dt section for the one boot dtb.

Rob

[1] https://lore.kernel.org/all/[email protected]/

Masahiro Yamada

author

2024-08-27 17:32 UTC #2 lore raw

Quoted 55 lines · Rob Herring

Quoted 48 lines · Masahiro Yamada
Some architectures can embed DTB(s) in vmlinux. Most of them expect a single DTB in the .dtb.init.rodata section.
For example, RISC-V previously allowed embedding multiple DTBs in vmlinux, but only the first DTB in the .dtb.init.rodata section was used. Which DTB was used was unpredictable, as it depended on the link order (i.e., the order in Makefile).
Commit 2672031b20f6 ("riscv: dts: Move BUILTIN_DTB_SOURCE to common Kconfig") changed the Makefiles to ensure only one DTB is embedded.
However, commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware") introduced another DTB into the .dtb.init.rodata section.
Since then, the symbol dump (sorted by address) for ARCH=riscv nommu_k210_defconfig is as follows:
    00000000801290e0 D __dtb_k210_generic_begin
    00000000801290e0 D __dtb_start
    000000008012b571 D __dtb_k210_generic_end
    000000008012b580 D __dtb_empty_root_begin
    000000008012b5c8 D __dtb_empty_root_end
    000000008012b5e0 D __dtb_end
The .dtb.init.rodata section now contains the following two DTB files:
arch/riscv/boot/dts/canaan/k210_generic.dtb
drivers/of/empty_root.dtb
This is not an immediate problem because the boot code picks up the first DTB. The second one, empty_root.dtb is just ignored.
However, as mentioned above, it is fragile to rely on the link order, as future Makefile changes may break the behavior.
The cmd_wrap_S_dtb rule in scripts/Makefile.lib is used for embedding a DTB into the .dtb.init.rodata, so that the arch boot code can find it by the __dtb_start symbol.
empty_root.dtb is looked up by its own symbol, so it does not need to be located in the .dtb.init.rodata. It can be moved to the .init.rodata section.
When CONFIG_OF_UNITTEST is enabled, more unittest DTBOs are embedded in the .dtb.init.rodata section. These are also looked up by name and for generic purposes, so they can be moved to the .init.rodata section as well.
Quoted 4 lines
I added a wrapper source file, drivers/of/empty_root_dtb.S, because this is the only wrapper used in driver/of/Makefile. I moved the rule for generating *.dtbo.S to drivers/of/unittest-data/Makefile because it is not used anywhere else.

Quoted 4 lines

That is likely to change. We've had fixup overlays (fixup an old dt to a new binding) added into the kernel from time to time. There were 2, but they've been removed. However, I just recently suggested adding some new ones[1].

Heh, surprising.

Will something like commit 841281fe52a7 come back?

Quoted 2 lines

It seems we need a named section when we access the dtb by variable name and an unnamed or boot dt section for the one boot dtb.

built-in boot dtb --> stay in .dtb.init.rodata
(arch boot code does not know the DTB name)

drivers and drivers/of/ --> can be moved to .init.rodata section
because look-up by name is possible.

So, do you want to keep the build rule in scripts/Makefile.lib, but for a different section?

Quoted 4 lines

Rob
[1] https://lore.kernel.org/all/[email protected]/

Signature · 2 lines

Best Regards
Masahiro Yamada

Rob Herring

maintainer

2024-08-27 20:57 UTC #3 lore raw

Quoted 63 lines · Masahiro Yamada

Quoted 55 lines · Rob Herring
Quoted 48 lines · Masahiro Yamada
Some architectures can embed DTB(s) in vmlinux. Most of them expect a single DTB in the .dtb.init.rodata section.
For example, RISC-V previously allowed embedding multiple DTBs in vmlinux, but only the first DTB in the .dtb.init.rodata section was used. Which DTB was used was unpredictable, as it depended on the link order (i.e., the order in Makefile).
Commit 2672031b20f6 ("riscv: dts: Move BUILTIN_DTB_SOURCE to common Kconfig") changed the Makefiles to ensure only one DTB is embedded.
However, commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware") introduced another DTB into the .dtb.init.rodata section.
Since then, the symbol dump (sorted by address) for ARCH=riscv nommu_k210_defconfig is as follows:
    00000000801290e0 D __dtb_k210_generic_begin
    00000000801290e0 D __dtb_start
    000000008012b571 D __dtb_k210_generic_end
    000000008012b580 D __dtb_empty_root_begin
    000000008012b5c8 D __dtb_empty_root_end
    000000008012b5e0 D __dtb_end
The .dtb.init.rodata section now contains the following two DTB files:
arch/riscv/boot/dts/canaan/k210_generic.dtb
drivers/of/empty_root.dtb
This is not an immediate problem because the boot code picks up the first DTB. The second one, empty_root.dtb is just ignored.
However, as mentioned above, it is fragile to rely on the link order, as future Makefile changes may break the behavior.
The cmd_wrap_S_dtb rule in scripts/Makefile.lib is used for embedding a DTB into the .dtb.init.rodata, so that the arch boot code can find it by the __dtb_start symbol.
empty_root.dtb is looked up by its own symbol, so it does not need to be located in the .dtb.init.rodata. It can be moved to the .init.rodata section.
When CONFIG_OF_UNITTEST is enabled, more unittest DTBOs are embedded in the .dtb.init.rodata section. These are also looked up by name and for generic purposes, so they can be moved to the .init.rodata section as well.
Quoted 4 lines
I added a wrapper source file, drivers/of/empty_root_dtb.S, because this is the only wrapper used in driver/of/Makefile. I moved the rule for generating *.dtbo.S to drivers/of/unittest-data/Makefile because it is not used anywhere else.
Quoted 4 lines
That is likely to change. We've had fixup overlays (fixup an old dt to a new binding) added into the kernel from time to time. There were 2, but they've been removed. However, I just recently suggested adding some new ones[1].

Quoted 3 lines

Heh, surprising.
Will something like commit 841281fe52a7 come back?

Yeah, I expect the QCom DWC3 stuff is going to have N overlays for N SoCs.

Quoted 9 lines

Quoted 2 lines
It seems we need a named section when we access the dtb by variable name and an unnamed or boot dt section for the one boot dtb.
built-in boot dtb --> stay in .dtb.init.rodata
(arch boot code does not know the DTB name)
drivers and drivers/of/ --> can be moved to .init.rodata section
because look-up by name is possible.

Right.

Quoted 2 lines

So, do you want to keep the build rule in scripts/Makefile.lib, but for a different section?

Yes.

Rob

kernel test robot

build bot

2024-08-31 08:49 UTC #4 lore raw

In reply to Masahiro Yamada

kernel test robot noticed the following build warnings:

Hi Masahiro,

kernel test robot noticed the following build warnings:

[auto build test WARNING on masahiroy-kbuild/for-next]
[also build test WARNING on masahiroy-kbuild/fixes linus/master v6.11-rc5] [cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Masahiro-Yamada/of-move-empty_root-and-unittest-data-DTBs-to-init-rodata-section/20240826-205316
base:   https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
patch link:    https://lore.kernel.org/r/20240826124802.1552738-1-masahiroy%40kernel.org
patch subject: [PATCH] of: move empty_root and unittest data DTBs to .init.rodata section
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240831/[email protected]/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240831/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: vmlinux: section mismatch in reference: testdrv_probe+0x30 (section: .text) -> overlays (section: .init.data)

WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_objpool.o

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki