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?
+ 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.
+ 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
