Re: [PATCH bpf-next] bpf, arm64: Support up to 12 function arguments

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Sep 17, 2023 at 5:09 PM Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:
>
> From: Xu Kuohai <xukuohai@xxxxxxxxxx>
>
> Currently arm64 bpf trampoline supports up to 8 function arguments.
> According to the statistics from commit
> 473e3150e30a ("bpf, x86: allow function arguments up to 12 for TRACING"),
> there are about 200 functions accept 9 to 12 arguments, so adding support
> for up to 12 function arguments.

Thank you Xu, this will be a nice addition! :)

> Due to bpf only supports function arguments up to 16 bytes, according to
> AAPCS64, starting from the first argument, each argument is first
> attempted to be loaded to 1 or 2 smallest registers from x0-x7, if there
> are no enough registers to hold the entire argument, then all remaining
> arguments starting from this one are pushed to the stack for passing.

If I read the section 6.8.2 of the AAPCS64 correctly, there is a
corner case which I believe isn't covered by this logic.

void f(u128 a, u128 b, u128, c, u64 d, u128 e, u64 f) {}
- a goes on x0 and x1
- b goes on x2 and x3
- c goes on x4 and x5
- d goes on x6
- e spills on the stack because it doesn't fit in the remaining regs
- f goes on x7

Maybe it would be good to add something pathological like this to the
selftests ?

Otherwise I only have minor nitpicks.

