diff mbox series

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

Message ID 20240826124802.1552738-1-masahiroy@kernel.org
State Changes Requested
Headers show
Series of: move empty_root and unittest data DTBs to .init.rodata section | expand

Checks

Context Check Description
robh/checkpatch warning total: 0 errors, 3 warnings, 107 lines checked
robh/patch-applied success

Commit Message

Masahiro Yamada Aug. 26, 2024, 12:48 p.m. UTC 
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 <masahiroy@kernel.org>
---

 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

Comments

Rob Herring Aug. 27, 2024, 3:51 p.m. UTC | #1 
On Mon, Aug 26, 2024 at 09:48:01PM +0900, Masahiro Yamada wrote:
> 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.

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/20240812212139.GA1797954-robh@kernel.org/
Masahiro Yamada Aug. 27, 2024, 5:32 p.m. UTC | #2 
On Wed, Aug 28, 2024 at 12:52 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Aug 26, 2024 at 09:48:01PM +0900, Masahiro Yamada wrote:
> > 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.
>
> 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?




> 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?



> Rob
>
> [1] https://lore.kernel.org/all/20240812212139.GA1797954-robh@kernel.org/
>
Rob Herring Aug. 27, 2024, 8:57 p.m. UTC | #3 
On Tue, Aug 27, 2024 at 12:32 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Aug 28, 2024 at 12:52 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Aug 26, 2024 at 09:48:01PM +0900, Masahiro Yamada wrote:
> > > 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.
> >
> > 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?

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

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

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

Yes.

Rob
kernel test robot Aug. 31, 2024, 8:49 a.m. UTC | #4 
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/202408311656.JgYqHrnC-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240831/202408311656.JgYqHrnC-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408311656.JgYqHrnC-lkp@intel.com/

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
diff mbox series

Patch

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 251d33532148..c6eb4f6df6e6 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -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
diff --git a/drivers/of/empty_root_dtb.S b/drivers/of/empty_root_dtb.S
new file mode 100644
index 000000000000..482d6a33f4ae
--- /dev/null
+++ b/drivers/of/empty_root_dtb.S
@@ -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
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 68103ad230ee..f719256988eb 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -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[];
 
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 01a966e39f23..fe280a36cf88 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -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))))
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index c830f346df45..21ee856e9d74 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -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),
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index a5ac8ed1936f..d0dfdf043ce2 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -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)
 
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 207325eaf1d1..a5584ca72857 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -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)