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

[Yonghong Song <yhs@fb.com>]

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