On 2024/3/26 2:34, Alexei Starovoitov wrote:
On Mon, Mar 25, 2024 at 8:28 AM Pu Lehui <pulehui@xxxxxxxxxxxxxxx> wrote:
On 2024/3/25 2:40, Alexei Starovoitov wrote:
On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@xxxxxxxxxxxxxxx> 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
