Message-ID: <a72a142c-a1f3-6f6b-76c9-dd67b7b50d7e@fb.com>
Date:   Fri, 18 Jan 2019 01:42:16 +0000
From:   Yonghong Song <yhs@...com>
To:     Edward Cree <ecree@...arflare.com>,
        Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next] bpf: btf: add btf documentation



On 1/17/19 7:13 AM, Edward Cree wrote:
> On 16/01/19 00:58, Yonghong Song wrote:
>> This patch added documentation for BTF (BPF Debug Format).
>> The document is placed under linux:Documentation/bpf directory.
>>
>> Signed-off-by: Yonghong Song <yhs@...com>
> I like this a lot overall, it does a good job of explaining how the
>   various pieces fit together.
> See inline for review comments.
> 
>> ---
>>   Documentation/bpf/btf.rst   | 787 ++++++++++++++++++++++++++++++++++++
>>   Documentation/bpf/index.rst |   7 +
>>   2 files changed, 794 insertions(+)
>>   create mode 100644 Documentation/bpf/btf.rst
>>
>> diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
>> new file mode 100644
>> index 000000000000..3dfa8edd22ac
>> --- /dev/null
>> +++ b/Documentation/bpf/btf.rst
>> @@ -0,0 +1,787 @@
>> +=====================
>> +BPF Type Format (BTF)
>> +=====================
>> +
>> +1. Introduction
>> +***************
>> +
>> +BTF (BPF Type Format) is the meta data format which
>> +encodes the debug info related to BPF program/map.
>> +The name BTF was used initially to describe
>> +data types. The BTF was later extended to include
>> +function info for defined subroutines, and line info
>> +for source/line information.
>> +
>> +The debug info is used for map pretty print, function
>> +signature, etc. The function signature enables better
>> +bpf program/function kernel symbol.
>> +The line info helps generate
>> +source annotated translated byte code, jited code
>> +and verifier log.
>> +
>> +The BTF specification contains two parts,
>> +  * BTF kernel API
>> +  * BTF ELF file format
>> +
>> +The kernel API is the contract between
>> +user space and kernel. The kernel verifies
>> +the BTF info before using it.
>> +The ELF file format is a user space contract
>> +between ELF file and libbpf loader.
>> +
>> +The type and string sections are part of the
>> +BTF kernel API, describing the debug info
>> +(mostly types related) referenced by the bpf program.
>> +These two sections are discussed in
>> +details in Section 2.
>> +
>> +2. BTF Type/String Encoding
>> +***************************
>> +
>> +The file ``include/uapi/linux/btf.h`` provides high
>> +level definition on how types/strings are encoded.
>> +
>> +The beginning of data blob must be::
>> +
>> +    struct btf_header {
>> +        __u16   magic;
>> +        __u8    version;
>> +        __u8    flags;
>> +        __u32   hdr_len;
>> +
>> +        /* All offsets are in bytes relative to the end of this header */
>> +        __u32   type_off;       /* offset of type section       */
>> +        __u32   type_len;       /* length of type section       */
>> +        __u32   str_off;        /* offset of string section     */
>> +        __u32   str_len;        /* length of string section     */
>> +    };
>> +
>> +The magic is ``0xeB9F``, which has different encoding for big and little
>> +endian system, and can be used to test whether BTF is generated for
>> +big or little endian target.
>> +The btf_header is designed to be extensible with hdr_len specifying
>> +the struct btf_header length when the data blob is generated.
> Should probably specify here whether hdr_len includes the whole header or
>   starts from offsetofend(hdr_len).  (I believe it's the whole thing.)

It includes the whole header. Will add more description in the next 
revision.

> 
>> +
>> +2.1 String Encoding
>> +===================
>> +
>> +The first byte of string section must be ``'\0'`` to represent a null string.
> Perhaps "empty string" is more precise than "null string"?
>> +The rest of string table is a cancatenation of other strings.
> sp: concatenation.
> Possibly also state that those other strings are nul-terminated.

Will do.

>> +
>> +2.2 Type Encoding
>> +=================
>> +
>> +The type id ``0`` is reserved for ``void`` type.
>> +The type section is parsed sequentially and the type id is assigned to
>> +each recognized type starting from id ``1``.
>> +Currently, the following types are supported::
>> +
>> +    #define BTF_KIND_INT            1       /* Integer      */
>> +    #define BTF_KIND_PTR            2       /* Pointer      */
>> +    #define BTF_KIND_ARRAY          3       /* Array        */
>> +    #define BTF_KIND_STRUCT         4       /* Struct       */
>> +    #define BTF_KIND_UNION          5       /* Union        */
>> +    #define BTF_KIND_ENUM           6       /* Enumeration  */
>> +    #define BTF_KIND_FWD            7       /* Forward      */
>> +    #define BTF_KIND_TYPEDEF        8       /* Typedef      */
>> +    #define BTF_KIND_VOLATILE       9       /* Volatile     */
>> +    #define BTF_KIND_CONST          10      /* Const        */
>> +    #define BTF_KIND_RESTRICT       11      /* Restrict     */
>> +    #define BTF_KIND_FUNC           12      /* Function     */
>> +    #define BTF_KIND_FUNC_PROTO     13      /* Function Proto       */
>> +
>> +Note that the type section encodes debug info, not just pure types.
>> +``BTF_KIND_FUNC`` is not a type, and it represents a defined subprogram.
>> +
>> +Each type contains the following common data::
>> +
>> +    struct btf_type {
>> +        __u32 name_off;
>> +        /* "info" bits arrangement
>> +         * 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-30: unused
>> +         * bit     31: kind_flag, currently used by
>> +         *             struct, union and fwd
>> +         */
>> +        __u32 info;
>> +        /* "size" is used by INT, ENUM, STRUCT and UNION.
>> +         * "size" tells the size of the type it is describing.
>> +         *
>> +         * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
>> +         * FUNC and FUNC_PROTO.
>> +         * "type" is a type_id referring to another type.
>> +         */
>> +        union {
>> +                __u32 size;
>> +                __u32 type;
>> +        };
>> +    };
>> +
>> +For certain kinds, the common data are followed by kind specific data.
>> +The ``name_off`` in ``struct btf_type`` specifies the offset in the string table.
>> +The following details encoding of each kind.
>> +

[...]

>> +
>> +2.2.3 BTF_KIND_ARRAY
>> +~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: 0
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_ARRAY
>> +  * ``info.vlen``: 0
>> +  * ``size/type``: 0, not used
>> +
>> +btf_type is followed by one "struct btf_array"::
>> +
>> +    struct btf_array {
>> +        __u32   type;
>> +        __u32   index_type;
>> +        __u32   nelems;
>> +    };
>> +
>> +The ``struct btf_array`` encoding:
>> +  * ``type``: the element type
>> +  * ``index_type``: the index type
> Is this ever anything but u32?  What is the purpose of this field's existence?

index_type can be any regular int type (u8, u16, u32, u64, u128).
The dwarf has this as well. The original idea is to keep it in a way 
similar to dwarf. But I agree that as this point, it is not really
useful beyond BTF verification. I will add some descriptions
in the document to provide some historical context.

>> +  * ``nelems``: the number of elements for this array.
>> +
>> +For a multiple dimensional array, e.g., ``a[5][6]``, the btf_array.nelems
>> +equals ``30``.
> Does this mean that there is nothing in BTF to distinguish a multi-dimensional
>   array from a single-dimensional array of the same size?
> Why is this done, rather than chaining BTF_ARRAY records?

This reflects the current pahole and llvm implementation.
The BTF_KIND_ARRAY does allow multiple dimensional arrays.

int a[5][10]

t1: int
t2: btf_array:
        type: t1
        nelems: 10
t3: btf_array:
        type: t2
        nelems: 5

The llvm and pahole implementation collapses into one dimension 
partially due to simpler implementation. But the BTF design itself
does allow expressing multiple dimensional arrays.

Will add more descriptions here as well.

>> ``nelems = 0`` is also allowed.
>> +
>> +2.2.4 BTF_KIND_STRUCT
>> +~~~~~~~~~~~~~~~~~~~~~
>> +2.2.5 BTF_KIND_UNION
>> +~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: 0 or offset to a valid C identifier
>> +  * ``info.kind_flag``: 0 or 1
>> +  * ``info.kind``: BTF_KIND_STRUCT or BTF_KIND_UNION
>> +  * ``info.vlen``: the number of struct/union members
>> +  * ``info.size``: the size of the struct/union in bytes
>> +
>> +``btf_type`` is followed by ``info.vlen`` number of ``struct btf_member``.::
>> +
>> +    struct btf_member {
>> +        __u32   name_off;
>> +        __u32   type;
>> +        __u32   offset;
>> +    };
>> +
>> +``struct btf_member`` encoding:
>> +  * ``name_off``: offset to a valid C identifier
>> +  * ``type``: the member type
>> +  * ``offset``: <see below>
>> +
>> +If the type info ``kind_flag`` is not set, the offset contains
>> +only bit offset of the member. Note that the base type of the
>> +bitfield can only be int or enum type. If the bitfield size
>> +is 32, the base type can be either int or enum type.
>> +If the bitfield size is not 32, the base type must be int,
>> +and int type ``BTF_INT_BITS()`` encodes the bitfield size.
>> +
>> +If the ``kind_flag`` is set, the ``btf_member.offset``
>> +contains both member bitfield size and bit offset. The
>> +bitfield size and bit offset are calculated as below.::
>> +
>> +  #define BTF_MEMBER_BITFIELD_SIZE(val)   ((val) >> 24)
>> +  #define BTF_MEMBER_BIT_OFFSET(val)      ((val) & 0xffffff)
>> +
>> +In this case, if the base type is an int type, it must
>> +be a regular int type:
>> +
>> +  * ``BTF_INT_OFFSET()`` must be 0.
>> +  * ``BTF_INT_BITS()`` must be equal to ``{1,2,4,8,16} * 8``.
> Probably worth referencing here the patch that added kind_flag, as that
>   explains why these two different modes exist.

Make sense, will add.

>> +
>> +2.2.6 BTF_KIND_ENUM
>> +~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: 0 or offset to a valid C identifier
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_ENUM
>> +  * ``info.vlen``: number of enum values
>> +  * ``size``: 4
>> +
>> +``btf_type`` is followed by ``info.vlen`` number of ``struct btf_enum``.::
>> +
>> +    struct btf_enum {
>> +        __u32   name_off;
>> +        __s32   val;
>> +    };
>> +
>> +The ``btf_enum`` encoding:
>> +  * ``name_off``: offset to a valid C identifier
>> +  * ``val``: any value
>> +
>> +2.2.7 BTF_KIND_FWD
>> +~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: offset to a valid C identifier
>> +  * ``info.kind_flag``: 0 for struct, 1 for union
>> +  * ``info.kind``: BTF_KIND_FWD
>> +  * ``info.vlen``: 0
>> +  * ``type``: 0
>> +
>> +No additional type data follow ``btf_type``.
>> +
>> +2.2.8 BTF_KIND_TYPEDEF
>> +~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: offset to a valid C identifier
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_TYPEDEF
>> +  * ``info.vlen``: 0
>> +  * ``type``: the type to be redefined
> This is unclear phrasing.  How about:
>   * ``type``: the type to be given a name
> Because a typedef doesn't 'redefine' ``type``, it defines the _name_ as
>   referring to ``type``.
> (I realise my phrasing isn't the best either, but I can't figure out how
>   to further improve it.)

Thanks for pointing this out, letting me think about this as well.

>> +
>> +No additional type data follow ``btf_type``.
>> +
>> +2.2.9 BTF_KIND_VOLATILE
>> +~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: 0
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_VOLATILE
>> +  * ``info.vlen``: 0
>> +  * ``type``: the type having volatile modifier
> This is again a little bit blurry.  ``type`` doesn't "have volatile
>   modifier"; it is rather the type _to which_ a volatile modifier is
>   applied to create the type defined by this record.
> Maybe something like "the type to be volatile-qualified"?
> (Note that the C standard refers to const, volatile and restrict as
>   'qualifiers', not 'modifiers', and we should probably follow that
>   terminology.)

Right, qualifiers better tan modifiers. Modifiers are kernel internal
descriptions for them. We should not use them bluntly in documentation.

> 
>> +
>> +No additional type data follow ``btf_type``.
>> +
>> +2.2.10 BTF_KIND_CONST
>> +~~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: 0
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_CONST
>> +  * ``info.vlen``: 0
>> +  * ``type``: the type having const modifier
>> +
>> +No additional type data follow ``btf_type``.
>> +
>> +2.2.11 BTF_KIND_RESTRICT
>> +~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: 0
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_RESTRICT
>> +  * ``info.vlen``: 0
>> +  * ``type``: the type having restrict modifier
>> +
>> +No additional type data follow ``btf_type``.
>> +
>> +2.2.12 BTF_KIND_FUNC
>> +~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: offset to a valid C identifier
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_FUNC
>> +  * ``info.vlen``: 0
>> +  * ``type``: a BTF_KIND_FUNC_PROTO type
> You should put an explanation here of the semantics of this.  Above it was
>   mentioned that BTF_KIND_FUNC does not declare a type, but that should be
>   expanded on here to more fully explain the relationship between
>   BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO.  Perhaps something like:
> A BTF_KIND_FUNC defines, not a type, but a subprogram (function) whose
>   signature is defined by ``type``; the subprogram is thus an instance of
>   that type.  The BTF_KIND_FUNC may in turn be referenced by a func_info in
>   the `.BTF.ext section`__ (ELF) or in the arguments to BPF_PROG_LOAD__ (ABI).
> .. __: `4.2 .BTF.ext section`_
> .. __: `3.3 BPF_PROG_LOAD`_

Thanks for suggestion. Will do.

>> +
>> +No additional type data follow ``btf_type``.
>> +
>> +2.2.13 BTF_KIND_FUNC_PROTO
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> +  * ``name_off``: 0
>> +  * ``info.kind_flag``: 0
>> +  * ``info.kind``: BTF_KIND_FUNC_PROTO
>> +  * ``info.vlen``: # of parameters
>> +  * ``type``: the return type
>> +
>> +``btf_type`` is followed by ``info.vlen`` number of ``struct btf_param``.::
>> +
>> +    struct btf_param {
>> +        __u32   name_off;
>> +        __u32   type;
>> +    };
>> +
>> +If a BTF_KIND_FUNC_PROTO type is referred by a BTF_KIND_FUNC type,
>> +then ``btf_param.name_off`` must point to a valid C identifier
>> +except for the possible last argument representing the variable
>> +argument. The btf_param.type refers to parameter type.
>> +
>> +If the function has the variable arguments, the last parameter
> s/has the/has/

Will change.

>> +is encoded with ``name_off = 0`` and ``type = 0``.
>> +
>> +3. BTF Kernel API
>> +*****************
>> +
>> +The following bpf syscall command involves BTF:
>> +   * BPF_BTF_LOAD: load a blob of BTF data into kernel
>> +   * BPF_MAP_CREATE: map creation with btf key and value type info.
>> +   * BPF_PROG_LOAD: prog load with btf function and line info.
>> +   * BPF_BTF_GET_FD_BY_ID: get a btf fd
>> +   * BPF_OBJ_GET_INFO_BY_FD: btf, func_info, line_info
>> +     and other btf related info are returned.
>> +
>> +The workflow typically looks like:
>> +::
>> +
>> +  Application:
>> +      BPF_BTF_LOAD
>> +          |
>> +          v
>> +      BPF_MAP_CREATE & BPF_PROG_LOAD
>> +          |
>> +          V
>> +      ......
>> +
>> +  Introspection tool:
>> +      ......
>> +          |
>> +          V
>> +      BPF_OBJ_GET_INFO_BY_FD (get bpf_prog_info/bpf_map_info with btf_id)
>> +          |
>> +          V
>> +      BPF_BTF_GET_FD_BY_ID (get btf_fd)
>> +          |
>> +          V
>> +      BPF_OBJ_GET_INFO_BY_FD (get btf)
>> +          |
>> +          V
>> +      pretty print types, dump func signatures and line info, etc.
>> +
>> +
>> +3.1 BPF_BTF_LOAD
>> +================
>> +
>> +Load a blob of BTF data into kernel. A blob of data
>> +described in Section 2 can be directly loaded into the kernel.
>> +A ``btf_fd`` returns to userspace.
>> +
>> +3.2 BPF_MAP_CREATE
>> +==================
>> +
>> +A map can be created with ``btf_fd`` and specified key/value type id.::
>> +
>> +    __u32   btf_fd;         /* fd pointing to a BTF type data */
>> +    __u32   btf_key_type_id;        /* BTF type_id of the key */
>> +    __u32   btf_value_type_id;      /* BTF type_id of the value */
>> +
>> +In libbtf, if the map is specified like below in the bpf program:
> Should this say libbpf?

Right, it is libbpf.

>> +::
>> +
>> +    struct bpf_map_def SEC("maps") btf_map = {
>> +        .type = BPF_MAP_TYPE_ARRAY,
>> +        .key_size = sizeof(int),
>> +        .value_size = sizeof(struct ipv_counts),
>> +        .max_entries = 4,
>> +    };
>> +    BPF_ANNOTATE_KV_PAIR(btf_map, int, struct ipv_counts);
>> +
>> +Here, the parameters for macro BPF_ANNOTATE_KV_PAIR are map name,
>> +key and value types for the map.
>> +During ELF parsing, libbpf is able to extract key/value type_id's
>> +and assigned them to BPF_MAP_CREATE attributes automatically.
>> +
>> +3.3 BPF_PROG_LOAD
>> +=================

[...]

>> +
>> +3.4 BPF_BTF_GET_FD_BY_ID
>> +========================
>> +
>> +  Given a btf id, a btf fd is returned.
>> +
>> +3.5 BPF_OBJ_GET_INFO_BY_FD
>> +==========================
>> +
>> +Users can get btf blob, bpf_map_info and bpf_prog_info.
>> +bpf_map_info returns btf_id, key/value type id.
> What exactly is btf_id in this case?  The type_id of the
>   BPF_ANNOTATE_KV_PAIR struct?

btf_id is a ID inside the kernel for one loaded BTF, the id won't change
until it is unloaded. different processes referencing it needs to get
a fd in their respective process space. I will provide more
explanation here in the doc.

>> +bpf_prog_info returns btf_id, func_info and line info
>> +for translated bpf byte codes, and jited_line_info.
> In this case presumably btf_id is the type_id of the BTF_KIND_FUNC;
>   perhaps that should be stated explicitly too.
>> +
>> +4. ELF File Format Interface
>> +****************************
>> +
>> +4.1 .BTF section
>> +================
>> +
> Really you should state what this section is _supposed_ to contain
>   before starting to talk about what existing implementations generate.

Sure. will do.

>> +pahole currently generates .BTF section with the same format
>> +as described in Section 2. pahole doesn't generate
>> +BTF_KIND_FUNC yet.
>> +
>> +llvm generates two sections .BTF and .BTF.ext.
>> +The .BTF section has the same specification as in Section 2.
>> +The .BTF.ext section encodes func_info and line_info which
>> +needs loader manipulation before loading into the kernel.
>> +
>> +4.2 .BTF.ext section
>> +====================
>> +
>> +The specification for .BTF.ext section is defined at
>> +``tools/lib/bpf/btf.h`` and ``tools/lib/bpf/btf.c``.
>> +
>> +The current header of .BTF.ext section::
>> +
>> +    struct btf_ext_header {
>> +        __u16   magic;
>> +        __u8    version;
>> +        __u8    flags;
>> +        __u32   hdr_len;
>> +
>> +        /* All offsets are in bytes relative to the end of this header */
>> +        __u32   func_info_off;
>> +        __u32   func_info_len;
>> +        __u32   line_info_off;
>> +        __u32   line_info_len;
>> +    };
>> +
>> +It is very similar to .BTF section. Instead of type/string section,
>> +it contains func_info and line_info section.
> Perhaps there should be a link back to §3.3 here, as that has the definitions
>   of structs bpf_func_info and bpf_line_info.

Good suggestions.

Thanks for the review! Will address all of them and resubmit the patch.

> 
> -Ed

Powered by blists - more mailing lists