Re: [PATCH bpf-next v4 02/13] bpf: Add helpers to describe the R0:R2 return register pair
Eduard Zingerman <[email protected]>
| Newsgroups | org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On Mon, 2026-08-10 at 17:09 -0700, Yonghong Song wrote:
> LLVM 23 added support for returning a value in two registers for an
> __int128, or a struct/union whose size is greater than 8 but not more than
> 16 bytes: such a value comes back in the R0:R2 register pair, with R2
> holding the upper half. See LLVM patches [1] and [2].
>
> Later patches teach the JIT, precision backtracking, live register analysis
> and the verifier itself about that convention. All of them need to answer
> the same question: does this subprogram return its value in a register
> pair? Add the shared helpers up front so that those patches can be ordered
> independently of each other:
>
> - subprog_ret_type() resolves a subprogram's BTF return type. It is
> factored out of subprog_returns_void(). The verifier_bug_if(!func) and
> !func_proto checks it replaces are redundant, since
> check_btf_func_early() already rejects a func_info record whose type_id
> is not a BTF_KIND_FUNC pointing at a BTF_KIND_FUNC_PROTO. A check on
> prog->aux->{btf,func_info} is added instead: unlike
> subprog_returns_void(), which is only used for global subprograms, later
> callers ask about static subprograms too, and those may belong to a
> program loaded without BTF.
>
> - ret_regs_cnt() maps the size of a return value to the number of
> registers holding it.
>
> - bpf_ret_reg_pair() answers the question above. Its users query it at
> every subprogram call and at every subprogram exit, that is once per
> verifier state rather than once per subprogram, so the answer is
> precomputed into bpf_subprog_info->ret_reg_pair by
> bpf_compute_subprog_ret_regs() and the helper itself is a flag test.
> It lives in bpf_verifier.h because kernel/bpf/backtrack.c and
> kernel/bpf/liveness.c need it as well.
>
> bpf_compute_subprog_ret_regs() runs in bpf_check() right before
> bpf_compute_live_registers(), which is the first of those users: by then
> BTF func_info has been validated and the subprogram list is final.
>
> No functional change, bpf_ret_reg_pair() has no callers yet.
>
> [1] https://github.com/llvm/llvm-project/pull/190894
> [2] https://github.com/llvm/llvm-project/pull/206876
>
> Signed-off-by: Yonghong Song <[email protected]>
> ---
I still think that bpf_compute_live_registers() can be used to compute
this information w/o the need to resort to BTF.
...
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 40150390dd50..58a177d26c46 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -382,27 +382,57 @@ bool bpf_subprog_is_global(const struct bpf_verifier_env *env, int subprog)
> return aux && aux[subprog].linkage == BTF_FUNC_GLOBAL;
> }
>
> -static bool subprog_returns_void(struct bpf_verifier_env *env, int subprog)
> +/* Return type of a subprogram, NULL if it cannot be resolved */
> +static const struct btf_type *subprog_ret_type(struct bpf_verifier_env *env, int subprog)
> {
> - const struct btf_type *type, *func, *func_proto;
> + const struct btf_type *func, *func_proto;
> const struct btf *btf = env->prog->aux->btf;
> u32 btf_id;
>
> + if (!btf || !env->prog->aux->func_info)
> + return NULL;
> +
> btf_id = env->prog->aux->func_info[subprog].type_id;
>
> + /* Both already validated by prepare_btf_func() at prog load. */
> func = btf_type_by_id(btf, btf_id);
> - if (verifier_bug_if(!func, env, "btf_id %u not found", btf_id))
> - return false;
> -
> func_proto = btf_type_by_id(btf, func->type);
> - if (!func_proto)
> - return false;
>
> - type = btf_type_skip_modifiers(btf, func_proto->type, NULL);
> - if (!type)
> - return false;
> + return btf_type_skip_modifiers(btf, func_proto->type, NULL);
> +}
> +
> +static bool subprog_returns_void(struct bpf_verifier_env *env, int subprog)
> +{
> + const struct btf_type *type = subprog_ret_type(env, subprog);
>
> - return btf_type_is_void(type);
> + return type && btf_type_is_void(type);
> +}
> +
> +/*
> + * Number of registers holding a function return value: a value of up to 8
> + * bytes is returned in R0, a value of more than 8 bytes and no more than 16
> + * bytes (an __int128 or a struct/union of such size) is returned in the R0:R2
> + * register pair, with R2 holding the upper half.
> + */
> +static u32 ret_regs_cnt(u32 size)
> +{
> + return size > 8 && size <= 16 ? 2 : 1;
> +}
> +
> +/*
> + * Resolve the return convention of every subprogram once, so that
> + * bpf_ret_reg_pair() is a plain flag test on the hot paths that use it.
> + */
> +static void bpf_compute_subprog_ret_regs(struct bpf_verifier_env *env)
> +{
> + const struct btf_type *type;
> + int subprog;
> +
> + for (subprog = 0; subprog < env->subprog_cnt; subprog++) {
> + type = subprog_ret_type(env, subprog);
> + if (type && (btf_type_is_struct(type) || btf_type_is_scalar(type)))
> + subprog_info(env, subprog)->ret_reg_pair = ret_regs_cnt(type->size) > 1;
Nit: there is a btf.c:btf_resolve_size() api function, it would be
better to use it instead of calculating the size ad-hoc.
Should the jit_required flag be set here instead of the main
verification pass?
> + }
> }
>
> static const char *subprog_name(const struct bpf_verifier_env *env, int subprog)
...