Re: [PATCH bpf] riscv, bpf: Fix kfunc parameters incompatibility between bpf and riscv abi

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Pu Lehui <pulehui@huaweicloud.com>
Cc: bpf <bpf@vger.kernel.org>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	"Network Development" <netdev@vger.kernel.org>,
	"Björn Töpel" <bjorn@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Martin KaFai Lau" <martin.lau@linux.dev>,
	"Eduard Zingerman" <eddyz87@gmail.com>,
	"Song Liu" <song@kernel.org>, "Yonghong Song" <yhs@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@kernel.org>,
	"Stanislav Fomichev" <sdf@google.com>,
	"Hao Luo" <haoluo@google.com>, "Jiri Olsa" <jolsa@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Luke Nelson" <luke.r.nels@gmail.com>,
	"Pu Lehui" <pulehui@huawei.com>
Subject: Re: [PATCH bpf] riscv, bpf: Fix kfunc parameters incompatibility between bpf and riscv abi
Date: Mon, 25 Mar 2024 11:34:05 -0700	[thread overview]
Message-ID: <CAADnVQKnTe-7KhziOnGSesbz1WDkNp4nyCN3qp-y=ab0jMxr3Q@mail.gmail.com> (raw)
In-Reply-To: <c0890fc2-53ea-401a-a3b4-a9bf6181a867@huaweicloud.com>

On Mon, Mar 25, 2024 at 8:28 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
>
>
>
> On 2024/3/25 2:40, Alexei Starovoitov wrote:
> > On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
> [SNIP]
> >>
> >> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> >> index 869e4282a2c4..e3fc39370f7d 100644
> >> --- a/arch/riscv/net/bpf_jit_comp64.c
> >> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
> >>                  if (ret < 0)
> >>                          return ret;
> >>
> >> +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> >> +                       const struct btf_func_model *fm;
> >> +                       int idx;
> >> +
> >> +                       fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
> >> +                       if (!fm)
> >> +                               return -EINVAL;
> >> +
> >> +                       for (idx = 0; idx < fm->nr_args; idx++) {
> >> +                               u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
> >> +
> >> +                               if (fm->arg_size[idx] == sizeof(int))
> >> +                                       emit_sextw(reg, reg, ctx);
> >> +                       }
> >> +               }
> >> +
> >
> > The btf_func_model usage looks good.
> > Glad that no new flags were necessary, since both int and uint
> > need to be sign extend the existing arg_size was enough.
> >
> > Since we're at it. Do we need to do zero extension of return value ?
> > There is
> > __bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
> > but the selftest with it is too simple:
> >          return bpf_kfunc_call_test2((struct sock *)sk, 1, 2); >
> > Could you extend this selftest with a return of large int/uint
> > with 31th bit set to force sign extension in native
>
> Sorry for late. riscv64 will sign-extend int/uint return values. I
> thought this would be a good test, so I tried the following:
> ```
> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym; <-- here change int to u32
> int kfunc_call_test2(struct __sk_buff *skb)
> {
>         long tmp;
>
>         tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>         return (tmp >> 32) + tmp;
> }
> ```
> As expected, if the return value is sign-extended, the bpf program will
> return 0xfffffff1. If the return value is zero-extended, the bpf program
> will return 0xfffffff2. But in fact, riscv returns 0xfffffff2. Upon
> further discovery, it seems clang will compensate for unsigned return
> values. Curious!
> for example:
> ```
> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
> int kfunc_call_test2(struct __sk_buff *skb)
> {
>         long tmp;
>
>         tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>         bpf_printk("tmp: 0x%lx", tmp);
>         return (tmp >> 32) + tmp;
> }
> ```
> and the bytecode will be:
> ```
>   0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
> 0xf0000000 ll
>   2:       b7 02 00 00 02 00 00 00 r2 = 0x2
>   3:       85 10 00 00 ff ff ff ff call -0x1
>   4:       bf 06 00 00 00 00 00 00 r6 = r0
>   5:       bf 63 00 00 00 00 00 00 r3 = r6
>   6:       67 03 00 00 20 00 00 00 r3 <<= 0x20 <-- zero extension
>   7:       77 03 00 00 20 00 00 00 r3 >>= 0x20
>   8:       18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
> 10:       b7 02 00 00 0b 00 00 00 r2 = 0xb
> 11:       85 00 00 00 06 00 00 00 call 0x6
> 12:       bf 60 00 00 00 00 00 00 r0 = r6
> 13:       95 00 00 00 00 00 00 00 exit
> ```
>
> another example:
> ```
> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
> int kfunc_call_test2(struct __sk_buff *skb)
> {
>         long tmp;
>
>         tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>         return (tmp >> 20) + tmp; <-- change from 32 to 20
> }
> ```
> and the bytecode will be:
> ```
>   0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
> 0xf0000000 ll
>   2:       b7 02 00 00 02 00 00 00 r2 = 0x2
>   3:       85 10 00 00 ff ff ff ff call -0x1
>   4:       18 02 00 00 00 00 f0 ff 00 00 00 00 00 00 00 00 r2 =
> 0xfff00000 ll <-- 32-bit truncation
>   6:       bf 01 00 00 00 00 00 00 r1 = r0
>   7:       5f 21 00 00 00 00 00 00 r1 &= r2
>   8:       77 01 00 00 14 00 00 00 r1 >>= 0x14
>   9:       0f 01 00 00 00 00 00 00 r1 += r0
> 10:       bf 10 00 00 00 00 00 00 r0 = r1
> 11:       95 00 00 00 00 00 00 00 exit
> ```
>
> It is difficult to construct this test case.

Yeah.
I also tried a bunch of experiments with llvm and gcc-bpf.
Both compilers emit zero extension when u32 is being used as u64.

> > kernel risc-v code ?
> > I suspect the bpf side will be confused.
> > Which would mean that risc-v JIT in addition to:
> >          if (insn->src_reg != BPF_PSEUDO_CALL)
> >              emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
> >
> > need to conditionally do:
> >   if (fm->ret_size == sizeof(int))
> >     emit_zextw(bpf_to_rv_reg(BPF_REG_0, ctx),
> >                bpf_to_rv_reg(BPF_REG_0, ctx), ctx);
> > ?
>
> Agree on zero-extending int/uint return values when returning from
> kfunc to bpf ctx. I will add it in next version. Thanks.

Looking at existing compilers behavior it's probably unnecessary.
I think this patch is fine as-is.
I'll apply it shortly.