[PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[Nick Desaulniers]

Since this file steamrolls KBUILD_CFLAGS, we have to redefine these
symbols. This will prevent warnings in source files in this directory
when Clang supports asm goto.

Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 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
+
 hostprogs-y	:= mkpiggy
 HOST_EXTRACFLAGS += -I$(srctree)/tools/include
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[Nick Desaulniers]

bumping for review.

On Fri, Sep 7, 2018 at 1:26 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Since this file steamrolls KBUILD_CFLAGS, we have to redefine these
> symbols. This will prevent warnings in source files in this directory
> when Clang supports asm goto.
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  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
> +
>  hostprogs-y    := mkpiggy
>  HOST_EXTRACFLAGS += -I$(srctree)/tools/include
>
> --
> 2.19.0.rc2.392.g5ba43deb5a-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[Borislav Petkov]

On Tue, Sep 25, 2018 at 11:41:19AM -0700, Nick Desaulniers wrote:
> bumping for review.
> 
> On Fri, Sep 7, 2018 at 1:26 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > Since this file steamrolls KBUILD_CFLAGS, we have to redefine these
> > symbols.

Why do we have to redefine these symbols?

I don't see arch/x86/boot/ using asm_volatile_goto() and
CC_HAVE_ASM_GOTO anywhere.

> > This will prevent warnings in source files in this directory
> > when Clang supports asm goto.

This statement I can't grok either. Maybe I'm slow or maybe I need more
background info...

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[Nick Desaulniers]

On Tue, Sep 25, 2018 at 12:08 PM Borislav Petkov <bp@suse.de> wrote:
>
> On Tue, Sep 25, 2018 at 11:41:19AM -0700, Nick Desaulniers wrote:
> > bumping for review.
> >
> > On Fri, Sep 7, 2018 at 1:26 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > Since this file steamrolls KBUILD_CFLAGS, we have to redefine these
> > > symbols.
>
> Why do we have to redefine these symbols?
>
> I don't see arch/x86/boot/ using asm_volatile_goto() and
> CC_HAVE_ASM_GOTO anywhere.

They may not explicitly invoke asm_volatile_goto(), but many
translation units under arch/x86/boot/compressed/ share this include
path, triggering this 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]
#warning "Compiler lacks ASM_GOTO support. Add -D __BPF_TRACING__ to
your compiler arguments"
 ^

specifically 6 instances from:
In file included from arch/x86/boot/compressed/early_serial_console.c:1:
In file included from arch/x86/boot/compressed/cmdline.c:2:
In file included from arch/x86/boot/compressed/error.c:7:
In file included from arch/x86/boot/compressed/kaslr.c:29:
In file included from arch/x86/boot/compressed/kaslr_64.c:22:
In file included from arch/x86/boot/compressed/misc.c:15:

Note that the check in arch/x86/include/asm/cpufeature.h has the check:

143 #if defined(__clang__) && !defined(CC_HAVE_ASM_GOTO)

so this is hidden from GCC.  I can include this information in the
commit message for a v2 if you'd like?

>
> > > This will prevent warnings in source files in this directory
> > > when Clang supports asm goto.
>
> This statement I can't grok either. Maybe I'm slow or maybe I need more
> background info...

We're in the process of adding asm goto support to Clang.  I'm helping
test by working through compiling the Linux kernel with Clang for
x86_64.  The patch set still has some kinks we're working out, but
having this fix in place will make for smoother sailing once we're
good to go, and we would appreciate having it.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[Nick Desaulniers]

On Tue, Sep 25, 2018 at 2:08 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Sep 25, 2018 at 12:08 PM Borislav Petkov <bp@suse.de> wrote:
> >
> > On Tue, Sep 25, 2018 at 11:41:19AM -0700, Nick Desaulniers wrote:
> > > bumping for review.
> > >
> > > On Fri, Sep 7, 2018 at 1:26 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > >
> > > > Since this file steamrolls KBUILD_CFLAGS, we have to redefine these
> > > > symbols.

Orthogonally, do you know *why* this Makefile overwrites
KBUILD_CFLAGS?  We take great care to set various compiler flags in
the top level Makefile, so to reset them lower in the tree seems
troublesome on first glance.  Take for instance the fact that GCC
changed the default C standard from gnu89 to gnu11 in GCC 5.  So a
kernel dev with gcc 5+ is now building a kernel with some translation
units built as gnu89, and some as gnu11.

