diff mbox series

C inlined assembly for reproducing max<min

Message ID 20231122144018.4047232-1-tao.lyu@epfl.ch (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series C inlined assembly for reproducing max<min | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch, async
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-3 fail Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / test
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / test
bpf/vmtest-bpf-next-VM_Test-12 success Logs for x86_64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for x86_64-llvm-16 / build / build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 fail Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for x86_64-gcc / test
bpf/vmtest-bpf-next-VM_Test-6 fail Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-llvm-16 / veristat
bpf/vmtest-bpf-next-VM_Test-14 success Logs for x86_64-llvm-16 / test

Commit Message

Tao Lyu Nov. 22, 2023, 2:40 p.m. UTC 
Hi Yonghong,

Thanks for your reply.
The C inlined assembly code is attached.
I'm using clang-16, but it still fails.

Best,
Tao

Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
---
 .../selftests/bpf/prog_tests/verifier.c       |  2 ++
 .../selftests/bpf/progs/verifier_range.c      | 25 +++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_range.c

Comments

Yonghong Song Nov. 22, 2023, 6:08 p.m. UTC | #1 
On 11/22/23 9:40 AM, Tao Lyu wrote:
> Hi Yonghong,
>
> Thanks for your reply.
> The C inlined assembly code is attached.
> I'm using clang-16, but it still fails.
>
> Best,
> Tao
>
> Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
> ---
>   .../selftests/bpf/prog_tests/verifier.c       |  2 ++
>   .../selftests/bpf/progs/verifier_range.c      | 25 +++++++++++++++++++
>   2 files changed, 27 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_range.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index e5c61aa6604a..3a5d746f392d 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -77,6 +77,7 @@
>   #include "verifier_xadd.skel.h"
>   #include "verifier_xdp.skel.h"
>   #include "verifier_xdp_direct_packet_access.skel.h"
> +#include "verifier_range.skel.h"
>   
>   #define MAX_ENTRIES 11
>   
> @@ -184,6 +185,7 @@ void test_verifier_var_off(void)              { RUN(verifier_var_off); }
>   void test_verifier_xadd(void)                 { RUN(verifier_xadd); }
>   void test_verifier_xdp(void)                  { RUN(verifier_xdp); }
>   void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_packet_access); }
> +void test_verifier_range(void)                { RUN(verifier_range); }
>   
>   static int init_test_val_map(struct bpf_object *obj, char *map_name)
>   {
> diff --git a/tools/testing/selftests/bpf/progs/verifier_range.c b/tools/testing/selftests/bpf/progs/verifier_range.c
> new file mode 100644
> index 000000000000..27597eb8135c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_range.c
> @@ -0,0 +1,25 @@
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +SEC("?tc")
> +__log_level(2)
> +int test_verifier_range(void)
> +{
> +    asm volatile (
> +        "r5 = 100; \
> +        r5 /= 3; \
> +        w5 >>= 7; \
> +        r5 &= -386969681; \
> +        r5 -= -884670597; \
> +        w0 = w5; \
> +        if w0 & 0x894b6a55 goto +2; \

So actually it is 'if w0 & 0x894b6a55 goto +2' failed
the compilation.

Indeed, the above operation is not supported in llvm.
See
   https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/BPFInstrFormats.td#L62-L74
the missing BPFJumpOp<0x4> which corresponds to JSET.

The following llvm patch (on top of llvm-project main branch):

diff --git a/llvm/lib/Target/BPF/BPFInstrFormats.td b/llvm/lib/Target/BPF/BPFInstrFormats.td
index 841d97efc01c..6ed83d877ac0 100644
--- a/llvm/lib/Target/BPF/BPFInstrFormats.td
+++ b/llvm/lib/Target/BPF/BPFInstrFormats.td
@@ -63,6 +63,7 @@ def BPF_JA   : BPFJumpOp<0x0>;
  def BPF_JEQ  : BPFJumpOp<0x1>;
  def BPF_JGT  : BPFJumpOp<0x2>;
  def BPF_JGE  : BPFJumpOp<0x3>;
+def BPF_JSET : BPFJumpOp<0x4>;
  def BPF_JNE  : BPFJumpOp<0x5>;
  def BPF_JSGT : BPFJumpOp<0x6>;
  def BPF_JSGE : BPFJumpOp<0x7>;
diff --git a/llvm/lib/Target/BPF/BPFInstrInfo.td b/llvm/lib/Target/BPF/BPFInstrInfo.td
index 305cbbd34d27..9e75f35efe70 100644
--- a/llvm/lib/Target/BPF/BPFInstrInfo.td
+++ b/llvm/lib/Target/BPF/BPFInstrInfo.td
@@ -246,6 +246,70 @@ class JMP_RI_32<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond>
    let BPFClass = BPF_JMP32;
  }
  
+class JSET_RR<string OpcodeStr>
+    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
+                   (outs),
+                   (ins GPR:$dst, GPR:$src, brtarget:$BrDst),
+                   "if $dst "#OpcodeStr#" $src goto $BrDst",
+                   []> {
+  bits<4> dst;
+  bits<4> src;
+  bits<16> BrDst;
+
+  let Inst{55-52} = src;
+  let Inst{51-48} = dst;
+  let Inst{47-32} = BrDst;
+  let BPFClass = BPF_JMP;
+}
+
+class JSET_RI<string OpcodeStr>
+    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
+                   (outs),
+                   (ins GPR:$dst, i64imm:$imm, brtarget:$BrDst),
+                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
+                   []> {
+  bits<4> dst;
+  bits<16> BrDst;
+  bits<32> imm;
+
+  let Inst{51-48} = dst;
+  let Inst{47-32} = BrDst;
+  let Inst{31-0} = imm;
+  let BPFClass = BPF_JMP;
+}
+
+class JSET_RR_32<string OpcodeStr>
+    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
+                   (outs),
+                   (ins GPR32:$dst, GPR32:$src, brtarget:$BrDst),
+                   "if $dst "#OpcodeStr#" $src goto $BrDst",
+                   []> {
+  bits<4> dst;
+  bits<4> src;
+  bits<16> BrDst;
+
+  let Inst{55-52} = src;
+  let Inst{51-48} = dst;
+  let Inst{47-32} = BrDst;
+  let BPFClass = BPF_JMP32;
+}
+
+class JSET_RI_32<string OpcodeStr>
+    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
+                   (outs),
+                   (ins GPR32:$dst, i32imm:$imm, brtarget:$BrDst),
+                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
+                   []> {
+  bits<4> dst;
+  bits<16> BrDst;
+  bits<32> imm;
+
+  let Inst{51-48} = dst;
+  let Inst{47-32} = BrDst;
+  let Inst{31-0} = imm;
+  let BPFClass = BPF_JMP32;
+}
+
  multiclass J<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond, PatLeaf Cond32> {
    def _rr : JMP_RR<Opc, OpcodeStr, Cond>;
    def _ri : JMP_RI<Opc, OpcodeStr, Cond>;
@@ -265,6 +329,10 @@ defm JULT : J<BPF_JLT, "<", BPF_CC_LTU, BPF_CC_LTU_32>;
  defm JULE : J<BPF_JLE, "<=", BPF_CC_LEU, BPF_CC_LEU_32>;
  defm JSLT : J<BPF_JSLT, "s<", BPF_CC_LT, BPF_CC_LT_32>;
  defm JSLE : J<BPF_JSLE, "s<=", BPF_CC_LE, BPF_CC_LE_32>;
+def JSET_RR    : JSET_RR<"&">;
+def JSET_RI    : JSET_RI<"&">;
+def JSET_RR_32 : JSET_RR_32<"&">;
+def JSET_RI_32 : JSET_RI_32<"&">;
  }
  
  // ALU instructions

can solve your inline asm issue. We will discuss whether llvm compiler
should be implementing this instruction from source or not.


> +        r2 = 1; \
> +        r2 = 1; \
> +        r0 = 0; \
> +        "
> +    );
> +    return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
Alexei Starovoitov Nov. 22, 2023, 6:15 p.m. UTC | #2 
On Wed, Nov 22, 2023 at 10:08 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> > +SEC("?tc")
> > +__log_level(2)
> > +int test_verifier_range(void)
> > +{
> > +    asm volatile (
> > +        "r5 = 100; \
> > +        r5 /= 3; \
> > +        w5 >>= 7; \
> > +        r5 &= -386969681; \
> > +        r5 -= -884670597; \
> > +        w0 = w5; \
> > +        if w0 & 0x894b6a55 goto +2; \
>
> So actually it is 'if w0 & 0x894b6a55 goto +2' failed
> the compilation.
>
> Indeed, the above operation is not supported in llvm.
> See
>    https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/BPFInstrFormats.td#L62-L74
> the missing BPFJumpOp<0x4> which corresponds to JSET.
>
> The following llvm patch (on top of llvm-project main branch):
>
> diff --git a/llvm/lib/Target/BPF/BPFInstrFormats.td b/llvm/lib/Target/BPF/BPFInstrFormats.td
> index 841d97efc01c..6ed83d877ac0 100644
> --- a/llvm/lib/Target/BPF/BPFInstrFormats.td
> +++ b/llvm/lib/Target/BPF/BPFInstrFormats.td
> @@ -63,6 +63,7 @@ def BPF_JA   : BPFJumpOp<0x0>;
>   def BPF_JEQ  : BPFJumpOp<0x1>;
>   def BPF_JGT  : BPFJumpOp<0x2>;
>   def BPF_JGE  : BPFJumpOp<0x3>;
> +def BPF_JSET : BPFJumpOp<0x4>;
>   def BPF_JNE  : BPFJumpOp<0x5>;
>   def BPF_JSGT : BPFJumpOp<0x6>;
>   def BPF_JSGE : BPFJumpOp<0x7>;
> diff --git a/llvm/lib/Target/BPF/BPFInstrInfo.td b/llvm/lib/Target/BPF/BPFInstrInfo.td
> index 305cbbd34d27..9e75f35efe70 100644
> --- a/llvm/lib/Target/BPF/BPFInstrInfo.td
> +++ b/llvm/lib/Target/BPF/BPFInstrInfo.td
> @@ -246,6 +246,70 @@ class JMP_RI_32<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond>
>     let BPFClass = BPF_JMP32;
>   }
>
> +class JSET_RR<string OpcodeStr>
> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
> +                   (outs),
> +                   (ins GPR:$dst, GPR:$src, brtarget:$BrDst),
> +                   "if $dst "#OpcodeStr#" $src goto $BrDst",
> +                   []> {
> +  bits<4> dst;
> +  bits<4> src;
> +  bits<16> BrDst;
> +
> +  let Inst{55-52} = src;
> +  let Inst{51-48} = dst;
> +  let Inst{47-32} = BrDst;
> +  let BPFClass = BPF_JMP;
> +}
> +
> +class JSET_RI<string OpcodeStr>
> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
> +                   (outs),
> +                   (ins GPR:$dst, i64imm:$imm, brtarget:$BrDst),
> +                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
> +                   []> {
> +  bits<4> dst;
> +  bits<16> BrDst;
> +  bits<32> imm;
> +
> +  let Inst{51-48} = dst;
> +  let Inst{47-32} = BrDst;
> +  let Inst{31-0} = imm;
> +  let BPFClass = BPF_JMP;
> +}
> +
> +class JSET_RR_32<string OpcodeStr>
> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
> +                   (outs),
> +                   (ins GPR32:$dst, GPR32:$src, brtarget:$BrDst),
> +                   "if $dst "#OpcodeStr#" $src goto $BrDst",
> +                   []> {
> +  bits<4> dst;
> +  bits<4> src;
> +  bits<16> BrDst;
> +
> +  let Inst{55-52} = src;
> +  let Inst{51-48} = dst;
> +  let Inst{47-32} = BrDst;
> +  let BPFClass = BPF_JMP32;
> +}
> +
> +class JSET_RI_32<string OpcodeStr>
> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
> +                   (outs),
> +                   (ins GPR32:$dst, i32imm:$imm, brtarget:$BrDst),
> +                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
> +                   []> {
> +  bits<4> dst;
> +  bits<16> BrDst;
> +  bits<32> imm;
> +
> +  let Inst{51-48} = dst;
> +  let Inst{47-32} = BrDst;
> +  let Inst{31-0} = imm;
> +  let BPFClass = BPF_JMP32;
> +}
> +
>   multiclass J<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond, PatLeaf Cond32> {
>     def _rr : JMP_RR<Opc, OpcodeStr, Cond>;
>     def _ri : JMP_RI<Opc, OpcodeStr, Cond>;
> @@ -265,6 +329,10 @@ defm JULT : J<BPF_JLT, "<", BPF_CC_LTU, BPF_CC_LTU_32>;
>   defm JULE : J<BPF_JLE, "<=", BPF_CC_LEU, BPF_CC_LEU_32>;
>   defm JSLT : J<BPF_JSLT, "s<", BPF_CC_LT, BPF_CC_LT_32>;
>   defm JSLE : J<BPF_JSLE, "s<=", BPF_CC_LE, BPF_CC_LE_32>;
> +def JSET_RR    : JSET_RR<"&">;
> +def JSET_RI    : JSET_RI<"&">;
> +def JSET_RR_32 : JSET_RR_32<"&">;
> +def JSET_RI_32 : JSET_RI_32<"&">;
>   }
>
>   // ALU instructions
>
> can solve your inline asm issue. We will discuss whether llvm compiler
> should be implementing this instruction from source or not.

I'd say 'yes'. clang/llvm should support such asm syntax.

Jose, Eduard,
Thoughts?
Jose E. Marchesi Nov. 22, 2023, 6:37 p.m. UTC | #3 
> On Wed, Nov 22, 2023 at 10:08 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> > +SEC("?tc")
>> > +__log_level(2)
>> > +int test_verifier_range(void)
>> > +{
>> > +    asm volatile (
>> > +        "r5 = 100; \
>> > +        r5 /= 3; \
>> > +        w5 >>= 7; \
>> > +        r5 &= -386969681; \
>> > +        r5 -= -884670597; \
>> > +        w0 = w5; \
>> > +        if w0 & 0x894b6a55 goto +2; \
>>
>> So actually it is 'if w0 & 0x894b6a55 goto +2' failed
>> the compilation.
>>
>> Indeed, the above operation is not supported in llvm.
>> See
>>    https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/BPFInstrFormats.td#L62-L74
>> the missing BPFJumpOp<0x4> which corresponds to JSET.
>>
>> The following llvm patch (on top of llvm-project main branch):
>>
>> diff --git a/llvm/lib/Target/BPF/BPFInstrFormats.td b/llvm/lib/Target/BPF/BPFInstrFormats.td
>> index 841d97efc01c..6ed83d877ac0 100644
>> --- a/llvm/lib/Target/BPF/BPFInstrFormats.td
>> +++ b/llvm/lib/Target/BPF/BPFInstrFormats.td
>> @@ -63,6 +63,7 @@ def BPF_JA   : BPFJumpOp<0x0>;
>>   def BPF_JEQ  : BPFJumpOp<0x1>;
>>   def BPF_JGT  : BPFJumpOp<0x2>;
>>   def BPF_JGE  : BPFJumpOp<0x3>;
>> +def BPF_JSET : BPFJumpOp<0x4>;
>>   def BPF_JNE  : BPFJumpOp<0x5>;
>>   def BPF_JSGT : BPFJumpOp<0x6>;
>>   def BPF_JSGE : BPFJumpOp<0x7>;
>> diff --git a/llvm/lib/Target/BPF/BPFInstrInfo.td b/llvm/lib/Target/BPF/BPFInstrInfo.td
>> index 305cbbd34d27..9e75f35efe70 100644
>> --- a/llvm/lib/Target/BPF/BPFInstrInfo.td
>> +++ b/llvm/lib/Target/BPF/BPFInstrInfo.td
>> @@ -246,6 +246,70 @@ class JMP_RI_32<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond>
>>     let BPFClass = BPF_JMP32;
>>   }
>>
>> +class JSET_RR<string OpcodeStr>
>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
>> +                   (outs),
>> +                   (ins GPR:$dst, GPR:$src, brtarget:$BrDst),
>> +                   "if $dst "#OpcodeStr#" $src goto $BrDst",
>> +                   []> {
>> +  bits<4> dst;
>> +  bits<4> src;
>> +  bits<16> BrDst;
>> +
>> +  let Inst{55-52} = src;
>> +  let Inst{51-48} = dst;
>> +  let Inst{47-32} = BrDst;
>> +  let BPFClass = BPF_JMP;
>> +}
>> +
>> +class JSET_RI<string OpcodeStr>
>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
>> +                   (outs),
>> +                   (ins GPR:$dst, i64imm:$imm, brtarget:$BrDst),
>> +                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
>> +                   []> {
>> +  bits<4> dst;
>> +  bits<16> BrDst;
>> +  bits<32> imm;
>> +
>> +  let Inst{51-48} = dst;
>> +  let Inst{47-32} = BrDst;
>> +  let Inst{31-0} = imm;
>> +  let BPFClass = BPF_JMP;
>> +}
>> +
>> +class JSET_RR_32<string OpcodeStr>
>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
>> +                   (outs),
>> +                   (ins GPR32:$dst, GPR32:$src, brtarget:$BrDst),
>> +                   "if $dst "#OpcodeStr#" $src goto $BrDst",
>> +                   []> {
>> +  bits<4> dst;
>> +  bits<4> src;
>> +  bits<16> BrDst;
>> +
>> +  let Inst{55-52} = src;
>> +  let Inst{51-48} = dst;
>> +  let Inst{47-32} = BrDst;
>> +  let BPFClass = BPF_JMP32;
>> +}
>> +
>> +class JSET_RI_32<string OpcodeStr>
>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
>> +                   (outs),
>> +                   (ins GPR32:$dst, i32imm:$imm, brtarget:$BrDst),
>> +                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
>> +                   []> {
>> +  bits<4> dst;
>> +  bits<16> BrDst;
>> +  bits<32> imm;
>> +
>> +  let Inst{51-48} = dst;
>> +  let Inst{47-32} = BrDst;
>> +  let Inst{31-0} = imm;
>> +  let BPFClass = BPF_JMP32;
>> +}
>> +
>>   multiclass J<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond, PatLeaf Cond32> {
>>     def _rr : JMP_RR<Opc, OpcodeStr, Cond>;
>>     def _ri : JMP_RI<Opc, OpcodeStr, Cond>;
>> @@ -265,6 +329,10 @@ defm JULT : J<BPF_JLT, "<", BPF_CC_LTU, BPF_CC_LTU_32>;
>>   defm JULE : J<BPF_JLE, "<=", BPF_CC_LEU, BPF_CC_LEU_32>;
>>   defm JSLT : J<BPF_JSLT, "s<", BPF_CC_LT, BPF_CC_LT_32>;
>>   defm JSLE : J<BPF_JSLE, "s<=", BPF_CC_LE, BPF_CC_LE_32>;
>> +def JSET_RR    : JSET_RR<"&">;
>> +def JSET_RI    : JSET_RI<"&">;
>> +def JSET_RR_32 : JSET_RR_32<"&">;
>> +def JSET_RI_32 : JSET_RI_32<"&">;
>>   }
>>
>>   // ALU instructions
>>
>> can solve your inline asm issue. We will discuss whether llvm compiler
>> should be implementing this instruction from source or not.
>
> I'd say 'yes'. clang/llvm should support such asm syntax.
>
> Jose, Eduard,
> Thoughts?

We already support it in GAS:


  $ echo 'if w0 & 0x894b6a55 goto +2' | bpf-unknown-none-as -mdialect=pseudoc -
  $ bpf-unknown-none-objdump -M hex,pseudoc -d a.out
  
  a.out:     file format elf64-bpfle
  
  
  Disassembly of section .text:
  
  0000000000000000 <.text>:
     0:	46 00 02 00 55 6a 4b 89 	if w0&0x894b6a55 goto 0x2


We weren't aware we were diverging with llvm by doing so.  We support
syntax for all the conditional jump instructions using the following
operators:

  BPF_JEQ    ==
  BPF_JGT    >
  BPF_JSGT   s>
  BPF_JGE    >=
  BPF_JSGE   s>=
  BPF_JLT    <
  BPF_JLST   s<
  BPF_JLE    <=
  BPF_JSLE   s<=
  BPF_JSET   &
  BPF_JNE    !=
Eduard Zingerman Nov. 22, 2023, 6:39 p.m. UTC | #4 
On Wed, 2023-11-22 at 10:15 -0800, Alexei Starovoitov wrote:
[...]
> >   multiclass J<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond, PatLeaf Cond32> {
> >     def _rr : JMP_RR<Opc, OpcodeStr, Cond>;
> >     def _ri : JMP_RI<Opc, OpcodeStr, Cond>;
> > @@ -265,6 +329,10 @@ defm JULT : J<BPF_JLT, "<", BPF_CC_LTU, BPF_CC_LTU_32>;
> >   defm JULE : J<BPF_JLE, "<=", BPF_CC_LEU, BPF_CC_LEU_32>;
> >   defm JSLT : J<BPF_JSLT, "s<", BPF_CC_LT, BPF_CC_LT_32>;
> >   defm JSLE : J<BPF_JSLE, "s<=", BPF_CC_LE, BPF_CC_LE_32>;
> > +def JSET_RR    : JSET_RR<"&">;
> > +def JSET_RI    : JSET_RI<"&">;
> > +def JSET_RR_32 : JSET_RR_32<"&">;
> > +def JSET_RI_32 : JSET_RI_32<"&">;
> >   }
> > 
> >   // ALU instructions
> > 
> > can solve your inline asm issue. We will discuss whether llvm compiler
> > should be implementing this instruction from source or not.
> 
> I'd say 'yes'. clang/llvm should support such asm syntax.
> 
> Jose, Eduard,
> Thoughts?

I agree, since instruction is documented it should have assembly
representation. All other instructions from instruction-set.rst seem
to have one.
Yonghong Song Nov. 22, 2023, 6:51 p.m. UTC | #5 
On 11/22/23 1:37 PM, Jose E. Marchesi wrote:
>> On Wed, Nov 22, 2023 at 10:08 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>> +SEC("?tc")
>>>> +__log_level(2)
>>>> +int test_verifier_range(void)
>>>> +{
>>>> +    asm volatile (
>>>> +        "r5 = 100; \
>>>> +        r5 /= 3; \
>>>> +        w5 >>= 7; \
>>>> +        r5 &= -386969681; \
>>>> +        r5 -= -884670597; \
>>>> +        w0 = w5; \
>>>> +        if w0 & 0x894b6a55 goto +2; \
>>> So actually it is 'if w0 & 0x894b6a55 goto +2' failed
>>> the compilation.
>>>
>>> Indeed, the above operation is not supported in llvm.
>>> See
>>>     https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/BPFInstrFormats.td#L62-L74
>>> the missing BPFJumpOp<0x4> which corresponds to JSET.
>>>
>>> The following llvm patch (on top of llvm-project main branch):
>>>
>>> diff --git a/llvm/lib/Target/BPF/BPFInstrFormats.td b/llvm/lib/Target/BPF/BPFInstrFormats.td
>>> index 841d97efc01c..6ed83d877ac0 100644
>>> --- a/llvm/lib/Target/BPF/BPFInstrFormats.td
>>> +++ b/llvm/lib/Target/BPF/BPFInstrFormats.td
>>> @@ -63,6 +63,7 @@ def BPF_JA   : BPFJumpOp<0x0>;
>>>    def BPF_JEQ  : BPFJumpOp<0x1>;
>>>    def BPF_JGT  : BPFJumpOp<0x2>;
>>>    def BPF_JGE  : BPFJumpOp<0x3>;
>>> +def BPF_JSET : BPFJumpOp<0x4>;
>>>    def BPF_JNE  : BPFJumpOp<0x5>;
>>>    def BPF_JSGT : BPFJumpOp<0x6>;
>>>    def BPF_JSGE : BPFJumpOp<0x7>;
>>> diff --git a/llvm/lib/Target/BPF/BPFInstrInfo.td b/llvm/lib/Target/BPF/BPFInstrInfo.td
>>> index 305cbbd34d27..9e75f35efe70 100644
>>> --- a/llvm/lib/Target/BPF/BPFInstrInfo.td
>>> +++ b/llvm/lib/Target/BPF/BPFInstrInfo.td
>>> @@ -246,6 +246,70 @@ class JMP_RI_32<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond>
>>>      let BPFClass = BPF_JMP32;
>>>    }
>>>
>>> +class JSET_RR<string OpcodeStr>
>>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
>>> +                   (outs),
>>> +                   (ins GPR:$dst, GPR:$src, brtarget:$BrDst),
>>> +                   "if $dst "#OpcodeStr#" $src goto $BrDst",
>>> +                   []> {
>>> +  bits<4> dst;
>>> +  bits<4> src;
>>> +  bits<16> BrDst;
>>> +
>>> +  let Inst{55-52} = src;
>>> +  let Inst{51-48} = dst;
>>> +  let Inst{47-32} = BrDst;
>>> +  let BPFClass = BPF_JMP;
>>> +}
>>> +
>>> +class JSET_RI<string OpcodeStr>
>>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
>>> +                   (outs),
>>> +                   (ins GPR:$dst, i64imm:$imm, brtarget:$BrDst),
>>> +                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
>>> +                   []> {
>>> +  bits<4> dst;
>>> +  bits<16> BrDst;
>>> +  bits<32> imm;
>>> +
>>> +  let Inst{51-48} = dst;
>>> +  let Inst{47-32} = BrDst;
>>> +  let Inst{31-0} = imm;
>>> +  let BPFClass = BPF_JMP;
>>> +}
>>> +
>>> +class JSET_RR_32<string OpcodeStr>
>>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_X.Value,
>>> +                   (outs),
>>> +                   (ins GPR32:$dst, GPR32:$src, brtarget:$BrDst),
>>> +                   "if $dst "#OpcodeStr#" $src goto $BrDst",
>>> +                   []> {
>>> +  bits<4> dst;
>>> +  bits<4> src;
>>> +  bits<16> BrDst;
>>> +
>>> +  let Inst{55-52} = src;
>>> +  let Inst{51-48} = dst;
>>> +  let Inst{47-32} = BrDst;
>>> +  let BPFClass = BPF_JMP32;
>>> +}
>>> +
>>> +class JSET_RI_32<string OpcodeStr>
>>> +    : TYPE_ALU_JMP<BPF_JSET.Value, BPF_K.Value,
>>> +                   (outs),
>>> +                   (ins GPR32:$dst, i32imm:$imm, brtarget:$BrDst),
>>> +                   "if $dst "#OpcodeStr#" $imm goto $BrDst",
>>> +                   []> {
>>> +  bits<4> dst;
>>> +  bits<16> BrDst;
>>> +  bits<32> imm;
>>> +
>>> +  let Inst{51-48} = dst;
>>> +  let Inst{47-32} = BrDst;
>>> +  let Inst{31-0} = imm;
>>> +  let BPFClass = BPF_JMP32;
>>> +}
>>> +
>>>    multiclass J<BPFJumpOp Opc, string OpcodeStr, PatLeaf Cond, PatLeaf Cond32> {
>>>      def _rr : JMP_RR<Opc, OpcodeStr, Cond>;
>>>      def _ri : JMP_RI<Opc, OpcodeStr, Cond>;
>>> @@ -265,6 +329,10 @@ defm JULT : J<BPF_JLT, "<", BPF_CC_LTU, BPF_CC_LTU_32>;
>>>    defm JULE : J<BPF_JLE, "<=", BPF_CC_LEU, BPF_CC_LEU_32>;
>>>    defm JSLT : J<BPF_JSLT, "s<", BPF_CC_LT, BPF_CC_LT_32>;
>>>    defm JSLE : J<BPF_JSLE, "s<=", BPF_CC_LE, BPF_CC_LE_32>;
>>> +def JSET_RR    : JSET_RR<"&">;
>>> +def JSET_RI    : JSET_RI<"&">;
>>> +def JSET_RR_32 : JSET_RR_32<"&">;
>>> +def JSET_RI_32 : JSET_RI_32<"&">;
>>>    }
>>>
>>>    // ALU instructions
>>>
>>> can solve your inline asm issue. We will discuss whether llvm compiler
>>> should be implementing this instruction from source or not.
>> I'd say 'yes'. clang/llvm should support such asm syntax.
>>
>> Jose, Eduard,
>> Thoughts?
> We already support it in GAS:
>
>
>    $ echo 'if w0 & 0x894b6a55 goto +2' | bpf-unknown-none-as -mdialect=pseudoc -
>    $ bpf-unknown-none-objdump -M hex,pseudoc -d a.out
>    
>    a.out:     file format elf64-bpfle
>    
>    
>    Disassembly of section .text:
>    
>    0000000000000000 <.text>:
>       0:	46 00 02 00 55 6a 4b 89 	if w0&0x894b6a55 goto 0x2
>
>
> We weren't aware we were diverging with llvm by doing so.  We support
> syntax for all the conditional jump instructions using the following
> operators:
>
>    BPF_JEQ    ==
>    BPF_JGT    >
>    BPF_JSGT   s>
>    BPF_JGE    >=
>    BPF_JSGE   s>=
>    BPF_JLT    <
>    BPF_JLST   s<
>    BPF_JLE    <=
>    BPF_JSLE   s<=
>    BPF_JSET   &
>    BPF_JNE    !=

Sounds good. Eduard inthe other thread has similar opinion. Will add asm support in llvm soon.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index e5c61aa6604a..3a5d746f392d 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -77,6 +77,7 @@ 
 #include "verifier_xadd.skel.h"
 #include "verifier_xdp.skel.h"
 #include "verifier_xdp_direct_packet_access.skel.h"
+#include "verifier_range.skel.h"
 
 #define MAX_ENTRIES 11
 
@@ -184,6 +185,7 @@  void test_verifier_var_off(void)              { RUN(verifier_var_off); }
 void test_verifier_xadd(void)                 { RUN(verifier_xadd); }
 void test_verifier_xdp(void)                  { RUN(verifier_xdp); }
 void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_packet_access); }
+void test_verifier_range(void)                { RUN(verifier_range); }
 
 static int init_test_val_map(struct bpf_object *obj, char *map_name)
 {
diff --git a/tools/testing/selftests/bpf/progs/verifier_range.c b/tools/testing/selftests/bpf/progs/verifier_range.c
new file mode 100644
index 000000000000..27597eb8135c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_range.c
@@ -0,0 +1,25 @@ 
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+SEC("?tc")
+__log_level(2)
+int test_verifier_range(void)
+{
+    asm volatile (
+        "r5 = 100; \
+        r5 /= 3; \
+        w5 >>= 7; \
+        r5 &= -386969681; \
+        r5 -= -884670597; \
+        w0 = w5; \
+        if w0 & 0x894b6a55 goto +2; \
+        r2 = 1; \
+        r2 = 1; \
+        r0 = 0; \
+        "
+    );
+    return 0;
+}
+
+char _license[] SEC("license") = "GPL";