Re: [PATCH bpf-next v2 3/6] bpf: x86: Support in-register struct arguments

[Yonghong Song <yhs@fb.com>]

On 8/15/22 3:44 PM, Alexei Starovoitov wrote:
> On Thu, Aug 11, 2022 at 10:24 PM Yonghong Song <yhs@fb.com> 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).