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

[Yonghong Song <yhs@fb.com>]

On 5/10/22 4:38 PM, Andrii Nakryiko wrote:
> On Tue, May 10, 2022 at 3:40 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 5/9/22 4:25 PM, Andrii Nakryiko wrote:
>>> On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>> Add BTF_KIND_ENUM64 support. Deprecated btf__add_enum() and
>>>> btf__add_enum_value() and introduced the following new APIs
>>>>     btf__add_enum32()
>>>>     btf__add_enum32_value()
>>>>     btf__add_enum64()
>>>>     btf__add_enum64_value()
>>>> due to new kind and introduction of kflag.
>>>>
>>>> To support old kernel with enum64, the sanitization is
>>>> added to replace BTF_KIND_ENUM64 with a bunch of
>>>> pointer-to-void types.
>>>>
>>>> The enum64 value relocation is also supported. The enum64
>>>> forward resolution, with enum type as forward declaration
>>>> and enum64 as the actual definition, is also supported.
>>>>
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>> ---
>>>>    tools/lib/bpf/btf.c                           | 226 +++++++++++++++++-
>>>>    tools/lib/bpf/btf.h                           |  21 ++
>>>>    tools/lib/bpf/btf_dump.c                      |  94 ++++++--
>>>>    tools/lib/bpf/libbpf.c                        |  64 ++++-
>>>>    tools/lib/bpf/libbpf.map                      |   4 +
>>>>    tools/lib/bpf/libbpf_internal.h               |   2 +
>>>>    tools/lib/bpf/linker.c                        |   2 +
>>>>    tools/lib/bpf/relo_core.c                     |  93 ++++---
>>>>    .../selftests/bpf/prog_tests/btf_dump.c       |  10 +-
>>>>    .../selftests/bpf/prog_tests/btf_write.c      |   6 +-
>>>>    10 files changed, 450 insertions(+), 72 deletions(-)
>>>>
>>>
> 
> [...]
> 
>>>
>>>
>>>> +       t->size = tsize;
>>>> +
>>>> +       return btf_commit_type(btf, sz);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Append new BTF_KIND_ENUM type with:
>>>> + *   - *name* - name of the enum, can be NULL or empty for anonymous enums;
>>>> + *   - *is_unsigned* - whether the enum values are unsigned or not;
>>>> + *
>>>> + * Enum initially has no enum values in it (and corresponds to enum forward
>>>> + * declaration). Enumerator values can be added by btf__add_enum64_value()
>>>> + * immediately after btf__add_enum() succeeds.
>>>> + *
>>>> + * Returns:
>>>> + *   - >0, type ID of newly added BTF type;
>>>> + *   - <0, on error.
>>>> + */
>>>> +int btf__add_enum32(struct btf *btf, const char *name, bool is_unsigned)
>>>
>>> given it's still BTF_KIND_ENUM in UAPI, let's keep 32-bit ones as just
>>> btf__add_enum()/btf__add_enum_value() and not deprecate anything.
>>> ENUM64 can be thought about as more of a special case, so I think it's
>>> ok.
>>
>> The current btf__add_enum api:
>> LIBBPF_API int btf__add_enum(struct btf *btf, const char *name, __u32
>> bytes_sz);
>>
>> The issue is it doesn't have signedness parameter. if the user input
>> is
>>      enum { A = -1, B = 0, C = 1 };
>> the actual printout btf format will be
>>      enum { A 4294967295, B = 0, C = 1}
>> does not match the original source.
> 
> Oh, I didn't realize that's the reason. I still like btf__add_enum()
> name much better, can you please do the same macro trick that I did
> for bpf_prog_load() based on the number of arguments? We'll be able to
> preserve good API name and add extra argument. Once this lands we'll
> need to update pahole to added signedness bit, but otherwise I don't
> think there are many other users of these APIs currently (I might be
> wrong, but macro magic gives us backwards compat anyway).
> 
>>
>>>
>>>> +{
>>>> +       return btf_add_enum_common(btf, name, is_unsigned, BTF_KIND_ENUM, 4);
>>>> +}
>>>> +
>>>
>>> [...]
>>>
>>>>    /*
> 
> [...]
> 
>>>> @@ -764,8 +792,13 @@ static int bpf_core_calc_enumval_relo(const struct bpf_core_relo *relo,
>>>>                   if (!spec)
>>>>                           return -EUCLEAN; /* request instruction poisoning */
>>>>                   t = btf_type_by_id(spec->btf, spec->spec[0].type_id);
>>>> -               e = btf_enum(t) + spec->spec[0].idx;
>>>> -               *val = e->val;
>>>> +               if (btf_is_enum(t)) {
>>>> +                       e = btf_enum(t) + spec->spec[0].idx;
>>>> +                       *val = e->val;
>>>> +               } else {
>>>> +                       e64 = btf_enum64(t) + spec->spec[0].idx;
>>>> +                       *val = btf_enum64_value(e64);
>>>> +               }
>>>
>>> I think with sign bit we now have further complication: for 32-bit
>>> enums we need to sign extend 32-bit values to s64 and then cast as
>>> u64, no? Seems like a helper to abstract that is good to have here.
>>> Otherwise relocating enum ABC { D = -1 } will produce invalid ldimm64
>>> instruction, right?
>>
>> We should be fine here. For enum32, we have
>> struct btf_enum {
>>           __u32   name_off;
>>           __s32   val;
>> };
>> So above *val = e->val will first sign extend from __s32 to __s64
>> and then the __u64. Let me have a helper with additional comments
>> to make it clear.
>>
> 
> Ok, great! Let's just shorten this as I suggested below?

