On Fri, Aug 30, 2024 at 12:30 AM Tony Ambardar <tony.ambardar@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
> + } \
> + } \
> + _val; \
> +})
> +
for the rest, Alexei, can you please review and give your ack?
