On Sun, Aug 14, 2022 at 10:29:11PM -0700, Yonghong Song wrote:
>
>
> On 8/14/22 1:24 PM, Jiri Olsa wrote:
> > On Thu, Aug 11, 2022 at 10:24:35PM -0700, Yonghong Song wrote:
> >
> > SNIP
> >
> > > }
> > > static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
> > > @@ -2020,6 +2081,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > > 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];
> > > + int struct_val_off, extra_nregs = 0;
> > > u8 **branches = NULL;
> > > u8 *prog;
> > > bool save_ret;
> > > @@ -2028,6 +2090,20 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > > if (nr_args > 6)
> > > return -ENOTSUPP;
> > > + for (i = 0; i < MAX_BPF_FUNC_ARGS; i++) {
> > > + if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
> > > + /* Only support up to 16 bytes struct which should keep
> > > + * values in registers.
> > > + */
> >
> > it seems that if the struct contains 'double' field, it's passed in
> > SSE register, which we don't support is save/restore
>
> That is right.
>
> >
> > we should probably check struct's BTF in btf_distill_func_proto and
> > fail if we found anything else than regular regs types?
>
> The reason I didn't add float/double checking is that I didn't actually
> find any float/double struct members in either vmlinux.h or in
> arch/x86 directory. Could you help double check as well?
ok I checked on fedora's BTF and could not find any
still the check might be good or at least mention
that in comment
>
> >
> > > + if (m->arg_size[i] > 16)
> > > + return -ENOTSUPP;
> > > +
> > > + extra_nregs += (m->arg_size[i] + 7) / 8 - 1;
> > > + }
> > > + }
> > > + if (nr_args + extra_nregs > 6)
> >
> > should this value be minus the number of actually found struct arguments?
>
> In the above we have
> extra_nregs += (m->arg_size[i] + 7) / 8 - 1;
> already did the 'minus' part.
there it is ;-) ok
jirka
>
> >
> > > + return -ENOTSUPP;
> > > +
> > > /* Generated trampoline stack layout:
> > > *
> > > * RBP + 8 [ return address ]
> > > @@ -2066,6 +2142,13 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > > stack_size += (sizeof(struct bpf_tramp_run_ctx) + 7) & ~0x7;
> > > run_ctx_off = stack_size;
> > > + /* For structure argument */
> > > + for (i = 0; i < MAX_BPF_FUNC_ARGS; i++) {
> > > + if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
> > > + stack_size += (m->arg_size[i] + 7) & ~0x7;
> > > + }
> > > + struct_val_off = stack_size;
> >
> > could you please update the 'Generated trampoline stack layout' table
> > above with this offset
>
> Okay, will do in the next revision.
>
> >
> > thanks,
> > jirka
> >
> > > +
> > > if (flags & BPF_TRAMP_F_SKIP_FRAME) {
> > > /* skip patched call instruction and point orig_call to actual
> > > * body of the kernel function.
> > > @@ -2101,7 +2184,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > > emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -ip_off);
> > > }
> > > - save_regs(m, &prog, nr_args, regs_off);
> > > + save_regs(m, &prog, nr_args, regs_off, struct_val_off);
> > > if (flags & BPF_TRAMP_F_CALL_ORIG) {
> > > /* arg1: mov rdi, im */
> > > @@ -2131,7 +2214,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > > }
> > > if (flags & BPF_TRAMP_F_CALL_ORIG) {
> > > - restore_regs(m, &prog, nr_args, regs_off);
> > > + restore_regs(m, &prog, nr_args, regs_off, struct_val_off);
> > > if (flags & BPF_TRAMP_F_ORIG_STACK) {
> > > emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
> > > @@ -2172,7 +2255,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > > }
> > > if (flags & BPF_TRAMP_F_RESTORE_REGS)
> > > - restore_regs(m, &prog, nr_args, regs_off);
> > > + restore_regs(m, &prog, nr_args, regs_off, struct_val_off);
> > > /* This needs to be done regardless. If there were fmod_ret programs,
> > > * the return value is only updated on the stack and still needs to be
> > > --
> > > 2.30.2
> > >
