bpf: btf: fix struct/union/fwd types with kind_flag

Refactor function btf_int_bits_seq_show() by creating function btf_bitfield_seq_show() which has no dependence on btf and btf_type. The function btf_bitfield_seq_show() will be in later patch to directly dump bitfield member values.

Signed-off-by Yonghong Song <[email protected]>

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c
typedef int ___int;
enum A { A1, A2, A3 };

  struct t {
    int a[5];
    ___int b:4;
    volatile enum A c:4;
  } g;
  -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t size=24 vlen=3
        a type_id=5 bits_offset=0
        b type_id=9 bits_offset=160
        c type_id=11 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3
  [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
  [9] TYPEDEF ___int type_id=8
  [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
  [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t kind_flag=1 size=24 vlen=3
        a type_id=5 bitfield_size=0 bits_offset=0
        b type_id=1 bitfield_size=4 bits_offset=160
        c type_id=7 bitfield_size=4 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

  $ cat tt.c
  struct t;
  union u;
  int foo(struct t *t, union u *u) { return 0; }
  $ gcc -c -g -O2 tt.c
  $ pahole -JV tt.o
  [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [2] FWD t type_id=0
  [3] PTR (anon) type_id=2
  [4] FWD u type_id=0
  [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

  $ pahole -JV tt.o
  [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [2] FWD t kind_flag=0 type_id=0
  [3] PTR (anon) kind_flag=0 type_id=2
  [4] FWD u kind_flag=1 type_id=0
  [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
 include/uapi/linux/btf.h |  15 ++-
 kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +4 -1

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
 	 * bits  0-15: vlen (e.g. # of struct's members)
 	 * bits 16-23: unused
 	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
 	 */
 	__u32 info;
 	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
 #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
 #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
 #define BTF_KIND_UNKN		0	/* Unknown	*/
 #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
 struct btf_member {
 	__u32	name_off;

include/uapi/linux/btf.h +1 -0

+
 /* BTF_KIND_FUNC_PROTO is followed by multiple "struct btf_param".
  * The exact number of btf_param is stored in the vlen (of the
  * info in "struct btf_type").

kernel/bpf/btf.c +5 -1

index 72caa799e82f..ec3f80d3bef6 100644
@@ -164,7 +164,7 @@
 #define BITS_ROUNDUP_BYTES(bits) \
 	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
 
-#define BTF_INFO_MASK 0x0f00ffff
+#define BTF_INFO_MASK 0x8f00ffff
 #define BTF_INT_MASK 0x0fffffff
 #define BTF_TYPE_ID_VALID(type_id) ((type_id) <= BTF_MAX_TYPE)
 #define BTF_STR_OFFSET_VALID(name_off) ((name_off) <= BTF_MAX_NAME_OFFSET)

@@ -274,6 +274,10 @@ struct btf_kind_operations {
 			    const struct btf_type *struct_type,
 			    const struct btf_member *member,
 			    const struct btf_type *member_type);
+	int (*check_kflag_member)(struct btf_verifier_env *env,
+				  const struct btf_type *struct_type,
+				  const struct btf_member *member,
+				  const struct btf_type *member_type);
 	void (*log_details)(struct btf_verifier_env *env,
 			    const struct btf_type *t);
 	void (*seq_show)(const struct btf *btf, const struct btf_type *t,

@@ -419,6 +423,25 @@ static u16 btf_type_vlen(const struct btf_type *t)
 	return BTF_INFO_VLEN(t->info);
 }
 

kernel/bpf/btf.c +17 -0

+	} else if (nr_bits > int_bitsize) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Invalid member bitfield_size");
+		return -EINVAL;
+	}
+
+	struct_size = struct_type->size;
+	bytes_end = BITS_ROUNDUP_BYTES(struct_bits_off + nr_bits);
+	if (struct_size < bytes_end) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Member exceeds struct_size");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+

Quoted 165 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c

   typedef int ___int;
   enum A { A1, A2, A3 };
   struct t {
     int a[5];
     ___int b:4;
     volatile enum A c:4;
   } g;
   -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t size=24 vlen=3
         a type_id=5 bits_offset=0
         b type_id=9 bits_offset=160
         c type_id=11 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3
   [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
   [9] TYPEDEF ___int type_id=8
   [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
   [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t kind_flag=1 size=24 vlen=3
         a type_id=5 bitfield_size=0 bits_offset=0
         b type_id=1 bitfield_size=4 bits_offset=160
         c type_id=7 bitfield_size=4 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

   $ cat tt.c
   struct t;
   union u;
   int foo(struct t *t, union u *u) { return 0; }
   $ gcc -c -g -O2 tt.c
   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t type_id=0
   [3] PTR (anon) type_id=2
   [4] FWD u type_id=0
   [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t kind_flag=0 type_id=0
   [3] PTR (anon) kind_flag=0 type_id=2
   [4] FWD u kind_flag=1 type_id=0
   [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
  include/uapi/linux/btf.h |  15 ++-
  kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
  2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +5 -2

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
  	 * bits  0-15: vlen (e.g. # of struct's members)
  	 * bits 16-23: unused
  	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
  	 */
  	__u32 info;
  	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
  #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
  #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
  #define BTF_KIND_UNKN		0	/* Unknown	*/
  #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
  struct btf_member {
  	__u32	name_off;
  	__u32	type;
-	__u32	offset;	/* offset in bits */
+	__u32	offset;	/* [bitfield_size and] offset in bits */
  };
 

include/uapi/linux/btf.h struct btf_array { · line 123

Quoted 5 lines

include/uapi/linux/btf.h +5 -0

+/* If the type info kind_flag set, the btf_member.offset
+ * contains both member bit offset and bitfield size, and
+ * bitfield size will set for struct/union bitfield members.
+ * Otherwise, it contains only bit offset.
+ */

nit. It may be better to move this comment to the btf_member.offset above.

include/uapi/linux/btf.h struct btf_array { · line 125

Quoted 3 lines

include/uapi/linux/btf.h +2 -0

+#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
+#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)

After re-thinking this setup again, I still think having these macros in btf.h to also do the kflag checking would be nice.

Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't depend on other facts, the btf.h raw user must check kflag anyway before calling BTF_MEMBER_BIT*().
Forcing a kflag check before the user can access these convenient 0xfffff and >>24 conversions may enforce this kflag check to some extend.

Since it is in uapi, it will not be easy to change later. The above concern could be overkill ;), just want to ensure it has been thought through a bit more here.

It could be as easy as moving the new btf_member_bit*() from btf.c to here and remove these two macros (or move them back to btf.c).

Quoted 33 lines

include/uapi/linux/btf.h +1 -0

+
  /* BTF_KIND_FUNC_PROTO is followed by multiple "struct btf_param".
   * The exact number of btf_param is stored in the vlen (of the
   * info in "struct btf_type").

kernel/bpf/btf.c +5 -1

index 72caa799e82f..ec3f80d3bef6 100644
@@ -164,7 +164,7 @@
  #define BITS_ROUNDUP_BYTES(bits) \
  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
 
-#define BTF_INFO_MASK 0x0f00ffff
+#define BTF_INFO_MASK 0x8f00ffff
  #define BTF_INT_MASK 0x0fffffff
  #define BTF_TYPE_ID_VALID(type_id) ((type_id) <= BTF_MAX_TYPE)
  #define BTF_STR_OFFSET_VALID(name_off) ((name_off) <= BTF_MAX_NAME_OFFSET)

@@ -274,6 +274,10 @@ struct btf_kind_operations {
  			    const struct btf_type *struct_type,
  			    const struct btf_member *member,
  			    const struct btf_type *member_type);
+	int (*check_kflag_member)(struct btf_verifier_env *env,
+				  const struct btf_type *struct_type,
+				  const struct btf_member *member,
+				  const struct btf_type *member_type);
  	void (*log_details)(struct btf_verifier_env *env,
  			    const struct btf_type *t);
  	void (*seq_show)(const struct btf *btf, const struct btf_type *t,

@@ -419,6 +423,25 @@ static u16 btf_type_vlen(const struct btf_type *t)
  	return BTF_INFO_VLEN(t->info);
  }
 

[ ... ]

kernel/bpf/btf.c static int btf_enum_check_member(struct btf_verifier_env *env, · line 2038

Quoted 12 lines

kernel/bpf/btf.c +12 -0

+static int btf_enum_check_kflag_member(struct btf_verifier_env *env,
+				       const struct btf_type *struct_type,
+				       const struct btf_member *member,
+				       const struct btf_type *member_type)
+{
+	u32 struct_bits_off, nr_bits, bytes_end, struct_size;
+	u32 int_bitsize = sizeof(int) * BITS_PER_BYTE;
+
+	struct_bits_off = BTF_MEMBER_BIT_OFFSET(member->offset);
+	nr_bits = BTF_MEMBER_BITFIELD_SIZE(member->offset);
+	if (!nr_bits) {
+		nr_bits = int_bitsize;

For non bitfield enum (i.e. !nr_bits), does it have to check for byte alignment also? i.e. BITS_PER_BYTE_MASKED(struct_bits_off).

Others look good.

Quoted 17 lines

kernel/bpf/btf.c +17 -0

+	} else if (nr_bits > int_bitsize) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Invalid member bitfield_size");
+		return -EINVAL;
+	}
+
+	struct_size = struct_type->size;
+	bytes_end = BITS_ROUNDUP_BYTES(struct_bits_off + nr_bits);
+	if (struct_size < bytes_end) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Member exceeds struct_size");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+

Quoted 160 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c
typedef int ___int;
enum A { A1, A2, A3 };

  struct t {
    int a[5];
    ___int b:4;
    volatile enum A c:4;
  } g;
  -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t size=24 vlen=3
        a type_id=5 bits_offset=0
        b type_id=9 bits_offset=160
        c type_id=11 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3
  [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
  [9] TYPEDEF ___int type_id=8
  [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
  [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t kind_flag=1 size=24 vlen=3
        a type_id=5 bitfield_size=0 bits_offset=0
        b type_id=1 bitfield_size=4 bits_offset=160
        c type_id=7 bitfield_size=4 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

  $ cat tt.c
  struct t;
  union u;
  int foo(struct t *t, union u *u) { return 0; }
  $ gcc -c -g -O2 tt.c
  $ pahole -JV tt.o
  [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [2] FWD t type_id=0
  [3] PTR (anon) type_id=2
  [4] FWD u type_id=0
  [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

  $ pahole -JV tt.o
  [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [2] FWD t kind_flag=0 type_id=0
  [3] PTR (anon) kind_flag=0 type_id=2
  [4] FWD u kind_flag=1 type_id=0
  [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
 include/uapi/linux/btf.h |  15 ++-
 kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +4 -1

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
 	 * bits  0-15: vlen (e.g. # of struct's members)
 	 * bits 16-23: unused
 	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
 	 */
 	__u32 info;
 	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
 #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
 #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
 #define BTF_KIND_UNKN		0	/* Unknown	*/
 #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
 struct btf_member {
 	__u32	name_off;

include/uapi/linux/btf.h struct btf_array { · line 123

Quoted 10 lines

include/uapi/linux/btf.h +6 -1

 	__u32	type;
-	__u32	offset;	/* offset in bits */
+	__u32	offset;	/* [bitfield_size and] offset in bits */
 };
 
+/* If the type info kind_flag set, the btf_member.offset
+ * contains both member bit offset and bitfield size, and
+ * bitfield size will set for struct/union bitfield members.
+ * Otherwise, it contains only bit offset.
+ */

nit. It may be better to move this comment to the btf_member.offset above.

include/uapi/linux/btf.h struct btf_array { · line 125

Quoted 2 lines

include/uapi/linux/btf.h +2 -0

+#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
+#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)

After re-thinking this setup again, I still think having these macros in btf.h to also do the kflag checking would be nice.

Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't depend on other facts, the btf.h raw user must check kflag anyway before calling BTF_MEMBER_BIT*().
Forcing a kflag check before the user can access these convenient 0xfffff and >>24 conversions may enforce this kflag check to some extend.

Since it is in uapi, it will not be easy to change later. The above concern could be overkill ;), just want to ensure it has been thought through a bit more here.

It could be as easy as moving the new btf_member_bit*() from btf.c to here and remove these two macros (or move them back to btf.c).

Quoted 187 lines · Martin Lau

Quoted 160 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c
typedef int ___int;
enum A { A1, A2, A3 };

  struct t {
    int a[5];
    ___int b:4;
    volatile enum A c:4;
  } g;
  -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t size=24 vlen=3
        a type_id=5 bits_offset=0
        b type_id=9 bits_offset=160
        c type_id=11 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3
  [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
  [9] TYPEDEF ___int type_id=8
  [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
  [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t kind_flag=1 size=24 vlen=3
        a type_id=5 bitfield_size=0 bits_offset=0
        b type_id=1 bitfield_size=4 bits_offset=160
        c type_id=7 bitfield_size=4 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

  $ cat tt.c
  struct t;
  union u;
  int foo(struct t *t, union u *u) { return 0; }
  $ gcc -c -g -O2 tt.c
  $ pahole -JV tt.o
  [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [2] FWD t type_id=0
  [3] PTR (anon) type_id=2
  [4] FWD u type_id=0
  [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

  $ pahole -JV tt.o
  [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [2] FWD t kind_flag=0 type_id=0
  [3] PTR (anon) kind_flag=0 type_id=2
  [4] FWD u kind_flag=1 type_id=0
  [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
 include/uapi/linux/btf.h |  15 ++-
 kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +4 -1

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
 	 * bits  0-15: vlen (e.g. # of struct's members)
 	 * bits 16-23: unused
 	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
 	 */
 	__u32 info;
 	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
 #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
 #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
 #define BTF_KIND_UNKN		0	/* Unknown	*/
 #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
 struct btf_member {
 	__u32	name_off;

include/uapi/linux/btf.h struct btf_array { · line 123

Quoted 10 lines

include/uapi/linux/btf.h +6 -1

 	__u32	type;
-	__u32	offset;	/* offset in bits */
+	__u32	offset;	/* [bitfield_size and] offset in bits */
 };
 
+/* If the type info kind_flag set, the btf_member.offset
+ * contains both member bit offset and bitfield size, and
+ * bitfield size will set for struct/union bitfield members.
+ * Otherwise, it contains only bit offset.
+ */

nit. It may be better to move this comment to the btf_member.offset above.

include/uapi/linux/btf.h struct btf_array { · line 125

Quoted 2 lines

include/uapi/linux/btf.h +2 -0

+#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
+#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)

After re-thinking this setup again, I still think having these macros in btf.h to also do the kflag checking would be nice.

Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't depend on other facts, the btf.h raw user must check kflag anyway before calling BTF_MEMBER_BIT*().
Forcing a kflag check before the user can access these convenient 0xfffff and >>24 conversions may enforce this kflag check to some extend.

Quoted 6 lines

Since it is in uapi, it will not be easy to change later. The above concern could be overkill ;), just want to ensure it has been thought through a bit more here.

It could be as easy as moving the new btf_member_bit*() from btf.c to here and remove these two macros (or move them back to btf.c).

I think moving:

kernel/bpf/btf.c +6 -0

+static u32 btf_member_bitfield_size(const struct btf_type *struct_type,
+                                   const struct btf_member *member)
+{
+       return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
+                                          : 0;
+}

into uapi/btf.h may or may not be useful for btf uapi users. What are the chances that these static inline helpers will be reused by BTF logic in libbpf or other libs?
At this point we don't know.

So I would keep btf.h minimal.

I agree that BTF_MEMBER_BIT_OFFSET() shouldn't be reused blindly. The users have to do BTF_INFO_KFLAG() check first.
But this is the case for pretty much all of BTF data structures.

Quoted 203 lines · Alexei Starovoitov

Quoted 187 lines · Martin Lau

Quoted 160 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c

   typedef int ___int;
   enum A { A1, A2, A3 };
   struct t {
     int a[5];
     ___int b:4;
     volatile enum A c:4;
   } g;
   -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t size=24 vlen=3
         a type_id=5 bits_offset=0
         b type_id=9 bits_offset=160
         c type_id=11 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3
   [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
   [9] TYPEDEF ___int type_id=8
   [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
   [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t kind_flag=1 size=24 vlen=3
         a type_id=5 bitfield_size=0 bits_offset=0
         b type_id=1 bitfield_size=4 bits_offset=160
         c type_id=7 bitfield_size=4 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

   $ cat tt.c
   struct t;
   union u;
   int foo(struct t *t, union u *u) { return 0; }
   $ gcc -c -g -O2 tt.c
   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t type_id=0
   [3] PTR (anon) type_id=2
   [4] FWD u type_id=0
   [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t kind_flag=0 type_id=0
   [3] PTR (anon) kind_flag=0 type_id=2
   [4] FWD u kind_flag=1 type_id=0
   [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
  include/uapi/linux/btf.h |  15 ++-
  kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
  2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +4 -1

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
  	 * bits  0-15: vlen (e.g. # of struct's members)
  	 * bits 16-23: unused
  	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
  	 */
  	__u32 info;
  	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
  #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
  #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
  #define BTF_KIND_UNKN		0	/* Unknown	*/
  #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
  struct btf_member {
  	__u32	name_off;

include/uapi/linux/btf.h struct btf_array { · line 123

Quoted 10 lines

include/uapi/linux/btf.h +6 -1

  	__u32	type;
-	__u32	offset;	/* offset in bits */
+	__u32	offset;	/* [bitfield_size and] offset in bits */
  };
 
+/* If the type info kind_flag set, the btf_member.offset
+ * contains both member bit offset and bitfield size, and
+ * bitfield size will set for struct/union bitfield members.
+ * Otherwise, it contains only bit offset.
+ */

nit. It may be better to move this comment to the btf_member.offset above.

include/uapi/linux/btf.h struct btf_array { · line 125

Quoted 2 lines

include/uapi/linux/btf.h +2 -0

+#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
+#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)

After re-thinking this setup again, I still think having these macros in btf.h to also do the kflag checking would be nice.

Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't depend on other facts, the btf.h raw user must check kflag anyway before calling BTF_MEMBER_BIT*().
Forcing a kflag check before the user can access these convenient 0xfffff and >>24 conversions may enforce this kflag check to some extend.

Quoted 6 lines

Since it is in uapi, it will not be easy to change later. The above concern could be overkill ;), just want to ensure it has been thought through a bit more here.

It could be as easy as moving the new btf_member_bit*() from btf.c to here and remove these two macros (or move them back to btf.c).

I think moving:

kernel/bpf/btf.c +6 -0

+static u32 btf_member_bitfield_size(const struct btf_type *struct_type,
+                                   const struct btf_member *member)
+{
+       return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
+                                          : 0;
+}

Quoted 6 lines

into uapi/btf.h may or may not be useful for btf uapi users. What are the chances that these static inline helpers will be reused by BTF logic in libbpf or other libs?
At this point we don't know.

So I would keep btf.h minimal.

ok. Make sense

Quoted 3 lines

I agree that BTF_MEMBER_BIT_OFFSET() shouldn't be reused blindly. The users have to do BTF_INFO_KFLAG() check first.
But this is the case for pretty much all of BTF data structures.

Other similar situation in btf.h (i.e. a single u32 field can be interpreted differently) has at least an union as an indication (e.g. size and type in btf_type)

Here we cannot add the union (bitfield_offset:24 and bitfield_size:8) and we cannot change the name "offset" also. I am worry about m->offset will directly be used without checking the BTF_INFO_KFLAG().

may be a "union { __u32 offset; __u32 bitsize_offset; };"......

Quoted 223 lines · Martin Lau

Quoted 203 lines · Alexei Starovoitov

Quoted 187 lines · Martin Lau

Quoted 160 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c

   typedef int ___int;
   enum A { A1, A2, A3 };
   struct t {
     int a[5];
     ___int b:4;
     volatile enum A c:4;
   } g;
   -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t size=24 vlen=3
         a type_id=5 bits_offset=0
         b type_id=9 bits_offset=160
         c type_id=11 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3
   [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
   [9] TYPEDEF ___int type_id=8
   [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
   [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t kind_flag=1 size=24 vlen=3
         a type_id=5 bitfield_size=0 bits_offset=0
         b type_id=1 bitfield_size=4 bits_offset=160
         c type_id=7 bitfield_size=4 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

   $ cat tt.c
   struct t;
   union u;
   int foo(struct t *t, union u *u) { return 0; }
   $ gcc -c -g -O2 tt.c
   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t type_id=0
   [3] PTR (anon) type_id=2
   [4] FWD u type_id=0
   [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t kind_flag=0 type_id=0
   [3] PTR (anon) kind_flag=0 type_id=2
   [4] FWD u kind_flag=1 type_id=0
   [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
  include/uapi/linux/btf.h |  15 ++-
  kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
  2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +4 -1

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
  	 * bits  0-15: vlen (e.g. # of struct's members)
  	 * bits 16-23: unused
  	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
  	 */
  	__u32 info;
  	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
  #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
  #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
  #define BTF_KIND_UNKN		0	/* Unknown	*/
  #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
  struct btf_member {
  	__u32	name_off;

include/uapi/linux/btf.h struct btf_array { · line 123

Quoted 10 lines

include/uapi/linux/btf.h +6 -1

  	__u32	type;
-	__u32	offset;	/* offset in bits */
+	__u32	offset;	/* [bitfield_size and] offset in bits */
  };
 
+/* If the type info kind_flag set, the btf_member.offset
+ * contains both member bit offset and bitfield size, and
+ * bitfield size will set for struct/union bitfield members.
+ * Otherwise, it contains only bit offset.
+ */

nit. It may be better to move this comment to the btf_member.offset above.

include/uapi/linux/btf.h struct btf_array { · line 125

Quoted 2 lines

include/uapi/linux/btf.h +2 -0

+#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
+#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)

After re-thinking this setup again, I still think having these macros in btf.h to also do the kflag checking would be nice.

Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't depend on other facts, the btf.h raw user must check kflag anyway before calling BTF_MEMBER_BIT*().
Forcing a kflag check before the user can access these convenient 0xfffff and >>24 conversions may enforce this kflag check to some extend.

Quoted 6 lines

Since it is in uapi, it will not be easy to change later. The above concern could be overkill ;), just want to ensure it has been thought through a bit more here.

It could be as easy as moving the new btf_member_bit*() from btf.c to here and remove these two macros (or move them back to btf.c).

I think moving:

kernel/bpf/btf.c +6 -0

+static u32 btf_member_bitfield_size(const struct btf_type *struct_type,
+                                   const struct btf_member *member)
+{
+       return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
+                                          : 0;
+}

Quoted 6 lines

into uapi/btf.h may or may not be useful for btf uapi users. What are the chances that these static inline helpers will be reused by BTF logic in libbpf or other libs?
At this point we don't know.

So I would keep btf.h minimal.

ok. Make sense

Quoted 3 lines

I agree that BTF_MEMBER_BIT_OFFSET() shouldn't be reused blindly. The users have to do BTF_INFO_KFLAG() check first.
But this is the case for pretty much all of BTF data structures.

Other similar situation in btf.h (i.e. a single u32 field can be interpreted differently) has at least an union as an indication (e.g. size and type in btf_type)

Here we cannot add the union (bitfield_offset:24 and bitfield_size:8) and we cannot change the name "offset" also. I am worry about m->offset will directly be used without checking the BTF_INFO_KFLAG().

Quoted 1 line

may be a "union { __u32 offset; __u32 bitsize_offset; };"......

The union with two __u32 is great idea. Maybe the bitsize_offset becomes "bitfield_size_offset" to reflect its real intention?

Quoted 228 lines · Yonghong Song

Quoted 224 lines · Martin Lau

Quoted 203 lines · Alexei Starovoitov

Quoted 187 lines · Martin Lau

Quoted 160 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,

    -bash-4.4$ cat t.c
    typedef int ___int;
    enum A { A1, A2, A3 };
    struct t {
      int a[5];
      ___int b:4;
      volatile enum A c:4;
    } g;
    -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
    $ pahole -JV t.o
    [1] TYPEDEF ___int type_id=2
    [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
    [3] ENUM A size=4 vlen=3
          A1 val=0
          A2 val=1
          A3 val=2
    [4] STRUCT t size=24 vlen=3
          a type_id=5 bits_offset=0
          b type_id=9 bits_offset=160
          c type_id=11 bits_offset=164
    [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
    [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
    [7] VOLATILE (anon) type_id=3
    [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
    [9] TYPEDEF ___int type_id=8
    [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
    [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

    $ pahole -JV t.o
    [1] TYPEDEF ___int type_id=2
    [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
    [3] ENUM A size=4 vlen=3
          A1 val=0
          A2 val=1
          A3 val=2
    [4] STRUCT t kind_flag=1 size=24 vlen=3
          a type_id=5 bitfield_size=0 bits_offset=0
          b type_id=1 bitfield_size=4 bits_offset=160
          c type_id=7 bitfield_size=4 bits_offset=164
    [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
    [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
    [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

    $ cat tt.c
    struct t;
    union u;
    int foo(struct t *t, union u *u) { return 0; }
    $ gcc -c -g -O2 tt.c
    $ pahole -JV tt.o
    [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
    [2] FWD t type_id=0
    [3] PTR (anon) type_id=2
    [4] FWD u type_id=0
    [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

    $ pahole -JV tt.o
    [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
    [2] FWD t kind_flag=0 type_id=0
    [3] PTR (anon) kind_flag=0 type_id=2
    [4] FWD u kind_flag=1 type_id=0
    [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
   include/uapi/linux/btf.h |  15 ++-
   kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
   2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +4 -1

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
   	 * bits  0-15: vlen (e.g. # of struct's members)
   	 * bits 16-23: unused
   	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
   	 */
   	__u32 info;
   	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
   #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
   #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
   #define BTF_KIND_UNKN		0	/* Unknown	*/
   #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
   struct btf_member {
   	__u32	name_off;

include/uapi/linux/btf.h struct btf_array { · line 123

Quoted 10 lines

include/uapi/linux/btf.h +6 -1

   	__u32	type;
-	__u32	offset;	/* offset in bits */
+	__u32	offset;	/* [bitfield_size and] offset in bits */
   };
 
+/* If the type info kind_flag set, the btf_member.offset
+ * contains both member bit offset and bitfield size, and
+ * bitfield size will set for struct/union bitfield members.
+ * Otherwise, it contains only bit offset.
+ */

nit. It may be better to move this comment to the btf_member.offset above.

include/uapi/linux/btf.h struct btf_array { · line 125

Quoted 2 lines

include/uapi/linux/btf.h +2 -0

+#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
+#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)

After re-thinking this setup again, I still think having these macros in btf.h to also do the kflag checking would be nice.

Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't depend on other facts, the btf.h raw user must check kflag anyway before calling BTF_MEMBER_BIT*().
Forcing a kflag check before the user can access these convenient 0xfffff and >>24 conversions may enforce this kflag check to some extend.

Quoted 6 lines

Since it is in uapi, it will not be easy to change later. The above concern could be overkill ;), just want to ensure it has been thought through a bit more here.

It could be as easy as moving the new btf_member_bit*() from btf.c to here and remove these two macros (or move them back to btf.c).

I think moving:

kernel/bpf/btf.c +6 -0

+static u32 btf_member_bitfield_size(const struct btf_type *struct_type,
+                                   const struct btf_member *member)
+{
+       return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
+                                          : 0;
+}

Quoted 6 lines

into uapi/btf.h may or may not be useful for btf uapi users. What are the chances that these static inline helpers will be reused by BTF logic in libbpf or other libs?
At this point we don't know.

So I would keep btf.h minimal.

ok. Make sense

Quoted 3 lines

I agree that BTF_MEMBER_BIT_OFFSET() shouldn't be reused blindly. The users have to do BTF_INFO_KFLAG() check first.
But this is the case for pretty much all of BTF data structures.

Other similar situation in btf.h (i.e. a single u32 field can be interpreted differently) has at least an union as an indication (e.g. size and type in btf_type)

Here we cannot add the union (bitfield_offset:24 and bitfield_size:8) and we cannot change the name "offset" also. I am worry about m->offset will directly be used without checking the BTF_INFO_KFLAG().

may be a "union { __u32 offset; __u32 bitsize_offset; };"......

Quoted 3 lines

The union with two __u32 is great idea. Maybe the bitsize_offset becomes "bitfield_size_offset" to reflect its real intention?

SGTM. Probably also spell out when to use "offset" and when to use "bitfield_size_offset" like the union in "struct btf_type". The BTF_MEMBER_BIT*() macro may also need to adjust to access the bitfield_size_offset instead to make the case clearer.

Quoted 1 line

may be a "union { __u32 offset; __u32 bitsize_offset; };"......

The union with two __u32 is great idea. Maybe the bitsize_offset becomes "bitfield_size_offset" to reflect its real intention?

Quoted 187 lines · Martin Lau

Quoted 160 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c

   typedef int ___int;
   enum A { A1, A2, A3 };
   struct t {
     int a[5];
     ___int b:4;
     volatile enum A c:4;
   } g;
   -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t size=24 vlen=3
         a type_id=5 bits_offset=0
         b type_id=9 bits_offset=160
         c type_id=11 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3
   [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
   [9] TYPEDEF ___int type_id=8
   [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
   [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t kind_flag=1 size=24 vlen=3
         a type_id=5 bitfield_size=0 bits_offset=0
         b type_id=1 bitfield_size=4 bits_offset=160
         c type_id=7 bitfield_size=4 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3

Issue #2 and solution:
======================

Current forward type in BTF does not specify whether the original type is struct or union. This will not work for type pretty print and BTF-to-header-file conversion as struct/union must be specified.

   $ cat tt.c
   struct t;
   union u;
   int foo(struct t *t, union u *u) { return 0; }
   $ gcc -c -g -O2 tt.c
   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t type_id=0
   [3] PTR (anon) type_id=2
   [4] FWD u type_id=0
   [5] PTR (anon) type_id=4

To fix this issue, similar to issue #1, type->info bit 31 is used. If the bit is set, it is union type. Otherwise, it is a struct type.

   $ pahole -JV tt.o
   [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [2] FWD t kind_flag=0 type_id=0
   [3] PTR (anon) kind_flag=0 type_id=2
   [4] FWD u kind_flag=1 type_id=0
   [5] PTR (anon) kind_flag=0 type_id=4

Pahole/LLVM change:
===================

The new kind_flag functionality has been implemented in pahole and llvm:
https://github.com/yonghong-song/pahole/tree/bitfield
https://github.com/yonghong-song/llvm/tree/bitfield

Note that pahole hasn't implemented func/func_proto kind and .BTF.ext. So to print function signature with bpftool, the llvm compiler should be used.

Fixes 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")

Signed-off-by Martin KaFai Lau <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
  include/uapi/linux/btf.h |  15 ++-
  kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
  2 files changed, 267 insertions(+), 22 deletions(-)

include/uapi/linux/btf.h +4 -1

index 14f66948fc95..34aba40ed926 100644
@@ -34,7 +34,9 @@ struct btf_type {
  	 * bits  0-15: vlen (e.g. # of struct's members)
  	 * bits 16-23: unused
  	 * bits 24-27: kind (e.g. int, ptr, array...etc)
-	 * bits 28-31: unused
+	 * bits 28-30: unused
+	 * bit     31: kind_flag, currently used by
+	 *             struct, union and fwd
  	 */
  	__u32 info;
  	/* "size" is used by INT, ENUM, STRUCT and UNION.

@@ -52,6 +54,7 @@ struct btf_type {
 
  #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
  #define BTF_INFO_VLEN(info)	((info) & 0xffff)
+#define BTF_INFO_KFLAG(info)	((info) >> 31)
 
  #define BTF_KIND_UNKN		0	/* Unknown	*/
  #define BTF_KIND_INT		1	/* Integer	*/

@@ -110,9 +113,17 @@ struct btf_array {
  struct btf_member {
  	__u32	name_off;

include/uapi/linux/btf.h struct btf_array { · line 123

Quoted 10 lines

include/uapi/linux/btf.h +6 -1

  	__u32	type;
-	__u32	offset;	/* offset in bits */
+	__u32	offset;	/* [bitfield_size and] offset in bits */
  };
 
+/* If the type info kind_flag set, the btf_member.offset
+ * contains both member bit offset and bitfield size, and
+ * bitfield size will set for struct/union bitfield members.
+ * Otherwise, it contains only bit offset.
+ */

nit. It may be better to move this comment to the btf_member.offset above.

include/uapi/linux/btf.h struct btf_array { · line 125

Quoted 2 lines

include/uapi/linux/btf.h +2 -0

+#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
+#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)

After re-thinking this setup again, I still think having these macros in btf.h to also do the kflag checking would be nice.

Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't depend on other facts, the btf.h raw user must check kflag anyway before calling BTF_MEMBER_BIT*().
Forcing a kflag check before the user can access these convenient 0xfffff and >>24 conversions may enforce this kflag check to some extend.

Quoted 6 lines

Since it is in uapi, it will not be easy to change later. The above concern could be overkill ;), just want to ensure it has been thought through a bit more here.

It could be as easy as moving the new btf_member_bit*() from btf.c to here and remove these two macros (or move them back to btf.c).

I think moving:

kernel/bpf/btf.c +6 -0

+static u32 btf_member_bitfield_size(const struct btf_type *struct_type,
+                                   const struct btf_member *member)
+{
+       return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
+                                          : 0;
+}

into uapi/btf.h may or may not be useful for btf uapi users. What are the chances that these static inline helpers will be reused by BTF logic in libbpf or other libs?
At this point we don't know.
So I would keep btf.h minimal.
I agree that BTF_MEMBER_BIT_OFFSET() shouldn't be reused blindly. The users have to do BTF_INFO_KFLAG() check first.
But this is the case for pretty much all of BTF data structures. In that sense BTF_MEMBER_BITFIELD_SIZE/BTF_MEMBER_BIT_OFFSET serve as a documentation and fit the style of btf.h header. imo having these two macros in uapi/btf.h is better than replacing them with comment only.

After this set the whole BTF really needs to be documented.

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c
typedef int ___int;
enum A { A1, A2, A3 };

  struct t {
    int a[5];
    ___int b:4;
    volatile enum A c:4;
  } g;
  -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t size=24 vlen=3
        a type_id=5 bits_offset=0
        b type_id=9 bits_offset=160
        c type_id=11 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3
  [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
  [9] TYPEDEF ___int type_id=8
  [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
  [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

This kernel patch intends to support the new BTF encoding:

  $ pahole -JV t.o
  [1] TYPEDEF ___int type_id=2
  [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
  [3] ENUM A size=4 vlen=3
        A1 val=0
        A2 val=1
        A3 val=2
  [4] STRUCT t kind_flag=1 size=24 vlen=3
        a type_id=5 bitfield_size=0 bits_offset=0
        b type_id=1 bitfield_size=4 bits_offset=160
        c type_id=7 bitfield_size=4 bits_offset=164
  [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
  [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
  [7] VOLATILE (anon) type_id=3

kernel/bpf/btf.c +23 -0

+static int btf_int_check_kflag_member(struct btf_verifier_env *env,
+				      const struct btf_type *struct_type,
+				      const struct btf_member *member,
+				      const struct btf_type *member_type)
+{
+	u32 struct_bits_off, nr_bits, nr_int_data_bits, bytes_offset;
+	u32 int_data = btf_type_int(member_type);
+	u32 struct_size = struct_type->size;
+	u32 nr_copy_bits;
+
+	/* a regular int type is required for the kflag int member */
+	if (!btf_type_int_is_regular(member_type)) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Invalid member base type");
+		return -EINVAL;
+	}
+
+	/* check sanity of bitfield size */
+	nr_bits = BTF_MEMBER_BITFIELD_SIZE(member->offset);
+	struct_bits_off = BTF_MEMBER_BIT_OFFSET(member->offset);
+	nr_int_data_bits = BTF_INT_BITS(int_data);
+	if (!nr_bits) {
+		/* Not a bitfield member, member offset must be at byte

kernel/bpf/btf.c +22 -0

+		btf_verifier_log_member(env, struct_type, member,
+					"Invalid member bitfield_size");
+		return -EINVAL;
+	}
+
+	bytes_offset = BITS_ROUNDDOWN_BYTES(struct_bits_off);
+	nr_copy_bits = nr_bits + BITS_PER_BYTE_MASKED(struct_bits_off);
+	if (nr_copy_bits > BITS_PER_U64) {
+		btf_verifier_log_member(env, struct_type, member,
+					"nr_copy_bits exceeds 64");
+		return -EINVAL;
+	}
+
+	if (struct_size < bytes_offset ||
+	    struct_size - bytes_offset < BITS_ROUNDUP_BYTES(nr_copy_bits)) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Member exceeds struct_size");
+		return -EINVAL;
+	}
+
+	return 0;
+}

Quoted 64 lines · Yonghong Song

This patch fixed two issues with BTF. One is related to struct/union bitfield encoding and the other is related to forward type.

Issue #1 and solution:
======================

Current btf encoding of bitfield follows what pahole generates. For each bitfield, pahole will duplicate the type chain and put the bitfield size at the final int or enum type. Since the BTF enum type cannot encode bit size, pahole workarounds the issue by generating
an int type whenever the enum bit size is not 32.

For example,
-bash-4.4$ cat t.c

   typedef int ___int;
   enum A { A1, A2, A3 };
   struct t {
     int a[5];
     ___int b:4;
     volatile enum A c:4;
   } g;
   -bash-4.4$ gcc -c -O2 -g t.c
The current kernel supports the following BTF encoding:
   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t size=24 vlen=3
         a type_id=5 bits_offset=0
         b type_id=9 bits_offset=160
         c type_id=11 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3
   [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
   [9] TYPEDEF ___int type_id=8
   [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
   [11] VOLATILE (anon) type_id=10

Two issues are in the above:
. by changing enum type to int, we lost the original
type information and this will not be ideal later when we try to convert BTF to a header file.
. the type duplication for bitfields will cause
BTF bloat. Duplicated types cannot be deduplicated later if the bitfield size is different.

To fix this issue, this patch implemented a compatible change for BTF struct type encoding:
. the bit 31 of struct_type->info, previously reserved,
now is used to indicate whether bitfield_size is encoded in btf_member or not.
. if bit 31 of struct_type->info is set,
btf_member->offset will encode like:
bit 0 - 23: bit offset
bit 24 - 31: bitfield size
if bit 31 is not set, the old behavior is preserved:
bit 0 - 31: bit offset

Quoted 4 lines

So if the struct contains a bit field, the maximum bit offset will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum bitfield size will be 256 which is enough for today as maximum bitfield in compiler can be 128 where int128 type is supported.

Looks good in general, just small nit below.

Quoted 15 lines

This kernel patch intends to support the new BTF encoding:

   $ pahole -JV t.o
   [1] TYPEDEF ___int type_id=2
   [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
   [3] ENUM A size=4 vlen=3
         A1 val=0
         A2 val=1
         A3 val=2
   [4] STRUCT t kind_flag=1 size=24 vlen=3
         a type_id=5 bitfield_size=0 bits_offset=0
         b type_id=1 bitfield_size=4 bits_offset=160
         c type_id=7 bitfield_size=4 bits_offset=164
   [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
   [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
   [7] VOLATILE (anon) type_id=3

[...]

Quoted 29 lines

kernel/bpf/btf.c +29 -0

+static int btf_int_check_kflag_member(struct btf_verifier_env *env,
+				      const struct btf_type *struct_type,
+				      const struct btf_member *member,
+				      const struct btf_type *member_type)
+{
+	u32 struct_bits_off, nr_bits, nr_int_data_bits, bytes_offset;
+	u32 int_data = btf_type_int(member_type);
+	u32 struct_size = struct_type->size;
+	u32 nr_copy_bits;
+
+	/* a regular int type is required for the kflag int member */
+	if (!btf_type_int_is_regular(member_type)) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Invalid member base type");
+		return -EINVAL;
+	}
+
+	/* check sanity of bitfield size */
+	nr_bits = BTF_MEMBER_BITFIELD_SIZE(member->offset);
+	struct_bits_off = BTF_MEMBER_BIT_OFFSET(member->offset);
+	nr_int_data_bits = BTF_INT_BITS(int_data);
+	if (!nr_bits) {
+		/* Not a bitfield member, member offset must be at byte
+		 * boundary.
+		 */
+		if (BITS_PER_BYTE_MASKED(struct_bits_off)) {
+			btf_verifier_log_member(env, struct_type, member,
+						"Invalid member offset");
+			return -EINVAL;

kernel/bpf/btf.c static int btf_int_check_member(struct btf_verifier_env *env, · line 1095

Quoted 4 lines

kernel/bpf/btf.c +4 -0

+		}
+
+		nr_bits = nr_int_data_bits;
+	} else if (nr_bits > nr_int_data_bits) {

Should the test here not include the bit offset as well aka nr_copy_bits? Thus test would be e.g. (nr_copy_bits > nr_int_data_bits || nr_copy_bits > BITS_PER_U64) ...

Wrt to future 256 bit support, doesn't UAPI on BTF_INT_BITS() only support up to max 255 bits?

Quoted 23 lines

kernel/bpf/btf.c +22 -0

+		btf_verifier_log_member(env, struct_type, member,
+					"Invalid member bitfield_size");
+		return -EINVAL;
+	}
+
+	bytes_offset = BITS_ROUNDDOWN_BYTES(struct_bits_off);
+	nr_copy_bits = nr_bits + BITS_PER_BYTE_MASKED(struct_bits_off);
+	if (nr_copy_bits > BITS_PER_U64) {
+		btf_verifier_log_member(env, struct_type, member,
+					"nr_copy_bits exceeds 64");
+		return -EINVAL;
+	}
+
+	if (struct_size < bytes_offset ||
+	    struct_size - bytes_offset < BITS_ROUNDUP_BYTES(nr_copy_bits)) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Member exceeds struct_size");
+		return -EINVAL;
+	}
+
+	return 0;
+}

Commit 970289fc0a83 ("bpf: add bpffs pretty print for cgroup local storage maps") added bpffs pretty print for cgroup local storage maps. The commit worked for struct without kind_flag set.

This patch refactored and made pretty print also work with kind_flag set for the struct.

Signed-off-by Yonghong Song <[email protected]>

---
 include/linux/btf.h        |  5 ++++-
 kernel/bpf/btf.c           | 37 ++++++++++++++++++++++++++++---------
 kernel/bpf/local_storage.c | 17 ++++-------------
 3 files changed, 36 insertions(+), 23 deletions(-)

include/linux/btf.h +4 -1

index 58000d7e06e3..12502e25e767 100644
@@ -7,6 +7,7 @@
 #include <linux/types.h>
 
 struct btf;
+struct btf_member;
 struct btf_type;
 union bpf_attr;
 

@@ -46,7 +47,9 @@ void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
 		       struct seq_file *m);
 int btf_get_fd_by_id(u32 id);
 u32 btf_id(const struct btf *btf);
-bool btf_type_is_reg_int(const struct btf_type *t, u32 expected_size);
+bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
+			   const struct btf_member *m,
+			   u32 expected_offset, u32 expected_size);
 
 #ifdef CONFIG_BPF_SYSCALL
 const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);

kernel/bpf/btf.c +11 -7

index ec3f80d3bef6..30028fa187df 100644
@@ -546,22 +546,41 @@ static bool btf_type_int_is_regular(const struct btf_type *t)
 }
 
 /*
- * Check that given type is a regular int and has the expected size.
+ * Check that given struct member is a regular int with expected
+ * offset and size.
  */
-bool btf_type_is_reg_int(const struct btf_type *t, u32 expected_size)
+bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
+			   const struct btf_member *m,
+			   u32 expected_offset, u32 expected_size)
 {
-	u8 nr_bits, nr_bytes;
-	u32 int_data;
+	const struct btf_type *t;
+	u32 id, int_data;
+	u8 nr_bits;
 
-	if (!btf_type_is_int(t))
+	id = m->type;
+	t = btf_type_id_size(btf, &id, NULL);
+	if (!t || !btf_type_is_int(t))
 		return false;
 
 	int_data = btf_type_int(t);
 	nr_bits = BTF_INT_BITS(int_data);
-	nr_bytes = BITS_ROUNDUP_BYTES(nr_bits);
-	if (BITS_PER_BYTE_MASKED(nr_bits) ||

kernel/bpf/btf.c +9 -0

+		       BITS_ROUNDUP_BYTES(bit_offset) == expected_offset &&
+		       BITS_ROUNDUP_BYTES(nr_bits) == expected_size;
+	}
+
+	if (BTF_INT_OFFSET(int_data) ||
+	    BITS_PER_BYTE_MASKED(m->offset) ||
+	    BITS_ROUNDUP_BYTES(m->offset) != expected_offset ||
+	    BITS_PER_BYTE_MASKED(nr_bits) ||
+	    BITS_ROUNDUP_BYTES(nr_bits) != expected_size)
 		return false;
 
 	return true;

kernel/bpf/local_storage.c +4 -13

index 5eca03da0989..07a34ef562a0 100644
@@ -315,9 +315,8 @@ static int cgroup_storage_check_btf(const struct bpf_map *map,
 				    const struct btf_type *key_type,
 				    const struct btf_type *value_type)
 {
-	const struct btf_type *t;
 	struct btf_member *m;
-	u32 id, size;
+	u32 offset, size;
 
 	/* Key is expected to be of struct bpf_cgroup_storage_key type,
 	 * which is:

@@ -338,25 +337,17 @@ static int cgroup_storage_check_btf(const struct bpf_map *map,
 	 * The first field must be a 64 bit integer at 0 offset.
 	 */
 	m = (struct btf_member *)(key_type + 1);
-	if (m->offset)
-		return -EINVAL;
-	id = m->type;
-	t = btf_type_id_size(btf, &id, NULL);
 	size = FIELD_SIZEOF(struct bpf_cgroup_storage_key, cgroup_inode_id);
-	if (!t || !btf_type_is_reg_int(t, size))
+	if (!btf_member_is_reg_int(btf, key_type, m, 0, size))
 		return -EINVAL;
 
 	/*
 	 * The second field must be a 32 bit integer at 64 bit offset.
 	 */
 	m++;
-	if (m->offset != offsetof(struct bpf_cgroup_storage_key, attach_type) *
-	    BITS_PER_BYTE)
-		return -EINVAL;
-	id = m->type;
-	t = btf_type_id_size(btf, &id, NULL);
+	offset = offsetof(struct bpf_cgroup_storage_key, attach_type);
 	size = FIELD_SIZEOF(struct bpf_cgroup_storage_key, attach_type);
-	if (!t || !btf_type_is_reg_int(t, size))
+	if (!btf_member_is_reg_int(btf, key_type, m, offset, size))
 		return -EINVAL;
 
 	return 0;

Signature · 1 line

2.17.1

Sync include/uapi/linux/btf.h to tools/include/uapi/linux/btf.h.

This patch added unit tests for different types handling type->info.kind_flag. The following new tests are added:

  $ test_btf
  ...
  BTF raw test[82] (invalid int kind_flag): OK
  BTF raw test[83] (invalid ptr kind_flag): OK
  BTF raw test[84] (invalid array kind_flag): OK
  BTF raw test[85] (invalid enum kind_flag): OK
  BTF raw test[86] (valid fwd kind_flag): OK
  BTF raw test[87] (invalid typedef kind_flag): OK
  BTF raw test[88] (invalid volatile kind_flag): OK
  BTF raw test[89] (invalid const kind_flag): OK
  BTF raw test[90] (invalid restrict kind_flag): OK
  BTF raw test[91] (invalid func kind_flag): OK
  BTF raw test[92] (invalid func_proto kind_flag): OK
  BTF raw test[93] (valid struct kind_flag, bitfield_size = 0): OK
  BTF raw test[94] (valid struct kind_flag, int member, bitfield_size != 0): OK
  BTF raw test[95] (valid union kind_flag, int member, bitfield_size != 0): OK

  BTF raw test[96] (valid struct kind_flag, enum member, bitfield_size != 0): OK
  BTF raw test[97] (valid union kind_flag, enum member, bitfield_size != 0): OK
  BTF raw test[98] (valid struct kind_flag, typedef member, bitfield_size != 0): OK
  BTF raw test[99] (valid union kind_flag, typedef member, bitfield_size != 0): OK
  BTF raw test[100] (invalid struct type, bitfield_size greater than struct size): OK
  BTF raw test[101] (invalid struct type, kind_flag bitfield base_type int not regular): OK
  BTF raw test[102] (invalid struct type, kind_flag base_type int not regular): OK
  BTF raw test[103] (invalid union type, bitfield_size greater than struct size): OK

...
PASS:122 SKIP:0 FAIL:0

The second parameter name of macro
BTF_INFO_ENC(kind, root, vlen)
in selftests test_btf.c is also renamed from "root" to "kind_flag". Note that before this patch "root" is not used and always 0.

Quoted 18 lines · Yonghong Song

This patch added unit tests for different types handling type->info.kind_flag. The following new tests are added:

   $ test_btf
   ...
   BTF raw test[82] (invalid int kind_flag): OK
   BTF raw test[83] (invalid ptr kind_flag): OK
   BTF raw test[84] (invalid array kind_flag): OK
   BTF raw test[85] (invalid enum kind_flag): OK
   BTF raw test[86] (valid fwd kind_flag): OK
   BTF raw test[87] (invalid typedef kind_flag): OK
   BTF raw test[88] (invalid volatile kind_flag): OK
   BTF raw test[89] (invalid const kind_flag): OK
   BTF raw test[90] (invalid restrict kind_flag): OK
   BTF raw test[91] (invalid func kind_flag): OK
   BTF raw test[92] (invalid func_proto kind_flag): OK
   BTF raw test[93] (valid struct kind_flag, bitfield_size = 0): OK
   BTF raw test[94] (valid struct kind_flag, int member, bitfield_size != 0): OK
   BTF raw test[95] (valid union kind_flag, int member, bitfield_size != 0): OK

Quoted 8 lines

   BTF raw test[96] (valid struct kind_flag, enum member, bitfield_size != 0): OK
   BTF raw test[97] (valid union kind_flag, enum member, bitfield_size != 0): OK
   BTF raw test[98] (valid struct kind_flag, typedef member, bitfield_size != 0): OK
   BTF raw test[99] (valid union kind_flag, typedef member, bitfield_size != 0): OK
   BTF raw test[100] (invalid struct type, bitfield_size greater than struct size): OK
   BTF raw test[101] (invalid struct type, kind_flag bitfield base_type int not regular): OK
   BTF raw test[102] (invalid struct type, kind_flag base_type int not regular): OK
   BTF raw test[103] (invalid union type, bitfield_size greater than struct size): OK

Would it be useful to add some
"struct kind_flag, (int|enum) member, bitfield_size == 0, wrong byte alignment" tests to catch the "!nr_bits && BITS_PER_BYTE_MASKED(struct_bits_off)" in patch 2. The extra tests could be a follow up though.

Or the above tests have covered that already?

Quoted 8 lines

   ...
   PASS:122 SKIP:0 FAIL:0

The second parameter name of macro
BTF_INFO_ENC(kind, root, vlen)
in selftests test_btf.c is also renamed from "root" to "kind_flag". Note that before this patch "root" is not used and always 0.

For the tests:

Acked-by Martin KaFai Lau <[email protected]>

The new tests are added to test bpffs map pretty print in kernel with kind_flag for structure type.

  $ test_btf -p
  ......
  BTF pretty print array(#1)......OK
  BTF pretty print array(#2)......OK
  PASS:8 SKIP:0 FAIL:0

The core dump funcitonality in btf_dumper_int_bits() is refactored into a separate function btf_dumper_bitfield() which will be used by the next patch.

Signed-off-by Yonghong Song <[email protected]>

---
 tools/bpf/bpftool/btf_dumper.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

tools/bpf/bpftool/btf_dumper.c +3 -6

index 5cdb2ef8b6f1..ec1da87c3d65 100644
@@ -73,20 +73,17 @@ static int btf_dumper_array(const struct btf_dumper *d, __u32 type_id,
 	return ret;
 }
 
-static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset,
+static void btf_dumper_bitfield(__u32 nr_bits, __u8 bit_offset,
 				const void *data, json_writer_t *jw,
 				bool is_plain_text)
 {
 	int left_shift_bits, right_shift_bits;
-	int nr_bits = BTF_INT_BITS(int_type);
-	int total_bits_offset;
 	int bytes_to_copy;
 	int bits_to_copy;
 	__u64 print_num;
 
-	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
-	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
-	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+	data += BITS_ROUNDDOWN_BYTES(bit_offset);
+	bit_offset = BITS_PER_BYTE_MASKED(bit_offset);
 	bits_to_copy = bit_offset + nr_bits;
 	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
 

@@ -109,6 +106,19 @@ static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset,
 		jsonw_printf(jw, "%llu", print_num);
 }
 

tools/bpf/bpftool/btf_dumper.c +3 -0

+			    is_plain_text);
+}
+
 static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset,
 			  const void *data, json_writer_t *jw,
 			  bool is_plain_text)

Signature · 1 line

2.17.1

Quoted 41 lines · Yonghong Song

The core dump funcitonality in btf_dumper_int_bits() is refactored into a separate function btf_dumper_bitfield() which will be used by the next patch.

Signed-off-by Yonghong Song <[email protected]>

---
  tools/bpf/bpftool/btf_dumper.c | 22 ++++++++++++++++------
  1 file changed, 16 insertions(+), 6 deletions(-)

tools/bpf/bpftool/btf_dumper.c +3 -6

index 5cdb2ef8b6f1..ec1da87c3d65 100644
@@ -73,20 +73,17 @@ static int btf_dumper_array(const struct btf_dumper *d, __u32 type_id,
  	return ret;
  }
 
-static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset,
+static void btf_dumper_bitfield(__u32 nr_bits, __u8 bit_offset,
  				const void *data, json_writer_t *jw,
  				bool is_plain_text)
  {
  	int left_shift_bits, right_shift_bits;
-	int nr_bits = BTF_INT_BITS(int_type);
-	int total_bits_offset;
  	int bytes_to_copy;
  	int bits_to_copy;
  	__u64 print_num;
 
-	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
-	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
-	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+	data += BITS_ROUNDDOWN_BYTES(bit_offset);
+	bit_offset = BITS_PER_BYTE_MASKED(bit_offset);
  	bits_to_copy = bit_offset + nr_bits;
  	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
 

@@ -109,6 +106,19 @@ static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset,
  		jsonw_printf(jw, "%llu", print_num);
  }
 

tools/bpf/bpftool/btf_dumper.c static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset, · line 118

Quoted 10 lines

tools/bpf/bpftool/btf_dumper.c +10 -0

+
+static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset,
+				const void *data, json_writer_t *jw,
+				bool is_plain_text)
+{
+	int nr_bits = BTF_INT_BITS(int_type);
+	int total_bits_offset;
+
+	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
+	btf_dumper_bitfield(nr_bits, total_bits_offset, data, jw,

The 2nd arg is "__u8 bit_offset". Can you check if total_bits_offset is fine here, considering BTF_INT_OFFSET() is 8 bits itself. A comment would help if it is safe.

Other than that,

Acked-by Martin KaFai Lau <[email protected]>

Quoted 9 lines

tools/bpf/bpftool/btf_dumper.c +3 -0

+			    is_plain_text);
+}
+
  static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset,
  			  const void *data, json_writer_t *jw,
  			  bool is_plain_text)

Signature · 1 line

2.17.1

The following example shows map pretty print with structures which include bitfield members.

  enum A { A1, A2, A3, A4, A5 };
  typedef enum A ___A;
  struct tmp_t {
       char a1:4;
       int  a2:4;
       int  :4;
       __u32 a3:4;
       int b;
       ___A b1:4;
       enum A b2:4;
  };
  struct bpf_map_def SEC("maps") tmpmap = {
       .type = BPF_MAP_TYPE_ARRAY,
       .key_size = sizeof(__u32),
       .value_size = sizeof(struct tmp_t),
       .max_entries = 1,
  };
  BPF_ANNOTATE_KV_PAIR(tmpmap, int, struct tmp_t);

and the following map update in the bpf program:

  key = 0;
  struct tmp_t t = {};
  t.a1 = 2;
  t.a2 = 4;
  t.a3 = 6;
  t.b = 7;
  t.b1 = 8;
  t.b2 = 10;
  bpf_map_update_elem(&tmpmap, &key, &t, 0);

With this patch, I am able to print out the map values correctly with this patch:
bpftool map dump id 187

  [{
        "key": 0,
        "value": {
            "a1": 0x2,
            "a2": 0x4,
            "a3": 0x6,
            "b": 7,
            "b1": 0x8,
            "b2": 0xa
        }
    }

]

The following example shows forward type can be properly printed in function prototype with modified tst_btf_haskv.c.

struct t;
union u;

  __attribute__((noinline))
  static int test_long_fname_1(struct dummy_tracepoint_args *arg,
                               struct t *p1, union u *p2,
                               __u32 unused)
  ...
  int _dummy_tracepoint(struct dummy_tracepoint_args *arg) {
    return test_long_fname_1(arg, 0, 0);
  }

  $ bpftool p d xlated id 24
  ...
  int test_long_fname_1(struct dummy_tracepoint_args * arg,
                        struct t * p1, union u * p2,
                        __u32 unused)
  ...

Signed-off-by Yonghong Song <[email protected]>

---
 tools/bpf/bpftool/btf_dumper.c | 36 +++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

tools/bpf/bpftool/btf_dumper.c +1 -0

index ec1da87c3d65..23fc6a84d82b 100644
@@ -190,6 +190,7 @@ static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id,
 	const struct btf_type *t;
 	struct btf_member *m;
 	const void *data_off;
+	int kind_flag;
 	int ret = 0;
 	int i, vlen;
 

@@ -197,18 +198,32 @@ static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id,
 	if (!t)
 		return -EINVAL;
 

tools/bpf/bpftool/btf_dumper.c +15 -5

+			bitfield_size = BTF_MEMBER_BITFIELD_SIZE(bit_offset);
+			bit_offset = BTF_MEMBER_BIT_OFFSET(bit_offset);
+		}
+
 		jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
-		ret = btf_dumper_do_type(d, m[i].type,
-					 BITS_PER_BYTE_MASKED(m[i].offset),
-					 data_off);
-		if (ret)
-			break;
+		if (bitfield_size) {
+			btf_dumper_bitfield(bitfield_size, bit_offset,
+					    data, d->jw, d->is_plain_text);
+		} else {
+			data_off = data + BITS_ROUNDDOWN_BYTES(bit_offset);
+			ret = btf_dumper_do_type(d, m[i].type,
+						 BITS_PER_BYTE_MASKED(bit_offset),
+						 data_off);
+			if (ret)
+				break;
+		}
 	}

tools/bpf/bpftool/btf_dumper.c +1 -0

 	jsonw_end_object(d->jw);

@@ -295,6 +310,7 @@ static int __btf_dumper_type_only(const struct btf *btf, __u32 type_id,
 
 	switch (BTF_INFO_KIND(t->info)) {
 	case BTF_KIND_INT:
+	case BTF_KIND_TYPEDEF:
 		BTF_PRINT_ARG("%s ", btf__name_by_offset(btf, t->name_off));
 		break;
 	case BTF_KIND_STRUCT:

tools/bpf/bpftool/btf_dumper.c +5 -1

-		return -1;
+		BTF_PRINT_ARG("%s %s ",
+			      BTF_INFO_KFLAG(t->info) ? "union" : "struct",
+			      btf__name_by_offset(btf, t->name_off));
+		break;
 	case BTF_KIND_VOLATILE:
 		BTF_PRINT_ARG("volatile ");
 		BTF_PRINT_TYPE(t->type);

@@ -345,6 +362,7 @@ static int __btf_dumper_type_only(const struct btf *btf, __u32 type_id,
 		if (pos == -1)
 			return -1;
 		break;
+	case BTF_KIND_UNKN:
 	default:
 		return -1;
 	}

Signature · 1 line

2.17.1

Quoted 93 lines · Yonghong Song

The following example shows map pretty print with structures which include bitfield members.

   enum A { A1, A2, A3, A4, A5 };
   typedef enum A ___A;
   struct tmp_t {
        char a1:4;
        int  a2:4;
        int  :4;
        __u32 a3:4;
        int b;
        ___A b1:4;
        enum A b2:4;
   };
   struct bpf_map_def SEC("maps") tmpmap = {
        .type = BPF_MAP_TYPE_ARRAY,
        .key_size = sizeof(__u32),
        .value_size = sizeof(struct tmp_t),
        .max_entries = 1,
   };
   BPF_ANNOTATE_KV_PAIR(tmpmap, int, struct tmp_t);

and the following map update in the bpf program:

   key = 0;
   struct tmp_t t = {};
   t.a1 = 2;
   t.a2 = 4;
   t.a3 = 6;
   t.b = 7;
   t.b1 = 8;
   t.b2 = 10;
   bpf_map_update_elem(&tmpmap, &key, &t, 0);

With this patch, I am able to print out the map values correctly with this patch:
bpftool map dump id 187

   [{
         "key": 0,
         "value": {
             "a1": 0x2,
             "a2": 0x4,
             "a3": 0x6,
             "b": 7,
             "b1": 0x8,
             "b2": 0xa
         }
     }

]

The following example shows forward type can be properly printed in function prototype with modified tst_btf_haskv.c.

   struct t;
   union  u;

   __attribute__((noinline))
   static int test_long_fname_1(struct dummy_tracepoint_args *arg,
                                struct t *p1, union u *p2,
                                __u32 unused)
   ...
   int _dummy_tracepoint(struct dummy_tracepoint_args *arg) {
     return test_long_fname_1(arg, 0, 0);
   }

   $ bpftool p d xlated id 24
   ...
   int test_long_fname_1(struct dummy_tracepoint_args * arg,
                         struct t * p1, union u * p2,
                         __u32 unused)
   ...

Signed-off-by Yonghong Song <[email protected]>

---
  tools/bpf/bpftool/btf_dumper.c | 36 +++++++++++++++++++++++++---------
  1 file changed, 27 insertions(+), 9 deletions(-)

tools/bpf/bpftool/btf_dumper.c +1 -0

index ec1da87c3d65..23fc6a84d82b 100644
@@ -190,6 +190,7 @@ static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id,
  	const struct btf_type *t;
  	struct btf_member *m;
  	const void *data_off;
+	int kind_flag;
  	int ret = 0;
  	int i, vlen;
 

@@ -197,18 +198,32 @@ static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id,
  	if (!t)
  		return -EINVAL;
 

tools/bpf/bpftool/btf_dumper.c static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id, · line 209

Quoted 10 lines

tools/bpf/bpftool/btf_dumper.c +2 -1

+	kind_flag = BTF_INFO_KFLAG(t->info);
  	vlen = BTF_INFO_VLEN(t->info);
  	jsonw_start_object(d->jw);
  	m = (struct btf_member *)(t + 1);
 
  	for (i = 0; i < vlen; i++) {
-		data_off = data + BITS_ROUNDDOWN_BYTES(m[i].offset);
 
 
+		__u32 bit_offset = m[i].offset;

Without always checking the kind_flag first, here is an example that accessing the .offset looks incorrect at the first glance.

tools/bpf/bpftool/btf_dumper.c static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id, · line 210

Quoted 3 lines

tools/bpf/bpftool/btf_dumper.c +3 -0

+		__u32 bitfield_size = 0;
+
+		if (kind_flag) {

but then I noticed it is fine here ;)

Quoted 26 lines

tools/bpf/bpftool/btf_dumper.c +15 -5

+			bitfield_size = BTF_MEMBER_BITFIELD_SIZE(bit_offset);
+			bit_offset = BTF_MEMBER_BIT_OFFSET(bit_offset);
+		}
+
  		jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
-		ret = btf_dumper_do_type(d, m[i].type,
-					 BITS_PER_BYTE_MASKED(m[i].offset),
-					 data_off);
-		if (ret)
-			break;
+		if (bitfield_size) {
+			btf_dumper_bitfield(bitfield_size, bit_offset,
+					    data, d->jw, d->is_plain_text);
+		} else {
+			data_off = data + BITS_ROUNDDOWN_BYTES(bit_offset);
+			ret = btf_dumper_do_type(d, m[i].type,
+						 BITS_PER_BYTE_MASKED(bit_offset),
+						 data_off);
+			if (ret)
+				break;
+		}
  	}

  
  	jsonw_end_object(d->jw);

tools/bpf/bpftool/btf_dumper.c +0 -0

@@ -295,6 +310,7 @@ static int __btf_dumper_type_only(const struct btf *btf, __u32 type_id,
 

tools/bpf/bpftool/btf_dumper.c static int __btf_dumper_type_only(const struct btf *btf, __u32 type_id, · line 323

Quoted 13 lines

tools/bpf/bpftool/btf_dumper.c +1 -2

  	switch (BTF_INFO_KIND(t->info)) {
  	case BTF_KIND_INT:
+	case BTF_KIND_TYPEDEF:
  		BTF_PRINT_ARG("%s ", btf__name_by_offset(btf, t->name_off));
  		break;
  	case BTF_KIND_STRUCT:

@@ -318,10 +334,11 @@ static int __btf_dumper_type_only(const struct btf *btf, __u32 type_id,
  		BTF_PRINT_TYPE(t->type);
  		BTF_PRINT_ARG("* ");
  		break;
-	case BTF_KIND_UNKN:
  	case BTF_KIND_FWD:
-	case BTF_KIND_TYPEDEF:

hmm.... Is this TYPEDEF change related to this patch? If not, a comment in the commit message would help.

Other than that,

Acked-by Martin KaFai Lau <[email protected]>

Quoted 19 lines

tools/bpf/bpftool/btf_dumper.c +5 -1

-		return -1;
+		BTF_PRINT_ARG("%s %s ",
+			      BTF_INFO_KFLAG(t->info) ? "union" : "struct",
+			      btf__name_by_offset(btf, t->name_off));
+		break;
  	case BTF_KIND_VOLATILE:
  		BTF_PRINT_ARG("volatile ");
  		BTF_PRINT_TYPE(t->type);

@@ -345,6 +362,7 @@ static int __btf_dumper_type_only(const struct btf *btf, __u32 type_id,
  		if (pos == -1)
  			return -1;
  		break;
+	case BTF_KIND_UNKN:
  	default:
  		return -1;
  	}

Signature · 1 line

2.17.1