Re: [RFC bpf-next 02/15] libbpf: Add support for BTF kinds LOC_PARAM, LOC_PROTO and LOCSEC

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-10-08 at 18:34 +0100, Alan Maguire wrote:

[...]

> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 18907f0fcf9f..0abd7831d6b4 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c

[...]

> @@ -588,6 +621,34 @@ static int btf_validate_type(const struct btf *btf, const struct btf_type *t, __

offtopic: we should probably switch this function to use field and string iterators.

>  		}
>  		break;
>  	}
> +	case BTF_KIND_LOC_PARAM:
> +		break;
> +	case BTF_KIND_LOC_PROTO: {
> +		__u32 *p = btf_loc_proto_params(t);
> +
> +		n = btf_vlen(t);
> +		for (i = 0; i < n; i++, p++) {
> +			err = btf_validate_id(btf, *p, id);
> +			if (err)
> +				return err;
> +		}
> +		break;
> +	}
> +	case BTF_KIND_LOCSEC: {
> +		const struct btf_loc *l = btf_locsec_locs(t);
> +
> +		n = btf_vlen(t);
> +		for (i = 0; i < n; i++, l++) {
> +			err = btf_validate_str(btf, l->name_off, "loc name", id);
> +			if (!err)
> +				err = btf_validate_id(btf, l->func_proto, id);
> +			if (!err)
> +				btf_validate_id(btf, l->loc_proto, id);
                              ^^^^
                       Missing `err =`?

> +			if (err)
> +				return err;
> +		}
> +		break;
> +	}
>  	default:
>  		pr_warn("btf: type [%u]: unrecognized kind %u\n", id, kind);
>  		return -EINVAL;
> @@ -2993,6 +3054,183 @@ int btf__add_decl_attr(struct btf *btf, const char *value, int ref_type_id,
>  	return btf_add_decl_tag(btf, value, ref_type_id, component_idx, 1);
>  }
>  
> +/*
> + * Append new BTF_KIND_LOC_PARAM with either
> + *   - *value* set as __u64 value following btf_type, with info->kflag set to 1
> + *     if *is_value* is true; or
> + *   - *reg* number, *flags* and *offset* set if *is_value* is set to 0, and
> + *    info->kflag set to 0.
> + * Returns:
> + *   -  >0, type ID of newly added BTF type;
> + *   - <0, on error.
> + */
> +int btf__add_loc_param(struct btf *btf, __s32 size, bool is_value, __u64 value,
> +		       __u16 reg, __u16 flags, __s32 offset)

Probably, would be more convenient to have several functions, e.g.:
- btf__add_loc_param_const()
- btf__add_loc_param_reg()
- btf__add_loc_param_deref()

Should `size` be some kind of enum?
E.g. with values like S64, S32, ..., U64.
So the the usage would be like:

  btf__add_loc_param_const(btf, U64, 0xdeadbeef);

Wdyt?

> +{
> +	struct btf_loc_param *p;
> +	struct btf_type *t;
> +	int sz;
> +
> +	if (btf_ensure_modifiable(btf))
> +		return libbpf_err(-ENOMEM);
> +
> +	sz = sizeof(struct btf_type) + sizeof(__u64);
> +	t = btf_add_type_mem(btf, sz);
> +	if (!t)
> +		return libbpf_err(-ENOMEM);
> +
> +	t->name_off = 0;
> +	t->info = btf_type_info(BTF_KIND_LOC_PARAM, 0, is_value);
> +	t->size = size;
> +
> +	p = btf_loc_param(t);
> +
> +	if (is_value) {
> +		p->val_lo32 = value & 0xffffffff;
> +		p->val_hi32 = value >> 32;
> +	} else {
> +		p->reg = reg;
> +		p->flags = flags;
> +		p->offset = offset;
> +	}
> +	return btf_commit_type(btf, sz);
> +}

[...]

