Re: [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot

[Masahiro Yamada <masahiroy@kernel.org>]

On Thu, Mar 31, 2022 at 10:45 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Nick,
>
> On Wed, 30 Mar 2022 11:04:50 -0700
> Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> > On Mon, Mar 28, 2022 at 7:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > KNOWN ISSUE:
> > >
> > > According to the report from Padmanabha[3] and my analysis [4], the embedded
> > > bootconfig data may not be updated if you do incremental build the kernel
> > > with CONFIG_LTO_CLANG_THIN.
> > >
> > > [3] https://lore.kernel.org/all/20220321183500.GA4065@pswork/T/#u
> > > [4] https://lore.kernel.org/all/20220327115526.cc4b0ff55fc53c97683c3e4d@kernel.org/
> > >
> > > This seems like clang's LTO Thin mode issue. It may not detect the inline
> > > asm depends on external files.
> > >
> > > I think the possible workaround is to split the inline asm which includes
> > > '.incbin' directive into an asm file. But this should be done in another
> > > seires because there are other features which uses '.incbin'. (e.g.
> > > /proc/config.gz)
> >
> > Hi Masami,
> > I saw Padmanabha's report (thanks for the report); sorry for not
> > responding sooner, I've been traveling recently for a funeral.
>
> Oh, sorry about that. I think this is not so hurry.
>
> >
> > Any chance we can use
> >
> > CFLAGS_REMOVE_<file>.o := $(CC_FLAGS_LTO)
> >
> > a la
> > commit d2dcd3e37475 ("x86, cpu: disable LTO for cpu.c")
>
> Hm, this looks good to me. Let me confirm that works.
> (Does this mean the bootconfig.o will be compiled to elf binary?)


Why are you using  ".incbin" in the C file in the first place?


You might have mimicked  kernel/configs.c
but there was a reason;  kernel/configs.c can be a module,
but we cannot put MODULE_LICENSE() in *.S  file.
(commit 13610aa908dcfce77135bb799c0a10d0172da6ba)


In this case, CONFIG_EMBED_BOOT_CONFIG is a bool
option.

Why don't you simply move the asm() part to a separate
bootconfig-data.S ?

Clang lto flags are only passed to *.c files,
so we do not need to be worried about  .incbin in *.S files.


Then, Makefile will be even cleaner (no ifeq-block)


lib-$(CONFIG_BOOT_CONFIG) += bootconfig.o
obj-$(CONFIG_EMBED_BOOT_CONFIG) += bootconfig-data.o

$(obj)/bootconfig-data.o: $(obj)/default.bconf

targets += default.bconf
filechk_defbconf = cat $(or $(real-prereqs), /dev/null)
$(obj)/default.bconf: $(CONFIG_EMBED_BOOT_CONFIG_FILE) FORCE
        $(call filechk,defbconf)





BTW, why lib-$(CONFIG_BOOT_CONFIG),
instead of obj-$(CONFIG_BOOT_CONFIG)  ?



>
> >
> > with a comment linking to
> > https://github.com/ClangBuiltLinux/linux/issues/1618
>
> Thanks for reporting!
>
> >
> > for the Translation Units using .incbin, until we have had more time
> > to triage+fix?
>
> Yes. For this series, I can update with above one, but it doesn't cover the
> other parts. And since this issue is independent from the bootconfig,
> I think we need a wider patch series to mitigate this issue on config.gz
> (and other .incbin users) too.
>
> Thank you,
>
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>




--
Best Regards
Masahiro Yamada