On 10/05/2024 20:14, Eduard Zingerman wrote:
> 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.
>
thanks for reviewing! Replies below..
> [...]
>
>> +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.
>
You're right; sorry, you mentioned that last time too and I missed
fixing it for v3.
>> + 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.
>
good idea; we can use t->size for both enum and enum64 cases above.
>> + 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;
>> +}
>
> [...]
