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

[Yonghong Song]

Hi, Peter,

Just wanted to ping again so that you did not miss the email below.
Please let me know your opinion.

Thanks!

Yonghong


On 4/23/18 9:50 AM, Yonghong Song wrote:
> Hi, Peter,
>
> Please see comments below.
>
> On 4/23/18 3:52 AM, Peter Zijlstra wrote:
> > On Fri, Apr 20, 2018 at 11:06:03AM -0700, Yonghong Song wrote:
> > > On 4/20/18 1:19 AM, Peter Zijlstra wrote:
> >
> > > > Hurm, so adding __BPF__ for BPF compiles isn't an option? It seems 
> > > > to me
> > > > having a CPP flag to identify BPF compile context might be useful in
> > > > general.
> > >
> > > With "clang -target bpf", we already have __BPF__ defined.
> > > For tracing, esp. ptrace.h is included, "clang -target <native_arch>" 
> > > where
> > > "-target <native_arch>" can be omitted, is typically used.
> >
> > > The reason is the native architecture header files typically
> > > include a lot of various asm related stuff where "-target bpf" cannot
> > > really handle. We relay on native clang to flush out all these
> > > asm constructs and only bpf program needed stuff survives
> > > reach to backend compiler.
> >
> > So because 'clang -target bpf' is 'broken', you do a work-around using
>
> 'clang -target bpf' is 'broken' in this case because the x86 arch has
> a lot of inline asm's in the header file where bpf target cannot handle.
> For most networking related bpf programs where `asm` is rarely involved,
> `clang -target bpf` works fine most of time.
>
> > 'clang -target <native_arch>'. But because that doesn't set __BPF__ you
>
> `clang -target <native_arch>` should work, regardless of whether __BPF__
> macro is setup or not. The reason it doesn't work now is due to its 
> lacking asm-goto support. So to use `clang -target <native_arch>` is not
> really a workaround for `target bpf`. It by itself should work.
>
> > want to add NO_BPF_WORKAROUND to the kernel instead of adding __BPF__ to
> > your build rules to better mimick -target bpf, which you should be
> > using.
> >
> > How is that sane? Why not use 'clang -target <native_arch> -D__BPF__'
>
> To workaround the asm-goto issue, the suggested macro __BPF__ can be 
> added to user space and kernel. But note that `clang -target 
> <native_arch>` will not define the macro __BPF__, so this requires
> user space change.
>
> Also, to make sure people understand that this is a WORKAROUND for
> asm-goto issue and is not a lasting thing we want to support. I have
> the following change for cpufeature.h:
>
> diff --git a/arch/x86/include/asm/cpufeature.h 
> b/arch/x86/include/asm/cpufeature.h
> index b27da9602a6d..c832118defa1 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)
>
> +#ifndef __BPF_WORKAROUND__
>  Â/*
>   * 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))
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 4d6a6edd4bf6..b229e5090e4a 100644
>
> As mentioned above, user space needs to add this new macro definition.
> Specifically for kernel/samples/bpf:
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 4d6a6edd4bf6..b229e5090e4a 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -255,7 +255,7 @@ $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
>  Â$(obj)/%.o: $(src)/%.c
>  ÂÂÂÂÂÂÂ $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) 
> -I$(obj) \
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -I$(srctree)/tools/testing/selftests/bpf/ \
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -D__KERNEL__ -D__BPF_WORKAROUND__ -Wno-unused-value 
> -Wno-pointer-sign \
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -D__TARGET_ARCH_$(ARCH) 
> -Wno-compare-distinct-pointer-types \
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -Wno-gnu-variable-sized-type-not-at-end \
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -Wno-address-of-packed-member -Wno-tautological-compare \
>
> Please let me know whether this approach is okay to you or not,
> whether the name __BPF_WORKAROUND__ is better than __BPF__ or not, or we 
> could use the earlier approach which does not require user space change.
>
> Thanks!