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

[Yonghong Song <yhs@fb.com>]

On 8/15/22 12:29 AM, Jiri Olsa wrote:
> 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

I will mention in the comment. thanks!

> 
>>
>>>
>>>> +			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
> 
[...]