> +int btf__add_locsec_loc(struct btf *btf, const char *name, __u32 func_proto, __u32 loc_proto,
> +			__u32 offset)
> +{
> +	struct btf_type *t;
> +	struct btf_loc *l;
> +	int name_off, sz;
> +
> +	if (!name || !name[0])
> +		return libbpf_err(-EINVAL);
> +
> +	if (validate_type_id(func_proto) || validate_type_id(loc_proto))
> +		return libbpf_err(-EINVAL);
> +
> +	/* last type should be BTF_KIND_LOCSEC */
> +	if (btf->nr_types == 0)
> +		return libbpf_err(-EINVAL);
> +	t = btf_last_type(btf);
> +	if (!btf_is_locsec(t))
> +		return libbpf_err(-EINVAL);
> +
> +	/* decompose and invalidate raw data */
> +	if (btf_ensure_modifiable(btf))
> +		return libbpf_err(-ENOMEM);
> +
> +	name_off = btf__add_str(btf, name);
> +	if (name_off < 0)
> +		return name_off;
> +
> +	sz = sizeof(*l);
> +	l = btf_add_type_mem(btf, sz);
> +	if (!l)
> +		return libbpf_err(-ENOMEM);
> +
> +	l->name_off = name_off;
> +	l->func_proto = func_proto;
> +	l->loc_proto = loc_proto;
> +	l->offset = offset;
> +
> +	/* update parent type's vlen */
> +	t = btf_last_type(btf);
> +	btf_type_inc_vlen(t);

Since vlen is only u16, maybe check for overflow and report an error here?

> +
> +	btf->hdr->type_len += sz;
> +	btf->hdr->str_off += sz;
> +	return 0;
> +}
> +
>  struct btf_ext_sec_info_param {
>  	__u32 off;
>  	__u32 len;

[...]

> @@ -5075,6 +5361,45 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
>  		break;
>  	}
>  
> +	case BTF_KIND_LOC_PROTO: {
> +		__u32 *p1, *p2;
> +		__u16 i, vlen;
> +
> +		p1 = btf_loc_proto_params(t);
> +		vlen = btf_vlen(t);
> +
> +		for (i = 0; i < vlen; i++, p1++) {
> +			ref_type_id = btf_dedup_ref_type(d, *p1);
> +			if (ref_type_id < 0)
> +				return ref_type_id;
> +			*p1 = ref_type_id;
> +		}
> +
> +		h = btf_hash_loc_proto(t);
> +		for_each_dedup_cand(d, hash_entry, h) {
> +			cand_id = hash_entry->value;
> +			cand = btf_type_by_id(d->btf, cand_id);
> +			if (!btf_equal_common(t, cand))
> +				continue;

Nit: having btf_equal_loc_proto() would have been more readable here.

> +			vlen = btf_vlen(cand);
> +			p1 = btf_loc_proto_params(t);
> +			p2 = btf_loc_proto_params(cand);
> +			if (vlen == 0) {
> +				new_id = cand_id;
> +				break;
> +			}
> +			for (i = 0; i < vlen; i++, p1++, p2++) {
> +				if (*p1 != *p2)
> +					break;
> +				new_id = cand_id;
> +				break;
> +			}
> +			if (new_id == cand_id)
> +				break;

Why `break` and not `continue`?
Also, BTF_KIND_FUNC_PROTO does not have this special case, why the difference?

> +		}
> +		break;
> +	}
> +
>  	default:
>  		return -EINVAL;
>  	}

[...]

> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 6388392f49a0..95bdda2f4a2d 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -328,6 +328,9 @@ static int btf_dump_mark_referenced(struct btf_dump *d)
>  		case BTF_KIND_ENUM64:
>  		case BTF_KIND_FWD:
>  		case BTF_KIND_FLOAT:
> +		case BTF_KIND_LOC_PARAM:
> +		case BTF_KIND_LOC_PROTO:
> +		case BTF_KIND_LOCSEC:
>  			break;
>  
>  		case BTF_KIND_VOLATILE:
> @@ -339,7 +342,6 @@ static int btf_dump_mark_referenced(struct btf_dump *d)
>  		case BTF_KIND_VAR:
>  		case BTF_KIND_DECL_TAG:
>  		case BTF_KIND_TYPE_TAG:
> -			d->type_states[t->type].referenced = 1;
>  			break;

This change seems unrelated, why is it necessary?

>  
>  		case BTF_KIND_ARRAY: {

[...]