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?