Re: [RFC PATCH 3/5] ebpf: Added declaration/initialization routines.

[Andrew Melnichenko]

Hi all,

On Thu, Mar 30, 2023 at 11:33 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Mar 30, 2023 at 03:15:20AM +0300, Andrew Melnychenko wrote:
> > Now, the binary objects may be retrieved by id/name.
> > It would require for future qmp commands that may require specific
> > eBPF blob.
> >
> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> > ---
> >  ebpf/ebpf.c      | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  ebpf/ebpf.h      | 25 +++++++++++++++++++++++++
> >  ebpf/ebpf_rss.c  |  4 ++++
> >  ebpf/meson.build |  1 +
> >  4 files changed, 78 insertions(+)
> >  create mode 100644 ebpf/ebpf.c
> >  create mode 100644 ebpf/ebpf.h
> >
> > diff --git a/ebpf/ebpf.c b/ebpf/ebpf.c
> > new file mode 100644
> > index 0000000000..86320d72f5
> > --- /dev/null
> > +++ b/ebpf/ebpf.c
> > @@ -0,0 +1,48 @@
> > +/*
> > + * QEMU eBPF binary declaration routine.
> > + *
> > + * Developed by Daynix Computing LTD (http://www.daynix.com)
> > + *
> > + * Authors:
> > + *  Andrew Melnychenko <andrew@daynix.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > + * later.  See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/queue.h"
> > +#include "ebpf/ebpf.h"
> > +
> > +struct ElfBinaryDataEntry {
> > +    const char *id;
> > +    const void * (*fn)(size_t *);
>
> It feels odd to be storing the function there, as that's just
> an artifact of how the EBPF rss program is acquired. IMHO
> this should just be
>
>    const void *data;
>    size_t datalen;
>

Yeah, have an idea to store data/size instead. the skeleton provides
them in function,
didn't want to make dirty hacks for the constructor. So I've passed
the function straight to
the structure.

> > +
> > +    QSLIST_ENTRY(ElfBinaryDataEntry) node;
> > +};
> > +
> > +static QSLIST_HEAD(, ElfBinaryDataEntry) ebpf_elf_obj_list =
> > +                                            QSLIST_HEAD_INITIALIZER();
> > +
> > +void ebpf_register_binary_data(const char *id, const void * (*fn)(size_t 
> > *))
> > +{
> > +    struct ElfBinaryDataEntry *data = NULL;
> > +
> > +    data = g_malloc0(sizeof(*data));
>
> We prefer g_new0 over g_malloc and initialize when declaring eg
>
>     struct ElfBinaryDataEntry *data = g_new0(struct ElfBinaryDataEntry, 1);
>

Ok, thank you.

> > +    data->fn = fn;
> > +    data->id = id;
> > +
> > +    QSLIST_INSERT_HEAD(&ebpf_elf_obj_list, data, node);
> > +}
> > +
> > +const void *ebpf_find_binary_by_id(const char *id, size_t *sz)
> > +{
> > +    struct ElfBinaryDataEntry *it = NULL;
> > +    QSLIST_FOREACH(it, &ebpf_elf_obj_list, node) {
> > +        if (strcmp(id, it->id) == 0) {
> > +            return it->fn(sz);
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > diff --git a/ebpf/ebpf.h b/ebpf/ebpf.h
> > new file mode 100644
> > index 0000000000..fd705cb73e
> > --- /dev/null
> > +++ b/ebpf/ebpf.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * QEMU eBPF binary declaration routine.
> > + *
> > + * Developed by Daynix Computing LTD (http://www.daynix.com)
> > + *
> > + * Authors:
> > + *  Andrew Melnychenko <andrew@daynix.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > + * later.  See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef EBPF_H
> > +#define EBPF_H
> > +
> > +void ebpf_register_binary_data(const char *id, const void * (*fn)(size_t 
> > *));
>
> IMHO it would be better as
>
> void ebpf_register_binary_data(const char *id, const void *data, size_t 
> datalen);
>
>
> > +const void *ebpf_find_binary_by_id(const char *id, size_t *sz);
> > +
> > +#define ebpf_binary_init(id, fn)                                           
> > \
> > +static void __attribute__((constructor)) ebpf_binary_init_ ## fn(void)     
> > \
> > +{                                                                          
> > \
> > +    ebpf_register_binary_data(id, fn);                                     
> > \
>
> size_t datalen;
> const void *data = fn(&datalen);
> ebpf_register_binary_data(oid, data, datalen);
>

Yeah, it can work like that. For some reason, I want that register
function and the constructor
have the same call/mention.

>
> > +}
> > +
> > +#endif /* EBPF_H */
> > diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
> > index 08015fecb1..b4038725f2 100644
> > --- a/ebpf/ebpf_rss.c
> > +++ b/ebpf/ebpf_rss.c
> > @@ -21,6 +21,8 @@
> >
> >  #include "ebpf/ebpf_rss.h"
> >  #include "ebpf/rss.bpf.skeleton.h"
> > +#include "ebpf/ebpf.h"
> > +
> >  #include "trace.h"
> >
> >  void ebpf_rss_init(struct EBPFRSSContext *ctx)
> > @@ -237,3 +239,5 @@ void ebpf_rss_unload(struct EBPFRSSContext *ctx)
> >      ctx->obj = NULL;
> >      ctx->program_fd = -1;
> >  }
> > +
> > +ebpf_binary_init("rss", rss_bpf__elf_bytes)
> > diff --git a/ebpf/meson.build b/ebpf/meson.build
> > index 2dd0fd8948..67c3f53aa9 100644
> > --- a/ebpf/meson.build
> > +++ b/ebpf/meson.build
> > @@ -1 +1,2 @@
> > +softmmu_ss.add(files('ebpf.c'))
> >  softmmu_ss.add(when: libbpf, if_true: files('ebpf_rss.c'), if_false: 
> > files('ebpf_rss-stub.c'))
> > --
> > 2.39.1
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>