On Mon, Aug 26, 2024 at 3:58 AM Tony Ambardar tony.ambardar@gmail.com 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@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
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(-)
[...]
+/*
- 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);[...]