| Message ID | 20240324103306.2202954-1-pulehui@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [bpf] riscv, bpf: Fix kfunc parameters incompatibility between bpf and riscv abi | expand |
Pu Lehui <pulehui@huaweicloud.com> writes: > From: Pu Lehui <pulehui@huawei.com> > > We encountered a failing case when running selftest in no_alu32 mode: > > The failure case is `kfunc_call/kfunc_call_test4` and its source code is > like bellow: > ``` > long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym; > int kfunc_call_test4(struct __sk_buff *skb) > { > ... > tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000); > ... > } > ``` > > And its corresponding asm code is: > ``` > 0: r1 = -3 > 1: r2 = -30 > 2: r3 = 0xffffff38 # opcode: 18 03 00 00 38 ff ff ff 00 00 00 00 00 00 00 00 > 4: r4 = -1000 > 5: call bpf_kfunc_call_test4 > ``` > > insn 2 is parsed to ld_imm64 insn to emit 0x00000000ffffff38 imm, and > converted to int type and then send to bpf_kfunc_call_test4. But since > it is zero-extended in the bpf calling convention, riscv jit will > directly treat it as an unsigned 32-bit int value, and then fails with > the message "actual 4294966063 != expected -1234". > > The reason is the incompatibility between bpf and riscv abi, that is, > bpf will do zero-extension on uint, but riscv64 requires sign-extension > on int or uint. We can solve this problem by sign extending the 32-bit > parameters in kfunc. > > The issue is related to [0], and thanks to Yonghong and Alexei. > > Link: https://github.com/llvm/llvm-project/pull/84874 [0] > Fixes: d40c3847b485 ("riscv, bpf: Add kfunc support for RV64") > Signed-off-by: Pu Lehui <pulehui@huawei.com> > --- > arch/riscv/net/bpf_jit_comp64.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > 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); > + } > + } > + > ret = emit_call(addr, fixed_addr, ctx); > if (ret) > return ret; > -- > 2.34.1 Thanks for doing this, it fixes the issue I was seeing with arena_htab selftest after enabling arena on RISCV. Tested-by: Puranjay Mohan <puranjay12@gmail.com> Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@huaweicloud.com> wrote: > > From: Pu Lehui <pulehui@huawei.com> > > We encountered a failing case when running selftest in no_alu32 mode: > > The failure case is `kfunc_call/kfunc_call_test4` and its source code is > like bellow: > ``` > long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym; > int kfunc_call_test4(struct __sk_buff *skb) > { > ... > tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000); > ... > } > ``` > > And its corresponding asm code is: > ``` > 0: r1 = -3 > 1: r2 = -30 > 2: r3 = 0xffffff38 # opcode: 18 03 00 00 38 ff ff ff 00 00 00 00 00 00 00 00 > 4: r4 = -1000 > 5: call bpf_kfunc_call_test4 > ``` > > insn 2 is parsed to ld_imm64 insn to emit 0x00000000ffffff38 imm, and > converted to int type and then send to bpf_kfunc_call_test4. But since > it is zero-extended in the bpf calling convention, riscv jit will > directly treat it as an unsigned 32-bit int value, and then fails with > the message "actual 4294966063 != expected -1234". > > The reason is the incompatibility between bpf and riscv abi, that is, > bpf will do zero-extension on uint, but riscv64 requires sign-extension > on int or uint. We can solve this problem by sign extending the 32-bit > parameters in kfunc. > > The issue is related to [0], and thanks to Yonghong and Alexei. > > Link: https://github.com/llvm/llvm-project/pull/84874 [0] > Fixes: d40c3847b485 ("riscv, bpf: Add kfunc support for RV64") > Signed-off-by: Pu Lehui <pulehui@huawei.com> > --- > arch/riscv/net/bpf_jit_comp64.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > 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 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); ?
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.
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.
On 2024/3/26 2:34, Alexei Starovoitov wrote: > 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. Alright, feel free to apply it. Thanks
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); + } + } + ret = emit_call(addr, fixed_addr, ctx); if (ret) return ret;