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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 26, 2024 at 3:58 AM Tony Ambardar <tony.ambardar@xxxxxxxxx> wrote:
>
> On Fri, Aug 23, 2024 at 12:47:56PM -0700, Andrii Nakryiko wrote:
> > On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@xxxxxxxxx> wrote:
> > >
> > > From: Tony Ambardar <tony.ambardar@xxxxxxxxx>
> > >
> > > 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(-)
> > >
> >
> > [...]
> >
> > > +/*
> > > + * 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 setting the correct value in situ and is simpler than
> > > + * writing a separate converter for *all fields* of *all records* included
> > > + * in union bpf_attr.
> > > + */
> > > +#define move_tgt_endian(lval, rval) {                          \
> > > +       if (!gen->swapped_endian)                               \
> > > +               lval = (rval);                                  \
> >
> > add {}  here and for else branch, please
> >
> > > +       else                                                    \
> > > +               switch (sizeof(lval)) {                         \
> > > +               case 2:                                         \
> > > +                       lval = bswap_16(rval);                  \
> > > +                       break;                                  \
> > > +               case 4:                                         \
> > > +                       lval = bswap_32(rval);                  \
> > > +                       break;                                  \
> > > +               case 8:                                         \
> > > +                       lval = bswap_64(rval);                  \
> > > +                       break;                                  \
> >
> > I'd also go for more compact:
> >
> >
> > case 2: lval = bswap_16(rval); break;
> > case 4: lval = bswap_32(rval); break;
>
> The irony is that I had this originally but checkpatch complained loudly.

checkpatch might be a guidance, but it's definitely not a set in stone rule

[...]

> > > +       move_tgt_endian(attr.func_info_rec_size, load_attr->func_info_rec_size);
> > > +       move_tgt_endian(attr.func_info_cnt, load_attr->func_info_cnt);
> >
> > this is quite intrusive, maybe instead of imperative move_tgt_endian()
> > macro, develop the one that could be used as
> >
> > attr.func_info_cnt = tgt_endian(load_attr->func_info_cnt);
>
> I had considered this but it's not reliable since the source var size may
> not match the dest and the bswap will be improperly sized e.g. note above
> that move_tgt_endian() uses the _dest_ var to size the bswap.
>
> While I completely agree this is intrusive, it's still safe and correct.
> The other idea I played with is to leave the assignments alone and fix up
> struct fields' endianness afterwards via macro. Something like:
>
>   attr.map_type = map_type;
>   attr.key_size = key_size;
>   attr.value_size = value_size;
>   attr.map_flags = map_attr->map_flags;
>   attr.map_extra = map_attr->map_extra;
>
>   BSWAP_FIELDS(attr, map_type, key_size, value_size, map_flags, map_extra);
>
> But this would require some funky macro magic, possibly in a small header.
> What do you think? Does something similar exist already in kernel sources?

do we intentionally have mismatched assignments? If not, I'd still go
with the cleaner casting-like approach. And even if we have one or few
intentional cases, we can just explicitly cast

> >
> > ? I.e., working as an expression, taking into account the need to swap
> > and byte size of the argument. Should be doable.
> >
> > > +       func_info = add_data(gen, load_attr->func_info, func_info_tot_sz);
> > > +       pr_debug("gen: prog_load: func_info: off %d cnt %d rec size %d\n",
> > > +                func_info, load_attr->func_info_cnt,
> > > +                load_attr->func_info_rec_size);
> >
> > [...]





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux