Re: [PATCH bpf-next v3 5/7] libbpf: Add new BPF_PROG2 macro

[Yonghong Song <yhs@fb.com>]

On 8/29/22 5:32 PM, Kui-Feng Lee wrote:
> On Sat, 2022-08-27 at 19:55 -0700, Yonghong Song wrote:
>> To support struct arguments in trampoline based programs,
>> existing BPF_PROG doesn't work any more since
>> the type size is needed to find whether a parameter
>> takes one or two registers. So this patch added a new
>> BPF_PROG2 macro to support such trampoline programs.
>>
>> The idea is suggested by Andrii. For example, if the
>> to-be-traced function has signature like
>>    typedef struct {
>>         void *x;
>>         int t;
>>    } sockptr;
>>    int blah(sockptr x, char y);
>>
>> In the new BPF_PROG2 macro, the argument can be
>> represented as
>>    __bpf_prog_call(
>>       ({ union {
>>            struct { __u64 x, y; } ___z;
>>            sockptr x;
>>          } ___tmp = { .___z = { ctx[0], ctx[1] }};
>>          ___tmp.x;
>>       }),
>>       ({ union {
>>            struct { __u8 x; } ___z;
>>            char y;
>>          } ___tmp = { .___z = { ctx[2] }};
>>          ___tmp.y;
>>       }));
>> In the above, the values stored on the stack are properly
>> assigned to the actual argument type value by using 'union'
>> magic. Note that the macro also works even if no arguments
>> are with struct types.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/bpf_tracing.h | 82
>> +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 82 insertions(+)
>>
>> diff --git a/tools/lib/bpf/bpf_tracing.h
>> b/tools/lib/bpf/bpf_tracing.h
>> index 5fdb93da423b..c59ae9c8ccbd 100644
>> --- a/tools/lib/bpf/bpf_tracing.h
>> +++ b/tools/lib/bpf/bpf_tracing.h
>> @@ -438,6 +438,88 @@ typeof(name(0)) name(unsigned long long
>> *ctx)                                  \
>>   static __always_inline
>> typeof(name(0))                                     \
>>   ____##name(unsigned long long *ctx, ##args)
>>   
>> +#ifndef ____bpf_nth
>> +#define ____bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11,
>> _12, _13, _14, _15, _16, _17, _18, _19, _20, _21, _22, _23, _24, N,
>> ...) N
> 
> ____bpf_nth() is a confusing name.  I will expect to give any number n
> to return the nth argument.  However, here it return the value of an
> argument at a specific/fixed position.  ____bpf_25th() would make more
> sense for me.

In bpf_tracing.h, we have
#define ___bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, N, 
...) N
for 12 arguments.

The above is similar to ___bpf_nth (with one less _ compared to 
____bpf_nth).

On the other hand, I can just modify the existing ___bpf_nth for 24 
arguments so we don't need ____bpf_nth any more.
Will do this in the next revision.

> 
>> +#endif
>> +#ifndef ____bpf_narg
>> +#define ____bpf_narg(...) ____bpf_nth(_, ##__VA_ARGS__, 12, 12, 11,
>> 11, 10, 10, 9, 9, 8, 8, 7, 7, 6, 6, 5, 5, 4, 4, 3, 3, 2, 2, 1, 1, 0)
>> +#endif
>> +
>> +#define BPF_REG_CNT(t) \
>> +       (__builtin_choose_expr(sizeof(t) == 1, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 2, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 4, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 8, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 16, 2,      \
>> +                              (void)0))))))
> 
> nit: Using ternary operator will still work, right?
> For example,
> 
>   (sizeof(t) == 1 || sizeof(t) == 2 || sizeof(t) == 4 || sizeof(t) == 8)
> ?  1 : (sizeof(t) == 16 ? 2 : 0)
> 
> Compilers should be able to optimize it as a const value.

Good idea. Will change in the next revision.

> 
>> +
>> +#define ____bpf_reg_cnt0()                     (0)
>> +#define ____bpf_reg_cnt1(t, x)                 (____bpf_reg_cnt0() +
>> BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt2(t, x,
>> args...)                (____bpf_reg_cnt1(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt3(t, x,
>> args...)                (____bpf_reg_cnt2(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt4(t, x,
>> args...)                (____bpf_reg_cnt3(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt5(t, x,
>> args...)                (____bpf_reg_cnt4(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt6(t, x,
>> args...)                (____bpf_reg_cnt5(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt7(t, x,
>> args...)                (____bpf_reg_cnt6(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt8(t, x,
>> args...)                (____bpf_reg_cnt7(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt9(t, x,
>> args...)                (____bpf_reg_cnt8(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt10(t, x,
>> args...)       (____bpf_reg_cnt9(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt11(t, x,
>> args...)       (____bpf_reg_cnt10(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt12(t, x,
>> args...)       (____bpf_reg_cnt11(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt(args...)
>> ___bpf_apply(____bpf_reg_cnt, ____bpf_narg(args))(args)
>> +
>> +#define ____bpf_union_arg(t, x, n) \
>> +       __builtin_choose_expr(sizeof(t) == 1, ({ union { struct {
>> __u8 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]}}; ___tmp.x; }), \
>> +       __builtin_choose_expr(sizeof(t) == 2, ({ union { struct {
>> __u16 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }),
>> \
>> +       __builtin_choose_expr(sizeof(t) == 4, ({ union { struct {
>> __u32 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }),
>> \
>> +       __builtin_choose_expr(sizeof(t) == 8, ({ union { struct {
>> __u64 x; } ___z; t x; } ___tmp = {.___z = {ctx[n]} }; ___tmp.x; }), \
>> +       __builtin_choose_expr(sizeof(t) == 16, ({ union { struct {
>> __u64 x, y; } ___z; t x; } ___tmp = {.___z = {ctx[n], ctx[n + 1]} };
>> ___tmp.x; }), \
>> +                             (void)0)))))
> 
> Is it possible to cast &ctx[n] to the pointer of the target type?
> For example,
> 
>    *(t*)&ctx[n]
> 
> Did I miss any thing?

This won't work.

ctx[n] represents a u64 value.
Let us say type is 'u32'.   *(u32 *)&ctx[n]
works for little endian, but won't work for big endian
system.

> 
>> +
>> +#define ____bpf_ctx_arg0(n, args...)
>> +#define ____bpf_ctx_arg1(n, t, x)              ,
>> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt1(t, x))
>> +#define ____bpf_ctx_arg2(n, t, x, args...)     ,
>> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt2(t, x, args))
>> ____bpf_ctx_arg1(n, args)
>> +#define ____bpf_ctx_arg3(n, t, x, args...)     ,
>> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt3(t, x, args))
>> ____bpf_ctx_arg2(n, args)
[...]