[PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[Yonghong Song]

Commit d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")
removed X86_FAST_FEATURE_TESTS and make macro static_cpu_has() always
use __always_inline function _static_cpu_has() funciton.
The static_cpu_has() uses gcc feature asm_volatile_goto construct,
which is not supported by clang.

Currently, for BPF programs written in C, the only widely-supported
compiler is clang. Because of clang lacking support
of asm_volatile_goto construct, if you try to compile
bpf programs under samples/bpf/,
   $ make -j20 && make headers_install && make samples/bpf/
you will see a lot of failures like below:

  =========================
  clang  -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/7/include \
         -I/home/yhs/work/bpf-next/arch/x86/include \
         -I./arch/x86/include/generated  -I/home/yhs/work/bpf-next/include \
         -I./include -I/home/yhs/work/bpf-next/arch/x86/include/uapi \
         -I./arch/x86/include/generated/uapi \
         -I/home/yhs/work/bpf-next/include/uapi -I./include/generated/uapi \
         -include /home/yhs/work/bpf-next/include/linux/kconfig.h
         -Isamples/bpf \
	 -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf/ \
	 -D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
	 -D__TARGET_ARCH_x86 -Wno-compare-distinct-pointer-types \
	 -Wno-gnu-variable-sized-type-not-at-end \
	 -Wno-address-of-packed-member -Wno-tautological-compare \
	 -Wno-unknown-warning-option  \
	 -O2 -emit-llvm -c /home/yhs/work/bpf-next/samples/bpf/xdp_redirect_cpu_kern.c \
         -o -| llc -march=bpf -filetype=obj -o samples/bpf/xdp_redirect_cpu_kern.o
  In file included from /home/yhs/work/bpf-next/samples/bpf/xdp_redirect_cpu_kern.c:10:
  In file included from /home/yhs/work/bpf-next/include/uapi/linux/in.h:24:
  In file included from /home/yhs/work/bpf-next/include/linux/socket.h:8:
  In file included from /home/yhs/work/bpf-next/include/linux/uio.h:13:
  In file included from /home/yhs/work/bpf-next/include/linux/thread_info.h:38:
  In file included from /home/yhs/work/bpf-next/arch/x86/include/asm/thread_info.h:53:
  /home/yhs/work/bpf-next/arch/x86/include/asm/cpufeature.h:150:2: error: 'asm goto' constructs are not supported yet
        asm_volatile_goto("1: jmp 6f\n"
        ^
  /home/yhs/work/bpf-next/include/linux/compiler-gcc.h:296:42: note: expanded from macro 'asm_volatile_goto'
  #define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
                                           ^
  1 error generated.
  =========================
  ...
  In file included from /home/yhs/work/bpf-next/samples/bpf/tracex4_kern.c:7:
  In file included from /home/yhs/work/bpf-next/include/linux/ptrace.h:6:
  In file included from /home/yhs/work/bpf-next/include/linux/sched.h:14:
  In file included from /home/yhs/work/bpf-next/include/linux/pid.h:5:
  In file included from /home/yhs/work/bpf-next/include/linux/rculist.h:11:
  In file included from /home/yhs/work/bpf-next/include/linux/rcupdate.h:40:
  In file included from /home/yhs/work/bpf-next/include/linux/preempt.h:81:
  In file included from /home/yhs/work/bpf-next/arch/x86/include/asm/preempt.h:7:
  In file included from /home/yhs/work/bpf-next/include/linux/thread_info.h:38:
  In file included from /home/yhs/work/bpf-next/arch/x86/include/asm/thread_info.h:53:
  /home/yhs/work/bpf-next/arch/x86/include/asm/cpufeature.h:150:2: error: 'asm goto' constructs are not supported yet
  ...
  =========================

This patch adds a preprocessor guard CC_HAVE_ASM_GOTO around the
asm_volatile_goto based static_cpu_has(). If CC_HAVE_ASM_GOTO
is false, it will fall back to boot_cpu_has().

The CC_HAVE_ASM_GOTO is defined in top-level Makefile.
The kernel headers accessed by clang compiler do not define
this macro, so the above compilation error will disappear.
If later clang compiler starts to support asm_volatile_goto,
the change in this patch can be reverted and there should
be no impact on BPF compilation.

Fixes: d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 arch/x86/include/asm/cpufeature.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index b27da96..734bbce 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -140,6 +140,7 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
 
 #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)
 
+#ifdef CC_HAVE_ASM_GOTO
 /*
  * Static testing of CPU features.  Used the same as boot_cpu_has().
  * These will statically patch the target code for additional
@@ -195,6 +196,9 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
 		boot_cpu_has(bit) :				\
 		_static_cpu_has(bit)				\
 )
+#else
+#define static_cpu_has(bit)		boot_cpu_has(bit)
+#endif
 
 #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
 #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
-- 
2.9.5

Re: [PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[Peter Zijlstra]

On Tue, Apr 10, 2018 at 01:42:59PM -0700, Yonghong Song wrote:
> Commit d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")
> removed X86_FAST_FEATURE_TESTS and make macro static_cpu_has() always
> use __always_inline function _static_cpu_has() funciton.
> The static_cpu_has() uses gcc feature asm_volatile_goto construct,
> which is not supported by clang.

There will be more unconditional asm-goto usage, clang is in the process
of growing asm-goto.

Re: [PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[Alexei Starovoitov]

On 4/10/18 2:07 PM, Peter Zijlstra wrote:
> On Tue, Apr 10, 2018 at 01:42:59PM -0700, Yonghong Song wrote:
>> Commit d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")
>> removed X86_FAST_FEATURE_TESTS and make macro static_cpu_has() always
>> use __always_inline function _static_cpu_has() funciton.
>> The static_cpu_has() uses gcc feature asm_volatile_goto construct,
>> which is not supported by clang.
>
> There will be more unconditional asm-goto usage, clang is in the process
> of growing asm-goto.

Eventually yes, but we need a solution today.
Right now all of bpf based tracing is broken because we have to
compile on the fly with clang.
Instead of
#ifdef CC_HAVE_ASM_GOTO
we can replace it with
#ifndef __BPF__
or some other name,
but it's more hacky, since we'd need add -D__BPF__ to samples/bpf
and to all other places that use native clang to compile.

Whereas with 'ifdef CC_HAVE_ASM_GOTO' everything works as-is.
Top kernel makefile defines it and kernel is still compiled with
asm-goto, whereas native clang path for the purpose of compiling
bpf progs doesn't have this define and also fine.

imo this patch is the best solution _today_.

bpf compilation needs kernel headers only. If later you add
unconditional asm-goto to .c, it's all fine.

Re: [PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[Peter Zijlstra]

On Tue, Apr 10, 2018 at 02:28:04PM -0700, Alexei Starovoitov wrote:
> Instead of
> #ifdef CC_HAVE_ASM_GOTO
> we can replace it with
> #ifndef __BPF__
> or some other name,

I would prefer the BPF specific hack; otherwise we might be encouraging
people to build the kernel proper without asm-goto.

Re: [PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[Alexei Starovoitov]

On 4/13/18 11:19 AM, Peter Zijlstra wrote:
> On Tue, Apr 10, 2018 at 02:28:04PM -0700, Alexei Starovoitov wrote:
>> Instead of
>> #ifdef CC_HAVE_ASM_GOTO
>> we can replace it with
>> #ifndef __BPF__
>> or some other name,
>
> I would prefer the BPF specific hack; otherwise we might be encouraging
> people to build the kernel proper without asm-goto.
>

I don't understand this concern.

1.
arch/x86/Makefile does
ifndef CC_HAVE_ASM_GOTO
   $(error Compiler lacks asm-goto support.)
endif

which is pretty strong statement of the kernel direction.

2.
Even with this patch that adds #ifdef CC_HAVE_ASM_GOTO back
the x86 arch still needs asm-goto in the compiler to be built.
As far as I can see there are other places where asm-goto
is open coded.
So there is no 'encouraging'.

Whereas if we do bpf specific marco we'd need to explain that
to all bpf users and they would need to fix their user space scripts.
Amount of user space breakage is unknown at this point.

Re: [PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[Peter Zijlstra]

On Fri, Apr 13, 2018 at 01:42:14PM -0700, Alexei Starovoitov wrote:
> On 4/13/18 11:19 AM, Peter Zijlstra wrote:
> > On Tue, Apr 10, 2018 at 02:28:04PM -0700, Alexei Starovoitov wrote:
> > > Instead of
> > > #ifdef CC_HAVE_ASM_GOTO
> > > we can replace it with
> > > #ifndef __BPF__
> > > or some other name,
> > 
> > I would prefer the BPF specific hack; otherwise we might be encouraging
> > people to build the kernel proper without asm-goto.
> > 
> 
> I don't understand this concern.

The thing is; this will be a (temporary) BPF specific hack. Hiding it
behind something that looks 'normal' (CC_HAVE_ASM_GOTO) is just not
right.

Re: [PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO

[Yonghong Song]

On 4/14/18 3:11 AM, Peter Zijlstra wrote:
> On Fri, Apr 13, 2018 at 01:42:14PM -0700, Alexei Starovoitov wrote:
>> On 4/13/18 11:19 AM, Peter Zijlstra wrote:
>>> On Tue, Apr 10, 2018 at 02:28:04PM -0700, Alexei Starovoitov wrote:
>>>> Instead of
>>>> #ifdef CC_HAVE_ASM_GOTO
>>>> we can replace it with
>>>> #ifndef __BPF__
>>>> or some other name,
>>>
>>> I would prefer the BPF specific hack; otherwise we might be encouraging
>>> people to build the kernel proper without asm-goto.
>>>
>>
>> I don't understand this concern.
> 
> The thing is; this will be a (temporary) BPF specific hack. Hiding it
> behind something that looks 'normal' (CC_HAVE_ASM_GOTO) is just not
> right.

This is a fair concern. I will use a different macro and send v2 soon.
Thanks.