On Wed, Aug 17, 2022 at 09:56:23PM -0700, Yonghong Song wrote:
On 8/15/22 3:44 PM, Alexei Starovoitov wrote:
On Thu, Aug 11, 2022 at 10:24 PM Yonghong Song <yhs@xxxxxx> wrote:
In C, struct value can be passed as a function argument.
For small structs, struct value may be passed in
one or more registers. For trampoline based bpf programs,
This would cause complication since one-to-one mapping between
function argument and arch argument register is not valid
any more.
To support struct value argument and make bpf programs
easy to write, the bpf program function parameter is
changed from struct type to a pointer to struct type.
The following is a simplified example.
In one of later selftests, we have a bpf_testmod function:
struct bpf_testmod_struct_arg_2 {
long a;
long b;
};
noinline int
bpf_testmod_test_struct_arg_2(int a, struct bpf_testmod_struct_arg_2 b, int c) {
bpf_testmod_test_struct_arg_result = a + b.a + b.b + c;
return bpf_testmod_test_struct_arg_result;
}
When a bpf program is attached to the bpf_testmod function
bpf_testmod_test_struct_arg_2(), the bpf program may look like
SEC("fentry/bpf_testmod_test_struct_arg_2")
int BPF_PROG(test_struct_arg_3, int a, struct bpf_testmod_struct_arg_2 *b, int c)
{
t2_a = a;
t2_b_a = b->a;
t2_b_b = b->b;
t2_c = c;
return 0;
}
Basically struct value becomes a pointer to the struct.
The trampoline stack will be increased to store the stack values and
the pointer to these values will be saved in the stack slot corresponding
to that argument. For x86_64, the struct size is limited up to 16 bytes
so the struct can fit in one or two registers. The struct size of more
than 16 bytes is not supported now as our current use case is
for sockptr_t in the argument. We could handle such large struct's
in the future if we have concrete use cases.
The main changes are in save_regs() and restore_regs(). The following
illustrated the trampoline asm codes for save_regs() and restore_regs().
save_regs():
/* first argument */
mov DWORD PTR [rbp-0x18],edi
/* second argument: struct, save actual values and put the pointer to the slot */
lea rax,[rbp-0x40]
mov QWORD PTR [rbp-0x10],rax
mov QWORD PTR [rbp-0x40],rsi
mov QWORD PTR [rbp-0x38],rdx
/* third argument */
mov DWORD PTR [rbp-0x8],esi
restore_regs():
mov edi,DWORD PTR [rbp-0x18]
mov rsi,QWORD PTR [rbp-0x40]
mov rdx,QWORD PTR [rbp-0x38]
mov esi,DWORD PTR [rbp-0x8]
Not sure whether it was discussed before, but
why cannot we adjust the bpf side instead?
Technically struct passing between bpf progs was never
officially supported. llvm generates something.
Probably always passes by reference, but we can adjust
that behavior without breaking any programs because
we don't have bpf libraries. Programs are fully contained
in one or few files. libbpf can do static linking, but
without any actual libraries the chance of breaking
backward compat is close to zero.
Agree. At this point, we don't need to worry about
compatibility between bpf program and bpf program libraries.
Can we teach llvm to pass sizeof(struct) <= 16 in
two bpf registers?
Yes, we can. I just hacked llvm and was able to
do that.
Then we wouldn't need to have a discrepancy between
kernel function prototype and bpf fentry prog proto.
Both will have struct by value in the same spot.
The trampoline generation will be simpler for x86 and
its runtime faster too.
I tested x86 and arm64 both supports two registers
for a 16 byte struct.
The other architectures that pass small structs by reference
can do a bit more work in the trampoline: copy up to 16 byte
and bpf prog side will see it as they were passed in 'registers'.
wdyt?
I know systemz and Hexagon will pass by reference for any
struct size >= 8 bytes. Didn't complete check others.
But since x86 and arm64 supports direct value passing
with two registers, we should be okay. As you mentioned,
we could support systemz/hexagon style of struct passing
by copying the values to the stack.
But I have a problem how to define a user friendly
macro like BPF_PROG for user to use.
Let us say, we have a program like below:
SEC("fentry/bpf_testmod_test_struct_arg_1")
int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 *a, int b,
int c) {
...
}
We have BPF_PROG macro definition here:
#define BPF_PROG(name, args...) \
name(unsigned long long *ctx); \
static __always_inline typeof(name(0)) \
____##name(unsigned long long *ctx, ##args); \
typeof(name(0)) name(unsigned long long *ctx) \
{ \
_Pragma("GCC diagnostic push") \
_Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
return ____##name(___bpf_ctx_cast(args)); \
_Pragma("GCC diagnostic pop") \
} \
static __always_inline typeof(name(0)) \
____##name(unsigned long long *ctx, ##args)
Some we have static function definition
int ____test_struct_arg_1(unsigned long long *ctx, struct
bpf_testmod_struct_arg_2 *a, int b, int c);
But the function call inside the function test_struct_arg_1()
is
____test_struct_arg_1(ctx, ctx[0], ctx[1], ctx[2]);
We have two problems here:
____test_struct_arg_1(ctx, ctx[0], ctx[1], ctx[2])
does not match the static function declaration.
This is not problem if everything is int/ptr type.
If one of argument is structure type, we will have
type conversion problem. Let us this can be resolved
somehow through some hack.
More importantly, because some structure may take two
registers,
____test_struct_arg_1(ctx, ctx[0], ctx[1], ctx[2])
may not be correct. In my above example, if the
structure size is 16 bytes,
then the actual call should be
____test_struct_arg_1(ctx, ctx[0], ctx[1], ctx[2], ctx[3])
So we need to provide how many extra registers are needed
beyond ##args in the macro. I have not tried how to
resolve this but this extra information in the macro
definite is not user friendly.
Not sure what is the best way to handle this issue (##args is not precise
and needs addition registers for >8 struct arguments).
The kernel is using this trick to cast 8 byte structs to u64:
/* cast any integer, pointer, or small struct to u64 */
#define UINTTYPE(size) \
__typeof__(__builtin_choose_expr(size == 1, (u8)1, \
__builtin_choose_expr(size == 2, (u16)2, \
__builtin_choose_expr(size == 4, (u32)3, \
__builtin_choose_expr(size == 8, (u64)4, \
(void)5)))))
#define __CAST_TO_U64(x) ({ \
typeof(x) __src = (x); \
UINTTYPE(sizeof(x)) __dst; \
memcpy(&__dst, &__src, sizeof(__dst)); \
(u64)__dst; })
casting 16 byte struct to two u64 can be similar.
Ideally we would declare bpf prog as:
SEC("fentry/bpf_testmod_test_struct_arg_1")
int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 a, int b, int c) {
note there is no '*'. It's struct by value.
The main challenge is how to do the math in the BPF_PROG macros.
Currently it's doing:
#define ___bpf_ctx_cast1(x) ___bpf_ctx_cast0(), (void *)ctx[0]
#define ___bpf_ctx_cast2(x, args...) ___bpf_ctx_cast1(args), (void *)ctx[1]
#define ___bpf_ctx_cast3(x, args...) ___bpf_ctx_cast2(args), (void *)ctx[2]
#define ___bpf_ctx_cast4(x, args...) ___bpf_ctx_cast3(args), (void *)ctx[3]
The ctx[index] is one-to-one with argument position.
That 'index' needs to be calculated.
Maybe something like UINTTYPE() that applies to previous arguments?
#define REG_CNT(arg) \
__builtin_choose_expr(sizeof(arg) == 1, 1, \
__builtin_choose_expr(sizeof(arg) == 2, 1, \
__builtin_choose_expr(sizeof(arg) == 4, 1, \
__builtin_choose_expr(sizeof(arg) == 8, 1, \
__builtin_choose_expr(sizeof(arg) == 16, 2, \
(void)0)))))
#define ___bpf_reg_cnt0() 0
#define ___bpf_reg_cnt1(x) ___bpf_reg_cnt0() + REG_CNT(x)
#define ___bpf_reg_cnt2(x, args...) ___bpf_reg_cnt1(args) + REG_CNT(x)
#define ___bpf_reg_cnt(args...) ___bpf_apply(___bpf_reg_cnt, ___bpf_narg(args))(args)
This way the macro will calculate the index inside ctx[] array.
and then inside ___bpf_ctx_castN macro use ___bpf_reg_cnt.
Instead of:
___bpf_ctx_cast3(x, args...) ___bpf_ctx_cast2(args), (void *)ctx[2]
it will be
___bpf_ctx_cast3(x, args...) ___bpf_ctx_cast2(args), \
__builtin_choose_expr(sizeof(x) <= 8, (void *)ctx[___bpf_reg_cnt(args)],
*(typeof(x) *) &ctx[___bpf_reg_cnt(args)])
