Re: [PATCH 05/11] vmlinux.lds.h: Preserve DTB sections from being discarded after init

[Masahiro Yamada <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:"; \




-- 
Best Regards
Masahiro Yamada