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 Thu, Sep 21, 2023 at 7:21 AM Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:
> On 9/21/2023 6:17 AM, Florent Revest wrote:
> > On Sun, Sep 17, 2023 at 5:09 PM Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:
> >> 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
> >
>
> I guess you might have overlooked rule C.13 in AAPCS64. Non-floating type arguments
> are copied to stack under rule C.15/C.17. However, C.13 is applied before C.15/C.17,
> which means that NGRN is set to 8 before the stack is used. That is, all 8 parameter
> arguments are used up and any remaining arguments can only be passed by the stack.
>
> C.13    The NGRN is set to 8.
>
> C.14    The NSAA is rounded up to the larger of 8 or the Natural Alignment of the
>          argument’s type.
>
> C.15    If the argument is a composite type then the argument is copied to memory
>          at the adjusted NSAA. The NSAA is incremented by the size of the argument.
>          The argument has now been allocated.
>
> C.16    If the size of the argument is less than 8 bytes then the size of the argument
>          is set to 8 bytes. The effect is as if the argument was copied to the least
>          significant bits of a 64-bit register and the remaining bits filled with
>          unspecified values.
>
> C.17    The argument is copied to memory at the adjusted NSAA. The NSAA is incremented
>          by the size of the argument. The argument has now been allocated.
>
>
> And the following assembly code also shows 'e' and 'f' are passed by stack.
>
> int func(__int128 a, __int128 b, __int128 c, int64_t d, __int128 e, int64_t f)
> {
>          return e == 5 || f == 7;
> }
>
> asseembly:
>
> func:
>          sub     sp, sp, #64
>          stp     x0, x1, [sp, 48]
>          stp     x2, x3, [sp, 32]
>          stp     x4, x5, [sp, 16]
>          str     x6, [sp, 8]
>          ldr     x0, [sp, 64] // ** load the low 8 bytes of e from SP + 64 **
>          cmp     x0, 5
>          bne     .L27
>          ldr     x0, [sp, 72] // ** load the high 8 bytes of e from SP + 72 **
>          cmp     x0, 0
>          beq     .L22
> .L27:
>          ldr     x0, [sp, 80] // ** load f from SP + 80 **
>          cmp     x0, 7
>          bne     .L24
> .L22:
>          mov     w0, 1
>          b       .L26
> .L24:
>          mov     w0, 0
> .L26:
>          add     sp, sp, 64
>          ret

Ah, that's great! :) It keeps things easy then. Thank you for the explanation!

> Although the above case is fine, the current patch does not handle rule C.14 correctly.
> For example, for the following func, an 8-byte padding is inserted between f and g by
> C.14 to align g to 16 bytes, but this patch mistakenly treats it as part of g.
>
> int func(__int128 a, __int128 b, __int128 c, int64_t d, __int128 e, int64_t f, __int128 g)
> {
> }
>
> Maybe we could fix it by adding argument alignment to struct btf_func_model, I'll
> give it a try.

Good catch!

> > Maybe it would be good to add something pathological like this to the
> > selftests ?
> >
>
> OK, will do

Just a thought on that topic, maybe it would be preferable to have a
new separate test for this ? Currently we have a test for 128b long
arguments and a test for many arguments, it's good to have these
separate because they are two dimensions of bpf architectural support:
if someone adds support for one xor the other feature to an arch, it's
good to see a test go green. Since that new test would cover both long
and many arguments, it should probably be a new test.

> >> -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"
> >
>
> Sorry for these incorrect words :(, all will be fixed in the next version, thanks!

No problem!

> >>   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 ?
>
> OK, I'll remove it

Ack





[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