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

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

 



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

> +               }                                               \
> +       }                                                       \
> +       _val;                                                   \
> +})
> +


for the rest, Alexei, can you please review and give your ack?





[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