Hi all,
> Seems like this function is duplicating what glib should already be
> able to do.
>
Yea, but it's required a Linux specific header - without it, qemu builds
but crashes.
Could we use a compile-time determination of where we were (supposed)
> to be installed, and therefore where our helper should be installed,
> rather than the dynamic /proc/self/exe munging?
>
Yes, we can define something like CONFIG_QEMU_HELPERDIR ##
"qemu-ebpf-rss-helper", for RSS helper.
But I've tried to implement generic function for possible other helpers.
Yeah I think avoiding /proc/self/exe is desirable, because I can
> imagine scenarios where this can lead to picking the wrong helper.
> Better to always use the compile time install directory.
>
The main scenario that find_helper() should solve - non installed qemu use
helper from own build.
That's why reading /proc/self/exe is implemented.
So the intent is that we can make this list larger if we write other
> helper binaries. But this code is in an overall #ifdef CONFIG_LINUX,
> which means it won't work on other platforms.
>
Yes, for now, eBPF RSS is only for virtio-net + Linux TAP.
Checking F_OK (existence) instea of X_OK is odd.
>
Libvirt launches qemu to get different properties. That qemu may not have
permission to launch the helper.
It uses /proc/self/exe to find the running executable's directory. This
> is specific to Linux[*]. You get different behavior on Linux vs. other
> systems.
>
The code guarded by CONFIG_LINUX.
* If the host isn't Linux, it returns /usr/libexec/qemu-ebpf-rss-helper.
> Not good.
>
No, "query-helper-paths" will return an empty list.
* If Alice runs bld/x86_64-softmmu/qemu-system-x86_64, it also returns
> /usr/libexec/qemu-ebpf-rss-helper. Not good.
>
No, /proc/self/exe dereferences "bld/x86_64-softmmu/qemu-system-x86_64" to
"bld/qemu-system-x86_64"
and we will get bld/qemu-ebpf-rss-helper.
The name query-helper-paths is generic, the documented purpose "Query
> specific eBPF RSS helper" is specific.
>
> qemu-ebpf-rss-helper isn't necessarily the only helper that needs to be
> in sync with QEMU.
>
Yea, I'll update the document.
If we want to ensure the right helper runs, we should use a build
> identifier compiled into the programs, like we do for modules.
>
Thanks, I'll check. Overall, current idea was to avoid the use of the helper
from CONFIG_QEMU_HELPERDIR if qemu is not installed(like in your examples).
Helpers QEMU code runs itself should be run as
> CONFIG_QEMU_HELPERDIR/HELPER, with a suitable user override. This is
> how qemu-bridge-helper works.
>
> Helpers some other program runs are that other program's problem.
> They'll probably work the same: built-in default that can be overridden
> with configuration.
>
Well, for qemu it does not really matter how TAP fd was created. It can be
the helper, Libvirt itself, or a script.
In the end, "netdev" gets its fds and for qemu there is no difference. TAP
fd is TAP fd.
And Libvirt would use the same qemu-bridge-helper(from libvirt/qemu.conf)
for every qemu "emulator".
For eBPF we need to create specific maps(and/or thair quantities) that
contain specific structures and for different
qemu it may be different.
On Sat, Jun 12, 2021 at 8:28 AM Markus Armbruster <arm...@redhat.com> wrote:
> Andrew Melnychenko <and...@daynix.com> writes:
>
> > New qmp command to query ebpf helper.
> > It's crucial that qemu and helper are in sync and in touch.
> > Technically helper should pass eBPF fds that qemu may accept.
> > And different qemu's builds may have different eBPF programs and helpers.
> > Qemu returns helper that should "fit" to virtio-net.
> >
> > Signed-off-by: Andrew Melnychenko <and...@daynix.com>
> > ---
> > monitor/qmp-cmds.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
> > qapi/misc.json | 29 +++++++++++++++++
> > 2 files changed, 107 insertions(+)
> >
> > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> > index f7d64a6457..5dd2a58ea2 100644
> > --- a/monitor/qmp-cmds.c
> > +++ b/monitor/qmp-cmds.c
> > @@ -351,3 +351,81 @@ void qmp_display_reload(DisplayReloadOptions *arg,
> Error **errp)
> > abort();
> > }
> > }
> > +
> > +#ifdef CONFIG_LINUX
> > +
> > +static const char *get_dirname(char *path)
> > +{
> > + char *sep;
> > +
> > + sep = strrchr(path, '/');
> > + if (sep == path) {
> > + return "/";
> > + } else if (sep) {
> > + *sep = 0;
> > + return path;
> > + }
> > + return ".";
> > +}
> > +
> > +static char *find_helper(const char *name)
> > +{
> > + char qemu_exec[PATH_MAX];
> > + const char *qemu_dir = NULL;
> > + char *helper = NULL;
> > +
> > + if (name == NULL) {
> > + return NULL;
> > + }
> > +
> > + if (readlink("/proc/self/exe", qemu_exec, PATH_MAX) > 0) {
> > + qemu_dir = get_dirname(qemu_exec);
> > +
> > + helper = g_strdup_printf("%s/%s", qemu_dir, name);
> > + if (access(helper, F_OK) == 0) {
> > + return helper;
> > + }
> > + g_free(helper);
> > + }
> > +
> > + helper = g_strdup_printf("%s/%s", CONFIG_QEMU_HELPERDIR, name);
> > + if (access(helper, F_OK) == 0) {
> > + return helper;
> > + }
> > + g_free(helper);
> > +
> > + return NULL;
> > +}
>
> This returns the helper in the same directory as the running executable,
> or as a fallback the helper in CONFIG_QEMU_HELPERDIR.
>
> Checking F_OK (existence) instea of X_OK is odd.
>
> It uses /proc/self/exe to find the running executable's directory. This
> is specific to Linux[*]. You get different behavior on Linux vs. other
> systems.
>
> CONFIG_QEMU_HELPERDIR is $prefix/libexec/.
>
> If $prefix is /usr, then qemu-system-FOO is normally installed in
> /usr/bin/, and the helper in /usr/libexec/. We look for the helper in
> the wrong place first, and the right one only when it isn't in the wrong
> place. Feels overcomplicated and fragile.
>
> Consider the following scenario:
>
> * The system has a binary package's /usr/bin/qemu-system-x86_64 and
> /usr/libexec/qemu-ebpf-rss-helper installed
>
> * Alice builds her own QEMU with prefix /usr (and no intention to
> install), resulting in bld/qemu-system-x86_64, bld/qemu-ebpf-rss-path,
> and a symlink bld/x86_64-softmmu/qemu-system-x86_64.
>
> Now:
>
> * If Alice runs bld/qemu-system-x86_64, and the host is Linux,
> find_helper() returns bld/qemu-ebpf-rss-path. Good.
>
> * If the host isn't Linux, it returns /usr/libexec/qemu-ebpf-rss-helper.
> Not good.
>
> * If Alice runs bld/x86_64-softmmu/qemu-system-x86_64, it also returns
> /usr/libexec/qemu-ebpf-rss-helper. Not good.
>
> > +
> > +HelperPathList *qmp_query_helper_paths(Error **errp)
> > +{
> > + HelperPathList *ret = NULL;
> > + const char *helpers_list[] = {
> > +#ifdef CONFIG_EBPF
> > + "qemu-ebpf-rss-helper",
> > +#endif
> > + NULL
> > + };
> > + const char **helper_iter = helpers_list;
> > +
> > + for (; *helper_iter != NULL; ++helper_iter) {
> > + char *path = find_helper(*helper_iter);
> > + if (path) {
> > + HelperPath *helper = g_new0(HelperPath, 1);
> > + helper->name = g_strdup(*helper_iter);
> > + helper->path = path;
> > +
> > + QAPI_LIST_PREPEND(ret, helper);
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +#else
> > +
> > +HelperPathList *qmp_query_helper_paths(Error **errp)
> > +{
> > + return NULL;
> > +}
> > +
> > +#endif
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index 156f98203e..023bd2120d 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -519,3 +519,32 @@
> > 'data': { '*option': 'str' },
> > 'returns': ['CommandLineOptionInfo'],
> > 'allow-preconfig': true }
> > +
> > +##
> > +# @HelperPath:
> > +#
> > +# Name of the helper and binary location.
> > +##
> > +{ 'struct': 'HelperPath',
> > + 'data': {'name': 'str', 'path': 'str'} }
> > +
> > +##
> > +# @query-helper-paths:
> > +#
> > +# Query specific eBPF RSS helper for current qemu binary.
> > +#
> > +# Returns: list of object that contains name and path for helper.
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-helper-paths" }
> > +# <- { "return": [
> > +# {
> > +# "name": "qemu-ebpf-rss-helper",
> > +# "path": "/usr/local/libexec/qemu-ebpf-rss-helper"
> > +# }
> > +# ]
> > +# }
> > +#
> > +##
> > +{ 'command': 'query-helper-paths', 'returns': ['HelperPath'] }
>
> The name query-helper-paths is generic, the documented purpose "Query
> specific eBPF RSS helper" is specific.
>
> qemu-ebpf-rss-helper isn't necessarily the only helper that needs to be
> in sync with QEMU.
>
> I doubt a query command is a good way to help with using the right one.
> qemu-system-FOO doesn't really know where the right one is. Only the
> person or program that put them where they are does.
>
> If we want to ensure the right helper runs, we should use a build
> identifier compiled into the programs, like we do for modules.
>
> For modules, the program loading a module checks the module's build
> identifier matches its own.
>
> For programs talking to each other, the peers together check their build
> identifiers match.
>
> For programs where that isn't practical, the management application can
> check.
>
> This should be a lot more reliable.
>
> Helpers QEMU code runs itself should be run as
> CONFIG_QEMU_HELPERDIR/HELPER, with a suitable user override. This is
> how qemu-bridge-helper works.
>
> Helpers some other program runs are that other program's problem.
> They'll probably work the same: built-in default that can be overridden
> with configuration.
>
>
> [*] For detailed advice, see
>
> https://stackoverflow.com/questions/1023306/finding-current-executables-path-without-proc-self-exe
>
>
