Re: [PATCH bpf-next v2 3/6] bpf: x86: Support in-register struct arguments

[Jiri Olsa <olsajiri@gmail.com>]

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
> > >