bpf: handling non BPF register names in inline assembly with -target bpf

bpf: handling non BPF register names in inline assembly with -target bpf

[Yonghong Song]

Hi, Arnaldo,

When I studied the bpf compilation issue with latest linus/net-next 
kernel (https://patchwork.kernel.org/patch/10333829/), an alternative
approach I tried is to use __BPF__ macro. The following patch
introduced "#ifndef __BPF__" in arch/x86/include/asm/asm.h for
some inline assembly related to x86 "esp" register name.

==========
  commit ca26cffa4e4aaeb09bb9e308f95c7835cb149248
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon Dec 4 13:08:47 2017 -0300

     x86/asm: Allow again using asm.h when building for the 'bpf' clang 
target

     Up to f5caf621ee35 ("x86/asm: Fix inline asm call constraints for 
Clang")
     we were able to use x86 headers to build to the 'bpf' clang target, as
     done by the BPF code in tools/perf/.
  ...
diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 219faae..386a690 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -136,6 +136,7 @@
  #endif

  #ifndef __ASSEMBLY__
+#ifndef __BPF__
  /*
   * This output constraint should be used for any inline asm which has 
a "call"
   * instruction.  Otherwise the asm may be inserted before the frame 
pointer
@@ -145,5 +146,6 @@
  register unsigned long current_stack_pointer asm(_ASM_SP);
  #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
  #endif
+#endif
...
==========

I just landed a clang patch (clang 7.0.0 trunk)
https://reviews.llvm.org/rL329823
which will permit bpf clang target to accept ANY register
names. In this case, the inline assembly will be accepted by clang
and will be thrown away since variable current_stack_pointer is
not used in bpf programs.

If the inline assembly is indeed for BPF program, later llc AsmParser 
will do syntax and semantics checking again.

With the above clang patch, the above "#ifndef __BPF__" can be removed.
You can decide when is the appropriate time to use latest clang compiler
and remove the above "#ifndef __BPF__".

Thanks!

Yonghong

Re: bpf: handling non BPF register names in inline assembly with -target bpf

[Arnaldo Carvalho de Melo]

Em Wed, Apr 11, 2018 at 09:37:46AM -0700, Yonghong Song escreveu:
> Hi, Arnaldo,
 
> When I studied the bpf compilation issue with latest linus/net-next
> kernel (https://patchwork.kernel.org/patch/10333829/), an alternative
> approach I tried is to use __BPF__ macro.

You mean you used an alternative approach that does _not_ use the
__BPF__ macro, right? I looked at the patch and yeah, looks sane as
well, since the kernel build process already defines that
CC_HAVE_ASM_GOTO, checking if gcc has that feature, etc.

> The following patch introduced "#ifndef __BPF__" in
> arch/x86/include/asm/asm.h for some inline assembly related to x86
> "esp" register name.
 

> ==========
>  commit ca26cffa4e4aaeb09bb9e308f95c7835cb149248
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Mon Dec 4 13:08:47 2017 -0300
> 
>     x86/asm: Allow again using asm.h when building for the 'bpf'
> clang target
> 
>     Up to f5caf621ee35 ("x86/asm: Fix inline asm call constraints
> for Clang")
>     we were able to use x86 headers to build to the 'bpf' clang target, as
>     done by the BPF code in tools/perf/.
>  ...
> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
> index 219faae..386a690 100644
> --- a/arch/x86/include/asm/asm.h
> +++ b/arch/x86/include/asm/asm.h
> @@ -136,6 +136,7 @@
>  #endif
> 
>  #ifndef __ASSEMBLY__
> +#ifndef __BPF__
>  /*
>   * This output constraint should be used for any inline asm which
> has a "call"
>   * instruction.  Otherwise the asm may be inserted before the frame
> pointer
> @@ -145,5 +146,6 @@
>  register unsigned long current_stack_pointer asm(_ASM_SP);
>  #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>  #endif
> +#endif
> ...
> ==========
> 
> I just landed a clang patch (clang 7.0.0 trunk)
> https://reviews.llvm.org/rL329823
> which will permit bpf clang target to accept ANY register
> names. In this case, the inline assembly will be accepted by clang
> and will be thrown away since variable current_stack_pointer is
> not used in bpf programs.

Ok, then that ifndef __BPF__ above will not be needed anymore, but only
people with clang > that version will be able to build tools/perf/
 
> If the inline assembly is indeed for BPF program, later llc
> AsmParser will do syntax and semantics checking again.
> 
> With the above clang patch, the above "#ifndef __BPF__" can be removed.
> You can decide when is the appropriate time to use latest clang compiler
> and remove the above "#ifndef __BPF__".

So are you proposing that we have something similar to that
CC_HAVE_ASM_GOTO check in the kernel main Makefile, to define something
like CC_HAVE_ASM_REGS (or some better name), i.e. something like:

# check for 'asm(_ASM_SP)'
ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/cc-asm-regs.sh
$(CC) $(KBUILD_CFLAGS)), y)
  CC_HAVE_ASM_REGS := 1
  KBUILD_CFLAGS += -DCC_HAVE_ASM_REGS
  KBUILD_AFLAGS += -DCC_HAVE_ASM_REGS
endif

?

- Arnaldo

Re: bpf: handling non BPF register names in inline assembly with -target bpf

[Yonghong Song]

On 4/11/18 11:39 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 11, 2018 at 09:37:46AM -0700, Yonghong Song escreveu:
>> Hi, Arnaldo,
>   
>> When I studied the bpf compilation issue with latest linus/net-next
>> kernel (https://patchwork.kernel.org/patch/10333829/), an alternative
>> approach I tried is to use __BPF__ macro.
> 
> You mean you used an alternative approach that does _not_ use the
> __BPF__ macro, right? I looked at the patch and yeah, looks sane as

Right.

> well, since the kernel build process already defines that
> CC_HAVE_ASM_GOTO, checking if gcc has that feature, etc.
> 
>> The following patch introduced "#ifndef __BPF__" in
>> arch/x86/include/asm/asm.h for some inline assembly related to x86
>> "esp" register name.
>   
> 
>> ==========
>>   commit ca26cffa4e4aaeb09bb9e308f95c7835cb149248
>> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Date:   Mon Dec 4 13:08:47 2017 -0300
>>
>>      x86/asm: Allow again using asm.h when building for the 'bpf'
>> clang target
>>
>>      Up to f5caf621ee35 ("x86/asm: Fix inline asm call constraints
>> for Clang")
>>      we were able to use x86 headers to build to the 'bpf' clang target, as
>>      done by the BPF code in tools/perf/.
>>   ...
>> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>> index 219faae..386a690 100644
>> --- a/arch/x86/include/asm/asm.h
>> +++ b/arch/x86/include/asm/asm.h
>> @@ -136,6 +136,7 @@
>>   #endif
>>
>>   #ifndef __ASSEMBLY__
>> +#ifndef __BPF__
>>   /*
>>    * This output constraint should be used for any inline asm which
>> has a "call"
>>    * instruction.  Otherwise the asm may be inserted before the frame
>> pointer
>> @@ -145,5 +146,6 @@
>>   register unsigned long current_stack_pointer asm(_ASM_SP);
>>   #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>>   #endif
>> +#endif
>> ...
>> ==========
>>
>> I just landed a clang patch (clang 7.0.0 trunk)
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_rL329823&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=1D1Hmeg3QWPTsbr6rIHgNpk8Fiqk1jkAvrluQQsjk2Y&s=P5EaOqBvaqpDpx3di0Qm1W2fMKaooJJ7b6IkiGD6NU8&e=
>> which will permit bpf clang target to accept ANY register
>> names. In this case, the inline assembly will be accepted by clang
>> and will be thrown away since variable current_stack_pointer is
>> not used in bpf programs.
> 
> Ok, then that ifndef __BPF__ above will not be needed anymore, but only
> people with clang > that version will be able to build tools/perf/

Yes.

I have a suggestion, not sure whether it works for you or not.
The whole issue you needs to introduce this __BPF__ is because
you included ptrace.h with "clang -target bpf ...". Typically,
when people include ptrace.h, they use "clang <native_arch> ..."
and "llc -march=bpf ..." since ptrace.h and its dependent header
files added a lot of assembly codes and asm code related constructs or 
maybe other arch-spec #define as well, which bpf target cannot handle.

Look at test bpf-script-test-kbuild.c, I think you can drop
uapi/asm/ptrace.h from include file list. This way, you do not need
__BPF__ in x86/include/asm/asm.h. At the same time, you can
remove __BPF__ as well.

The clang compiler change I had is just a bonus. It intends to
help "clang -target bpf ..." users just in case their bpf program
header files contains some host-specific insn asm codes with host-arch
register names, for which I expect most users won't hit this.

>   
>> If the inline assembly is indeed for BPF program, later llc
>> AsmParser will do syntax and semantics checking again.
>>
>> With the above clang patch, the above "#ifndef __BPF__" can be removed.
>> You can decide when is the appropriate time to use latest clang compiler
>> and remove the above "#ifndef __BPF__".
> 
> So are you proposing that we have something similar to that
> CC_HAVE_ASM_GOTO check in the kernel main Makefile, to define something
> like CC_HAVE_ASM_REGS (or some better name), i.e. something like:
> 
> # check for 'asm(_ASM_SP)'
> ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/cc-asm-regs.sh
> $(CC) $(KBUILD_CFLAGS)), y)
>    CC_HAVE_ASM_REGS := 1
>    KBUILD_CFLAGS += -DCC_HAVE_ASM_REGS
>    KBUILD_AFLAGS += -DCC_HAVE_ASM_REGS
> endif

No, I am not suggesting a kernel change like this.
If you can change bpf-script-test-kbuild.c file and get rid of __BPF__,
it will be good. Otherwise, we can leave __BPF__ there for a while
until later clang 7.0 becomes mainstream and then we can remove it.
I just want to you to be aware that there is a clang feature to solve
your problem.

Thanks!

Yonghong

> ?
> 
> - Arnaldo
> 

Re: bpf: handling non BPF register names in inline assembly with -target bpf

[Arnaldo Carvalho de Melo]

Em Wed, Apr 11, 2018 at 12:22:37PM -0700, Yonghong Song escreveu:
> Look at test bpf-script-test-kbuild.c, I think you can drop
> uapi/asm/ptrace.h from include file list. This way, you do not need
> __BPF__ in x86/include/asm/asm.h. At the same time, you can
> remove __BPF__ as well.

That's even better, I'll try that approach, that code was done by
someone else, so I ended up not considering looking if removing a
unnecessary header would fix the issue, I'll check and if that works
I'll revert the patch that introduced that ifndef __BPF__.

- Arnaldo

Re: bpf: handling non BPF register names in inline assembly with -target bpf

[Arnaldo Carvalho de Melo]

Em Wed, Apr 11, 2018 at 04:47:29PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Apr 11, 2018 at 12:22:37PM -0700, Yonghong Song escreveu:
> > Look at test bpf-script-test-kbuild.c, I think you can drop
> > uapi/asm/ptrace.h from include file list. This way, you do not need
> > __BPF__ in x86/include/asm/asm.h. At the same time, you can
> > remove __BPF__ as well.
> 
> That's even better, I'll try that approach, that code was done by
> someone else, so I ended up not considering looking if removing a
> unnecessary header would fix the issue, I'll check and if that works
> I'll revert the patch that introduced that ifndef __BPF__.

So this is what I've pushed to my perf/core branch:

https://git.kernel.org/acme/c/707ba03243d2 (HEAD -> perf/core) Revert "x86/asm: Allow again using asm.h when building for the 'bpf' clang target"
https://git.kernel.org/acme/c/4364203f5563 pref tests bpf: Remove unused ptrace.h include from LLVM test

Thanks,

- Arnaldo

Re: bpf: handling non BPF register names in inline assembly with -target bpf

[Yonghong Song]

On 4/11/18 4:17 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 11, 2018 at 04:47:29PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Wed, Apr 11, 2018 at 12:22:37PM -0700, Yonghong Song escreveu:
>>> Look at test bpf-script-test-kbuild.c, I think you can drop
>>> uapi/asm/ptrace.h from include file list. This way, you do not need
>>> __BPF__ in x86/include/asm/asm.h. At the same time, you can
>>> remove __BPF__ as well.
>>
>> That's even better, I'll try that approach, that code was done by
>> someone else, so I ended up not considering looking if removing a
>> unnecessary header would fix the issue, I'll check and if that works
>> I'll revert the patch that introduced that ifndef __BPF__.
> 
> So this is what I've pushed to my perf/core branch:
> 
> https://git.kernel.org/acme/c/707ba03243d2 (HEAD -> perf/core) Revert "x86/asm: Allow again using asm.h when building for the 'bpf' clang target"
> https://git.kernel.org/acme/c/4364203f5563 pref tests bpf: Remove unused ptrace.h include from LLVM test

Looks good. Thank you!

> 
> Thanks,
> 
> - Arnaldo
> 

[tip:perf/urgent] perf tests bpf: Remove unused ptrace.h include from LLVM test

[tip-bot for Arnaldo Carvalho de Melo]

Commit-ID:  c13009c1ef3a94cfea212c86bbb94c8361e5de0c
Gitweb:     https://git.kernel.org/tip/c13009c1ef3a94cfea212c86bbb94c8361e5de0c
Author:     Arnaldo Carvalho de Melo <acme@redhat.com>
AuthorDate: Wed, 11 Apr 2018 17:57:32 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 12 Apr 2018 10:33:24 -0300

perf tests bpf: Remove unused ptrace.h include from LLVM test

The bpf-script-test-kbuild.c script, used in one of the LLVM subtests,
includes ptrace.h unnecessarily, and that ends up making it include a
header that uses asm(_ASM_SP), a feature that is not supported by clang
<= 4.0, breaking that 'perf test' entry.

This ended up leading to the ca26cffa4e4a ("x86/asm: Allow again using
asm.h when building for the 'bpf' clang target"), adding an ifndef
__BPF__ to the arch/x86/include/asm/asm.h file.

Newer clang versions accept that asm(_ASM_SP) construct, so just remove
the ptrace.h include, which paves the way for reverting ca26cffa4e4a
("x86/asm: Allow again using asm.h when building for the 'bpf' clang
target").

Suggested-by: Yonghong Song <yhs@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
Link: https://lkml.kernel.org/r/613f0a0d-c433-8f4d-dcc1-c9889deae39e@fb.com
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David Ahern <dsahern@gmail.com>
Cc: Dmitriy Vyukov <dvyukov@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Miguel Bernal Marin <miguel.bernal.marin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wang Nan <wangnan0@huawei.com>
Link: https://lkml.kernel.org/n/tip-clbcnzbakdp18ibme4wt43ib@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/bpf-script-test-kbuild.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/tests/bpf-script-test-kbuild.c b/tools/perf/tests/bpf-script-test-kbuild.c
index 3626924740d8..ff3ec8337f0a 100644
--- a/tools/perf/tests/bpf-script-test-kbuild.c
+++ b/tools/perf/tests/bpf-script-test-kbuild.c
@@ -9,7 +9,6 @@
 #define SEC(NAME) __attribute__((section(NAME), used))
 
 #include <uapi/linux/fs.h>
-#include <uapi/asm/ptrace.h>
 
 SEC("func=vfs_llseek")
 int bpf_func__vfs_llseek(void *ctx)

[tip:perf/urgent] Revert "x86/asm: Allow again using asm.h when building for the 'bpf' clang target"

[tip-bot for Arnaldo Carvalho de Melo]

Commit-ID:  fd97d39b0aa49a4beb429aec344604c1b689f089
Gitweb:     https://git.kernel.org/tip/fd97d39b0aa49a4beb429aec344604c1b689f089
Author:     Arnaldo Carvalho de Melo <acme@redhat.com>
AuthorDate: Wed, 11 Apr 2018 18:03:33 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 12 Apr 2018 10:33:27 -0300

Revert "x86/asm: Allow again using asm.h when building for the 'bpf' clang target"

This reverts commit ca26cffa4e4aaeb09bb9e308f95c7835cb149248.

Newer clang versions accept that asm(_ASM_SP) construct, and now that
the bpf-script-test-kbuild.c script, used in one of the 'perf test LLVM'
subtests doesn't include ptrace.h, which ended up including
arch/x86/include/asm/asm.h, we can revert this patch.

Suggested-by: Yonghong Song <yhs@fb.com>
Link: https://lkml.kernel.org/r/613f0a0d-c433-8f4d-dcc1-c9889deae39e@fb.com
Acked-by: Yonghong Song <yhs@fb.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David Ahern <dsahern@gmail.com>
Cc: Dmitriy Vyukov <dvyukov@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Miguel Bernal Marin <miguel.bernal.marin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wang Nan <wangnan0@huawei.com>
Link: https://lkml.kernel.org/n/tip-nqozcv8loq40tkqpfw997993@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 arch/x86/include/asm/asm.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 386a6900e206..219faaec51df 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -136,7 +136,6 @@
 #endif
 
 #ifndef __ASSEMBLY__
-#ifndef __BPF__
 /*
  * This output constraint should be used for any inline asm which has a "call"
  * instruction.  Otherwise the asm may be inserted before the frame pointer
@@ -146,6 +145,5 @@
 register unsigned long current_stack_pointer asm(_ASM_SP);
 #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
 #endif
-#endif
 
 #endif /* _ASM_X86_ASM_H */