Re: [PATCH bpf-next v4 2/8] bpf: x86: Support in-register struct arguments in trampoline programs

[Yonghong Song <yhs@fb.com>]

On 9/6/22 8:00 PM, Alexei Starovoitov wrote:
> On Wed, Aug 31, 2022 at 8:26 AM 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.
>>
>> The latest llvm16 added bpf support to pass by values
>> for struct up to 16 bytes ([1]). This is also true for
>> x86_64 architecture where two registers will hold
>> the struct value if the struct size is >8 and <= 16.
>> This may not be true if one of struct member is 'double'
>> type but in current linux source code we don't have
>> such instance yet, so we assume all >8 && <= 16 struct
>> holds two general purpose argument registers.
>>
>> Also change on-stack nr_args value to the number
>> of registers holding the arguments. This will
>> permit bpf_get_func_arg() helper to get all
>> argument values.
>>
>>   [1] https://reviews.llvm.org/D132144
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   arch/x86/net/bpf_jit_comp.c | 68 +++++++++++++++++++++++++++----------
>>   1 file changed, 51 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index c1f6c1c51d99..ae89f4143eb4 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -1751,34 +1751,60 @@ st:                     if (is_imm8(insn->off))
>>   static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
>>                        int stack_size)
>>   {
>> -       int i;
>> +       int i, j, arg_size, nr_regs;
>>          /* Store function arguments to stack.
>>           * For a function that accepts two pointers the sequence will be:
>>           * mov QWORD PTR [rbp-0x10],rdi
>>           * mov QWORD PTR [rbp-0x8],rsi
>>           */
>> -       for (i = 0; i < min(nr_args, 6); i++)
>> -               emit_stx(prog, bytes_to_bpf_size(m->arg_size[i]),
>> -                        BPF_REG_FP,
>> -                        i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
>> -                        -(stack_size - i * 8));
>> +       for (i = 0, j = 0; i < min(nr_args, 6); i++) {
>> +               if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
>> +                       nr_regs = (m->arg_size[i] + 7) / 8;
>> +                       arg_size = 8;
>> +               } else {
>> +                       nr_regs = 1;
>> +                       arg_size = m->arg_size[i];
>> +               }
> 
> This bit begs for a common helper, but I'm not sure
> whether it will look better, so applied as-is.
> 
> BPF_PROG2 also feels unusual as an API macro name.
> We probably should bikeshed a bit and follow up
> if a better name is found.

I didn't come up with a better name either. But happy
to change to a different name if we agreed on one.