The
 >>> *val = btf_is_enum(t)
 >>>       ? btf_enum(t)[spec->spec[0].idx]
 >>>       : btf_enum64(t)[spec->spec[0].idx];
won't work, but the following should work:
    *val = btf_is_enum(t)
	? btf_enum(t)[spec->spec[0].idx].val
	: btf_enum64_value(btf_enum64(t) + spec->spec[0].idx);
> 
>>>
>>> Also keep in mind that you can use btf_enum()/btf_enum64() as an
>>> array, so above you can write just as
>>>
>>> *val = btf_is_enum(t)
>>>       ? btf_enum(t)[spec->spec[0].idx]
>>>       : btf_enum64(t)[spec->spec[0].idx];
>>>
>>> But we need sign check and extension, so better to have a separate helper.
>>>
>>>>                   break;
>>>>           default:
>>>>                   return -EOPNOTSUPP;
>>>> @@ -1034,7 +1067,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
>>>>                   }
>>>>
>>>>                   insn[0].imm = new_val;
>>>> -               insn[1].imm = 0; /* currently only 32-bit values are supported */
>>>> +               insn[1].imm = new_val >> 32;
>>>
>>> for 32-bit instructions (ALU/ALU32, etc) we need to make sure that
>>> new_val fits in 32 bits. And we need to be careful about
>>> signed/unsigned, because for signed case all-zero or all-one upper 32
>>> bits are ok (sign extension). Can we know the expected signed/unsigned
>>> operation from bpf_insn itself? We should be, right?
>>
>> The core relocation insn for constant is
>>     move r1, <32bit value>
>> or
>>     ldimm_64 r1, <64bit value>
>> and there are no signedness information.
>> So the 64bit value (except sign extension) can only from
>> ldimm_64. We should be okay here, but I can double check.
> 
> not sure how full 64-bit -1 should be loaded into register then. Does
> compiler generate extra sign-extending bit shifts or embedded constant
> is considered to be a signed constant always?

For ldimm64 r1, -1,
the first insn imm will be 0xffffffff, and the second insn will also be 
0xffffffff. The final value will be
   ((u64)(u32)0xffffffff << 32) | (u32)0xffffffff


> 
>>
>>>
>>>>                   pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n",
>>>>                            prog_name, relo_idx, insn_idx,
>>>>                            (unsigned long long)imm, new_val);
>>>> @@ -1056,6 +1089,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
>>>>     */
> 
> [...]