| Message ID | cover.1724976539.git.tony.ambardar@gmail.com |
|---|---|
| Headers | show |
| Series | libbpf, selftests/bpf: Support cross-endian usage | expand |
On Fri, Aug 30, 2024 at 02:30:46PM -0700, Andrii Nakryiko wrote: > On Fri, Aug 30, 2024 at 12:30 AM Tony Ambardar <tony.ambardar@gmail.com> wrote: > > > > Track target endianness in 'struct bpf_gen' and process in-memory data in > > native byte-order, but on finalization convert the embedded loader BPF > > insns to target endianness. > > > > The light skeleton also includes a target-accessed data blob which is > > heterogeneous and thus difficult to convert to target byte-order on > > finalization. Add support functions to convert data to target endianness > > as it is added to the blob. > > > > Also add additional debug logging for data blob structure details and > > skeleton loading. > > > > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> > > --- > > tools/lib/bpf/bpf_gen_internal.h | 1 + > > tools/lib/bpf/gen_loader.c | 187 +++++++++++++++++++++++-------- > > tools/lib/bpf/libbpf.c | 1 + > > tools/lib/bpf/skel_internal.h | 3 +- > > 4 files changed, 147 insertions(+), 45 deletions(-) > > > > diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h > > index fdf44403ff36..6ff963a491d9 100644 > > --- a/tools/lib/bpf/bpf_gen_internal.h > > +++ b/tools/lib/bpf/bpf_gen_internal.h > > @@ -34,6 +34,7 @@ struct bpf_gen { > > void *data_cur; > > void *insn_start; > > void *insn_cur; > > + bool swapped_endian; > > ssize_t cleanup_label; > > __u32 nr_progs; > > __u32 nr_maps; > > diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c > > index cf3323fd47b8..4374399bc3f8 100644 > > --- a/tools/lib/bpf/gen_loader.c > > +++ b/tools/lib/bpf/gen_loader.c > > @@ -401,6 +401,15 @@ int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps) > > opts->insns_sz = gen->insn_cur - gen->insn_start; > > opts->data = gen->data_start; > > opts->data_sz = gen->data_cur - gen->data_start; > > + > > + /* use target endianness for embedded loader */ > > + if (gen->swapped_endian) { > > + struct bpf_insn *insn = (struct bpf_insn *)opts->insns; > > + int insn_cnt = opts->insns_sz / sizeof(struct bpf_insn); > > + > > + for (i = 0; i < insn_cnt; i++) > > + bpf_insn_bswap(insn++); > > + } > > } > > return gen->error; > > } > > @@ -414,6 +423,31 @@ void bpf_gen__free(struct bpf_gen *gen) > > free(gen); > > } > > > > +/* > > + * Fields of bpf_attr are set to values in native byte-order before being > > + * written to the target-bound data blob, and may need endian conversion. > > + * This macro allows providing the correct value in situ more simply than > > + * writing a separate converter for *all fields* of *all records* included > > + * in union bpf_attr. Note that sizeof(rval) should match the assignment > > + * target to avoid runtime problems. > > + */ > > +#define tgt_endian(rval) ({ \ > > + typeof(rval) _val; \ > > + if (!gen->swapped_endian) \ > > if/else has to have balanced branches w.r.t. {}. Either both should > have it or both shouldn't. In this case both should have it. > > > + _val = (rval); \ > > + else { \ > > + switch (sizeof(rval)) { \ > > + case 1: _val = (rval); break; \ > > + case 2: _val = bswap_16(rval); break; \ > > + case 4: _val = bswap_32(rval); break; \ > > + case 8: _val = bswap_64(rval); break; \ > > + default:_val = (rval); \ > > + pr_warn("unsupported bswap size!\n"); \ > > this is a weird formatting, but you can also just unconditionally > assign _val, and only swap it if gen->swapped_endian > > typeof(rval) _val = (rval); > > if (gen->swapped_endian) { > switch (...) { > case 1: ... > ... > case 8: ... > default: pr_warn("..."); > } > } > > _val; > > > seems simpler and cleaner, imo > Yes, agreed. Will update. > > + } \ > > + } \ > > + _val; \ > > +}) > > + > > > for the rest, Alexei, can you please review and give your ack?
On Fri, Aug 30, 2024 at 06:16:25PM -0700, Eduard Zingerman wrote: > On Fri, 2024-08-30 at 00:29 -0700, Tony Ambardar wrote: > > [...] > > > @@ -940,6 +942,21 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > > return 0; > > } > > > > +static void bpf_object_bswap_progs(struct bpf_object *obj) > > +{ > > + struct bpf_program *prog = obj->programs; > > + struct bpf_insn *insn; > > + int p, i; > > + > > + for (p = 0; p < obj->nr_programs; p++, prog++) { > > + insn = prog->insns; > > + for (i = 0; i < prog->insns_cnt; i++, insn++) > > + bpf_insn_bswap(insn); > > + pr_debug("prog '%s': converted %zu insns to native byte order\n", > > + prog->name, prog->insns_cnt); > > Nit: pr_debug already printed available programs at this point, > maybe move this call outside of both loops? > Good point. Will update to summarize # of programs converted instead. > > + } > > +} > > + > > static const struct btf_member * > > find_member_by_offset(const struct btf_type *t, __u32 bit_offset) > > { > > [...] >
Hello all, This patch series targets a long-standing BPF usability issue - the lack of general cross-compilation support - by enabling cross-endian usage of libbpf and bpftool, as well as supporting cross-endian build targets for selftests/bpf. Benefits include improved BPF development and testing for embedded systems based on e.g. big-endian MIPS, more build options e.g for s390x systems, and better accessibility to the very latest test tools e.g. 'test_progs'. Initial development and testing used mips64, since this arch makes switching the build byte-order trivial and is thus very handy for A/B testing. However, it lacks some key features (bpf2bpf call, kfuncs, etc) making for poor selftests/bpf coverage. Final testing takes the kernel and selftests/bpf cross-built from x86_64 to s390x, and runs the result under QEMU/s390x. That same configuration could also be used on kernel-patches/bpf CI for regression testing endian support or perhaps load-sharing s390x builds across x86_64 systems. This thread includes some background regarding testing on QEMU/s390x and the generally favourable results: https://lore.kernel.org/bpf/ZsEcsaa3juxxQBUf@kodidev-ubuntu/ Feedback and suggestions are welcome! Best regards, Tony Changelog: --------- v3 -> v4: - fix a use-after-free ELF data-handling error causing rare CI failures - move bswap functions for func/line/core-relo records to internal header - use bswap functions also for info blobs in light skeleton v2 -> v3: (feedback from Andrii) - improve some log and commit message formatting - restructure BTF.ext endianness safety checks and byte-swapping - use BTF.ext info record definitions for swapping, require BTF v1 - follow BTF API implementation more closely for BTF.ext - explicitly reject loading non-native endianness program into kernel - simplify linker output byte-order setting - drop redundant safety checks during linking - simplify endianness macro and improve blob setup code for light skel - no unexpected test failures after cross-compiling x86_64 -> s390x v1 -> v2: - fixed a light skeleton bug causing test_progs 'map_ptr' failure - simplified some BTF.ext related endianness logic - remove an 'inline' usage related to CI checkpatch failure - improve some formatting noted by checkpatch warnings - unexpected 'test_progs' failures drop 3 -> 2 (x86_64 to s390x cross) Tony Ambardar (8): libbpf: Improve log message formatting libbpf: Fix header comment typos for BTF.ext libbpf: Fix output .symtab byte-order during linking libbpf: Support BTF.ext loading and output in either endianness libbpf: Support opening bpf objects of either endianness libbpf: Support linking bpf objects of either endianness libbpf: Support creating light skeleton of either endianness selftests/bpf: Support cross-endian building tools/lib/bpf/bpf_gen_internal.h | 1 + tools/lib/bpf/btf.c | 196 ++++++++++++++++++++++++--- tools/lib/bpf/btf.h | 3 + tools/lib/bpf/btf_dump.c | 2 +- tools/lib/bpf/btf_relocate.c | 2 +- tools/lib/bpf/gen_loader.c | 187 +++++++++++++++++++------ tools/lib/bpf/libbpf.c | 54 ++++++-- tools/lib/bpf/libbpf.map | 2 + tools/lib/bpf/libbpf_internal.h | 48 ++++++- tools/lib/bpf/linker.c | 92 ++++++++++--- tools/lib/bpf/relo_core.c | 2 +- tools/lib/bpf/skel_internal.h | 3 +- tools/testing/selftests/bpf/Makefile | 7 +- 13 files changed, 502 insertions(+), 97 deletions(-)