On Fri, Jan 27, 2023 at 3:36 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> 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.
