Re: [PATCH bpf-next 01/12] bpf: Add btf enum64 support

[Dave Marchevsky <davemarchevsky@fb.com>]

On 5/1/22 3:00 PM, Yonghong Song wrote:   
> Currently, BTF only supports upto 32bit enum value with BTF_KIND_ENUM.
> But in kernel, some enum indeed has 64bit values, e.g.,
> in uapi bpf.h, we have
>   enum {
>         BPF_F_INDEX_MASK                = 0xffffffffULL,
>         BPF_F_CURRENT_CPU               = BPF_F_INDEX_MASK,
>         BPF_F_CTXLEN_MASK               = (0xfffffULL << 32),
>   };
> In this case, BTF_KIND_ENUM will encode the value of BPF_F_CTXLEN_MASK
> as 0, which certainly is incorrect.
> 
> This patch added a new btf kind, BTF_KIND_ENUM64, which permits
> 64bit value to cover the above use case. The BTF_KIND_ENUM64 has
> the following three bytes followed by the common type:
>   struct bpf_enum64 {
>     __u32 nume_off;
>     __u32 hi32;
>     __u32 lo32;
>   };
> Currently, btf type section has an alignment of 4 as all element types
> are u32. Representing the value with __u64 will introduce a pad
> for bpf_enum64 and may also introduce misalignment for the 64bit value.
> Hence, two members of hi32 and lo32 are chosen to avoid these issues.
> 
> The kflag is also introduced for BTF_KIND_ENUM and BTF_KIND_ENUM64
> to indicate whether the value is signed or unsigned. The kflag intends
> to provide consistent output of BTF C fortmat with the original
> source code. For example, the original BTF_KIND_ENUM bit value is 0xffffffff.
> The format C has two choices, print out 0xffffffff or -1 and current libbpf
> prints out as unsigned value. But if the signedness is preserved in btf,
> the value can be printed the same as the original source code.
> 
> The new BTF_KIND_ENUM64 is intended to support the enum value represented as
> 64bit value. But it can represent all BTF_KIND_ENUM values as well.
> The value size of BTF_KIND_ENUM64 is encoded to 8 to represent its intent.
> The compiler ([1]) and pahole will generate BTF_KIND_ENUM64 only if the value has
> to be represented with 64 bits.
> 
>   [1] https://reviews.llvm.org/D124641
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  include/linux/btf.h            |  18 ++++-
>  include/uapi/linux/btf.h       |  17 ++++-
>  kernel/bpf/btf.c               | 132 ++++++++++++++++++++++++++++++---
>  tools/include/uapi/linux/btf.h |  17 ++++-
>  4 files changed, 168 insertions(+), 16 deletions(-)

[...]

> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> index a9162a6c0284..2aac226a27b2 100644
> --- a/include/uapi/linux/btf.h
> +++ b/include/uapi/linux/btf.h
> @@ -36,10 +36,10 @@ struct btf_type {
>  	 * bits 24-28: kind (e.g. int, ptr, array...etc)
>  	 * bits 29-30: unused
>  	 * bit     31: kind_flag, currently used by
> -	 *             struct, union and fwd
> +	 *             struct, union, enum, fwd and enum64
>  	 */
>  	__u32 info;
> -	/* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC.
> +	/* "size" is used by INT, ENUM, STRUCT, UNION, DATASEC and ENUM64.
>  	 * "size" tells the size of the type it is describing.
>  	 *
>  	 * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
> @@ -63,7 +63,7 @@ enum {
>  	BTF_KIND_ARRAY		= 3,	/* Array	*/
>  	BTF_KIND_STRUCT		= 4,	/* Struct	*/
>  	BTF_KIND_UNION		= 5,	/* Union	*/
> -	BTF_KIND_ENUM		= 6,	/* Enumeration	*/
> +	BTF_KIND_ENUM		= 6,	/* Enumeration for int/unsigned int values */

nit: Maybe it would be more clear to say something like "Enumeration
representable in <= 32 bits", and something similar for ENUM64? This applies to
docs/bpf patch as well. I don't feel strongly about it.

>  	BTF_KIND_FWD		= 7,	/* Forward	*/
>  	BTF_KIND_TYPEDEF	= 8,	/* Typedef	*/
>  	BTF_KIND_VOLATILE	= 9,	/* Volatile	*/
> @@ -76,6 +76,7 @@ enum {
>  	BTF_KIND_FLOAT		= 16,	/* Floating point	*/
>  	BTF_KIND_DECL_TAG	= 17,	/* Decl Tag */
>  	BTF_KIND_TYPE_TAG	= 18,	/* Type Tag */
> +	BTF_KIND_ENUM64		= 19,	/* Enumeration for long/unsigned long values */
>  
>  	NR_BTF_KINDS,
>  	BTF_KIND_MAX		= NR_BTF_KINDS - 1,
> @@ -186,4 +187,14 @@ struct btf_decl_tag {
>         __s32   component_idx;
>  };

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 2f0b0440131c..17e24b362d3d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -307,6 +307,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
>  	[BTF_KIND_FLOAT]	= "FLOAT",
>  	[BTF_KIND_DECL_TAG]	= "DECL_TAG",
>  	[BTF_KIND_TYPE_TAG]	= "TYPE_TAG",
> +	[BTF_KIND_ENUM64]	= "ENUM64",
>  };
>  
>  const char *btf_type_str(const struct btf_type *t)
> @@ -664,6 +665,7 @@ static bool btf_type_has_size(const struct btf_type *t)
>  	case BTF_KIND_ENUM:
>  	case BTF_KIND_DATASEC:
>  	case BTF_KIND_FLOAT:
> +	case BTF_KIND_ENUM64:
>  		return true;
>  	}
>  

[...]

> @@ -1832,6 +1840,7 @@ __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>  		case BTF_KIND_UNION:
>  		case BTF_KIND_ENUM:
>  		case BTF_KIND_FLOAT:
> +		case BTF_KIND_ENUM64:
>  			size = type->size;
>  			goto resolved;

Is it possible to replace this check w/ btf_type_has_size that you also updated?
Looks like case's match, aside from BTF_KIND_DATASEC.