On 2/10/25 2:11 PM, Alan Maguire wrote: > On 07/02/2025 02:14, Ihor Solodrai wrote: >> When adding a kfunc prototype to BTF, check for the flags indicating >> bpf_arena pointers and emit a type tag encoding >> __attribute__((address_space(1))) for them. This also requires >> updating BTF type ids in the btf_encoder_func_state, which is done as >> a side effect in the tagging functions. >> >> This feature depends on recent update in libbpf, supporting arbitrarty >> attribute encoding [1]. >> >> [1] https://lore.kernel.org/bpf/20250130201239.1429648-1-ihor.solodrai@xxxxxxxxx/ >> >> Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx> > > a few minor issues below, but > > Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx> > >> --- >> btf_encoder.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 96 insertions(+), 1 deletion(-) >> >> diff --git a/btf_encoder.c b/btf_encoder.c >> index e9f4baf..d7837c2 100644 >> --- a/btf_encoder.c >> +++ b/btf_encoder.c >> @@ -40,7 +40,13 @@ >> #define BTF_SET8_KFUNCS (1 << 0) >> #define BTF_KFUNC_TYPE_TAG "bpf_kfunc" >> #define BTF_FASTCALL_TAG "bpf_fastcall" >> -#define KF_FASTCALL (1 << 12) >> +#define BPF_ARENA_ATTR "address_space(1)" >> + >> +/* kfunc flags, see include/linux/btf.h in the kernel source */ >> +#define KF_FASTCALL (1 << 12) >> +#define KF_ARENA_RET (1 << 13) >> +#define KF_ARENA_ARG1 (1 << 14) >> +#define KF_ARENA_ARG2 (1 << 15) >> >> struct btf_id_and_flag { >> uint32_t id; >> @@ -743,6 +749,91 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t >> return encoder->type_id_off + tag_type; >> } >> >> +static inline struct kfunc_info* btf_encoder__kfunc_by_name(struct btf_encoder *encoder, const char *name) { >> + struct kfunc_info *kfunc; >> + >> + list_for_each_entry(kfunc, &encoder->kfuncs, node) { >> + if (strcmp(kfunc->name, name) == 0) >> + return kfunc; >> + } >> + return NULL; >> +} >> + > > above function is only used within #if statement below, right? Should > probably move it there to avoid warnings. Right. I think some of these functions may go away, given Eduard's suggestions. > >> +#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6 >> +static int btf_encoder__tag_bpf_arena_ptr(struct btf *btf, int ptr_id) >> +{ >> + const struct btf_type *ptr; >> + int tagged_type_id; >> + >> + ptr = btf__type_by_id(btf, ptr_id); >> + if (!btf_is_ptr(ptr)) >> + return -EINVAL; >> + >> + tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type); >> + if (tagged_type_id < 0) >> + return tagged_type_id; >> + >> + return btf__add_ptr(btf, tagged_type_id); >> +} >> + >> +static int btf_encoder__tag_bpf_arena_arg(struct btf *btf, struct btf_encoder_func_state *state, int idx) >> +{ >> + int id; >> + >> + if (state->nr_parms <= idx) >> + return -EINVAL; >> + >> + id = btf_encoder__tag_bpf_arena_ptr(btf, state->parms[idx].type_id); >> + if (id < 0) { >> + btf__log_err(btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, id, >> + "Error adding BPF_ARENA_ATTR for an argument of kfunc '%s'", state->elf->name); > > nit: since we call this for arguments + return value, should we reflect > that in the function name/error message? maybe pass in the KF_ARENA_* > flag or something? It is what's happening. btf_encoder__tag_bpf_arena_ptr is called from btf_encoder__tag_bpf_arena_arg and separately for a return type: if (KF_ARENA_RET & kfunc->flags) { ret_type_id = btf_encoder__tag_bpf_arena_ptr(encoder->btf, state->ret_type_id); In both cases the return value is checked and the error message is different. I factored out *_arg version of this operation because it has to be done twice: for _ARG1 and _ARG2. Maybe I misunderstood you question? lmk > >> + return id; >> + } >> + state->parms[idx].type_id = id; >> + >> + return id; >> +} >> + >> +static int btf_encoder__add_bpf_arena_type_tags(struct btf_encoder *encoder, struct btf_encoder_func_state *state) >> +{ >> + struct kfunc_info *kfunc = NULL; >> + int ret_type_id; >> + int err = 0; >> + >> + if (!state || !state->elf || !state->elf->kfunc) >> + goto out; >> + >> + kfunc = btf_encoder__kfunc_by_name(encoder, state->elf->name); >> + if (!kfunc) >> + goto out; >> + >> + if (KF_ARENA_RET & kfunc->flags) { >> + ret_type_id = btf_encoder__tag_bpf_arena_ptr(encoder->btf, state->ret_type_id); >> + if (ret_type_id < 0) { >> + btf__log_err(encoder->btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, ret_type_id, >> + "Error adding BPF_ARENA_ATTR for return type of kfunc '%s'", state->elf->name); >> + err = ret_type_id; >> + goto out; >> + } >> + state->ret_type_id = ret_type_id; >> + } >> + >> + if (KF_ARENA_ARG1 & kfunc->flags) { >> + err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 0); >> + if (err < 0) >> + goto out; >> + } >> + >> + if (KF_ARENA_ARG2 & kfunc->flags) { >> + err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 1); >> + if (err < 0) >> + goto out; >> + } >> +out: >> + return err; > > not sure we need goto outs here; there are no resources to free etc so > we can just return err/return 0 where appropriate. ack > >> +} >> +#endif // LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6 >> + >> static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, >> struct btf_encoder_func_state *state) >> { >> @@ -762,6 +853,10 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f >> nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0); >> type_id = btf_encoder__tag_type(encoder, ftype->tag.type); >> } else if (state) { >> +#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6 >> + if (btf_encoder__add_bpf_arena_type_tags(encoder, state) < 0) > > kind of a nit I guess, but I think it might be clearer to make explicit > the work we only have to do for kfuncs, i.e. > > if (state->elf && state->elf->kfunc) { > /* do kfunc-specific work like arena ptr tag */ > > } > > I know the function has checks for this internally but I think it makes > it a bit clearer that it's only needed for a small subset of functions, > what do you think? Actually I did write it like that first. IIRC tagging has to be done before `type_id = state->ret_type_id;` and only when `state` is passed. Anyways, I agree it should be clearer that this happens just for some functions. > > >> + return -1; >> +#endif >> encoder = state->encoder; >> btf = state->encoder->btf; >> nr_params = state->nr_parms;