> Signed-off-by: Xu Kuohai <xukuohai@xxxxxxxxxx>
> ---
>  arch/arm64/net/bpf_jit_comp.c                | 171 ++++++++++++++-----
>  tools/testing/selftests/bpf/DENYLIST.aarch64 |   2 -
>  2 files changed, 131 insertions(+), 42 deletions(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 7d4af64e3982..a0cf526b07ea 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -1705,7 +1705,7 @@ bool bpf_jit_supports_subprog_tailcalls(void)
>  }
>
>  static void invoke_bpf_prog(struct jit_ctx *ctx, struct bpf_tramp_link *l,
> -                           int args_off, int retval_off, int run_ctx_off,
> +                           int bargs_off, int retval_off, int run_ctx_off,
>                             bool save_ret)
>  {
>         __le32 *branch;
> @@ -1747,7 +1747,7 @@ static void invoke_bpf_prog(struct jit_ctx *ctx, struct bpf_tramp_link *l,
>         /* save return value to callee saved register x20 */
>         emit(A64_MOV(1, A64_R(20), A64_R(0)), ctx);
>
> -       emit(A64_ADD_I(1, A64_R(0), A64_SP, args_off), ctx);
> +       emit(A64_ADD_I(1, A64_R(0), A64_SP, bargs_off), ctx);
>         if (!p->jited)
>                 emit_addr_mov_i64(A64_R(1), (const u64)p->insnsi, ctx);
>
> @@ -1772,7 +1772,7 @@ static void invoke_bpf_prog(struct jit_ctx *ctx, struct bpf_tramp_link *l,
>  }
>
>  static void invoke_bpf_mod_ret(struct jit_ctx *ctx, struct bpf_tramp_links *tl,
> -                              int args_off, int retval_off, int run_ctx_off,
> +                              int bargs_off, int retval_off, int run_ctx_off,
>                                __le32 **branches)
>  {
>         int i;
> @@ -1782,7 +1782,7 @@ static void invoke_bpf_mod_ret(struct jit_ctx *ctx, struct bpf_tramp_links *tl,
>          */
>         emit(A64_STR64I(A64_ZR, A64_SP, retval_off), ctx);
>         for (i = 0; i < tl->nr_links; i++) {
> -               invoke_bpf_prog(ctx, tl->links[i], args_off, retval_off,
> +               invoke_bpf_prog(ctx, tl->links[i], bargs_off, retval_off,
>                                 run_ctx_off, true);
>                 /* if (*(u64 *)(sp + retval_off) !=  0)
>                  *      goto do_fexit;
> @@ -1796,23 +1796,111 @@ static void invoke_bpf_mod_ret(struct jit_ctx *ctx, struct bpf_tramp_links *tl,
>         }
>  }
>
> -static void save_args(struct jit_ctx *ctx, int args_off, int nregs)
> +struct arg_aux {
> +       /* how many args are passed through registers, the rest args are

the rest of the* args

> +        * passed through stack
> +        */
> +       int args_in_reg;

Maybe args_in_regs ? since args can go in multiple regs

> +       /* how many registers used for passing arguments */

are* used

> +       int regs_for_arg;

And here regs_for_args ? Since It's the number of registers used for all args

> +       /* how many stack slots used for arguments, each slot is 8 bytes */

are* used

> +       int stack_slots_for_arg;

And here stack_slots_for_args, for the same reason as above?

> +};
> +
> +static void calc_arg_aux(const struct btf_func_model *m,
> +                        struct arg_aux *a)
>  {
>         int i;
> +       int nregs;
> +       int slots;
> +       int stack_slots;
> +
> +       /* verifier ensures m->nr_args <= MAX_BPF_FUNC_ARGS */
> +       for (i = 0, nregs = 0; i < m->nr_args; i++) {
> +               slots = (m->arg_size[i] + 7) / 8;
> +               if (nregs + slots <= 8) /* passed through register ? */
> +                       nregs += slots;
> +               else
> +                       break;
> +       }
> +
> +       a->args_in_reg = i;
> +       a->regs_for_arg = nregs;
>
> -       for (i = 0; i < nregs; i++) {
> -               emit(A64_STR64I(i, A64_SP, args_off), ctx);
> -               args_off += 8;
> +       /* the rest arguments are passed through stack */
> +       for (stack_slots = 0; i < m->nr_args; i++)
> +               stack_slots += (m->arg_size[i] + 7) / 8;
> +
> +       a->stack_slots_for_arg = stack_slots;
> +}
> +
> +static void clear_garbage(struct jit_ctx *ctx, int reg, int effective_bytes)
> +{
> +       if (effective_bytes) {
> +               int garbage_bits = 64 - 8 * effective_bytes;
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +               /* garbage bits are at the right end */
> +               emit(A64_LSR(1, reg, reg, garbage_bits), ctx);
> +               emit(A64_LSL(1, reg, reg, garbage_bits), ctx);
> +#else
> +               /* garbage bits are at the left end */
> +               emit(A64_LSL(1, reg, reg, garbage_bits), ctx);
> +               emit(A64_LSR(1, reg, reg, garbage_bits), ctx);
> +#endif
>         }
>  }
>
> -static void restore_args(struct jit_ctx *ctx, int args_off, int nregs)
> +static void save_args(struct jit_ctx *ctx, int bargs_off, int oargs_off,
> +                     const struct btf_func_model *m,
> +                     const struct arg_aux *a,
> +                     bool for_call_origin)
>  {
>         int i;
> +       int reg;
> +       int doff;
> +       int soff;
> +       int slots;
> +       u8 tmp = bpf2a64[TMP_REG_1];
> +
> +       /* store argument registers to stack for call bpf, or restore argument

to* call bpf or "for the bpf program"

> +        * registers from stack for the original function
> +        */
> +       for (reg = 0; reg < a->regs_for_arg; reg++) {
> +               emit(for_call_origin ?
> +                    A64_LDR64I(reg, A64_SP, bargs_off) :
> +                    A64_STR64I(reg, A64_SP, bargs_off),
> +                    ctx);
> +               bargs_off += 8;
> +       }
>
> -       for (i = 0; i < nregs; i++) {
> -               emit(A64_LDR64I(i, A64_SP, args_off), ctx);
> -               args_off += 8;
> +       soff = 32; /* on stack arguments start from FP + 32 */
> +       doff = (for_call_origin ? oargs_off : bargs_off);
> +
> +       /* save on stack arguments */
> +       for (i = a->args_in_reg; i < m->nr_args; i++) {
> +               slots = (m->arg_size[i] + 7) / 8;
> +               /* verifier ensures arg_size <= 16, so slots equals 1 or 2 */
> +               while (slots-- > 0) {
> +                       emit(A64_LDR64I(tmp, A64_FP, soff), ctx);
> +                       /* if there is unused space in the last slot, clear
> +                        * the garbage contained in the space.
> +                        */
> +                       if (slots == 0 && !for_call_origin)
> +                               clear_garbage(ctx, tmp, m->arg_size[i] % 8);
> +                       emit(A64_STR64I(tmp, A64_SP, doff), ctx);
> +                       soff += 8;
> +                       doff += 8;
> +               }
> +       }
> +}
> +
> +static void restore_args(struct jit_ctx *ctx, int bargs_off, int nregs)
> +{
> +       int reg;
> +
> +       for (reg = 0; reg < nregs; reg++) {
> +               emit(A64_LDR64I(reg, A64_SP, bargs_off), ctx);
> +               bargs_off += 8;
>         }
>  }
>
> @@ -1829,17 +1917,21 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nregs)
>   */
>  static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
>                               struct bpf_tramp_links *tlinks, void *orig_call,
> -                             int nregs, u32 flags)
> +                             const struct btf_func_model *m,
> +                             const struct arg_aux *a,
> +                             u32 flags)
>  {
>         int i;
>         int stack_size;
>         int retaddr_off;
>         int regs_off;
>         int retval_off;
> -       int args_off;
> +       int bargs_off;
>         int nregs_off;
>         int ip_off;
>         int run_ctx_off;
> +       int oargs_off;
> +       int nregs;
>         struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
>         struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
>         struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
> @@ -1859,19 +1951,26 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
>          *
>          * SP + retval_off  [ return value      ] BPF_TRAMP_F_CALL_ORIG or
>          *                                        BPF_TRAMP_F_RET_FENTRY_RET
> -        *
>          *                  [ arg reg N         ]
>          *                  [ ...               ]
> -        * SP + args_off    [ arg reg 1         ]
> +        * SP + bargs_off   [ arg reg 1         ] for bpf
>          *
>          * SP + nregs_off   [ arg regs count    ]
>          *
>          * SP + ip_off      [ traced function   ] BPF_TRAMP_F_IP_ARG flag
>          *
>          * SP + run_ctx_off [ bpf_tramp_run_ctx ]
> +        *
> +        *                  [ stack arg N       ]
> +        *                  [ ...               ]
> +        * SP + oargs_off   [ stack arg 1       ] for original func
>          */
>
>         stack_size = 0;
> +       oargs_off = stack_size;
> +       if (flags & BPF_TRAMP_F_CALL_ORIG)
> +               stack_size += 8 * a->stack_slots_for_arg;
> +
>         run_ctx_off = stack_size;
>         /* room for bpf_tramp_run_ctx */
>         stack_size += round_up(sizeof(struct bpf_tramp_run_ctx), 8);
> @@ -1885,9 +1984,10 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
>         /* room for args count */
>         stack_size += 8;
>
> -       args_off = stack_size;
> +       bargs_off = stack_size;
>         /* room for args */
> -       stack_size += nregs * 8;
> +       nregs = a->regs_for_arg + a->stack_slots_for_arg;

Maybe this name no longer makes sense ?





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux