| Message ID | 20210216011216.3168-3-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add BTF_KIND_FLOAT support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: netdev@vger.kernel.org john.fastabend@gmail.com kpsingh@kernel.org songliubraving@fb.com kafai@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Ilya Leoshkevich wrote: > The logic follows that of BTF_KIND_INT most of the time. Sanitization > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Does this match the code though? > kernels. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- [...] > @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) > } else if (!has_func_global && btf_is_func(t)) { > /* replace BTF_FUNC_GLOBAL with BTF_FUNC_STATIC */ > t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, 0); > + } else if (!has_float && btf_is_float(t)) { > + /* replace FLOAT with INT */ > + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0); Do we also need to encode the vlen here? #define BTF_INFO_ENC(kind, kind_flag, vlen) \ ((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN)) > } > } > } > @@ -3882,6 +3890,18 @@ static int probe_kern_btf_datasec(void) > strs, sizeof(strs))); > }
John Fastabend wrote: > Ilya Leoshkevich wrote: > > The logic follows that of BTF_KIND_INT most of the time. Sanitization > > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Does this match the code though? > > > kernels. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > [...] > > > > @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) > > } else if (!has_func_global && btf_is_func(t)) { > > /* replace BTF_FUNC_GLOBAL with BTF_FUNC_STATIC */ > > t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, 0); > > + } else if (!has_float && btf_is_float(t)) { > > + /* replace FLOAT with INT */ > > + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0); > > Do we also need to encode the vlen here? Sorry typo on my side, 't->size = ?' is what I was trying to point out. Looks like its set in the other case where we replace VAR with INT. > > #define BTF_INFO_ENC(kind, kind_flag, vlen) \ > ((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN)) > > > } > > } > > } > > @@ -3882,6 +3890,18 @@ static int probe_kern_btf_datasec(void) > > strs, sizeof(strs))); > > }
On Wed, 2021-02-17 at 13:12 -0800, John Fastabend wrote: > John Fastabend wrote: > > Ilya Leoshkevich wrote: > > > The logic follows that of BTF_KIND_INT most of the time. > > > Sanitization > > > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on > > > older > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Does this match the code though? > > > > > kernels. > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > --- > > > > [...] > > > > > > > @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct > > > bpf_object *obj, struct btf *btf) > > > } else if (!has_func_global && btf_is_func(t)) { > > > /* replace BTF_FUNC_GLOBAL with > > > BTF_FUNC_STATIC */ > > > t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, > > > 0); > > > + } else if (!has_float && btf_is_float(t)) { > > > + /* replace FLOAT with INT */ > > > + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, > > > 0); > > > > Do we also need to encode the vlen here? > > Sorry typo on my side, 't->size = ?' is what I was trying to point > out. > Looks like its set in the other case where we replace VAR with INT. The idea is to have the size of the INT equal to the size of the FLOAT that it replaces. I guess we can't do the same for VARs, because they don't have the size field, and if we don't have DATASECs, then we can't find the size of a VAR at all.
Ilya Leoshkevich wrote: > On Wed, 2021-02-17 at 13:12 -0800, John Fastabend wrote: > > John Fastabend wrote: > > > Ilya Leoshkevich wrote: > > > > The logic follows that of BTF_KIND_INT most of the time. > > > > Sanitization > > > > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on > > > > older > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > Does this match the code though? > > > > > > > kernels. > > > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > --- > > > > > > [...] > > > > > > > > > > @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct > > > > bpf_object *obj, struct btf *btf) > > > > } else if (!has_func_global && btf_is_func(t)) { > > > > /* replace BTF_FUNC_GLOBAL with > > > > BTF_FUNC_STATIC */ > > > > t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, > > > > 0); > > > > + } else if (!has_float && btf_is_float(t)) { > > > > + /* replace FLOAT with INT */ > > > > + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, > > > > 0); > > > > > > Do we also need to encode the vlen here? > > > > Sorry typo on my side, 't->size = ?' is what I was trying to point > > out. > > Looks like its set in the other case where we replace VAR with INT. > > The idea is to have the size of the INT equal to the size of the FLOAT > that it replaces. I guess we can't do the same for VARs, because they > don't have the size field, and if we don't have DATASECs, then we can't > find the size of a VAR at all. > Right, but KINT_INT has some extra constraints that don't appear to be in place for KIND_FLOAT. For example meta_check includes max size check. We should check these when libbpf does conversion as well? Otherwise kernel is going to give us an error that will be a bit hard to understand. Also what I am I missing here. I use the writers to build a float, btf__add_float(btf, "new_float", 8); This will create the btf_type struct approximately like this, btf_type t { .name = name_off; // points at my name .info = btf_type_info(BTF_KIND_FLOAT, 0, 0); .size = 8 }; But if I create an int_type with btf__add_int(btf, "net_int", 8); I will get a btf_type + __u32. When we do the conversion how do we skip the extra u32 setup? *(__u32 *)(t + 1) = (encoding << 24) | (byte_sz * 8); Should we set this up on the conversion as well? Otherwise later steps might try to read the __u32 piece to find some arbitrary memory? .John
On 2/15/21 5:12 PM, Ilya Leoshkevich wrote: > The logic follows that of BTF_KIND_INT most of the time. Sanitization > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older > kernels. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > tools/lib/bpf/btf.c | 44 +++++++++++++++++++++++++++++++++ > tools/lib/bpf/btf.h | 8 ++++++ > tools/lib/bpf/btf_dump.c | 4 +++ > tools/lib/bpf/libbpf.c | 29 +++++++++++++++++++++- > tools/lib/bpf/libbpf.map | 5 ++++ > tools/lib/bpf/libbpf_internal.h | 2 ++ > 6 files changed, 91 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index d9c10830d749..07a30e98c3de 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -291,6 +291,7 @@ static int btf_type_size(const struct btf_type *t) > case BTF_KIND_PTR: > case BTF_KIND_TYPEDEF: > case BTF_KIND_FUNC: > + case BTF_KIND_FLOAT: > return base_size; > case BTF_KIND_INT: > return base_size + sizeof(__u32); > @@ -338,6 +339,7 @@ static int btf_bswap_type_rest(struct btf_type *t) > case BTF_KIND_PTR: > case BTF_KIND_TYPEDEF: > case BTF_KIND_FUNC: > + case BTF_KIND_FLOAT: > return 0; > case BTF_KIND_INT: > *(__u32 *)(t + 1) = bswap_32(*(__u32 *)(t + 1)); > @@ -578,6 +580,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id) > case BTF_KIND_UNION: > case BTF_KIND_ENUM: > case BTF_KIND_DATASEC: > + case BTF_KIND_FLOAT: > size = t->size; > goto done; > case BTF_KIND_PTR: > @@ -621,6 +624,7 @@ int btf__align_of(const struct btf *btf, __u32 id) > switch (kind) { > case BTF_KIND_INT: > case BTF_KIND_ENUM: > + case BTF_KIND_FLOAT: > return min(btf_ptr_sz(btf), (size_t)t->size); > case BTF_KIND_PTR: > return btf_ptr_sz(btf); > @@ -2373,6 +2377,42 @@ int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz) > return btf_commit_type(btf, sz); > } > > +/* > + * Append new BTF_KIND_FLOAT type with: > + * - *name* - non-empty, non-NULL type name; > + * - *sz* - size of the type, in bytes; > + * Returns: > + * - >0, type ID of newly added BTF type; > + * - <0, on error. > + */ > +int btf__add_float(struct btf *btf, const char *name, size_t byte_sz) > +{ > + struct btf_type *t; > + int sz, name_off; > + > + /* non-empty name */ > + if (!name || !name[0]) > + return -EINVAL; Do we want to ensure byte_sz to be 2/4/8/16? Currently, the int type supports 1/2/4/8/16. In LLVM, the following are supported float types: case BuiltinType::Half: case BuiltinType::Float: case BuiltinType::LongDouble: case BuiltinType::Float16: case BuiltinType::BFloat16: case BuiltinType::Float128: case BuiltinType::Double: > + > + if (btf_ensure_modifiable(btf)) > + return -ENOMEM; > + > + sz = sizeof(struct btf_type); > + t = btf_add_type_mem(btf, sz); > + if (!t) > + return -ENOMEM; > + > + name_off = btf__add_str(btf, name); > + if (name_off < 0) > + return name_off; > + > + t->name_off = name_off; > + t->info = btf_type_info(BTF_KIND_FLOAT, 0, 0); > + t->size = byte_sz; > + > + return btf_commit_type(btf, sz); > +} > + [...]
On 2/15/21 5:12 PM, Ilya Leoshkevich wrote: > The logic follows that of BTF_KIND_INT most of the time. Sanitization > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older > kernels. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > tools/lib/bpf/btf.c | 44 +++++++++++++++++++++++++++++++++ > tools/lib/bpf/btf.h | 8 ++++++ > tools/lib/bpf/btf_dump.c | 4 +++ > tools/lib/bpf/libbpf.c | 29 +++++++++++++++++++++- > tools/lib/bpf/libbpf.map | 5 ++++ > tools/lib/bpf/libbpf_internal.h | 2 ++ > 6 files changed, 91 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index d9c10830d749..07a30e98c3de 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -291,6 +291,7 @@ static int btf_type_size(const struct btf_type *t) > case BTF_KIND_PTR: > case BTF_KIND_TYPEDEF: > case BTF_KIND_FUNC: > + case BTF_KIND_FLOAT: > return base_size; > case BTF_KIND_INT: > return base_size + sizeof(__u32); [...] > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index d43cc3f29dae..703988d68e73 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -178,6 +178,8 @@ enum kern_feature_id { > FEAT_PROG_BIND_MAP, > /* Kernel support for module BTFs */ > FEAT_MODULE_BTF, > + /* BTF_KIND_FLOAT support */ > + FEAT_BTF_FLOAT, > __FEAT_CNT, > }; > > @@ -1935,6 +1937,7 @@ static const char *btf_kind_str(const struct btf_type *t) > case BTF_KIND_FUNC_PROTO: return "func_proto"; > case BTF_KIND_VAR: return "var"; > case BTF_KIND_DATASEC: return "datasec"; > + case BTF_KIND_FLOAT: return "float"; > default: return "unknown"; > } > } > @@ -2384,15 +2387,17 @@ static bool btf_needs_sanitization(struct bpf_object *obj) > { > bool has_func_global = kernel_supports(FEAT_BTF_GLOBAL_FUNC); > bool has_datasec = kernel_supports(FEAT_BTF_DATASEC); > + bool has_float = kernel_supports(FEAT_BTF_FLOAT); > bool has_func = kernel_supports(FEAT_BTF_FUNC); > > - return !has_func || !has_datasec || !has_func_global; > + return !has_func || !has_datasec || !has_func_global || !has_float; > } > > static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) > { > bool has_func_global = kernel_supports(FEAT_BTF_GLOBAL_FUNC); > bool has_datasec = kernel_supports(FEAT_BTF_DATASEC); > + bool has_float = kernel_supports(FEAT_BTF_FLOAT); > bool has_func = kernel_supports(FEAT_BTF_FUNC); > struct btf_type *t; > int i, j, vlen; > @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) > } else if (!has_func_global && btf_is_func(t)) { > /* replace BTF_FUNC_GLOBAL with BTF_FUNC_STATIC */ > t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, 0); > + } else if (!has_float && btf_is_float(t)) { > + /* replace FLOAT with INT */ > + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0); You can replace float with a "pointer to void" type. > } > } > } > @@ -3882,6 +3890,18 @@ static int probe_kern_btf_datasec(void) > strs, sizeof(strs))); > } > > +static int probe_kern_btf_float(void) > +{ > + static const char strs[] = "\0float"; > + __u32 types[] = { > + /* float */ > + BTF_TYPE_FLOAT_ENC(1, 4), > + }; > + > + return probe_fd(libbpf__load_raw_btf((char *)types, sizeof(types), > + strs, sizeof(strs))); > +} > + > static int probe_kern_array_mmap(void) > { > struct bpf_create_map_attr attr = { [...]
On Wed, 2021-02-17 at 23:16 -0800, Yonghong Song wrote: > On 2/15/21 5:12 PM, Ilya Leoshkevich wrote: > > The logic follows that of BTF_KIND_INT most of the time. > > Sanitization > > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older > > kernels. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > tools/lib/bpf/btf.c | 44 > > +++++++++++++++++++++++++++++++++ > > tools/lib/bpf/btf.h | 8 ++++++ > > tools/lib/bpf/btf_dump.c | 4 +++ > > tools/lib/bpf/libbpf.c | 29 +++++++++++++++++++++- > > tools/lib/bpf/libbpf.map | 5 ++++ > > tools/lib/bpf/libbpf_internal.h | 2 ++ > > 6 files changed, 91 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > index d9c10830d749..07a30e98c3de 100644 > > --- a/tools/lib/bpf/btf.c > > +++ b/tools/lib/bpf/btf.c [...] > > @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct > > bpf_object *obj, struct btf *btf) > > } else if (!has_func_global && btf_is_func(t)) { > > /* replace BTF_FUNC_GLOBAL with > > BTF_FUNC_STATIC */ > > t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, > > 0); > > + } else if (!has_float && btf_is_float(t)) { > > + /* replace FLOAT with INT */ > > + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, > > 0); > > You can replace float with a "pointer to void" type. Wouldn't this cause problems with 32-bit floats on 64-bit machines? [...]
On Wed, 2021-02-17 at 22:58 -0800, Yonghong Song wrote: > On 2/15/21 5:12 PM, Ilya Leoshkevich wrote: > > The logic follows that of BTF_KIND_INT most of the time. > > Sanitization > > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older > > kernels. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > tools/lib/bpf/btf.c | 44 > > +++++++++++++++++++++++++++++++++ > > tools/lib/bpf/btf.h | 8 ++++++ > > tools/lib/bpf/btf_dump.c | 4 +++ > > tools/lib/bpf/libbpf.c | 29 +++++++++++++++++++++- > > tools/lib/bpf/libbpf.map | 5 ++++ > > tools/lib/bpf/libbpf_internal.h | 2 ++ > > 6 files changed, 91 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > index d9c10830d749..07a30e98c3de 100644 > > --- a/tools/lib/bpf/btf.c > > +++ b/tools/lib/bpf/btf.c [...] > > @@ -2373,6 +2377,42 @@ int btf__add_datasec(struct btf *btf, const > > char *name, __u32 byte_sz) > > return btf_commit_type(btf, sz); > > } > > > > +/* > > + * Append new BTF_KIND_FLOAT type with: > > + * - *name* - non-empty, non-NULL type name; > > + * - *sz* - size of the type, in bytes; > > + * Returns: > > + * - >0, type ID of newly added BTF type; > > + * - <0, on error. > > + */ > > +int btf__add_float(struct btf *btf, const char *name, size_t > > byte_sz) > > +{ > > + struct btf_type *t; > > + int sz, name_off; > > + > > + /* non-empty name */ > > + if (!name || !name[0]) > > + return -EINVAL; > > Do we want to ensure byte_sz to be 2/4/8/16? > Currently, the int type supports 1/2/4/8/16. > > In LLVM, the following are supported float types: > > case BuiltinType::Half: > case BuiltinType::Float: > case BuiltinType::LongDouble: > case BuiltinType::Float16: > case BuiltinType::BFloat16: > case BuiltinType::Float128: > case BuiltinType::Double: There can be 80-bit floats on x86: #include <stdio.h> int main() { printf("%zu\n", sizeof(long double)); } prints 12 when compiled with -m32 (not 10 due to alignment I assume). I guess this now completely kills the idea with sanitizing FLOATs to equally-sized INTs... [...]
On Wed, 2021-02-17 at 17:26 -0800, John Fastabend wrote: > Ilya Leoshkevich wrote: > > On Wed, 2021-02-17 at 13:12 -0800, John Fastabend wrote: > > > John Fastabend wrote: > > > > Ilya Leoshkevich wrote: > > > > > The logic follows that of BTF_KIND_INT most of the time. > > > > > Sanitization > > > > > replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on > > > > > older > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > Does this match the code though? > > > > > > > > > kernels. > > > > > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > > --- > > > > > > > > [...] > > > > > > > > > > > > > @@ -2445,6 +2450,9 @@ static void > > > > > bpf_object__sanitize_btf(struct > > > > > bpf_object *obj, struct btf *btf) > > > > > } else if (!has_func_global && > > > > > btf_is_func(t)) { > > > > > /* replace BTF_FUNC_GLOBAL with > > > > > BTF_FUNC_STATIC */ > > > > > t->info = BTF_INFO_ENC(BTF_KIND_FUNC, > > > > > 0, > > > > > 0); > > > > > + } else if (!has_float && btf_is_float(t)) { > > > > > + /* replace FLOAT with INT */ > > > > > + t->info = > > > > > BTF_INFO_ENC(BTF_KIND_FLOAT, 0, > > > > > 0); > > > > > > > > Do we also need to encode the vlen here? > > > > > > Sorry typo on my side, 't->size = ?' is what I was trying to > > > point > > > out. > > > Looks like its set in the other case where we replace VAR with > > > INT. > > > > The idea is to have the size of the INT equal to the size of the > > FLOAT > > that it replaces. I guess we can't do the same for VARs, because > > they > > don't have the size field, and if we don't have DATASECs, then we > > can't > > find the size of a VAR at all. > > > > Right, but KINT_INT has some extra constraints that don't appear to > be in > place for KIND_FLOAT. For example meta_check includes max size check. > We > should check these when libbpf does conversion as well? Otherwise > kernel > is going to give us an error that will be a bit hard to understand. Yeah, apparently floats can have non-power-of-2 sizes, which kills the idea with such a replacement. Maybe we should do exactly the same thing as we do for VARs after all. > Also what I am I missing here. I use the writers to build a float, > > btf__add_float(btf, "new_float", 8); > > This will create the btf_type struct approximately like this, > > btf_type t { > .name = name_off; // points at my name > .info = btf_type_info(BTF_KIND_FLOAT, 0, 0); > .size = 8 > }; > > But if I create an int_type with btf__add_int(btf, "net_int", 8); I > will > get a btf_type + __u32. When we do the conversion how do we skip the > extra u32 setup? > > *(__u32 *)(t + 1) = (encoding << 24) | (byte_sz * 8); > > Should we set this up on the conversion as well? Otherwise later > steps > might try to read the __u32 piece to find some arbitrary memory? Ah, you are absolutely right. I was hoping that e.g. btf_get_raw_data() would clean that up, but turns out it doesn't do that. Seems like I'll have to implement this myself.
On 2/18/21 5:34 AM, Ilya Leoshkevich wrote: > On Wed, 2021-02-17 at 23:16 -0800, Yonghong Song wrote: >> On 2/15/21 5:12 PM, Ilya Leoshkevich wrote: >>> The logic follows that of BTF_KIND_INT most of the time. >>> Sanitization >>> replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older >>> kernels. >>> >>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> >>> --- >>> tools/lib/bpf/btf.c | 44 >>> +++++++++++++++++++++++++++++++++ >>> tools/lib/bpf/btf.h | 8 ++++++ >>> tools/lib/bpf/btf_dump.c | 4 +++ >>> tools/lib/bpf/libbpf.c | 29 +++++++++++++++++++++- >>> tools/lib/bpf/libbpf.map | 5 ++++ >>> tools/lib/bpf/libbpf_internal.h | 2 ++ >>> 6 files changed, 91 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c >>> index d9c10830d749..07a30e98c3de 100644 >>> --- a/tools/lib/bpf/btf.c >>> +++ b/tools/lib/bpf/btf.c > > [...] > >>> @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct >>> bpf_object *obj, struct btf *btf) >>> } else if (!has_func_global && btf_is_func(t)) { >>> /* replace BTF_FUNC_GLOBAL with >>> BTF_FUNC_STATIC */ >>> t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, >>> 0); >>> + } else if (!has_float && btf_is_float(t)) { >>> + /* replace FLOAT with INT */ >>> + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, >>> 0); >> >> You can replace float with a "pointer to void" type. > > Wouldn't this cause problems with 32-bit floats on 64-bit machines? Oh, yes. You are right. I am just thinking of standalone float type, but obviously it could be struct/union member which makes it very important to keep the sanatized type having the same size. So replacing float with "point to void" won't work as you suggested. Looks like INT is the best candidate to replace, another is CHAR array. They do not match total size though. Maybe one of modifier type will be a good choice. For example, you can replace float with a BTF_KIND_CONST type and the base type for BTF_KIND_CONST type is an int type with the same size of float and you have to create that int type somewhere. In power-of-2 cases, it is possible this type already exists. > > [...] >
On 2/18/21 5:41 AM, Ilya Leoshkevich wrote: > On Wed, 2021-02-17 at 22:58 -0800, Yonghong Song wrote: >> On 2/15/21 5:12 PM, Ilya Leoshkevich wrote: >>> The logic follows that of BTF_KIND_INT most of the time. >>> Sanitization >>> replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older >>> kernels. >>> >>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> >>> --- >>> tools/lib/bpf/btf.c | 44 >>> +++++++++++++++++++++++++++++++++ >>> tools/lib/bpf/btf.h | 8 ++++++ >>> tools/lib/bpf/btf_dump.c | 4 +++ >>> tools/lib/bpf/libbpf.c | 29 +++++++++++++++++++++- >>> tools/lib/bpf/libbpf.map | 5 ++++ >>> tools/lib/bpf/libbpf_internal.h | 2 ++ >>> 6 files changed, 91 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c >>> index d9c10830d749..07a30e98c3de 100644 >>> --- a/tools/lib/bpf/btf.c >>> +++ b/tools/lib/bpf/btf.c > > [...] > >>> @@ -2373,6 +2377,42 @@ int btf__add_datasec(struct btf *btf, const >>> char *name, __u32 byte_sz) >>> return btf_commit_type(btf, sz); >>> } >>> >>> +/* >>> + * Append new BTF_KIND_FLOAT type with: >>> + * - *name* - non-empty, non-NULL type name; >>> + * - *sz* - size of the type, in bytes; >>> + * Returns: >>> + * - >0, type ID of newly added BTF type; >>> + * - <0, on error. >>> + */ >>> +int btf__add_float(struct btf *btf, const char *name, size_t >>> byte_sz) >>> +{ >>> + struct btf_type *t; >>> + int sz, name_off; >>> + >>> + /* non-empty name */ >>> + if (!name || !name[0]) >>> + return -EINVAL; >> >> Do we want to ensure byte_sz to be 2/4/8/16? >> Currently, the int type supports 1/2/4/8/16. >> >> In LLVM, the following are supported float types: >> >> case BuiltinType::Half: >> case BuiltinType::Float: >> case BuiltinType::LongDouble: >> case BuiltinType::Float16: >> case BuiltinType::BFloat16: >> case BuiltinType::Float128: >> case BuiltinType::Double: > > There can be 80-bit floats on x86: > > #include <stdio.h> > int main() { printf("%zu\n", sizeof(long double)); } > > prints 12 when compiled with -m32 (not 10 due to alignment I assume). Wow, I do not know we have long double float size = 12 for m32. In this case, maybe we can enforce float size of 2 and multiple of 4? I still like some enforcement, arbitrary float size seems wrong to me. > > I guess this now completely kills the idea with sanitizing FLOATs to > equally-sized INTs... My previous email suggests to use "const int" for sanitization. In kernel, we did not enforce the "int" type size as long as it is equal to or greater than the one represented by bits. if (BITS_ROUNDUP_BYTES(nr_bits) > t->size) { btf_verifier_log_type(env, t, "nr_bits exceeds type_size"); return -EINVAL; } But in libbpf, we do want to the int size to be power-of-2. /* byte_sz must be power of 2 */ if (!byte_sz || (byte_sz & (byte_sz - 1)) || byte_sz > 16) return -EINVAL; When people dump in-kernel sanitized btf and they may see an int with size 12, which will be weird. So maybe "const char_array" is the best choice here. > > [...] >
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index d9c10830d749..07a30e98c3de 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -291,6 +291,7 @@ static int btf_type_size(const struct btf_type *t) case BTF_KIND_PTR: case BTF_KIND_TYPEDEF: case BTF_KIND_FUNC: + case BTF_KIND_FLOAT: return base_size; case BTF_KIND_INT: return base_size + sizeof(__u32); @@ -338,6 +339,7 @@ static int btf_bswap_type_rest(struct btf_type *t) case BTF_KIND_PTR: case BTF_KIND_TYPEDEF: case BTF_KIND_FUNC: + case BTF_KIND_FLOAT: return 0; case BTF_KIND_INT: *(__u32 *)(t + 1) = bswap_32(*(__u32 *)(t + 1)); @@ -578,6 +580,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id) case BTF_KIND_UNION: case BTF_KIND_ENUM: case BTF_KIND_DATASEC: + case BTF_KIND_FLOAT: size = t->size; goto done; case BTF_KIND_PTR: @@ -621,6 +624,7 @@ int btf__align_of(const struct btf *btf, __u32 id) switch (kind) { case BTF_KIND_INT: case BTF_KIND_ENUM: + case BTF_KIND_FLOAT: return min(btf_ptr_sz(btf), (size_t)t->size); case BTF_KIND_PTR: return btf_ptr_sz(btf); @@ -2373,6 +2377,42 @@ int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz) return btf_commit_type(btf, sz); } +/* + * Append new BTF_KIND_FLOAT type with: + * - *name* - non-empty, non-NULL type name; + * - *sz* - size of the type, in bytes; + * Returns: + * - >0, type ID of newly added BTF type; + * - <0, on error. + */ +int btf__add_float(struct btf *btf, const char *name, size_t byte_sz) +{ + struct btf_type *t; + int sz, name_off; + + /* non-empty name */ + if (!name || !name[0]) + return -EINVAL; + + if (btf_ensure_modifiable(btf)) + return -ENOMEM; + + sz = sizeof(struct btf_type); + t = btf_add_type_mem(btf, sz); + if (!t) + return -ENOMEM; + + name_off = btf__add_str(btf, name); + if (name_off < 0) + return name_off; + + t->name_off = name_off; + t->info = btf_type_info(BTF_KIND_FLOAT, 0, 0); + t->size = byte_sz; + + return btf_commit_type(btf, sz); +} + /* * Append new data section variable information entry for current DATASEC type: * - *var_type_id* - type ID, describing type of the variable; @@ -3626,6 +3666,7 @@ static int btf_dedup_prep(struct btf_dedup *d) case BTF_KIND_FWD: case BTF_KIND_TYPEDEF: case BTF_KIND_FUNC: + case BTF_KIND_FLOAT: h = btf_hash_common(t); break; case BTF_KIND_INT: @@ -3722,6 +3763,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id) break; case BTF_KIND_FWD: + case BTF_KIND_FLOAT: h = btf_hash_common(t); for_each_dedup_cand(d, hash_entry, h) { cand_id = (__u32)(long)hash_entry->value; @@ -3983,6 +4025,7 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id, return btf_compat_enum(cand_type, canon_type); case BTF_KIND_FWD: + case BTF_KIND_FLOAT: return btf_equal_common(cand_type, canon_type); case BTF_KIND_CONST: @@ -4479,6 +4522,7 @@ static int btf_dedup_remap_type(struct btf_dedup *d, __u32 type_id) switch (btf_kind(t)) { case BTF_KIND_INT: case BTF_KIND_ENUM: + case BTF_KIND_FLOAT: break; case BTF_KIND_FWD: diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 1237bcd1dd17..c3b11bcebeda 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -132,6 +132,9 @@ LIBBPF_API int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz LIBBPF_API int btf__add_datasec_var_info(struct btf *btf, int var_type_id, __u32 offset, __u32 byte_sz); +/* float construction APIs */ +LIBBPF_API int btf__add_float(struct btf *btf, const char *name, size_t byte_sz); + struct btf_dedup_opts { unsigned int dedup_table_size; bool dont_resolve_fwds; @@ -294,6 +297,11 @@ static inline bool btf_is_datasec(const struct btf_type *t) return btf_kind(t) == BTF_KIND_DATASEC; } +static inline bool btf_is_float(const struct btf_type *t) +{ + return btf_kind(t) == BTF_KIND_FLOAT; +} + static inline __u8 btf_int_encoding(const struct btf_type *t) { return BTF_INT_ENCODING(*(__u32 *)(t + 1)); diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index 2f9d685bd522..5e957fcceee6 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -279,6 +279,7 @@ static int btf_dump_mark_referenced(struct btf_dump *d) case BTF_KIND_INT: case BTF_KIND_ENUM: case BTF_KIND_FWD: + case BTF_KIND_FLOAT: break; case BTF_KIND_VOLATILE: @@ -453,6 +454,7 @@ static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr) switch (btf_kind(t)) { case BTF_KIND_INT: + case BTF_KIND_FLOAT: tstate->order_state = ORDERED; return 0; @@ -1133,6 +1135,7 @@ static void btf_dump_emit_type_decl(struct btf_dump *d, __u32 id, case BTF_KIND_STRUCT: case BTF_KIND_UNION: case BTF_KIND_TYPEDEF: + case BTF_KIND_FLOAT: goto done; default: pr_warn("unexpected type in decl chain, kind:%u, id:[%u]\n", @@ -1247,6 +1250,7 @@ static void btf_dump_emit_type_chain(struct btf_dump *d, switch (kind) { case BTF_KIND_INT: + case BTF_KIND_FLOAT: btf_dump_emit_mods(d, decls); name = btf_name_of(d, t->name_off); btf_dump_printf(d, "%s", name); diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index d43cc3f29dae..703988d68e73 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -178,6 +178,8 @@ enum kern_feature_id { FEAT_PROG_BIND_MAP, /* Kernel support for module BTFs */ FEAT_MODULE_BTF, + /* BTF_KIND_FLOAT support */ + FEAT_BTF_FLOAT, __FEAT_CNT, }; @@ -1935,6 +1937,7 @@ static const char *btf_kind_str(const struct btf_type *t) case BTF_KIND_FUNC_PROTO: return "func_proto"; case BTF_KIND_VAR: return "var"; case BTF_KIND_DATASEC: return "datasec"; + case BTF_KIND_FLOAT: return "float"; default: return "unknown"; } } @@ -2384,15 +2387,17 @@ static bool btf_needs_sanitization(struct bpf_object *obj) { bool has_func_global = kernel_supports(FEAT_BTF_GLOBAL_FUNC); bool has_datasec = kernel_supports(FEAT_BTF_DATASEC); + bool has_float = kernel_supports(FEAT_BTF_FLOAT); bool has_func = kernel_supports(FEAT_BTF_FUNC); - return !has_func || !has_datasec || !has_func_global; + return !has_func || !has_datasec || !has_func_global || !has_float; } static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) { bool has_func_global = kernel_supports(FEAT_BTF_GLOBAL_FUNC); bool has_datasec = kernel_supports(FEAT_BTF_DATASEC); + bool has_float = kernel_supports(FEAT_BTF_FLOAT); bool has_func = kernel_supports(FEAT_BTF_FUNC); struct btf_type *t; int i, j, vlen; @@ -2445,6 +2450,9 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) } else if (!has_func_global && btf_is_func(t)) { /* replace BTF_FUNC_GLOBAL with BTF_FUNC_STATIC */ t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0, 0); + } else if (!has_float && btf_is_float(t)) { + /* replace FLOAT with INT */ + t->info = BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0); } } } @@ -3882,6 +3890,18 @@ static int probe_kern_btf_datasec(void) strs, sizeof(strs))); } +static int probe_kern_btf_float(void) +{ + static const char strs[] = "\0float"; + __u32 types[] = { + /* float */ + BTF_TYPE_FLOAT_ENC(1, 4), + }; + + return probe_fd(libbpf__load_raw_btf((char *)types, sizeof(types), + strs, sizeof(strs))); +} + static int probe_kern_array_mmap(void) { struct bpf_create_map_attr attr = { @@ -4061,6 +4081,9 @@ static struct kern_feature_desc { [FEAT_MODULE_BTF] = { "module BTF support", probe_module_btf, }, + [FEAT_BTF_FLOAT] = { + "BTF_KIND_FLOAT support", probe_kern_btf_float, + }, }; static bool kernel_supports(enum kern_feature_id feat_id) @@ -4940,6 +4963,8 @@ static int bpf_core_fields_are_compat(const struct btf *local_btf, local_id = btf_array(local_type)->type; targ_id = btf_array(targ_type)->type; goto recur; + case BTF_KIND_FLOAT: + return local_type->size == targ_type->size; default: pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n", btf_kind(local_type), local_id, targ_id); @@ -5122,6 +5147,8 @@ static int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id); goto recur; } + case BTF_KIND_FLOAT: + return local_type->size == targ_type->size; default: pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n", btf_kind_str(local_type), local_id, targ_id); diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 1c0fd2dd233a..ec898f464ab9 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -350,3 +350,8 @@ LIBBPF_0.3.0 { xsk_setup_xdp_prog; xsk_socket__update_xskmap; } LIBBPF_0.2.0; + +LIBBPF_0.4.0 { + global: + btf__add_float; +} LIBBPF_0.3.0; diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h index 969d0ac592ba..343f6eb05637 100644 --- a/tools/lib/bpf/libbpf_internal.h +++ b/tools/lib/bpf/libbpf_internal.h @@ -31,6 +31,8 @@ #define BTF_MEMBER_ENC(name, type, bits_offset) (name), (type), (bits_offset) #define BTF_PARAM_ENC(name, type) (name), (type) #define BTF_VAR_SECINFO_ENC(type, offset, size) (type), (offset), (size) +#define BTF_TYPE_FLOAT_ENC(name, sz) \ + BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0), sz) #ifndef likely #define likely(x) __builtin_expect(!!(x), 1)
The logic follows that of BTF_KIND_INT most of the time. Sanitization replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older kernels. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/lib/bpf/btf.c | 44 +++++++++++++++++++++++++++++++++ tools/lib/bpf/btf.h | 8 ++++++ tools/lib/bpf/btf_dump.c | 4 +++ tools/lib/bpf/libbpf.c | 29 +++++++++++++++++++++- tools/lib/bpf/libbpf.map | 5 ++++ tools/lib/bpf/libbpf_internal.h | 2 ++ 6 files changed, 91 insertions(+), 1 deletion(-)