On 5/10/22 4:18 PM, Andrii Nakryiko wrote:
On Tue, May 10, 2022 at 3:06 PM Yonghong Song <yhs@xxxxxx> wrote:
On 5/9/22 3:29 PM, Andrii Nakryiko wrote:
On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
[...]
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;
+ }
+
[...]
