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

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Question for Markus at the bottom....
>
> On Fri, May 12, 2023 at 03:29:01PM +0300, Andrew Melnychenko wrote:
>> 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.

Yes, but why is this useful?

Explaining why we want this patch is even more important than explaining
what it does.  If the commit message does badly at the latter, I can
still read the actual patch.  If it does badly at the former, I'm lost.

>> 
>> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>> ---
>>  monitor/qmp-cmds.c | 16 ++++++++++++++++
>>  qapi/misc.json     | 38 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 54 insertions(+)
>> 
>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> index b0f948d3376..259bc87ccb1 100644
>> --- a/monitor/qmp-cmds.c
>> +++ b/monitor/qmp-cmds.c
>> @@ -32,6 +32,7 @@
>>  #include "hw/mem/memory-device.h"
>>  #include "hw/intc/intc.h"
>>  #include "hw/rdma/rdma.h"
>> +#include "ebpf/ebpf.h"
>>  
>>  NameInfo *qmp_query_name(Error **errp)
>>  {
>> @@ -209,3 +210,18 @@ static void __attribute__((__constructor__)) monitor_init_qmp_commands(void)
>>                           qmp_marshal_qmp_capabilities,
>>                           QCO_ALLOW_PRECONFIG, 0);
>>  }
>> +
>> +EbpfObject *qmp_request_ebpf(EbpfProgramID id, Error **errp)
>> +{
>> +    EbpfObject *ret = NULL;
>> +    size_t size = 0;
>> +    const void *data = ebpf_find_binary_by_id(id, &size, errp);
>> +    if (!data) {
>> +        return NULL;
>> +    }
>> +
>> +    ret = g_new0(EbpfObject, 1);
>> +    ret->object = g_base64_encode(data, size);
>> +
>> +    return ret;
>> +}

I recently moved a load of commands from monitor/ to the appropriate
subsystem.  I'm reluctant to add back commands that aren't about
controlling the monitor.  Why not ebpf/ebpf-qmp-cmd.c, so MAINTAINERS
covers it properly?

>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 6ddd16ea283..e96dac8482b 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json

Why not qapi/ebpf.json, so MAINTAINERS covers it properly?

>> @@ -618,3 +618,41 @@
>>  { 'event': 'VFU_CLIENT_HANGUP',
>>    'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
>>              'dev-id': 'str', 'dev-qom-path': 'str' } }
>> +
>> +##
>> +# @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.

What is RSS, and why should I care?

>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'EbpfProgramID',
>> +  'data': [ { 'name': 'rss', 'if': 'CONFIG_EBPF' } ] }
>> +
>> +##
>> +# @request-ebpf:
>> +#
>> +# Function returns eBPF object that can be loaded with libbpf.

Command, not function.

Please use imperative mood like "Return X" instead of descriptions like
"Command returns X" or "Function returns X".

>> +# Management applications (g.e. libvirt) may load it and pass file
>> +# descriptors to QEMU. Which allows running QEMU without BPF capabilities.
>> +#
>> +# Returns: RSS eBPF object encoded in base64.
>> +#
>> +# Since: 8.1
>> +#
>> +##

So, this is basically a way to retrieve an eBPF program by some
well-known name.

Ignorant question: how are these programs desposited?

>> +{ 'command': 'request-ebpf',
>> +  'data': { 'id': 'EbpfProgramID' },
>> +  'returns': 'EbpfObject' }
>> +
>
> Fnuctionally this is fine so
>
>   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> A question for Markus though - is is perhaps better to mark all the
> command/enum/object as conditional on CONFIG_EBPF, rather than just
> reporting an empty EbpfProgramID enum when EBPF is disabled at build
> time ?

Using conditionals has two advantages:

1. Checking for the feature is commonly more straightforward

   Checking for presence of command with query-qmp-schema works fine
   both for old versions of QEMU (where the command doesn't exist) and
   new versions (where it exists, but is disabled).

   Without conditionals, you need two checks: command present, and
   command can actually do something.  More complicated even when the
   latter check is easy, as it is here.

2. Slightly leaner program when the feature is off