Re: [PATCH bpf-next v4 7/8] libbpf: Support creating light skeleton of either endianness

[Tony Ambardar <tony.ambardar@gmail.com>]

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?