[Clang][BPF] Allow sign extension for call parameters with int types #84874
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: None (yonghong-song) ChangesPu Lehui (pulehui@huaweicloud.com) reported an issue in private that at no_alu32 mode clang may generate code which produced incorrect result with riscv architecture. The affected bpf prog is kfunc_call_test4 at bpf selftests prog/kfunc_call_test.c. The following is the source code: For the above code, at no_alu32 mode (-mcpu=v2), the asm code looks like In bpf_kfunc_call_test4(), arguments with 'char', 'short' and 'long' are generated correctly, for r1, r2 and r4 in the above. But the argument r3 is generated with ld_imm64. Further investigation is found that
In riscv case, the 'r3' value (0xffffff38 ll) will be passed to riscv kernel code which does not do 32-bit sign extension and caused incorrect result. Why intel/arm64 does not have this issue? x86_64/arm64 supports subrgisters so for 'int' types, subregisters are directly used hence there is no issue. Considering BPF is a 64-bit arch, so I think it makes sense at IR level 'int' type argument should have the same sign-extension as 'char' or 'short' type. This will solve the above riscv issue. This patch will cause two codegen changes:
Tested with bpf selftests with all of no-alu32, cpu=v3 and cpu=v4, and all passed. Full diff: https://github.com/llvm/llvm-project/pull/84874.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/Targets/BPF.cpp b/clang/lib/CodeGen/Targets/BPF.cpp
index 2849222f7a1869..01937574779618 100644
--- a/clang/lib/CodeGen/Targets/BPF.cpp
+++ b/clang/lib/CodeGen/Targets/BPF.cpp
@@ -22,6 +22,19 @@ class BPFABIInfo : public DefaultABIInfo {
public:
BPFABIInfo(CodeGenTypes &CGT) : DefaultABIInfo(CGT) {}
+ bool isPromotableIntegerTypeForABI(QualType Ty) const {
+ if (ABIInfo::isPromotableIntegerTypeForABI(Ty) == true)
+ return true;
+
+ if (const auto *BT = Ty->getAs<BuiltinType>()) {
+ // For 'signed int' type, return true to allow sign-extension.
+ if (BT->getKind() == BuiltinType::Int)
+ return true;
+ }
+
+ return false;
+ }
+
ABIArgInfo classifyArgumentType(QualType Ty) const {
Ty = useFirstFieldIfTransparentUnion(Ty);
diff --git a/clang/test/CodeGen/bpf-abiinfo.c b/clang/test/CodeGen/bpf-abiinfo.c
index 366e8003f45572..6d259d8e6d6c73 100644
--- a/clang/test/CodeGen/bpf-abiinfo.c
+++ b/clang/test/CodeGen/bpf-abiinfo.c
@@ -22,3 +22,13 @@ int foo_int(void) {
if (bar_int() != 10) return 0; else return 1;
}
// CHECK: %call = call i32 @bar_int()
+
+void sprog1(short, int, int);
+void mprog1() {
+ sprog1(-3, 4, -5);
+// CHECK: call void @sprog1(i16 noundef signext -3, i32 noundef signext 4, i32 noundef signext -5)
+}
+void mprog2(long a, long b) {
+ sprog1(a, b, b);
+// CHECK: call void @sprog1(i16 noundef signext %{{[0-9a-z]+}}, i32 noundef signext %{{[0-9a-z]+}}, i32 noundef signext %{{[0-9a-z]+}})
+}
diff --git a/llvm/test/CodeGen/BPF/cc_args_int.ll b/llvm/test/CodeGen/BPF/cc_args_int.ll
new file mode 100644
index 00000000000000..79a9d27b87d709
--- /dev/null
+++ b/llvm/test/CodeGen/BPF/cc_args_int.ll
@@ -0,0 +1,34 @@
+; RUN: llc -march=bpfel -mcpu=v1 < %s | FileCheck --check-prefix=CHECK-V1 %s
+; RUN: llc -march=bpfel -mcpu=v2 < %s | FileCheck --check-prefix=CHECK-V2 %s
+; RUN: llc -march=bpfel -mcpu=v3 < %s | FileCheck --check-prefix=CHECK-V3 %s
+; RUN: llc -march=bpfel -mcpu=v4 < %s | FileCheck --check-prefix=CHECK-V4 %s
+
+declare dso_local void @bar(i16 noundef signext, i32 noundef signext, i32 noundef signext) local_unnamed_addr
+
+define void @test() {
+entry:
+ tail call void @bar(i16 noundef signext -3, i32 noundef signext 4, i32 noundef signext -5)
+; CHECK-V1: r2 = 4
+; CHECK-V1: r3 = -5
+; CHECK-V2: r2 = 4
+; CHECK-V2: r3 = -5
+; CHECK-V3: w2 = 4
+; CHECK-V3: w3 = -5
+; CHECK-V4: w2 = 4
+; CHECK-V4: w3 = -5
+ ret void
+}
+
+define dso_local void @test2(i64 noundef %a, i64 noundef %b) local_unnamed_addr {
+entry:
+ %conv = trunc i64 %a to i16
+ %conv1 = trunc i64 %b to i32
+ tail call void @bar(i16 noundef signext %conv, i32 noundef signext %conv1, i32 noundef signext %conv1)
+; CHECK-V1: r2 <<= 32
+; CHECK-V1: r2 s>>= 32
+; CHECK-V2: r2 <<= 32
+; CHECK-V2: r2 s>>= 32
+; CHECK-V3-NOT: r2 <<= 32
+; CHECK-V4-NOT: r2 <<= 32
+ ret void
+}
|
|
Thanks Yonghong. It works through the kfunc_call case in no_alu32 mode, as well as other cases in riscv64. |
|
|
||
| if (const auto *BT = Ty->getAs<BuiltinType>()) { | ||
| // For 'signed int' type, return true to allow sign-extension. | ||
| if (BT->getKind() == BuiltinType::Int) |
There was a problem hiding this comment.
should BuiltinType::UInt be included as well?
There was a problem hiding this comment.
should BuiltinType::UInt be included as well?
Yes, make sense. I will add that as well.
Pu Lehui (pulehui@huaweicloud.com) reported an issue in private
that at no_alu32 mode clang may generate code which produced
incorrect result with riscv architecture.
The affected bpf prog is kfunc_call_test4 at bpf selftests
prog/kfunc_call_test.c. The following is the source code:
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);
...
}
For the above code, at no_alu32 mode (-mcpu=v2), the asm code looks like
0: r1 = -3
1: r2 = -30
2: r3 = 0xffffff38 ll // 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
In bpf_kfunc_call_test4(), arguments with 'char', 'short' and 'long' are
generated correctly, for r1, r2 and r4 in the above. But the argument
r3 is generated with ld_imm64.
Further investigation is found that
- char/short type arguments are signed extended so naturally using MOV insn
- int type argument are zero extended so using ld_imm64 insn
- long type argument can do sign extension with 32-bit value so using MOV insn
In riscv case, the 'r3' value (0xffffff38 ll) will be passed to riscv kernel
code which does not do 32-bit sign extension and caused incorrect result.
Why intel/arm64 does not have this issue? x86_64/arm64 supports subrgisters
so for 'int' types, subregisters are directly used hence there is no issue.
Considering BPF is a 64-bit arch, so I think it makes sense at IR level
'int' type argument should have the same sign-extension as 'char' or 'short'
type. This will solve the above riscv issue.
This patch will cause two codegen changes:
- for an 'int' constant argument, a MOV insn will be used instead of a ld_imm64.
- for an 'int' register argument, for cpu=v1/v2, left/right shift will happen.
for cpu=v3/v4, there is no change from previous behavior as subregisters will
be used.
Tested with bpf selftests with all of no-alu32, cpu=v3 and cpu=v4, and
all passed.
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
|
The way LLVM backend works in general is that if an integer is smaller than a register, when type legalization converts that to a larger integer, the high bits are undefined. This generally doesn't cause any issues because nothing can actually observe those bits. Your analysis that concludes "int" is special is probably wrong; I think you're just getting lucky with heuristics for lowering constants. Some backends explicitly zero-extend or sign-extend function arguments to match ABI conventions, at least in some cases. The BPF backend is one of those backends: BPFTargetLowering::LowerCall will sign-extend arguments marked "signext". Sign-extending 32-bit integers is a choice you can make, I guess, but if you're not trying to conform to an external ABI specification, it doesn't really solve anything. |
yonghong-song
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@efriedma-quic Thanks for your comments. BPF llvm backend and kernel BPF jit try to make BPF<->other_architecture compatible. riscv started to support BPF starting in 2020 and probably BPF has limited usage so the bug is exposed until now. |
There was a problem hiding this comment.
Oh, this is making a bit more sense now... I didn't realize this was overriding a method that handles char/short already. And I guess for BPF targets, you can assume "long" is 64 bits.
That said, this isn't handling "unsigned int" correctly. "unsigned int" is supposed to be sign-extended on RISC-V targets. (How you resolve the conflict with PowerPC, where "unsigned int" is supposed to be zero-extended, I'm not sure.)
|
Hi @efriedma-quic,
Could you please elaborate a bit?
I assume that sign extension operations for RV64 take care of this detail. |
|
The following is an example to show 'unsigned int' case. The current behavior (i.e., without this patch): Basically, we didn't do zero extension for 'uint' argument in the above. We expect the callee 'foo' to do proper zero extension. If both 'foo' and 'far' are functions written in bpf programs, we should be okay. But if 'foo' is a kfunc (e.g., a kernel function for a specific arch e.g. riscv) and the arch will just use the full register 'r1' to do operation without zero extension of 32-bit value, the wrong results could happen. With this patch, we will have the following asm code, That is, we will do zero extension for uint argument here. This will guarantee correct value if 'foo' is a kernel function. If 'foo' is not a kernel function, the 'foo' will be able to use r2 directly as a 32-bit value without doing shifting, one example like below |
|
@pulehui Could you check whether with -mcpu=v2 (no-alu32 mode) we have 'unsigned int' related issue or not? Specifically, given a 'unsigned int' does riscv use subregister to access 32-bit value, or use 64-bit register to access the value without zero-extension of 32-bit value? |
Yes. For example, on RISC-V, the function
clang would guarantee sign-extension for signed parameters, and zero-extension for unsigned parameters. Which is not the same as the RISCV ABI rules.
A kernel function will expect the uint to be sign-extended, not zero-extended. |
|
For this one:
I suspect riscv bpf jit will do sign-extension. @pulehui can confirm. This may be true for 32-bit subregisters, but I am not sure whether this is true for full register or not. |
Sorry for late. riscv64 will do sign-extension for 'unsigned int' and riscv64 bpf JIT will do the same. We need to explicitly zero-extend it. But I don't have problems with ‘unsigned int‘ before this patch. if a = 2147483647(0x7FFFFFFFU, bit 31 is 0), it will be a mov insn. It will be valid 'unsigned int' in kfunc after sign-extension. but if a = 4294967294(0xFFFFFFFEU, bit 31 is 1), it will be a ld_imm64 insn. And it also be valid 'unsigned int' in kfunc. But after this patch, the two numbers get the same result as before this patch. I don't see ld_imm64 becoming a mov instruction, as well as explicit zero extension for 'unsigned int'. Is that expected? |
|
@yonghong-song , if I understand @efriedma-quic correctly (thank you for explanations), the following fragment is not RISC-V ABI conformant: The |
IIUC, we have the same result before and after this patch. So this patch didn't make things worse or better. So didn't really solve your problem for 'unsigned int'. |
I do not know how riscv handles the above additional left/right shift and not sure whether it introduced additional issue comparing to without left/right shift. I guess I will remove UInt support from this patch since it does not improve anything and may risk break something. We can add it back later if needed and other issue (with riscv) is clarified/resolved. |
|
Just uploaded a new revision which targets 'Int' type only, i.e, v1. |
yonghong-song
changed the title
|
@pulehui On top of this patch, the following is a hack to emit 'sign-extension' for uint types (unsigned char/short not affected). The llvm hack: The following is a simple example, But I really do not like to generate BPF codes for different architectures. Maybe riscv can propose some code patterns where bpf backend can generate generic code (for all arches) and those codes can be easier for riscv jit to do proper sign extension? |
Looks like it's a bug in risc-v JIT. We never promised that kfunc call from bpf calling convention into native cpu calling convention will be free on all architectures. That's why we have btf_func_model and JITs suppose to use it when calling conventions don't match. It seems risc-v JIT needs to emit sign extension when 'int' argument is passed into kfunc. |
|
@pulehui I will not merge this patch now since it does not really solve the whole riscv issue. As @4ast commented above, the verifier made btf_func_model to jit. For example, for each argument, you will know its size and flags has some information as well.The flags has So you will know whether it is a struct or signed or not. If you feel you need explicit UNSIGNED flag, you can add it in btf as well. Let us try to resolve the issue in riscv backend. Thanks! |
Thanks for the clarification. riscv64 will do sign-extension both for int and uint. For uint, zero-extension is required, but after my investigation, it will be handled in the callee. |
Thanks Yonghong and Alexei, will try to resolve this incompatibility between bpf abi and riscv abi. |
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
Tested-by: Puranjay Mohan <puranjay12@gmail.com>
Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
Tested-by: Puranjay Mohan <puranjay12@gmail.com>
Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
Link: https://lore.kernel.org/r/20240324103306.2202954-1-pulehui@huaweicloud.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
Tested-by: Puranjay Mohan <puranjay12@gmail.com>
Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
Link: https://lore.kernel.org/r/20240324103306.2202954-1-pulehui@huaweicloud.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
…v abi
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: llvm/llvm-project#84874 [0]
Fixes: d40c384 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
Tested-by: Puranjay Mohan <puranjay12@gmail.com>
Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
Link: https://lore.kernel.org/r/20240324103306.2202954-1-pulehui@huaweicloud.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Pu Lehui (pulehui@huaweicloud.com) reported an issue in private that at no_alu32 mode clang may generate code which produced incorrect result with riscv architecture.
The affected bpf prog is kfunc_call_test4 at bpf selftests prog/kfunc_call_test.c. The following is the source code:
For the above code, at no_alu32 mode (-mcpu=v2), the asm code looks like
0: r1 = -3
1: r2 = -30
2: r3 = 0xffffff38 ll // 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
In bpf_kfunc_call_test4(), arguments with 'char', 'short' and 'long' are generated correctly, for r1, r2 and r4 in the above. But the argument r3 is generated with ld_imm64.
Further investigation is found that
In riscv case, the 'r3' value (0xffffff38 ll) will be passed to riscv kernel code which does not do 32-bit sign extension and caused incorrect result.
Why intel/arm64 does not have this issue? x86_64/arm64 supports subrgisters so for 'int' types, subregisters are directly used hence there is no issue.
Considering BPF is a 64-bit arch, so I think it makes sense at IR level
'int' type argument should have the same sign-extension as 'char' or 'short'
type. This will solve the above riscv issue.
This patch will cause two codegen changes:
- for an 'int' constant argument, a MOV insn will be used instead of a ld_imm64.
- for an 'int' register argument, for cpu=v1/v2, left/right shift will happen.
for cpu=v3/v4, there is no change from previous behavior as subregisters will
be used.
Tested with bpf selftests with all of no-alu32, cpu=v3 and cpu=v4, and all passed.