From: Pu Lehui <pulehui@huaweicloud.com> To: Alexei Starovoitov <alexei.starovoitov@gmail.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 23:27:58 +0800 [thread overview] Message-ID: <c0890fc2-53ea-401a-a3b4-a9bf6181a867@huaweicloud.com> (raw) In-Reply-To: <CAADnVQLhfN7f6AFxa_19E0g2_YADEkrfPPffi43HeH9VCi8MqQ@mail.gmail.com> 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. > 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.
WARNING: multiple messages have this Message-ID (diff)
From: Pu Lehui <pulehui@huaweicloud.com> To: Alexei Starovoitov <alexei.starovoitov@gmail.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 23:27:58 +0800 [thread overview] Message-ID: <c0890fc2-53ea-401a-a3b4-a9bf6181a867@huaweicloud.com> (raw) In-Reply-To: <CAADnVQLhfN7f6AFxa_19E0g2_YADEkrfPPffi43HeH9VCi8MqQ@mail.gmail.com> 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. > 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. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2024-03-25 15:28 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-03-24 10:33 [PATCH bpf] riscv, bpf: Fix kfunc parameters incompatibility between bpf and riscv abi Pu Lehui 2024-03-24 10:33 ` Pu Lehui 2024-03-24 18:26 ` Puranjay Mohan 2024-03-24 18:26 ` Puranjay Mohan 2024-03-24 18:40 ` Alexei Starovoitov 2024-03-24 18:40 ` Alexei Starovoitov 2024-03-25 15:27 ` Pu Lehui [this message] 2024-03-25 15:27 ` Pu Lehui 2024-03-25 18:34 ` Alexei Starovoitov 2024-03-25 18:34 ` Alexei Starovoitov 2024-03-26 1:32 ` Pu Lehui 2024-03-26 1:32 ` Pu Lehui
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c0890fc2-53ea-401a-a3b4-a9bf6181a867@huaweicloud.com \ --to=pulehui@huaweicloud.com \ --cc=alexei.starovoitov@gmail.com \ --cc=andrii@kernel.org \ --cc=ast@kernel.org \ --cc=bjorn@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=eddyz87@gmail.com \ --cc=haoluo@google.com \ --cc=john.fastabend@gmail.com \ --cc=jolsa@kernel.org \ --cc=kpsingh@kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=luke.r.nels@gmail.com \ --cc=martin.lau@linux.dev \ --cc=netdev@vger.kernel.org \ --cc=palmer@dabbelt.com \ --cc=pulehui@huawei.com \ --cc=sdf@google.com \ --cc=song@kernel.org \ --cc=yhs@fb.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.