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

[Yonghong Song <yhs@fb.com>]

On 5/10/22 4:18 PM, Andrii Nakryiko wrote:
> On Tue, May 10, 2022 at 3:06 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 5/9/22 3:29 PM, Andrii Nakryiko wrote:
>>> On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> 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:
>>>
>>> you probably meant three fields, not bytes
>>
>> correct.
>>
>>>
>>>>     struct bpf_enum64 {
>>>>       __u32 nume_off;
>>>>       __u32 hi32;
>>>>       __u32 lo32;
>>>
>>> I'd like to nitpick on name here, as hi/lo of what? Maybe val_hi32 and
>>> val_lo32? Can we also reverse the order here? For x86 you'll be able
>>> to use &lo32 to get value directly if you really want, without a local
>>> copy. It also just logically seems better to have something low first,
>>> then high next.
>>
>> I can go with val_hi32, val_lo32 and put val_lo32 before val_hi32.
>> I don't have any preference for the ordering of these two fields.
>>
>>>
>>>
>>>>     };
>>>> 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>
[...]
>>>
>>>>           btf_show_end_type(show);
>>>>    }
>>>>
>>>> @@ -3770,6 +3779,109 @@ static struct btf_kind_operations enum_ops = {
>>>>           .show = btf_enum_show,
>>>>    };
>>>>
>>>> +static s32 btf_enum64_check_meta(struct btf_verifier_env *env,
>>>> +                                const struct btf_type *t,
>>>> +                                u32 meta_left)
>>>> +{
>>>> +       const struct btf_enum64 *enums = btf_type_enum64(t);
>>>> +       struct btf *btf = env->btf;
>>>> +       const char *fmt_str;
>>>> +       u16 i, nr_enums;
>>>> +       u32 meta_needed;
>>>> +
>>>> +       nr_enums = btf_type_vlen(t);
>>>> +       meta_needed = nr_enums * sizeof(*enums);
>>>> +
>>>> +       if (meta_left < meta_needed) {
>>>> +               btf_verifier_log_basic(env, t,
>>>> +                                      "meta_left:%u meta_needed:%u",
>>>> +                                      meta_left, meta_needed);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       if (t->size != 8) {
>>>
>>> technically there is nothing wrong with using enum64 for smaller
>>> sizes, right? Any particular reason to prevent this? We can just
>>> define that 64-bit value is sign-extended if enum is signed and has
>>> size < 8?
>>
>> My original idea is to support 64-bit enum only for ENUM64 kind.
>> But it is certainly possible to encode 32-bit enums as well for
>> ENUM64. So I will remove this restriction.
>>
>> The dwarf only generates sizes 4 (for up-to 32 bit values)
>> and 8 (for 64 bit values). But BTF_KIND_ENUM supports 1/2/4/8
>> sizes, so BTF_KIND_ENUM64 will also support 1/2/4/8 sizes.
> 
> Little known fact, but it's not true:
> 
> $ bpftool btf dump file /sys/kernel/btf/vmlinux| rg 'ENUM.*size=1' -A8
> [83476] ENUM 'hub_led_mode' size=1 vlen=8
>          'INDICATOR_AUTO' val=0
>          'INDICATOR_CYCLE' val=1
>          'INDICATOR_GREEN_BLINK' val=2
>          'INDICATOR_GREEN_BLINK_OFF' val=3
>          'INDICATOR_AMBER_BLINK' val=4
>          'INDICATOR_AMBER_BLINK_OFF' val=5
>          'INDICATOR_ALT_BLINK' val=6
>          'INDICATOR_ALT_BLINK_OFF' val=7
> 
> Defined as packed enum:
> 
> enum hub_led_mode {
>          INDICATOR_AUTO = 0,
>          INDICATOR_CYCLE,
>          /* software blinks for attention:  software, hardware, reserved */
>          INDICATOR_GREEN_BLINK, INDICATOR_GREEN_BLINK_OFF,
>          INDICATOR_AMBER_BLINK, INDICATOR_AMBER_BLINK_OFF,
>          INDICATOR_ALT_BLINK, INDICATOR_ALT_BLINK_OFF
> } __attribute__ ((packed));

I am not aware of this.... Good to know.

> 
> 
>>
>>>
>>>> +               btf_verifier_log_type(env, t, "Unexpected size");
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       /* enum type either no name or a valid one */
>>>> +       if (t->name_off &&
>>>> +           !btf_name_valid_identifier(env->btf, t->name_off)) {
>>>> +               btf_verifier_log_type(env, t, "Invalid name");
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>
>>> [...]