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);
> >
> > [...]