> >
> > Why do we have to redefine these symbols?
> >
> > I don't see arch/x86/boot/ using asm_volatile_goto() and
> > CC_HAVE_ASM_GOTO anywhere.
>
> They may not explicitly invoke asm_volatile_goto(), but many
> translation units under arch/x86/boot/compressed/ share this include
> path, triggering this 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]
> #warning "Compiler lacks ASM_GOTO support. Add -D __BPF_TRACING__ to
> your compiler arguments"
>  ^
>
> specifically 6 instances from:
> In file included from arch/x86/boot/compressed/early_serial_console.c:1:
> In file included from arch/x86/boot/compressed/cmdline.c:2:
> In file included from arch/x86/boot/compressed/error.c:7:
> In file included from arch/x86/boot/compressed/kaslr.c:29:
> In file included from arch/x86/boot/compressed/kaslr_64.c:22:
> In file included from arch/x86/boot/compressed/misc.c:15:
>
> Note that the check in arch/x86/include/asm/cpufeature.h has the check:
>
> 143 #if defined(__clang__) && !defined(CC_HAVE_ASM_GOTO)
>
> so this is hidden from GCC.  I can include this information in the
> commit message for a v2 if you'd like?
>
> >
> > > > This will prevent warnings in source files in this directory
> > > > when Clang supports asm goto.
> >
> > This statement I can't grok either. Maybe I'm slow or maybe I need more
> > background info...
>
> We're in the process of adding asm goto support to Clang.  I'm helping
> test by working through compiling the Linux kernel with Clang for
> x86_64.  The patch set still has some kinks we're working out, but
> having this fix in place will make for smoother sailing once we're
> good to go, and we would appreciate having it.
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[Borislav Petkov]

On Tue, Sep 25, 2018 at 02:13:16PM -0700, Nick Desaulniers wrote:
> Orthogonally, do you know *why* this Makefile overwrites
> KBUILD_CFLAGS?  We take great care to set various compiler flags in
> the top level Makefile, so to reset them lower in the tree seems
> troublesome on first glance.  Take for instance the fact that GCC
> changed the default C standard from gnu89 to gnu11 in GCC 5.  So a
> kernel dev with gcc 5+ is now building a kernel with some translation
> units built as gnu89, and some as gnu11.

Right, so I'd defer to hpa for a definitive answer here but AFAICT, the
reasoning is that because this is the special relocatable compressed
kernel and as such, it is redefining build flags completely so that
nothing leaks from kernel proper.

Which is funny because what happens is, that something *does* leak,
and what you're trying to fix is one incarnation. Because we do use
facilities from include/linux/. And looking at your error:

#warning "Compiler lacks ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"

I don't think it does matter to the compressed kernel whether the
compiler has asm goto or not.

And looking at arch/x86/boot/Makefile, it does:

KBUILD_CFLAGS   := $(REALMODE_CFLAGS) -D_SETUP

and that _SETUP guard is kinda used to separate that early boot code
from kernel proper.

And I wonder if adding such guard to arch/x86/boot/compressed/Makefile
and then protect the asm goto check in arch/x86/include/asm/cpufeature.h
with it, would be more in line with how this whole machinery should
work...

hpa?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[H. Peter Anvin]

On 09/26/18 02:08, Borislav Petkov wrote:
> On Tue, Sep 25, 2018 at 02:13:16PM -0700, Nick Desaulniers wrote:
>> Orthogonally, do you know *why* this Makefile overwrites
>> KBUILD_CFLAGS?  We take great care to set various compiler flags in
>> the top level Makefile, so to reset them lower in the tree seems
>> troublesome on first glance.  Take for instance the fact that GCC
>> changed the default C standard from gnu89 to gnu11 in GCC 5.  So a
>> kernel dev with gcc 5+ is now building a kernel with some translation
>> units built as gnu89, and some as gnu11.
> 
> Right, so I'd defer to hpa for a definitive answer here but AFAICT, the
> reasoning is that because this is the special relocatable compressed
> kernel and as such, it is redefining build flags completely so that
> nothing leaks from kernel proper.
> 
> Which is funny because what happens is, that something *does* leak,
> and what you're trying to fix is one incarnation. Because we do use
> facilities from include/linux/. And looking at your error:
> 
> #warning "Compiler lacks ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"
> 
> I don't think it does matter to the compressed kernel whether the
> compiler has asm goto or not.
> 
> And looking at arch/x86/boot/Makefile, it does:
> 
> KBUILD_CFLAGS   := $(REALMODE_CFLAGS) -D_SETUP
> 
> and that _SETUP guard is kinda used to separate that early boot code
> from kernel proper.
> 
> And I wonder if adding such guard to arch/x86/boot/compressed/Makefile
> and then protect the asm goto check in arch/x86/include/asm/cpufeature.h
> with it, would be more in line with how this whole machinery should
> work...
> 

It overrides CFLAGS because it is by necessity a 32-bit compile (16
bits, really, but that's done at the assembly level only.)  Some 64-bit
flags cause the 32-bit compile to error out.

	-hpa

Re: [PATCH] x86/boot: define CC_HAVE_ASM_GOTO

[Borislav Petkov]

On Tue, Sep 25, 2018 at 02:08:04PM -0700, Nick Desaulniers wrote:

<snip>

> so this is hidden from GCC.  I can include this information in the
> commit message for a v2 if you'd like?

Yes please - in a condensed form. But explaining what happens makes it
much more clear, thanks for taking the time.

> We're in the process of adding asm goto support to Clang.  I'm helping
> test by working through compiling the Linux kernel with Clang for
> x86_64.  The patch set still has some kinks we're working out, but
> having this fix in place will make for smoother sailing once we're
> good to go, and we would appreciate having it.

I see.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.