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. > +#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? > + 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. > +} > +#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? > + return -1; > +#endif > encoder = state->encoder; > btf = state->encoder->btf; > nr_params = state->nr_parms;
