Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180927204800.32210-1-ndesaulniers@google.com> <20180927215148.GE19687@zn.tnic>
In-Reply-To: <20180927215148.GE19687@zn.tnic>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Thu, 27 Sep 2018 15:17:41 -0700
Message-ID: <CAKwvOdmXcztP542kADhyJYN2=Fk3qyXif_MCs=kqPGE8QtTjvQ@mail.gmail.com>
Subject: Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO
To:     bp@alien8.de
Cc:     mingo@redhat.com, Thomas Gleixner <tglx@linutronix.de>,
        hpa@zytor.com, x86@kernel.org,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        Kees Cook <keescook@chromium.org>,
        Cao jin <caoj.fnst@cn.fujitsu.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Sep 27, 2018 at 2:51 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Sep 27, 2018 at 01:47:58PM -0700, ndesaulniers@google.com wrote:
> > Early prototypes of Clang with asm goto support produce 6 instances of
> > the following warning:
> >
> > In file included from arch/x86/boot/compressed/misc.h:20:
> > In file included from ./include/linux/elf.h:5:
> > In file included from ./arch/x86/include/asm/elf.h:8:
> > In file included from ./include/linux/thread_info.h:38:
> > In file included from ./arch/x86/include/asm/thread_info.h:53:
> > ./arch/x86/include/asm/cpufeature.h:150:2: warning: "Compiler lacks
> > ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"
> > [-W#warnings]
> > your compiler arguments"
> >  ^
> >
> > Since 6 files under arch/x86/boot/compressed/ include
> > arch/x86/boot/compressed/misc.h AND
> > arch/x86/boot/compressed/Makefile happens to redefine KBUILD_CFLAGS,
> > which set these variables in the top level MAKEFILE.
> >
> > Suggested-by: Borislav Petkov <bp@suse.de>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > v1 -> v2:
> >   Updated commit message to provide more context as per Borislav.
> >
> >  arch/x86/boot/compressed/Makefile | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> > index 28764dacf018..158c0b4e178a 100644
> > --- a/arch/x86/boot/compressed/Makefile
> > +++ b/arch/x86/boot/compressed/Makefile
> > @@ -56,6 +56,13 @@ KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \
> >  endif
> >  LDFLAGS_vmlinux := -T
> >
> > +# check for 'asm goto'
> > +ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
> > +  CC_HAVE_ASM_GOTO := 1
> > +  KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> > +  KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
> > +endif
>
> I would still like to know why can't we do the -D_SETUP thing here:
> https://lkml.kernel.org/r/20180926090841.GC5745@zn.tnic

That's another case that I look at and wonder "why does this exist?"
The _SETUP guard exists in only one place:
$ grep -rP 'ifdef\s+_SETUP'
arch/x86/boot/cpucheck.c:#ifdef _SETUP

which is already under arch/x86/boot/. arch/x86/boot/Makefile
unconditionally sets -D_SETUP, so what/who are we guarding against?
Looks like a guard that's ALWAYS true (and thus could be removed).

>
> instead of polluting this Makefile with defines which are not really
> needed in the compressed kernel build, except to silence build warnings.
>
> I mean, we can perpetuate that ugly hack and do:
>
> #define __BPF_TRACING__
>
> here in arch/x86/boot/compressed/misc.h which we could kill once clang
> can do asm goto...

Or, or... we don't redefine KBUILD_CFLAGS in arch/x86/boot/Makefile
(or any Makefile other than the top level one), and simply filter out
the flags we DONT want, a la:

drivers/firmware/efi/libstub/Makefile:
 16 cflags-$(CONFIG_ARM64)    := $(subst -pg,,$(KBUILD_CFLAGS)) ...

ie, using Make's subst function to copy KBUILD_CFLAGS, filter out
results, then use that for cflags-y.
https://www.gnu.org/software/make/manual/html_node/Text-Functions.html

I'm curious to know Masahiro's thoughts on this?  I can't help but
shake the feeling that reassigning KBUILD_CFLAGS should be considered
an anti-pattern and warned from checkpatch.pl.  For the reasons
enumerated above AND in v1:
https://lore.kernel.org/lkml/CAKwvOdmLSVH7EVGY1ExU1Fh_hvL=FUzhq-80snDfZ+QhCT2FOA@mail.gmail.com/
(though there may be additional context from hpa answering
https://lore.kernel.org/lkml/20180926090841.GC5745@zn.tnic/).

Relying on the compiler's default/implicit C standard (which changed
in gcc 5) for parts of the kernel but not others I feel like should be
a big red flag.

Shall I prototype up what such a change might look like (not
reassigning KBUILD_CFLAGS in arch/x86/boot/Makefile)?  Maybe it's
harder/uglier than I imagine?
-- 
Thanks,
~Nick Desaulniers