On Mon, 2024-08-12 at 22:36 -0700, Yonghong Song wrote:
[...]
> > @@ -16140,6 +16140,28 @@ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm)
> > }
> > }
> >
> > +/* Same as helper_nocsr_clobber_mask() but for kfuncs, see comment above */
> > +static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta)
> > +{
> > + const struct btf_param *params;
> > + u32 vlen, i, mask;
>
> In helper_nocsr_clobber_mask, we have u8 mask. To be consistent, can we have 'u8 mask' here?
> Are you worried that the number of arguments could be more than 7? This seems not the case
> right now.
Before the nocsr part for helpers landed there was a change request to
make helper_nocsr_clobber_mask() return u32. I modified the function
but forgot to change the type for 'mask' local variable.
The main point in using u32 is uniformity.
I can either change kfunc_nocsr_clobber_mask() to use u8 for mask,
or update helper_nocsr_clobber_mask() to use u32 for mask.
>
> > +
> > + params = btf_params(meta->func_proto);
> > + vlen = btf_type_vlen(meta->func_proto);
> > + mask = 0;
> > + if (!btf_type_is_void(btf_type_by_id(meta->btf, meta->func_proto->type)))
> > + mask |= BIT(BPF_REG_0);
> > + for (i = 0; i < vlen; ++i)
> > + mask |= BIT(BPF_REG_1 + i);
> > + return mask;
> > +}
> > +
> > +/* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */
> > +static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta)
> > +{
> > + return false;
> > +}
> > +
> > /* GCC and LLVM define a no_caller_saved_registers function attribute.
> > * This attribute means that function scratches only some of
> > * the caller saved registers defined by ABI.
> > @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env,
> > bpf_jit_inlines_helper_call(call->imm));
> > }
> >
> > + if (bpf_pseudo_kfunc_call(call)) {
> > + struct bpf_kfunc_call_arg_meta meta;
> > + int err;
> > +
> > + err = fetch_kfunc_meta(env, call, &meta, NULL);
> > + if (err < 0)
> > + /* error would be reported later */
> > + return;
> > +
> > + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta);
> > + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) &&
> > + verifier_inlines_kfunc_call(&meta);
>
> I think we do not need both meta.kfunc_flags & KF_NOCSR and
> verifier_inlines_kfunc_call(&meta). Only one of them is enough
> since they test very similar thing. You do need to ensure
> kfuncs with KF_NOCSR in special_kfunc_list though.
> WDYT?
I can remove the flag in favour of verifier_inlines_kfunc_call().
>
> > + }
> > +
> > if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS)
> > return;
> >
