Message-Id: <20220324101048.c929020fd15bc14b50a3fff1@kernel.org>
Date:   Thu, 24 Mar 2022 10:10:48 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Sami Tolvanen <samitolvanen@...gle.com>,
        Padmanabha Srinivasaiah <treasure4paddy@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        Randy Dunlap <rdunlap@...radead.org>,
        Nathan Chancellor <nathan@...nel.org>, llvm@...ts.linux.dev,
        Masahiro Yamada <masahiroy@...nel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] bootconfig: Support embedding a bootconfig file
 in kernel

Hello Nick,

On Wed, 23 Mar 2022 10:11:53 -0700
Nick Desaulniers <ndesaulniers@...gle.com> wrote:

> On Tue, Mar 22, 2022 at 5:16 PM Masami Hiramatsu <mhiramat@...nel.org> wrote:
> >
> > On Tue, 22 Mar 2022 20:02:19 +0100
> > Padmanabha Srinivasaiah <treasure4paddy@...il.com> wrote:
> >
> > > Hello Masami Hiramatsu,
> > >
> > > On Tue, Mar 22, 2022 at 12:03:11PM +0900, Masami Hiramatsu wrote:
> > > > On Mon, 21 Mar 2022 19:35:00 +0100
> > > > Padmanabha Srinivasaiah <treasure4paddy@...il.com> wrote:
> > > >
> > > > > Hello Masami Hiramatsu,
> > > > >
> > > > > On Fri, Mar 18, 2022 at 10:14:45AM +0900, Masami Hiramatsu wrote:
> > > > > > On Wed, 16 Mar 2022 20:16:49 +0100
> > > > > > Padmanabha Srinivasaiah <treasure4paddy@...il.com> wrote:
> > > > > >
> > > > > > > Hello Masami Hiramatsu,
> > > > > > >
> > > > > > > Also noted that a change in default.bconf requries a clean build, is it
> > > > > > > expected behaviour?
> > > > > >
> > > > > > default.bconf will be always updated if CONFIG_EMBED_BOOT_CONFIG=y. So you can
> > > > > > do incremental build. (I tested it with the incremental build environment)
> > > > > >
> > > > >
> > > > > Thanks, your observation made me to further experiment ther incremental build.
> > > > >
> > > > > Below are the observations I have:
> > > > >
> > > > > When I use GCC for a build; yes, the modified default.conf was observed on
> > > > > the target.
> > > > >
> > > > > But when I use clang; either with FULL or THIN LTO, the modified
> > > > > default.conf doesnt get reflected on the target.
> > > >
> > > > Hmm, curious. So you just add 'CC=clang' on the make command line, right?
> > > Yes, CC=clang ARCH=arm64 LLVM=1. As specified here:
> > > https://docs.kernel.org/kbuild/llvm.html.
> 
> You should just need LLVM=1 (and ARCH=arm64) at this point. LLVM=1
> implies CC=clang.

OK.

> 
> Also, here's the start of the lore thread for folks:
> https://lore.kernel.org/linux-doc/164724892075.731226.14103557516176115189.stgit@devnote2/

Thanks for the link!

> 
> > >
> > > > Can you confirm that following line in your build log,
> > > >
> > > >   GEN     lib/default.bconf
> > > >
> > > Yes, I do see above line. Indeed lib/default.bconf will get incremental
> > > change.
> > >
> > >   GEN     lib/default.bconf
> > >   CC      lib/bootconfig.o
> > >   AR      lib/lib.a
> > >
> > > > and the timestamp of lib/bootconfig.o is built after lib/default.bconf file?
> > > >
> > > Yes, verified timestamp for all above artifacts including vmlinux.o.
> > >
> > > ex:
> > > -rw-rw-r-- 1 psrinivasaia psrinivasaia 22K Mar 22 14:50
> > > ../out/lib/bootconfig.o
> > > -rw-rw-r-- 1 psrinivasaia psrinivasaia 355 Mar 22 14:50
> > > ../out/lib/default.bconf
> > > -rw-rw-r-- 1 psrinivasaia psrinivasaia 54M Mar 22 14:50 ../out/vmlinux.o
> > >
> > > As said incremnetal change was refelected in artifact default.bconf.
> > > But not in vmlinux.o/vmlinux, used below command to verify.
> >
> > Interesting! This sounds clang's issue, because the make command rebuilds
> > the object file including new default.bconf, but the linker (lld?)
> > doesn't link it again correctly.
> 
> Sounds like missing FORCE directives in the Makefiles, perhaps?

Hmm, as you can see in my patch, the default.bconf (contents) already
has the FORCE directive as below.

+ifeq ($(CONFIG_EMBED_BOOT_CONFIG),y)
+# Since the specified bootconfig file can be switched, we forcibly update the
+# default.bconf file always.
+$(obj)/default.bconf: FORCE
+       $(call cmd,defbconf)
+
+quiet_cmd_defbconf = GEN     $@
+      cmd_defbconf = cat < /dev/null $(CONFIG_EMBED_BOOT_CONFIG_FILE) > $@
+clean-files    += default.bconf
+$(obj)/bootconfig.o: $(obj)/default.bconf
+endif

And since bootconfig.o depends on the default.bconf, it is at least compiled
as Padmanabha reported above. If I missed something, please tell me.

> 
> Sami, do you recall any issues like this when implementing
> commit dc5723b02e52 ("kbuild: add support for Clang LTO")
> ?
> 
> >
> > >
> > > llvm-objdump  -s -j .init.data ../out/vmlinux
> > >
> > > On target too, /proc/bootconfig shows old data.
> > >
> > > > And is that related to CONFIG_LTO? What happen if CONFIG_LTO=n?
> > > >
> > > Yes;  CONFIG_LTO_NONE=y  issue not observed even with LLVM binutils.
> >
> > And this issue is related to LTO. Maybe LTO ignores the '.init.data'
> > section update. (Perhaps, LTO only checks the function code hash or
> > something like that instead of the timestamp, and ignore whole object
> > file if all of them are not updated.)
> 
> Sounds like this is a result of the above issue?

As I said above, I used FORCE for the default.bconf and confirmed that
the bootconfig.c is compiled (updated). Thus I think FORCE correctly
works.

I'm not sure how LTO is implemented, but if the LTO works based on the
intermediate representation(IR), I guess it doesn't handle inline asm
".incbin" directive in IR. I mean if the linker only checks the inline
asm as a "string" in the .c file, it will miss the update of the contents
of .incbin directive, because inline asm code itself is not changed.
However the object file itself is updated, since the .incbin directive
embeds an external file to the object file.
This is just my guess. I would like to ask LLVM maintainers to help
checking the safeness of using ".incbin" directive with LTO.
(Note that this is also affects other parts which uses .incbin, like
 /proc/config.gz)

Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists