On Fri, 2024-05-10 at 11:30 +0100, Alan Maguire wrote:
[...]
Hi Alan,
A two minor notes below, otherwise I think this looks good.
[...]
> +static int btf_add_distilled_type_ids(__u32 *id, void *ctx)
> +{
> + struct btf_distill *dist = ctx;
> + struct btf_type *t = btf_type_by_id(dist->pipe.src, *id);
> + int err;
> +
> + if (!*id)
> + return 0;
> + /* split BTF id, not needed */
> + if (*id >= dist->split_start_id)
> + return 0;
> + /* already added ? */
> + if (BTF_ID(dist->ids[*id]) > 0)
> + return 0;
> +
> + /* only a subset of base BTF types should be referenced from split
> + * BTF; ensure nothing unexpected is referenced.
> + */
> + switch (btf_kind(t)) {
> + case BTF_KIND_INT:
> + case BTF_KIND_FLOAT:
> + case BTF_KIND_FWD:
> + case BTF_KIND_ARRAY:
> + case BTF_KIND_STRUCT:
> + case BTF_KIND_UNION:
> + case BTF_KIND_TYPEDEF:
> + case BTF_KIND_ENUM:
> + case BTF_KIND_ENUM64:
> + case BTF_KIND_PTR:
> + case BTF_KIND_CONST:
> + case BTF_KIND_RESTRICT:
> + case BTF_KIND_VOLATILE:
> + case BTF_KIND_FUNC_PROTO:
I think BTF_KIND_TYPE_TAG should be in this list.
> + dist->ids[*id] |= *id;
> + break;
> + default:
> + pr_warn("unexpected reference to base type[%u] of kind [%u] when creating distilled base BTF.\n",
> + *id, btf_kind(t));
> + return -EINVAL;
> + }
[...]
> +static int btf_add_distilled_types(struct btf_distill *dist)
> +{
> + bool adding_to_base = dist->pipe.dst->start_id == 1;
> + int id = btf__type_cnt(dist->pipe.dst);
> + struct btf_type *t;
> + int i, err = 0;
> +
> + /* Add types for each of the required references to either distilled
> + * base or split BTF, depending on type characteristics.
> + */
> + for (i = 1; i < dist->split_start_id; i++) {
> + const char *name;
> + int kind;
> +
> + if (!BTF_ID(dist->ids[i]))
> + continue;
> + t = btf_type_by_id(dist->pipe.src, i);
> + kind = btf_kind(t);
> + name = btf__name_by_offset(dist->pipe.src, t->name_off);
> +
> + /* Named int, float, fwd struct, union, enum[64] are added to
> + * base; everything else is added to split BTF.
> + */
> + switch (kind) {
> + case BTF_KIND_INT:
> + case BTF_KIND_FLOAT:
> + case BTF_KIND_FWD:
> + case BTF_KIND_STRUCT:
> + case BTF_KIND_UNION:
> + case BTF_KIND_ENUM:
> + case BTF_KIND_ENUM64:
> + if ((adding_to_base && !t->name_off) || (!adding_to_base && t->name_off))
> + continue;
> + break;
> + default:
> + if (adding_to_base)
> + continue;
> + break;
> + }
> + if (dist->ids[i] & BTF_EMBEDDED_COMPOSITE) {
> + /* If a named struct/union in base BTF is referenced as a type
> + * in split BTF without use of a pointer - i.e. as an embedded
> + * struct/union - add an empty struct/union preserving size
> + * since size must be consistent when relocating split and
> + * possibly changed base BTF.
> + */
> + err = btf_add_composite(dist->pipe.dst, kind, name, t->size);
> + } else if (btf_is_eligible_named_fwd(t)) {
> + /* If not embedded, use a fwd for named struct/unions since we
> + * can match via name without any other details.
> + */
> + switch (kind) {
> + case BTF_KIND_STRUCT:
> + err = btf__add_fwd(dist->pipe.dst, name, BTF_FWD_STRUCT);
> + break;
> + case BTF_KIND_UNION:
> + err = btf__add_fwd(dist->pipe.dst, name, BTF_FWD_UNION);
> + break;
> + case BTF_KIND_ENUM:
> + err = btf__add_enum(dist->pipe.dst, name, sizeof(int));
I think that the size of the enum should be read from base BTF.
When inspecting BTF generated for selftests kernel config there
are 14 enums with size=1.
> + break;
> + case BTF_KIND_ENUM64:
> + err = btf__add_enum(dist->pipe.dst, name, sizeof(__u64));
> + break;
> + default:
> + pr_warn("unexpected kind [%u] when creating distilled base BTF.\n",
> + btf_kind(t));
> + return -EINVAL;
> + }
> + } else {
> + err = btf_add_type(&dist->pipe, t);
> + }
> + if (err < 0)
> + break;
> + dist->ids[i] = id++;
> + }
> + return err;
> +}
[...]
