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.
