Re: [PATCH v3 5/6] qmp: Added new command to retrieve eBPF blob.

[Andrew Melnichenko <andrew@daynix.com>]

Hi all,


On Fri, Jul 7, 2023 at 2:45 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Andrew Melnychenko <andrew@daynix.com> writes:
>
> > Added command "request-ebpf". This command returns
> > eBPF program encoded base64. The program taken from the
> > skeleton and essentially is an ELF object that can be
> > loaded in the future with libbpf.
> >
> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>
> In review of v2, I asked why we need to provide eBPF programs via QMP,
> and not simply via the filesystem, like all the other build artifacts.
> Daniel explained
>
>     Fetching it from QEMU gives us a strong guarantee that the eBPF
>     code actually matches the QEMU binary we're talking to, which is
>     useful if you're dealing with RPMs which can be upgraded behind
>     your back, or have multiple parallel installs of QEMU.
>
> I asked to work this rationale into the QAPI schema doc comments and the
> commit message.
>
> Only the doc comments are updated in this version.  Please update the
> commit message as well.

Ah, my bad. I'll update the commit message in v4.

>
> I remain unconvinced this justifies adding an entirely new transport.
> If we want to guard against mismatch, we can do it the usual way: bake
> the version into the build artifacts that must match, then check they
> do.
>
> Since I feel vetoing the change would overstep my authority as a QAPI
> maintainer, I'm asking you to treat my objection as advice.
>
> If you you elect not to follow my advice, I want you to describe the
> path not chosen in the commit message.  I.e. briefly explain the need
> for an exact version match, describe the ways to ensure it we
> considered, and the reasons for your pick.

In the v1 of this feature, we tried to implement it using an
additional file including
the ebpf (ebpf helper) built together with QEMU. Unfortunately, during long v1
discussion it was found that this approach still leaves an open issue in case
of QEMU update when the running instance is present + later hotplug of
virtio-net device.
The existing approach (v2,v3,...) seems to solve all the problems with
little effort.
We'll document all the reasons in the commit message.
Thank you for the advice.

>
> > ---
> >  qapi/ebpf.json        | 55 +++++++++++++++++++++++++++++++++++++++++++
> >  qapi/meson.build      |  1 +
> >  qapi/qapi-schema.json |  1 +
> >  3 files changed, 57 insertions(+)
> >  create mode 100644 qapi/ebpf.json
> >
> > diff --git a/qapi/ebpf.json b/qapi/ebpf.json
> > new file mode 100644
> > index 00000000000..6f9a1f1a0c9
> > --- /dev/null
> > +++ b/qapi/ebpf.json
> > @@ -0,0 +1,55 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> > +# See the COPYING file in the top-level directory.
> > +
> > +##
> > +# = Machines
>
> Copy-pasta :)

Oh :3

>
> > +##
> > +
> > +{ 'include': 'common.json' }
> > +
> > +##
> > +# @EbpfObject:
> > +#
> > +# Structure that holds eBPF ELF object encoded in base64.
> > +#
> > +# Since: 8.1
> > +#
> > +##
> > +{ 'struct': 'EbpfObject',
> > +  'data': {'object': 'str'} }
> > +
> > +##
> > +# @EbpfProgramID:
> > +#
> > +# An enumeration of the eBPF programs. Currently, only RSS is presented.
> > +# RSS - Receive side scaling, technology that allows steering traffic
> > +# between queues by calculation hash. Users may set up indirection table
> > +# and hash/packet types configurations. Used with virtio-net.
>
> Let's stick to the common way to document enum members, like this:
>
>    # The eBPF programs that can be gotten with request-ebpf.
>    #
>    # @rss: ...
>

Ok, I'll update it.

> > +#
> > +# Since: 8.1
> > +##
> > +{ 'enum': 'EbpfProgramID',
> > +  'data': [ { 'name': 'rss', 'if': 'CONFIG_EBPF' } ] }
>
> Uh, this makes just the member @rss conditional on CONFIG_EBPF.
> Why not the entire enum?
>

I'll change it for entire enum.

> > +
> > +##
> > +# @request-ebpf:
> > +#
> > +# Returns eBPF object that can be loaded with libbpf.
> > +# Management applications (g.e. libvirt) may load it and pass file
> > +# descriptors to QEMU. Which allows running QEMU without BPF capabilities.
> > +# It's crucial that eBPF program/map is compatible with QEMU, so it's
> > +# provided through QMP.
> > +#
> > +# Returns: RSS eBPF object encoded in base64.
> > +#
> > +# Since: 8.1
> > +#
> > +##
> > +{ 'command': 'request-ebpf',
> > +  'data': { 'id': 'EbpfProgramID' },
> > +  'returns': 'EbpfObject',
> > +  'if': 'CONFIG_EBPF' }
> > +
> > diff --git a/qapi/meson.build b/qapi/meson.build
> > index 9fd480c4d81..12a1cb1f9c2 100644
> > --- a/qapi/meson.build
> > +++ b/qapi/meson.build
> > @@ -52,6 +52,7 @@ qapi_all_modules = [
> >    'transaction',
> >    'virtio',
> >    'yank',
> > +  'ebpf',
>
> Keep this list alphabetically sorted, please.
>

Ok!

> >  ]
> >  if have_system
> >    qapi_all_modules += [
> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> > index 6594afba312..169cfe8d386 100644
> > --- a/qapi/qapi-schema.json
> > +++ b/qapi/qapi-schema.json
> > @@ -79,3 +79,4 @@
> >  { 'include': 'virtio.json' }
> >  { 'include': 'cryptodev.json' }
> >  { 'include': 'cxl.json' }
> > +{ 'include': 'ebpf.json' }
>
> This puts it at the end of the QMP reference manual.  Is this the best
> spot?
>

I think I'll put it next "net". Although, future eBPF programs can be
used not only for networks.
On the other hand, practical use of eBPF that I can come up with
related to the net, g.e. virtio/vhost net filters.