| Message ID | 12d0909b1612fb6d2caa42b4fda5e5ae63d623a3.1724159867.git.andrea.porta@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for RaspberryPi RP1 PCI device using a DT overlay | expand |
Quoting Andrea della Porta (2024-08-20 07:36:07) > The special section .dtb.init.rodata contains dtb and dtbo compiled > as objects inside the kernel and ends up in .init.data sectiion that > will be discarded early after the init phase. This is a problem for > builtin drivers that apply dtb overlay at runtime since this happens > later (e.g. during bus enumeration) and also for modules that should > be able to do it dynamically during runtime. > Move the dtb section to .data. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > include/asm-generic/vmlinux.lds.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > index ad6afc5c4918..3ae9097774b0 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -365,6 +365,7 @@ > TRACE_PRINTKS() \ > BPF_RAW_TP() \ > TRACEPOINT_STR() \ > + KERNEL_DTB() \ The KERNEL_DTB() macro shows the section name is dtb.init.rodata. Can you remove the ".init." part if this isn't initdata anymore? And shouldn't it be in the RO_DATA() macro? It would be nice to keep the initdata properties when this isn't used after init as well. Perhaps we need another macro and/or filename to indicate that the DTB{O} can be thrown away after init/module init.
Hi Stephen, On 12:46 Fri 30 Aug , Stephen Boyd wrote: > Quoting Andrea della Porta (2024-08-20 07:36:07) > > The special section .dtb.init.rodata contains dtb and dtbo compiled > > as objects inside the kernel and ends up in .init.data sectiion that > > will be discarded early after the init phase. This is a problem for > > builtin drivers that apply dtb overlay at runtime since this happens > > later (e.g. during bus enumeration) and also for modules that should > > be able to do it dynamically during runtime. > > Move the dtb section to .data. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > include/asm-generic/vmlinux.lds.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > > index ad6afc5c4918..3ae9097774b0 100644 > > --- a/include/asm-generic/vmlinux.lds.h > > +++ b/include/asm-generic/vmlinux.lds.h > > @@ -365,6 +365,7 @@ > > TRACE_PRINTKS() \ > > BPF_RAW_TP() \ > > TRACEPOINT_STR() \ > > + KERNEL_DTB() \ > > The KERNEL_DTB() macro shows the section name is dtb.init.rodata. Can > you remove the ".init." part if this isn't initdata anymore? And > shouldn't it be in the RO_DATA() macro? Ack. > > It would be nice to keep the initdata properties when this isn't used > after init as well. Perhaps we need another macro and/or filename to > indicate that the DTB{O} can be thrown away after init/module init. We can certainly add some more filename extension that would place the relevant data in a droppable section. Throwing away the dtb/o after init is like the actual KERNEL_DTB macro that is adding teh data to section .init.data, but this would mean t would be useful only at very early init stage, just like for CONFIG_OF_UNITTEST. Throwing after module init could be more difficult though, I think, for example we're not sure when to discard the section in case of deferred modules probe. Many thanks, Andrea
Quoting Andrea della Porta (2024-09-03 05:29:18) > On 12:46 Fri 30 Aug , Stephen Boyd wrote: > > Quoting Andrea della Porta (2024-08-20 07:36:07) > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > > > index ad6afc5c4918..3ae9097774b0 100644 > > > --- a/include/asm-generic/vmlinux.lds.h > > > +++ b/include/asm-generic/vmlinux.lds.h > > > > It would be nice to keep the initdata properties when this isn't used > > after init as well. Perhaps we need another macro and/or filename to > > indicate that the DTB{O} can be thrown away after init/module init. > > We can certainly add some more filename extension that would place the > relevant data in a droppable section. > Throwing away the dtb/o after init is like the actual KERNEL_DTB macro that > is adding teh data to section .init.data, but this would mean t would be > useful only at very early init stage, just like for CONFIG_OF_UNITTEST. > Throwing after module init could be more difficult though, I think, > for example we're not sure when to discard the section in case of deferred > modules probe. > This patch can fix a modpost warning seen in linux-next because I have added DT overlays from KUnit tests while kbuild has properly marked the overlay as initdata that is discarded. See [1] for details. In KUnit I doubt this really matters because most everything runs from __init code (even if it isn't marked that way). I'm thinking that we need to make dtbo Makefile target put the blob in the rodata section so it doesn't get thrown away and leave the builtin DTB as part of init.rodata. Did you already do that? I see the kbuild tree has removed the commit that caused the warning, but I suspect this may still be a problem. See [2] for the next series where overlays applied in the test happen from driver probe so __ref is added. If we simply copy the wrap command and make it so that overlays always go to the .rodata section we should be good. Maybe there's some way to figure out what is being wrapped so we don't have to copy the whole thing. Finally, it's unfortunate that the DTBO is copied when an overlay is applied. We'll waste memory after this patch, so of_overlay_fdt_apply() could be taught to reuse the blob passed in instead of copying it. -----8<---- diff --git a/scripts/Makefile.dtbs b/scripts/Makefile.dtbs index 55998b878e54..070e08082cd3 100644 --- a/scripts/Makefile.dtbs +++ b/scripts/Makefile.dtbs @@ -51,11 +51,25 @@ quiet_cmd_wrap_S_dtb = WRAP $@ echo '.balign STRUCT_ALIGNMENT'; \ } > $@ +quiet_cmd_wrap_S_dtbo = WRAP $@ + cmd_wrap_S_dtbo = { \ + symbase=__$(patsubst .%,%,$(suffix $<))_$(subst -,_,$(notdir $*)); \ + echo '\#include <asm-generic/vmlinux.lds.h>'; \ + echo '.section .rodata,"a"'; \ + echo '.balign STRUCT_ALIGNMENT'; \ + echo ".global $${symbase}_begin"; \ + echo "$${symbase}_begin:"; \ + echo '.incbin "$<" '; \ + echo ".global $${symbase}_end"; \ + echo "$${symbase}_end:"; \ + echo '.balign STRUCT_ALIGNMENT'; \ + } > $@ + $(obj)/%.dtb.S: $(obj)/%.dtb FORCE $(call if_changed,wrap_S_dtb) $(obj)/%.dtbo.S: $(obj)/%.dtbo FORCE - $(call if_changed,wrap_S_dtb) + $(call if_changed,wrap_S_dtbo) # Schema check # --------------------------------------------------------------------------- [1] https://lore.kernel.org/all/20240909112728.30a9bd35@canb.auug.org.au/ [2] https://lore.kernel.org/all/20240910094459.352572-1-masahiroy@kernel.org/
On Sun, Sep 22, 2024 at 5:47 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Andrea della Porta (2024-09-03 05:29:18) > > On 12:46 Fri 30 Aug , Stephen Boyd wrote: > > > Quoting Andrea della Porta (2024-08-20 07:36:07) > > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > > > > index ad6afc5c4918..3ae9097774b0 100644 > > > > --- a/include/asm-generic/vmlinux.lds.h > > > > +++ b/include/asm-generic/vmlinux.lds.h > > > > > > It would be nice to keep the initdata properties when this isn't used > > > after init as well. Perhaps we need another macro and/or filename to > > > indicate that the DTB{O} can be thrown away after init/module init. > > > > We can certainly add some more filename extension that would place the > > relevant data in a droppable section. > > Throwing away the dtb/o after init is like the actual KERNEL_DTB macro that > > is adding teh data to section .init.data, but this would mean t would be > > useful only at very early init stage, just like for CONFIG_OF_UNITTEST. > > Throwing after module init could be more difficult though, I think, > > for example we're not sure when to discard the section in case of deferred > > modules probe. > > > > This patch can fix a modpost warning seen in linux-next because I have > added DT overlays from KUnit tests while kbuild has properly marked the > overlay as initdata that is discarded. See [1] for details. In KUnit I > doubt this really matters because most everything runs from __init code > (even if it isn't marked that way). > > I'm thinking that we need to make dtbo Makefile target put the blob in > the rodata section so it doesn't get thrown away and leave the builtin > DTB as part of init.rodata. Did you already do that? I see the kbuild > tree has removed the commit that caused the warning, but I suspect this > may still be a problem. See [2] for the next series where overlays > applied in the test happen from driver probe so __ref is added. > > If we simply copy the wrap command and make it so that overlays always > go to the .rodata section we should be good. Maybe there's some way to > figure out what is being wrapped so we don't have to copy the whole > thing. > > Finally, it's unfortunate that the DTBO is copied when an overlay is > applied. We'll waste memory after this patch, so of_overlay_fdt_apply() > could be taught to reuse the blob passed in instead of copying it. > > -----8<---- > diff --git a/scripts/Makefile.dtbs b/scripts/Makefile.dtbs > index 55998b878e54..070e08082cd3 100644 > --- a/scripts/Makefile.dtbs > +++ b/scripts/Makefile.dtbs > @@ -51,11 +51,25 @@ quiet_cmd_wrap_S_dtb = WRAP $@ > echo '.balign STRUCT_ALIGNMENT'; \ > } > $@ > > +quiet_cmd_wrap_S_dtbo = WRAP $@ > + cmd_wrap_S_dtbo = { \ > + symbase=__$(patsubst .%,%,$(suffix $<))_$(subst -,_,$(notdir $*)); \ > + echo '\#include <asm-generic/vmlinux.lds.h>'; \ > + echo '.section .rodata,"a"'; \ > + echo '.balign STRUCT_ALIGNMENT'; \ > + echo ".global $${symbase}_begin"; \ > + echo "$${symbase}_begin:"; \ > + echo '.incbin "$<" '; \ > + echo ".global $${symbase}_end"; \ > + echo "$${symbase}_end:"; \ > + echo '.balign STRUCT_ALIGNMENT'; \ > + } > $@ > + > $(obj)/%.dtb.S: $(obj)/%.dtb FORCE > $(call if_changed,wrap_S_dtb) > > $(obj)/%.dtbo.S: $(obj)/%.dtbo FORCE > - $(call if_changed,wrap_S_dtb) > + $(call if_changed,wrap_S_dtbo) > > # Schema check > # --------------------------------------------------------------------------- > > [1] https://lore.kernel.org/all/20240909112728.30a9bd35@canb.auug.org.au/ > [2] https://lore.kernel.org/all/20240910094459.352572-1-masahiroy@kernel.org/ Rather, I'd modify my patch as follows: --- a/scripts/Makefile.dtbs +++ b/scripts/Makefile.dtbs @@ -34,12 +34,14 @@ $(obj)/dtbs-list: $(dtb-y) FORCE # Assembly file to wrap dtb(o) # --------------------------------------------------------------------------- +builtin-dtb-section = $(if $(filter arch/%, $(obj)),.dtb.init.rodata,.rodata) + # 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 $*)); \ echo '\#include <asm-generic/vmlinux.lds.h>'; \ - echo '.section .dtb.init.rodata,"a"'; \ + echo '.section $(builtin-dtb-section),"a"'; \ echo '.balign STRUCT_ALIGNMENT'; \ echo ".global $${symbase}_begin"; \ echo "$${symbase}_begin:"; \
Quoting Masahiro Yamada (2024-09-22 01:14:12) > > Rather, I'd modify my patch as follows: > > --- a/scripts/Makefile.dtbs > +++ b/scripts/Makefile.dtbs > @@ -34,12 +34,14 @@ $(obj)/dtbs-list: $(dtb-y) FORCE > # Assembly file to wrap dtb(o) > # --------------------------------------------------------------------------- > > +builtin-dtb-section = $(if $(filter arch/%, $(obj)),.dtb.init.rodata,.rodata) I think we want to free the empty root dtb that's always builtin. That is in drivers/of/ right? And I worry that an overlay could be in arch/ and then this breaks again. That's why it feels more correct to treat dtbo.o vs. dtb.o differently. Perhaps we can check $(obj) for dtbo vs dtb? Also, modpost code looks for .init* named sections and treats them as initdata already. Can we rename .dtb.init.rodata to .init.dtb.rodata so that modpost can find that?
On Tue, Sep 24, 2024 at 3:13 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Masahiro Yamada (2024-09-22 01:14:12) > > > > Rather, I'd modify my patch as follows: > > > > --- a/scripts/Makefile.dtbs > > +++ b/scripts/Makefile.dtbs > > @@ -34,12 +34,14 @@ $(obj)/dtbs-list: $(dtb-y) FORCE > > # Assembly file to wrap dtb(o) > > # --------------------------------------------------------------------------- > > > > +builtin-dtb-section = $(if $(filter arch/%, $(obj)),.dtb.init.rodata,.rodata) > > I think we want to free the empty root dtb that's always builtin. That > is in drivers/of/ right? drivers/of/empty_root.dts is really small. That is not a big deal even if empty_root.dtb remains in the .rodata section. > And I worry that an overlay could be in arch/ > and then this breaks again. That's why it feels more correct to treat > dtbo.o vs. dtb.o differently. Perhaps we can check $(obj) for dtbo vs > dtb? This is not a problem either. Checking $(obj)/ is temporary. See this later patch: https://lore.kernel.org/linux-kbuild/20240904234803.698424-16-masahiroy@kernel.org/T/#u After my work is completed, DTB and DTBO will go to the .rodata section unconditionally. > Also, modpost code looks for .init* named sections and treats them as > initdata already. Can we rename .dtb.init.rodata to .init.dtb.rodata so > that modpost can find that? My previous patch checked .dtb.init.rodata. I do not mind renaming it to .init.dtb.rodata.
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index ad6afc5c4918..3ae9097774b0 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -365,6 +365,7 @@ TRACE_PRINTKS() \ BPF_RAW_TP() \ TRACEPOINT_STR() \ + KERNEL_DTB() \ KUNIT_TABLE() /* @@ -683,7 +684,6 @@ TIMER_OF_TABLES() \ CPU_METHOD_OF_TABLES() \ CPUIDLE_METHOD_OF_TABLES() \ - KERNEL_DTB() \ IRQCHIP_OF_MATCH_TABLE() \ ACPI_PROBE_TABLE(irqchip) \ ACPI_PROBE_TABLE(timer) \
The special section .dtb.init.rodata contains dtb and dtbo compiled as objects inside the kernel and ends up in .init.data sectiion that will be discarded early after the init phase. This is a problem for builtin drivers that apply dtb overlay at runtime since this happens later (e.g. during bus enumeration) and also for modules that should be able to do it dynamically during runtime. Move the dtb section to .data. Signed-off-by: Andrea della Porta <andrea.porta@suse.com> --- include/asm-generic/vmlinux.lds.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)