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)])
x - is one of the arguments.
args - all args before 'x'. Doing __bpf_reg_cnt on them should calculate the index.
*(typeof(x) *)& should type cast to struct of 16 bytes.
Rough idea, of course.
Another alternative is instead of:
#define BPF_PROG(name, args...)
name(unsigned long long *ctx);
do:
#define BPF_PROG(name, args...)
struct XX {
macro inserts all 'args' here separated by ; so it becomes a proper struct
};
name(struct XX *ctx);
and then instead of doing ___bpf_ctx_castN for each argument
do single cast of all of 'u64 ctx[N]' passed from fentry into 'struct XX *'.
The problem with this approach that small args like char, short, int needs to
be declared in struct XX with __align__(8).
Both approaches may be workable?
