| Message ID | 20220829210546.755377-1-james.hilliard1@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 14e5ce79943a72b9bf0fff8a5867320a9fa3e40d |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] libbpf: add GCC support for bpf_tail_call_static | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Mon, 29 Aug 2022 15:05:46 -0600 you wrote: > The bpf_tail_call_static function is currently not defined unless > using clang >= 8. > > To support bpf_tail_call_static on GCC we can check if __clang__ is > not defined to enable bpf_tail_call_static. > > We need to use GCC assembly syntax when the compiler does not define > __clang__ as LLVM inline assembly is not fully compatible with GCC. > > [...] Here is the summary with links: - [v2] libbpf: add GCC support for bpf_tail_call_static https://git.kernel.org/bpf/bpf-next/c/14e5ce79943a You are awesome, thank you!
On Mon, Aug 29, 2022 at 2:05 PM James Hilliard <james.hilliard1@gmail.com> wrote: > > The bpf_tail_call_static function is currently not defined unless > using clang >= 8. > > To support bpf_tail_call_static on GCC we can check if __clang__ is > not defined to enable bpf_tail_call_static. > > We need to use GCC assembly syntax when the compiler does not define > __clang__ as LLVM inline assembly is not fully compatible with GCC. > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > Changes v1 -> v2: > - drop __BPF__ check as GCC now defines __bpf__ > --- > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 7349b16b8e2f..867b734839dd 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -131,7 +131,7 @@ > /* > * Helper function to perform a tail call with a constant/immediate map slot. > */ > -#if __clang_major__ >= 8 && defined(__bpf__) > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > static __always_inline void > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > { > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > __bpf_unreachable(); > > /* > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > - * pointer) and r3 (constant map index) from _different paths_ ending > + * Provide a hard guarantee that the compiler won't optimize setting r2 > + * (map pointer) and r3 (constant map index) from _different paths_ ending > * up at the _same_ call insn as otherwise we won't be able to use the > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > * > * Note on clobber list: we need to stay in-line with BPF calling > * convention, so even if we don't end up using r0, r4, r5, we need > - * to mark them as clobber so that LLVM doesn't end up using them > - * before / after the call. > + * to mark them as clobber so that the compiler doesn't end up using > + * them before / after the call. > */ > - asm volatile("r1 = %[ctx]\n\t" > + asm volatile( > +#ifdef __clang__ > + "r1 = %[ctx]\n\t" > "r2 = %[map]\n\t" > "r3 = %[slot]\n\t" > +#else > + "mov %%r1,%[ctx]\n\t" > + "mov %%r2,%[map]\n\t" > + "mov %%r3,%[slot]\n\t" > +#endif Hey James, I don't think it's a good idea to have a completely different BPF asm syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax is also what BPF users see in BPF verifier log and in llvm-objdump output, so that's what BPF users are familiar with. This will cause constant and unavoidable maintenance burden both for libraries like libbpf and end users and their BPF apps as well. Given you are trying to make GCC-BPF part of the BPF ecosystem, please think about how to help the ecosystem, move it forward and unify it, not how to branch out and have Clang vs GCC differences everywhere. There is a lot of embedded BPF asm in production applications, having to write something as trivial as `r1 = X` in GCC or Clang-specific ways is a huge burden. As such, we've reverted your patch ([0]). Please add de facto BPF asm syntax support to GCC-BPF and this change won't be necessary. [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf > "call 12" > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) > : "r0", "r1", "r2", "r3", "r4", "r5"); > -- > 2.34.1 >
On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Aug 29, 2022 at 2:05 PM James Hilliard > <james.hilliard1@gmail.com> wrote: > > > > The bpf_tail_call_static function is currently not defined unless > > using clang >= 8. > > > > To support bpf_tail_call_static on GCC we can check if __clang__ is > > not defined to enable bpf_tail_call_static. > > > > We need to use GCC assembly syntax when the compiler does not define > > __clang__ as LLVM inline assembly is not fully compatible with GCC. > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > Changes v1 -> v2: > > - drop __BPF__ check as GCC now defines __bpf__ > > --- > > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > index 7349b16b8e2f..867b734839dd 100644 > > --- a/tools/lib/bpf/bpf_helpers.h > > +++ b/tools/lib/bpf/bpf_helpers.h > > @@ -131,7 +131,7 @@ > > /* > > * Helper function to perform a tail call with a constant/immediate map slot. > > */ > > -#if __clang_major__ >= 8 && defined(__bpf__) > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > > static __always_inline void > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > { > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > __bpf_unreachable(); > > > > /* > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > > - * pointer) and r3 (constant map index) from _different paths_ ending > > + * Provide a hard guarantee that the compiler won't optimize setting r2 > > + * (map pointer) and r3 (constant map index) from _different paths_ ending > > * up at the _same_ call insn as otherwise we won't be able to use the > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > * > > * Note on clobber list: we need to stay in-line with BPF calling > > * convention, so even if we don't end up using r0, r4, r5, we need > > - * to mark them as clobber so that LLVM doesn't end up using them > > - * before / after the call. > > + * to mark them as clobber so that the compiler doesn't end up using > > + * them before / after the call. > > */ > > - asm volatile("r1 = %[ctx]\n\t" > > + asm volatile( > > +#ifdef __clang__ > > + "r1 = %[ctx]\n\t" > > "r2 = %[map]\n\t" > > "r3 = %[slot]\n\t" > > +#else > > + "mov %%r1,%[ctx]\n\t" > > + "mov %%r2,%[map]\n\t" > > + "mov %%r3,%[slot]\n\t" > > +#endif > > Hey James, > > I don't think it's a good idea to have a completely different BPF asm > syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax > is also what BPF users see in BPF verifier log and in llvm-objdump > output, so that's what BPF users are familiar with. Is the difference a BPF specific assembly format deviation or a generic deviation in assembler template syntax between GCC/llvm? https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate > > This will cause constant and unavoidable maintenance burden both for > libraries like libbpf and end users and their BPF apps as well. > > Given you are trying to make GCC-BPF part of the BPF ecosystem, please > think about how to help the ecosystem, move it forward and unify it, > not how to branch out and have Clang vs GCC differences everywhere. > There is a lot of embedded BPF asm in production applications, having > to write something as trivial as `r1 = X` in GCC or Clang-specific > ways is a huge burden. > > As such, we've reverted your patch ([0]). Please add de facto BPF asm > syntax support to GCC-BPF and this change won't be necessary. > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf > > > "call 12" > > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) > > : "r0", "r1", "r2", "r3", "r4", "r5"); > > -- > > 2.34.1 > >
On Fri, Sep 9, 2022 at 11:23 AM James Hilliard <james.hilliard1@gmail.com> wrote: > > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Aug 29, 2022 at 2:05 PM James Hilliard > > <james.hilliard1@gmail.com> wrote: > > > > > > The bpf_tail_call_static function is currently not defined unless > > > using clang >= 8. > > > > > > To support bpf_tail_call_static on GCC we can check if __clang__ is > > > not defined to enable bpf_tail_call_static. > > > > > > We need to use GCC assembly syntax when the compiler does not define > > > __clang__ as LLVM inline assembly is not fully compatible with GCC. > > > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > > --- > > > Changes v1 -> v2: > > > - drop __BPF__ check as GCC now defines __bpf__ > > > --- > > > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ > > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > > index 7349b16b8e2f..867b734839dd 100644 > > > --- a/tools/lib/bpf/bpf_helpers.h > > > +++ b/tools/lib/bpf/bpf_helpers.h > > > @@ -131,7 +131,7 @@ > > > /* > > > * Helper function to perform a tail call with a constant/immediate map slot. > > > */ > > > -#if __clang_major__ >= 8 && defined(__bpf__) > > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > > > static __always_inline void > > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > > { > > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > > __bpf_unreachable(); > > > > > > /* > > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > > > - * pointer) and r3 (constant map index) from _different paths_ ending > > > + * Provide a hard guarantee that the compiler won't optimize setting r2 > > > + * (map pointer) and r3 (constant map index) from _different paths_ ending > > > * up at the _same_ call insn as otherwise we won't be able to use the > > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > > > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > > * > > > * Note on clobber list: we need to stay in-line with BPF calling > > > * convention, so even if we don't end up using r0, r4, r5, we need > > > - * to mark them as clobber so that LLVM doesn't end up using them > > > - * before / after the call. > > > + * to mark them as clobber so that the compiler doesn't end up using > > > + * them before / after the call. > > > */ > > > - asm volatile("r1 = %[ctx]\n\t" > > > + asm volatile( > > > +#ifdef __clang__ > > > + "r1 = %[ctx]\n\t" > > > "r2 = %[map]\n\t" > > > "r3 = %[slot]\n\t" > > > +#else > > > + "mov %%r1,%[ctx]\n\t" > > > + "mov %%r2,%[map]\n\t" > > > + "mov %%r3,%[slot]\n\t" > > > +#endif > > > > Hey James, > > > > I don't think it's a good idea to have a completely different BPF asm > > syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax > > is also what BPF users see in BPF verifier log and in llvm-objdump > > output, so that's what BPF users are familiar with. > > Is the difference a BPF specific assembly format deviation or a generic > deviation in assembler template syntax between GCC/llvm? > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate > Sorry, I don't understand the question. I'm talking about the above snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest stayed the same. So this would be a "BPF specific assembly format" case, right? I don't know what else could be different with GCC-BPF assembly. > > > > This will cause constant and unavoidable maintenance burden both for > > libraries like libbpf and end users and their BPF apps as well. > > > > Given you are trying to make GCC-BPF part of the BPF ecosystem, please > > think about how to help the ecosystem, move it forward and unify it, > > not how to branch out and have Clang vs GCC differences everywhere. > > There is a lot of embedded BPF asm in production applications, having > > to write something as trivial as `r1 = X` in GCC or Clang-specific > > ways is a huge burden. > > > > As such, we've reverted your patch ([0]). Please add de facto BPF asm > > syntax support to GCC-BPF and this change won't be necessary. > > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf > > > > > "call 12" > > > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) > > > : "r0", "r1", "r2", "r3", "r4", "r5"); > > > -- > > > 2.34.1 > > >
On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Sep 9, 2022 at 11:23 AM James Hilliard > <james.hilliard1@gmail.com> wrote: > > > > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Mon, Aug 29, 2022 at 2:05 PM James Hilliard > > > <james.hilliard1@gmail.com> wrote: > > > > > > > > The bpf_tail_call_static function is currently not defined unless > > > > using clang >= 8. > > > > > > > > To support bpf_tail_call_static on GCC we can check if __clang__ is > > > > not defined to enable bpf_tail_call_static. > > > > > > > > We need to use GCC assembly syntax when the compiler does not define > > > > __clang__ as LLVM inline assembly is not fully compatible with GCC. > > > > > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > > > --- > > > > Changes v1 -> v2: > > > > - drop __BPF__ check as GCC now defines __bpf__ > > > > --- > > > > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ > > > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > > > index 7349b16b8e2f..867b734839dd 100644 > > > > --- a/tools/lib/bpf/bpf_helpers.h > > > > +++ b/tools/lib/bpf/bpf_helpers.h > > > > @@ -131,7 +131,7 @@ > > > > /* > > > > * Helper function to perform a tail call with a constant/immediate map slot. > > > > */ > > > > -#if __clang_major__ >= 8 && defined(__bpf__) > > > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > > > > static __always_inline void > > > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > > > { > > > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > > > __bpf_unreachable(); > > > > > > > > /* > > > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > > > > - * pointer) and r3 (constant map index) from _different paths_ ending > > > > + * Provide a hard guarantee that the compiler won't optimize setting r2 > > > > + * (map pointer) and r3 (constant map index) from _different paths_ ending > > > > * up at the _same_ call insn as otherwise we won't be able to use the > > > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > > > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > > > > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > > > * > > > > * Note on clobber list: we need to stay in-line with BPF calling > > > > * convention, so even if we don't end up using r0, r4, r5, we need > > > > - * to mark them as clobber so that LLVM doesn't end up using them > > > > - * before / after the call. > > > > + * to mark them as clobber so that the compiler doesn't end up using > > > > + * them before / after the call. > > > > */ > > > > - asm volatile("r1 = %[ctx]\n\t" > > > > + asm volatile( > > > > +#ifdef __clang__ > > > > + "r1 = %[ctx]\n\t" > > > > "r2 = %[map]\n\t" > > > > "r3 = %[slot]\n\t" > > > > +#else > > > > + "mov %%r1,%[ctx]\n\t" > > > > + "mov %%r2,%[map]\n\t" > > > > + "mov %%r3,%[slot]\n\t" > > > > +#endif > > > > > > Hey James, > > > > > > I don't think it's a good idea to have a completely different BPF asm > > > syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax > > > is also what BPF users see in BPF verifier log and in llvm-objdump > > > output, so that's what BPF users are familiar with. > > > > Is the difference a BPF specific assembly format deviation or a generic > > deviation in assembler template syntax between GCC/llvm? > > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate > > > > Sorry, I don't understand the question. I'm talking about the above > snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest > stayed the same. So this would be a "BPF specific assembly format" > case, right? I don't know what else could be different with GCC-BPF > assembly. I mean it's unclear if it's a generic assembly template format difference that applies to all targets or one that's BPF target specific. Anyways for now I sent a new patch so that bpf_tail_call_static is defined on non-clang compilers so that it will work when GCC-BPF supports the existing asm format. https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/ > > > > > > > This will cause constant and unavoidable maintenance burden both for > > > libraries like libbpf and end users and their BPF apps as well. > > > > > > Given you are trying to make GCC-BPF part of the BPF ecosystem, please > > > think about how to help the ecosystem, move it forward and unify it, > > > not how to branch out and have Clang vs GCC differences everywhere. > > > There is a lot of embedded BPF asm in production applications, having > > > to write something as trivial as `r1 = X` in GCC or Clang-specific > > > ways is a huge burden. > > > > > > As such, we've reverted your patch ([0]). Please add de facto BPF asm > > > syntax support to GCC-BPF and this change won't be necessary. > > > > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf > > > > > > > "call 12" > > > > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) > > > > : "r0", "r1", "r2", "r3", "r4", "r5"); > > > > -- > > > > 2.34.1 > > > >
> On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> >> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard >> <james.hilliard1@gmail.com> wrote: >> > >> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko >> > <andrii.nakryiko@gmail.com> wrote: >> > > >> > > On Mon, Aug 29, 2022 at 2:05 PM James Hilliard >> > > <james.hilliard1@gmail.com> wrote: >> > > > >> > > > The bpf_tail_call_static function is currently not defined unless >> > > > using clang >= 8. >> > > > >> > > > To support bpf_tail_call_static on GCC we can check if __clang__ is >> > > > not defined to enable bpf_tail_call_static. >> > > > >> > > > We need to use GCC assembly syntax when the compiler does not define >> > > > __clang__ as LLVM inline assembly is not fully compatible with GCC. >> > > > >> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >> > > > --- >> > > > Changes v1 -> v2: >> > > > - drop __BPF__ check as GCC now defines __bpf__ >> > > > --- >> > > > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ >> > > > 1 file changed, 13 insertions(+), 6 deletions(-) >> > > > >> > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >> > > > index 7349b16b8e2f..867b734839dd 100644 >> > > > --- a/tools/lib/bpf/bpf_helpers.h >> > > > +++ b/tools/lib/bpf/bpf_helpers.h >> > > > @@ -131,7 +131,7 @@ >> > > > /* >> > > > * Helper function to perform a tail call with a constant/immediate map slot. >> > > > */ >> > > > -#if __clang_major__ >= 8 && defined(__bpf__) >> > > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) >> > > > static __always_inline void >> > > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> > > > { >> > > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> > > > __bpf_unreachable(); >> > > > >> > > > /* >> > > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map >> > > > - * pointer) and r3 (constant map index) from _different paths_ ending >> > > > + * Provide a hard guarantee that the compiler won't optimize setting r2 >> > > > + * (map pointer) and r3 (constant map index) from _different paths_ ending >> > > > * up at the _same_ call insn as otherwise we won't be able to use the >> > > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel >> > > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key >> > > > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> > > > * >> > > > * Note on clobber list: we need to stay in-line with BPF calling >> > > > * convention, so even if we don't end up using r0, r4, r5, we need >> > > > - * to mark them as clobber so that LLVM doesn't end up using them >> > > > - * before / after the call. >> > > > + * to mark them as clobber so that the compiler doesn't end up using >> > > > + * them before / after the call. >> > > > */ >> > > > - asm volatile("r1 = %[ctx]\n\t" >> > > > + asm volatile( >> > > > +#ifdef __clang__ >> > > > + "r1 = %[ctx]\n\t" >> > > > "r2 = %[map]\n\t" >> > > > "r3 = %[slot]\n\t" >> > > > +#else >> > > > + "mov %%r1,%[ctx]\n\t" >> > > > + "mov %%r2,%[map]\n\t" >> > > > + "mov %%r3,%[slot]\n\t" >> > > > +#endif >> > > >> > > Hey James, >> > > >> > > I don't think it's a good idea to have a completely different BPF asm >> > > syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax >> > > is also what BPF users see in BPF verifier log and in llvm-objdump >> > > output, so that's what BPF users are familiar with. >> > >> > Is the difference a BPF specific assembly format deviation or a generic >> > deviation in assembler template syntax between GCC/llvm? >> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate >> > >> >> Sorry, I don't understand the question. I'm talking about the above >> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest >> stayed the same. So this would be a "BPF specific assembly format" >> case, right? I don't know what else could be different with GCC-BPF >> assembly. > > I mean it's unclear if it's a generic assembly template format difference > that applies to all targets or one that's BPF target specific. It is certainly BPF specific. I think that when I first wrote the BPF GNU toolchain port the assembly format used by LLVM was different than it is now: I certainly didn't invent the syntax the GNU assembler uses for BPF. It seems LLVM settled with that C-like syntax for assembly instead, which is very unconventional. > Anyways for now I sent a new patch so that bpf_tail_call_static is defined > on non-clang compilers so that it will work when GCC-BPF supports the > existing asm format. > https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/ I don't think this patch makes much sense: these guards are designed to avoid compilers that do not support the inline assembly (as presumably happen with clang < 8) to choke on the header file. That's also the case of GCC BPF at the moment. With this patch, people won't be currentty able to use bpf_helpers.h with GCC BPF even if they don't use bpf_tail_call_static. >> > > >> > > This will cause constant and unavoidable maintenance burden both for >> > > libraries like libbpf and end users and their BPF apps as well. >> > > >> > > Given you are trying to make GCC-BPF part of the BPF ecosystem, please >> > > think about how to help the ecosystem, move it forward and unify it, >> > > not how to branch out and have Clang vs GCC differences everywhere. >> > > There is a lot of embedded BPF asm in production applications, having >> > > to write something as trivial as `r1 = X` in GCC or Clang-specific >> > > ways is a huge burden. >> > > >> > > As such, we've reverted your patch ([0]). Please add de facto BPF asm >> > > syntax support to GCC-BPF and this change won't be necessary. >> > > >> > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf >> > > >> > > > "call 12" >> > > > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) >> > > > : "r0", "r1", "r2", "r3", "r4", "r5"); >> > > > -- >> > > > 2.34.1 >> > > >
On Sat, Sep 10, 2022 at 12:53 AM Jose E. Marchesi <jose.marchesi@oracle.com> wrote: > > > > On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > >> > >> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard > >> <james.hilliard1@gmail.com> wrote: > >> > > >> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko > >> > <andrii.nakryiko@gmail.com> wrote: > >> > > > >> > > On Mon, Aug 29, 2022 at 2:05 PM James Hilliard > >> > > <james.hilliard1@gmail.com> wrote: > >> > > > > >> > > > The bpf_tail_call_static function is currently not defined unless > >> > > > using clang >= 8. > >> > > > > >> > > > To support bpf_tail_call_static on GCC we can check if __clang__ is > >> > > > not defined to enable bpf_tail_call_static. > >> > > > > >> > > > We need to use GCC assembly syntax when the compiler does not define > >> > > > __clang__ as LLVM inline assembly is not fully compatible with GCC. > >> > > > > >> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >> > > > --- > >> > > > Changes v1 -> v2: > >> > > > - drop __BPF__ check as GCC now defines __bpf__ > >> > > > --- > >> > > > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ > >> > > > 1 file changed, 13 insertions(+), 6 deletions(-) > >> > > > > >> > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > >> > > > index 7349b16b8e2f..867b734839dd 100644 > >> > > > --- a/tools/lib/bpf/bpf_helpers.h > >> > > > +++ b/tools/lib/bpf/bpf_helpers.h > >> > > > @@ -131,7 +131,7 @@ > >> > > > /* > >> > > > * Helper function to perform a tail call with a constant/immediate map slot. > >> > > > */ > >> > > > -#if __clang_major__ >= 8 && defined(__bpf__) > >> > > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > >> > > > static __always_inline void > >> > > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > >> > > > { > >> > > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > >> > > > __bpf_unreachable(); > >> > > > > >> > > > /* > >> > > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > >> > > > - * pointer) and r3 (constant map index) from _different paths_ ending > >> > > > + * Provide a hard guarantee that the compiler won't optimize setting r2 > >> > > > + * (map pointer) and r3 (constant map index) from _different paths_ ending > >> > > > * up at the _same_ call insn as otherwise we won't be able to use the > >> > > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > >> > > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > >> > > > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > >> > > > * > >> > > > * Note on clobber list: we need to stay in-line with BPF calling > >> > > > * convention, so even if we don't end up using r0, r4, r5, we need > >> > > > - * to mark them as clobber so that LLVM doesn't end up using them > >> > > > - * before / after the call. > >> > > > + * to mark them as clobber so that the compiler doesn't end up using > >> > > > + * them before / after the call. > >> > > > */ > >> > > > - asm volatile("r1 = %[ctx]\n\t" > >> > > > + asm volatile( > >> > > > +#ifdef __clang__ > >> > > > + "r1 = %[ctx]\n\t" > >> > > > "r2 = %[map]\n\t" > >> > > > "r3 = %[slot]\n\t" > >> > > > +#else > >> > > > + "mov %%r1,%[ctx]\n\t" > >> > > > + "mov %%r2,%[map]\n\t" > >> > > > + "mov %%r3,%[slot]\n\t" > >> > > > +#endif > >> > > > >> > > Hey James, > >> > > > >> > > I don't think it's a good idea to have a completely different BPF asm > >> > > syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax > >> > > is also what BPF users see in BPF verifier log and in llvm-objdump > >> > > output, so that's what BPF users are familiar with. > >> > > >> > Is the difference a BPF specific assembly format deviation or a generic > >> > deviation in assembler template syntax between GCC/llvm? > >> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate > >> > > >> > >> Sorry, I don't understand the question. I'm talking about the above > >> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest > >> stayed the same. So this would be a "BPF specific assembly format" > >> case, right? I don't know what else could be different with GCC-BPF > >> assembly. > > > > I mean it's unclear if it's a generic assembly template format difference > > that applies to all targets or one that's BPF target specific. > > It is certainly BPF specific. > > I think that when I first wrote the BPF GNU toolchain port the assembly > format used by LLVM was different than it is now: I certainly didn't > invent the syntax the GNU assembler uses for BPF. > > It seems LLVM settled with that C-like syntax for assembly instead, > which is very unconventional. > > > Anyways for now I sent a new patch so that bpf_tail_call_static is defined > > on non-clang compilers so that it will work when GCC-BPF supports the > > existing asm format. > > https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/ > > I don't think this patch makes much sense: these guards are designed to > avoid compilers that do not support the inline assembly (as presumably > happen with clang < 8) to choke on the header file. That's also the > case of GCC BPF at the moment. > > With this patch, people won't be currentty able to use bpf_helpers.h > with GCC BPF even if they don't use bpf_tail_call_static. That doesn't seem to be an issue here, from what I can tell it's not a failure in the compiler but a failure in the assembler, so having bpf_tail_call_static unguarded in GCC doesn't break anything that isn't already broken. > > >> > > > >> > > This will cause constant and unavoidable maintenance burden both for > >> > > libraries like libbpf and end users and their BPF apps as well. > >> > > > >> > > Given you are trying to make GCC-BPF part of the BPF ecosystem, please > >> > > think about how to help the ecosystem, move it forward and unify it, > >> > > not how to branch out and have Clang vs GCC differences everywhere. > >> > > There is a lot of embedded BPF asm in production applications, having > >> > > to write something as trivial as `r1 = X` in GCC or Clang-specific > >> > > ways is a huge burden. > >> > > > >> > > As such, we've reverted your patch ([0]). Please add de facto BPF asm > >> > > syntax support to GCC-BPF and this change won't be necessary. > >> > > > >> > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf > >> > > > >> > > > "call 12" > >> > > > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) > >> > > > : "r0", "r1", "r2", "r3", "r4", "r5"); > >> > > > -- > >> > > > 2.34.1 > >> > > >
> On Sat, Sep 10, 2022 at 12:53 AM Jose E. Marchesi > <jose.marchesi@oracle.com> wrote: >> >> >> > On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko >> > <andrii.nakryiko@gmail.com> wrote: >> >> >> >> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard >> >> <james.hilliard1@gmail.com> wrote: >> >> > >> >> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko >> >> > <andrii.nakryiko@gmail.com> wrote: >> >> > > >> >> > > On Mon, Aug 29, 2022 at 2:05 PM James Hilliard >> >> > > <james.hilliard1@gmail.com> wrote: >> >> > > > >> >> > > > The bpf_tail_call_static function is currently not defined unless >> >> > > > using clang >= 8. >> >> > > > >> >> > > > To support bpf_tail_call_static on GCC we can check if __clang__ is >> >> > > > not defined to enable bpf_tail_call_static. >> >> > > > >> >> > > > We need to use GCC assembly syntax when the compiler does not define >> >> > > > __clang__ as LLVM inline assembly is not fully compatible with GCC. >> >> > > > >> >> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >> >> > > > --- >> >> > > > Changes v1 -> v2: >> >> > > > - drop __BPF__ check as GCC now defines __bpf__ >> >> > > > --- >> >> > > > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ >> >> > > > 1 file changed, 13 insertions(+), 6 deletions(-) >> >> > > > >> >> > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >> >> > > > index 7349b16b8e2f..867b734839dd 100644 >> >> > > > --- a/tools/lib/bpf/bpf_helpers.h >> >> > > > +++ b/tools/lib/bpf/bpf_helpers.h >> >> > > > @@ -131,7 +131,7 @@ >> >> > > > /* >> >> > > > * Helper function to perform a tail call with a constant/immediate map slot. >> >> > > > */ >> >> > > > -#if __clang_major__ >= 8 && defined(__bpf__) >> >> > > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) >> >> > > > static __always_inline void >> >> > > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> >> > > > { >> >> > > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> >> > > > __bpf_unreachable(); >> >> > > > >> >> > > > /* >> >> > > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map >> >> > > > - * pointer) and r3 (constant map index) from _different paths_ ending >> >> > > > + * Provide a hard guarantee that the compiler won't optimize setting r2 >> >> > > > + * (map pointer) and r3 (constant map index) from _different paths_ ending >> >> > > > * up at the _same_ call insn as otherwise we won't be able to use the >> >> > > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel >> >> > > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key >> >> > > > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> >> > > > * >> >> > > > * Note on clobber list: we need to stay in-line with BPF calling >> >> > > > * convention, so even if we don't end up using r0, r4, r5, we need >> >> > > > - * to mark them as clobber so that LLVM doesn't end up using them >> >> > > > - * before / after the call. >> >> > > > + * to mark them as clobber so that the compiler doesn't end up using >> >> > > > + * them before / after the call. >> >> > > > */ >> >> > > > - asm volatile("r1 = %[ctx]\n\t" >> >> > > > + asm volatile( >> >> > > > +#ifdef __clang__ >> >> > > > + "r1 = %[ctx]\n\t" >> >> > > > "r2 = %[map]\n\t" >> >> > > > "r3 = %[slot]\n\t" >> >> > > > +#else >> >> > > > + "mov %%r1,%[ctx]\n\t" >> >> > > > + "mov %%r2,%[map]\n\t" >> >> > > > + "mov %%r3,%[slot]\n\t" >> >> > > > +#endif >> >> > > >> >> > > Hey James, >> >> > > >> >> > > I don't think it's a good idea to have a completely different BPF asm >> >> > > syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax >> >> > > is also what BPF users see in BPF verifier log and in llvm-objdump >> >> > > output, so that's what BPF users are familiar with. >> >> > >> >> > Is the difference a BPF specific assembly format deviation or a generic >> >> > deviation in assembler template syntax between GCC/llvm? >> >> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate >> >> > >> >> >> >> Sorry, I don't understand the question. I'm talking about the above >> >> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest >> >> stayed the same. So this would be a "BPF specific assembly format" >> >> case, right? I don't know what else could be different with GCC-BPF >> >> assembly. >> > >> > I mean it's unclear if it's a generic assembly template format difference >> > that applies to all targets or one that's BPF target specific. >> >> It is certainly BPF specific. >> >> I think that when I first wrote the BPF GNU toolchain port the assembly >> format used by LLVM was different than it is now: I certainly didn't >> invent the syntax the GNU assembler uses for BPF. >> >> It seems LLVM settled with that C-like syntax for assembly instead, >> which is very unconventional. >> >> > Anyways for now I sent a new patch so that bpf_tail_call_static is defined >> > on non-clang compilers so that it will work when GCC-BPF supports the >> > existing asm format. >> > https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/ >> >> I don't think this patch makes much sense: these guards are designed to >> avoid compilers that do not support the inline assembly (as presumably >> happen with clang < 8) to choke on the header file. That's also the >> case of GCC BPF at the moment. >> >> With this patch, people won't be currentty able to use bpf_helpers.h >> with GCC BPF even if they don't use bpf_tail_call_static. > > That doesn't seem to be an issue here, from what I can tell it's not > a failure in the compiler but a failure in the assembler, so having > bpf_tail_call_static unguarded in GCC doesn't break anything > that isn't already broken. IMO it makes it worse, because: 1) With your patch the error message will happen at assembly time (invalid syntax in the intermediate .s file mixed with valid syntax) pointing to a location in a temporary .S file. With the guards, you get an error at compile time instead, pointing to the undefined function in the C source. IMO it is much easier to figure out what is wrong in the second case than in the first. 2) If/when we support the C-like assembly syntax in GCC, you will want to guard the function anyway with a GCC_MAJOR > 12 (or whatever) very much like it is done with clang >= 8. >> >> > > >> >> > > This will cause constant and unavoidable maintenance burden both for >> >> > > libraries like libbpf and end users and their BPF apps as well. >> >> > > >> >> > > Given you are trying to make GCC-BPF part of the BPF ecosystem, please >> >> > > think about how to help the ecosystem, move it forward and unify it, >> >> > > not how to branch out and have Clang vs GCC differences everywhere. >> >> > > There is a lot of embedded BPF asm in production applications, having >> >> > > to write something as trivial as `r1 = X` in GCC or Clang-specific >> >> > > ways is a huge burden. >> >> > > >> >> > > As such, we've reverted your patch ([0]). Please add de facto BPF asm >> >> > > syntax support to GCC-BPF and this change won't be necessary. >> >> > > >> >> > > [0] >> >> > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf >> >> > > >> >> > > > "call 12" >> >> > > > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) >> >> > > > : "r0", "r1", "r2", "r3", "r4", "r5"); >> >> > > > -- >> >> > > > 2.34.1 >> >> > > >
On Sat, Sep 10, 2022 at 2:43 AM Jose E. Marchesi <jose.marchesi@oracle.com> wrote: > > > > On Sat, Sep 10, 2022 at 12:53 AM Jose E. Marchesi > > <jose.marchesi@oracle.com> wrote: > >> > >> > >> > On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko > >> > <andrii.nakryiko@gmail.com> wrote: > >> >> > >> >> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard > >> >> <james.hilliard1@gmail.com> wrote: > >> >> > > >> >> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko > >> >> > <andrii.nakryiko@gmail.com> wrote: > >> >> > > > >> >> > > On Mon, Aug 29, 2022 at 2:05 PM James Hilliard > >> >> > > <james.hilliard1@gmail.com> wrote: > >> >> > > > > >> >> > > > The bpf_tail_call_static function is currently not defined unless > >> >> > > > using clang >= 8. > >> >> > > > > >> >> > > > To support bpf_tail_call_static on GCC we can check if __clang__ is > >> >> > > > not defined to enable bpf_tail_call_static. > >> >> > > > > >> >> > > > We need to use GCC assembly syntax when the compiler does not define > >> >> > > > __clang__ as LLVM inline assembly is not fully compatible with GCC. > >> >> > > > > >> >> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >> >> > > > --- > >> >> > > > Changes v1 -> v2: > >> >> > > > - drop __BPF__ check as GCC now defines __bpf__ > >> >> > > > --- > >> >> > > > tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ > >> >> > > > 1 file changed, 13 insertions(+), 6 deletions(-) > >> >> > > > > >> >> > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > >> >> > > > index 7349b16b8e2f..867b734839dd 100644 > >> >> > > > --- a/tools/lib/bpf/bpf_helpers.h > >> >> > > > +++ b/tools/lib/bpf/bpf_helpers.h > >> >> > > > @@ -131,7 +131,7 @@ > >> >> > > > /* > >> >> > > > * Helper function to perform a tail call with a constant/immediate map slot. > >> >> > > > */ > >> >> > > > -#if __clang_major__ >= 8 && defined(__bpf__) > >> >> > > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > >> >> > > > static __always_inline void > >> >> > > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > >> >> > > > { > >> >> > > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > >> >> > > > __bpf_unreachable(); > >> >> > > > > >> >> > > > /* > >> >> > > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > >> >> > > > - * pointer) and r3 (constant map index) from _different paths_ ending > >> >> > > > + * Provide a hard guarantee that the compiler won't optimize setting r2 > >> >> > > > + * (map pointer) and r3 (constant map index) from _different paths_ ending > >> >> > > > * up at the _same_ call insn as otherwise we won't be able to use the > >> >> > > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > >> >> > > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > >> >> > > > @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > >> >> > > > * > >> >> > > > * Note on clobber list: we need to stay in-line with BPF calling > >> >> > > > * convention, so even if we don't end up using r0, r4, r5, we need > >> >> > > > - * to mark them as clobber so that LLVM doesn't end up using them > >> >> > > > - * before / after the call. > >> >> > > > + * to mark them as clobber so that the compiler doesn't end up using > >> >> > > > + * them before / after the call. > >> >> > > > */ > >> >> > > > - asm volatile("r1 = %[ctx]\n\t" > >> >> > > > + asm volatile( > >> >> > > > +#ifdef __clang__ > >> >> > > > + "r1 = %[ctx]\n\t" > >> >> > > > "r2 = %[map]\n\t" > >> >> > > > "r3 = %[slot]\n\t" > >> >> > > > +#else > >> >> > > > + "mov %%r1,%[ctx]\n\t" > >> >> > > > + "mov %%r2,%[map]\n\t" > >> >> > > > + "mov %%r3,%[slot]\n\t" > >> >> > > > +#endif > >> >> > > > >> >> > > Hey James, > >> >> > > > >> >> > > I don't think it's a good idea to have a completely different BPF asm > >> >> > > syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax > >> >> > > is also what BPF users see in BPF verifier log and in llvm-objdump > >> >> > > output, so that's what BPF users are familiar with. > >> >> > > >> >> > Is the difference a BPF specific assembly format deviation or a generic > >> >> > deviation in assembler template syntax between GCC/llvm? > >> >> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate > >> >> > > >> >> > >> >> Sorry, I don't understand the question. I'm talking about the above > >> >> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest > >> >> stayed the same. So this would be a "BPF specific assembly format" > >> >> case, right? I don't know what else could be different with GCC-BPF > >> >> assembly. > >> > > >> > I mean it's unclear if it's a generic assembly template format difference > >> > that applies to all targets or one that's BPF target specific. > >> > >> It is certainly BPF specific. > >> > >> I think that when I first wrote the BPF GNU toolchain port the assembly > >> format used by LLVM was different than it is now: I certainly didn't > >> invent the syntax the GNU assembler uses for BPF. > >> > >> It seems LLVM settled with that C-like syntax for assembly instead, > >> which is very unconventional. > >> > >> > Anyways for now I sent a new patch so that bpf_tail_call_static is defined > >> > on non-clang compilers so that it will work when GCC-BPF supports the > >> > existing asm format. > >> > https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/ > >> > >> I don't think this patch makes much sense: these guards are designed to > >> avoid compilers that do not support the inline assembly (as presumably > >> happen with clang < 8) to choke on the header file. That's also the > >> case of GCC BPF at the moment. > >> > >> With this patch, people won't be currentty able to use bpf_helpers.h > >> with GCC BPF even if they don't use bpf_tail_call_static. > > > > That doesn't seem to be an issue here, from what I can tell it's not > > a failure in the compiler but a failure in the assembler, so having > > bpf_tail_call_static unguarded in GCC doesn't break anything > > that isn't already broken. > > IMO it makes it worse, because: > > 1) With your patch the error message will happen at assembly time > (invalid syntax in the intermediate .s file mixed with valid syntax) > pointing to a location in a temporary .S file. With the guards, you > get an error at compile time instead, pointing to the undefined > function in the C source. IMO it is much easier to figure out what > is wrong in the second case than in the first. A compile time error may be somewhat misleading as one would suspect the issue is with a missing include or something along those lines, even though the issue is with the inline assembly. > > 2) If/when we support the C-like assembly syntax in GCC, you will want > to guard the function anyway with a GCC_MAJOR > 12 (or whatever) very > much like it is done with clang >= 8. Wouldn't we need to check the GAS version instead of the GCC version? AFAIU GAS version isn't known at compile time so a version check may not make sense here for GCC. > > >> >> > > > >> >> > > This will cause constant and unavoidable maintenance burden both for > >> >> > > libraries like libbpf and end users and their BPF apps as well. > >> >> > > > >> >> > > Given you are trying to make GCC-BPF part of the BPF ecosystem, please > >> >> > > think about how to help the ecosystem, move it forward and unify it, > >> >> > > not how to branch out and have Clang vs GCC differences everywhere. > >> >> > > There is a lot of embedded BPF asm in production applications, having > >> >> > > to write something as trivial as `r1 = X` in GCC or Clang-specific > >> >> > > ways is a huge burden. > >> >> > > > >> >> > > As such, we've reverted your patch ([0]). Please add de facto BPF asm > >> >> > > syntax support to GCC-BPF and this change won't be necessary. > >> >> > > > >> >> > > [0] > >> >> > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf > >> >> > > > >> >> > > > "call 12" > >> >> > > > :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) > >> >> > > > : "r0", "r1", "r2", "r3", "r4", "r5"); > >> >> > > > -- > >> >> > > > 2.34.1 > >> >> > > >
On Sat, Sep 10, 2022 at 1:43 AM Jose E. Marchesi <jose.marchesi@oracle.com> wrote: > > 2) If/when we support the C-like assembly syntax in GCC, Thank you for considering supporting the standard BPF assembly syntax in GCC. I agree that C-like asm looks unusual. The main reason to pick that style was the ease of understanding and to avoid gnu vs intel asm order confusion. We didn't want to deal with question whether 'mov r1, r2' means r1->r2 or r2->r1. The C style asm r1=r2 is unambiguous.
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index 7349b16b8e2f..867b734839dd 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -131,7 +131,7 @@ /* * Helper function to perform a tail call with a constant/immediate map slot. */ -#if __clang_major__ >= 8 && defined(__bpf__) +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) static __always_inline void bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) { @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) __bpf_unreachable(); /* - * Provide a hard guarantee that LLVM won't optimize setting r2 (map - * pointer) and r3 (constant map index) from _different paths_ ending + * Provide a hard guarantee that the compiler won't optimize setting r2 + * (map pointer) and r3 (constant map index) from _different paths_ ending * up at the _same_ call insn as otherwise we won't be able to use the * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key @@ -148,12 +148,19 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) * * Note on clobber list: we need to stay in-line with BPF calling * convention, so even if we don't end up using r0, r4, r5, we need - * to mark them as clobber so that LLVM doesn't end up using them - * before / after the call. + * to mark them as clobber so that the compiler doesn't end up using + * them before / after the call. */ - asm volatile("r1 = %[ctx]\n\t" + asm volatile( +#ifdef __clang__ + "r1 = %[ctx]\n\t" "r2 = %[map]\n\t" "r3 = %[slot]\n\t" +#else + "mov %%r1,%[ctx]\n\t" + "mov %%r2,%[map]\n\t" + "mov %%r3,%[slot]\n\t" +#endif "call 12" :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot) : "r0", "r1", "r2", "r3", "r4", "r5");
The bpf_tail_call_static function is currently not defined unless using clang >= 8. To support bpf_tail_call_static on GCC we can check if __clang__ is not defined to enable bpf_tail_call_static. We need to use GCC assembly syntax when the compiler does not define __clang__ as LLVM inline assembly is not fully compatible with GCC. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- Changes v1 -> v2: - drop __BPF__ check as GCC now defines __bpf__ --- tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)