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@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
>
Yes, agreed. Will update.
> > + } \
> > + } \
> > + _val; \
> > +})
> > +
>
>
> for the rest, Alexei, can you please review and give your ack?
