Message-ID: <c0890fc2-53ea-401a-a3b4-a9bf6181a867@huaweicloud.com>
Date: Mon, 25 Mar 2024 23:27:58 +0800
From: Pu Lehui <pulehui@...weicloud.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: bpf <bpf@...r.kernel.org>, linux-riscv <linux-riscv@...ts.infradead.org>,
 Network Development <netdev@...r.kernel.org>, Björn Töpel <bjorn@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
 <eddyz87@...il.com>, Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
 Luke Nelson <luke.r.nels@...il.com>, Pu Lehui <pulehui@...wei.com>
Subject: Re: [PATCH bpf] riscv, bpf: Fix kfunc parameters incompatibility
 between bpf and riscv abi



On 2024/3/25 2:40, Alexei Starovoitov wrote:
> On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@...weicloud.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.

Powered by blists - more mailing lists