| Message ID | 20230125213817.1424447-25-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Support bpf trampoline for s390x | expand |
On Wed, Jan 25, 2023 at 10:38:17PM +0100, Ilya Leoshkevich wrote: > + > + /* Sign-extend the kfunc arguments. */ > + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { > + m = bpf_jit_find_kfunc_model(fp, insn); > + if (!m) > + return -1; > + > + for (j = 0; j < m->nr_args; j++) { > + if (sign_extend(jit, BPF_REG_1 + j, > + m->arg_size[j], > + m->arg_flags[j])) > + return -1; > + } > + } Is this because s390 doesn't have subregisters? Could you give an example where it's necessary? I'm guessing a bpf prog compiled with alu32 and operates on signed int that is passed into a kfunc that expects 'int' in 64-bit reg? > > +bool bpf_jit_supports_kfunc_call(void) > +{ > + return true; > +} Timely :) Thanks for working it.
On Wed, 2023-01-25 at 17:28 -0800, Alexei Starovoitov wrote: > On Wed, Jan 25, 2023 at 10:38:17PM +0100, Ilya Leoshkevich wrote: > > + > > + /* Sign-extend the kfunc arguments. */ > > + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { > > + m = bpf_jit_find_kfunc_model(fp, insn); > > + if (!m) > > + return -1; > > + > > + for (j = 0; j < m->nr_args; j++) { > > + if (sign_extend(jit, BPF_REG_1 + j, > > + m->arg_size[j], > > + m->arg_flags[j])) > > + return -1; > > + } > > + } > > Is this because s390 doesn't have subregisters? > Could you give an example where it's necessary? > I'm guessing a bpf prog compiled with alu32 and operates on signed > int > that is passed into a kfunc that expects 'int' in 64-bit reg? Precisely. The test added in 13/24 fails without this: verify_success:PASS:skel 0 nsec verify_success:PASS:bpf_object__find_program_by_name 0 nsec verify_success:PASS:kfunc_call_test4 0 nsec verify_success:FAIL:retval unexpected retval: actual 4294966065 != expected -1234 #94/10 kfunc_call/kfunc_call_test4:FAIL Looking at the assembly: ; long noinline bpf_kfunc_call_test4(signed char a, short b, int c, long d) 0000000000936a78 <bpf_kfunc_call_test4>: 936a78: c0 04 00 00 00 00 jgnop 936a78 <bpf_kfunc_call_test4> ; return (long)a + (long)b + (long)c + d; 936a7e: b9 08 00 45 agr %r4,%r5 936a82: b9 08 00 43 agr %r4,%r3 936a86: b9 08 00 24 agr %r2,%r4 936a8a: c0 f4 00 1e 3b 27 jg cfe0d8 <__s390_indirect_jump_r14> As per the s390x ABI, bpf_kfunc_call_test4() has the right to assume that a, b and c are sign-extended by the caller, which results in using 64-bit additions (agr) without any additional conversions. On the JITed code side (without this hunk) we have: ; tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000); ; 5: b4 10 00 00 ff ff ff fd w1 = -3 0x3ff7fdcdad4: llilf %r2,0xfffffffd ; 6: b4 20 00 00 ff ff ff e2 w2 = -30 0x3ff7fdcdada: llilf %r3,0xffffffe2 ; 7: b4 30 00 00 ff ff ff 38 w3 = -200 0x3ff7fdcdae0: llilf %r4,0xffffff38 ; 8: b7 40 00 00 ff ff fc 18 r4 = -1000 0x3ff7fdcdae6: lgfi %r5,-1000 0x3ff7fdcdaec: mvc 64(4,%r15),160(%r15) 0x3ff7fdcdaf2: lgrl %r1,bpf_kfunc_call_test4@GOT 0x3ff7fdcdaf8: brasl %r14,__s390_indirect_jump_r1 This first 3 llilfs are 32-bit loads, that need to be sign-extended to 64 bits. > > +bool bpf_jit_supports_kfunc_call(void) > > +{ > > + return true; > > +} > > Timely :) Thanks for working it.
On Fri, Jan 27, 2023 at 3:36 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Wed, 2023-01-25 at 17:28 -0800, Alexei Starovoitov wrote: > > On Wed, Jan 25, 2023 at 10:38:17PM +0100, Ilya Leoshkevich wrote: > > > + > > > + /* Sign-extend the kfunc arguments. */ > > > + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { > > > + m = bpf_jit_find_kfunc_model(fp, insn); > > > + if (!m) > > > + return -1; > > > + > > > + for (j = 0; j < m->nr_args; j++) { > > > + if (sign_extend(jit, BPF_REG_1 + j, > > > + m->arg_size[j], > > > + m->arg_flags[j])) > > > + return -1; > > > + } > > > + } > > > > Is this because s390 doesn't have subregisters? > > Could you give an example where it's necessary? > > I'm guessing a bpf prog compiled with alu32 and operates on signed > > int > > that is passed into a kfunc that expects 'int' in 64-bit reg? > > Precisely. The test added in 13/24 fails without this: > > verify_success:PASS:skel 0 nsec > verify_success:PASS:bpf_object__find_program_by_name 0 nsec > verify_success:PASS:kfunc_call_test4 0 nsec > verify_success:FAIL:retval unexpected retval: actual 4294966065 != > expected -1234 > #94/10 kfunc_call/kfunc_call_test4:FAIL > > Looking at the assembly: > > ; long noinline bpf_kfunc_call_test4(signed char a, short b, int c, > long d) > 0000000000936a78 <bpf_kfunc_call_test4>: > 936a78: c0 04 00 00 00 00 jgnop 936a78 > <bpf_kfunc_call_test4> > ; return (long)a + (long)b + (long)c + d; > 936a7e: b9 08 00 45 agr %r4,%r5 > 936a82: b9 08 00 43 agr %r4,%r3 > 936a86: b9 08 00 24 agr %r2,%r4 > 936a8a: c0 f4 00 1e 3b 27 jg cfe0d8 > <__s390_indirect_jump_r14> > > As per the s390x ABI, bpf_kfunc_call_test4() has the right to assume > that a, b and c are sign-extended by the caller, which results in using > 64-bit additions (agr) without any additional conversions. > > On the JITed code side (without this hunk) we have: > > ; tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000); > ; 5: b4 10 00 00 ff ff ff fd w1 = -3 > 0x3ff7fdcdad4: llilf %r2,0xfffffffd > ; 6: b4 20 00 00 ff ff ff e2 w2 = -30 > 0x3ff7fdcdada: llilf %r3,0xffffffe2 > ; 7: b4 30 00 00 ff ff ff 38 w3 = -200 > 0x3ff7fdcdae0: llilf %r4,0xffffff38 > ; 8: b7 40 00 00 ff ff fc 18 r4 = -1000 > 0x3ff7fdcdae6: lgfi %r5,-1000 > 0x3ff7fdcdaec: mvc 64(4,%r15),160(%r15) > 0x3ff7fdcdaf2: lgrl %r1,bpf_kfunc_call_test4@GOT > 0x3ff7fdcdaf8: brasl %r14,__s390_indirect_jump_r1 > > This first 3 llilfs are 32-bit loads, that need to be sign-extended > to 64 bits. All makes sense. Please add this explanation to the commit log. When 2nd arch appears that needs similar sign extension in the caller we can add this logic to the verifier. In parallel we're working on new sign extension instructions. Doing sign extension with shifts in the verifier won't be efficient, so we need to wait for them.
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c index f4cdecb32629..74b38e817bd8 100644 --- a/arch/s390/net/bpf_jit_comp.c +++ b/arch/s390/net/bpf_jit_comp.c @@ -1401,9 +1401,10 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp, */ case BPF_JMP | BPF_CALL: { - u64 func; + const struct btf_func_model *m; bool func_addr_fixed; - int ret; + int j, ret; + u64 func; ret = bpf_jit_get_func_addr(fp, insn, extra_pass, &func, &func_addr_fixed); @@ -1425,6 +1426,21 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp, /* mvc STK_OFF_TCCNT(4,%r15),N(%r15) */ _EMIT6(0xd203f000 | STK_OFF_TCCNT, 0xf000 | (STK_OFF_TCCNT + STK_OFF + stack_depth)); + + /* Sign-extend the kfunc arguments. */ + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { + m = bpf_jit_find_kfunc_model(fp, insn); + if (!m) + return -1; + + for (j = 0; j < m->nr_args; j++) { + if (sign_extend(jit, BPF_REG_1 + j, + m->arg_size[j], + m->arg_flags[j])) + return -1; + } + } + /* lgrl %w1,func */ EMIT6_PCREL_RILB(0xc4080000, REG_W1, _EMIT_CONST_U64(func)); /* %r1() */ @@ -1980,6 +1996,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) return fp; } +bool bpf_jit_supports_kfunc_call(void) +{ + return true; +} + int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t, void *old_addr, void *new_addr) {
Implement calling kernel functions from eBPF. In general, the eBPF ABI is fairly close to that of s390x, with one important difference: on s390x callers should sign-extend signed arguments. Handle that by using information returned by bpf_jit_find_kfunc_model(). At the moment bpf_jit_find_kfunc_model() does not seem to play nicely with XDP metadata functions: add_kfunc_call() adds an "abstract" bpf_*() version to kfunc_btf_tab, but then fixup_kfunc_call() puts the concrete version into insn->imm, which bpf_jit_find_kfunc_model() cannot find. But this seems to be a common code problem. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- arch/s390/net/bpf_jit_comp.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)