Re: [libvirt] [PATCH v4 14/20] qemu: add vhost-user-gpu helper unit

On 9/13/19 8:50 AM, marcandre.lureau(a)redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; > > Similar to the qemu_tpm.c, add a unit with a few functions to > start/stop and setup the cgroup of the external vhost-user-gpu > process. See function documentation. > > The vhost-user connection fd is set on qemuDomainVideoPrivate struct. > > Signed-off-by: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; > --- > src/qemu/Makefile.inc.am | 2 + > src/qemu/qemu_domain.c | 5 +- > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_vhost_user_gpu.c | 276 +++++++++++++++++++++++++++++++++ > src/qemu/qemu_vhost_user_gpu.h | 49 ++++++ > 5 files changed, 332 insertions(+), 2 deletions(-) > create mode 100644 src/qemu/qemu_vhost_user_gpu.c > create mode 100644 src/qemu/qemu_vhost_user_gpu.h > > diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am > index 449550e64b..8040bf9366 100644 > --- a/src/qemu/Makefile.inc.am > +++ b/src/qemu/Makefile.inc.am > @@ -66,6 +66,8 @@ QEMU_DRIVER_SOURCES = \ > qemu/qemu_tpm.h \ > qemu/qemu_vhost_user.c \ > qemu/qemu_vhost_user.h \ > + qemu/qemu_vhost_user_gpu.c \ > + qemu/qemu_vhost_user_gpu.h \ > $(NULL) > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 9437694940..ab923aa917 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -1341,8 +1341,11 @@ qemuDomainVideoPrivateNew(void) > > > static void > -qemuDomainVideoPrivateDispose(void *obj ATTRIBUTE_UNUSED) > +qemuDomainVideoPrivateDispose(void *obj) > { > + qemuDomainVideoPrivatePtr priv = obj; > + > + VIR_FORCE_CLOSE(priv->vhost_user_fd); > } > > > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index 89bd77b3c0..8afb4993d3 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -509,7 +509,7 @@ typedef qemuDomainVideoPrivate *qemuDomainVideoPrivatePtr; > struct _qemuDomainVideoPrivate { > virObject parent; > > - bool tmp_to_remove; > + int vhost_user_fd; > }; > > > diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c > new file mode 100644 > index 0000000000..b259b58434 > --- /dev/null > +++ b/src/qemu/qemu_vhost_user_gpu.c > @@ -0,0 +1,276 @@ > +/* > + * qemu_vhost_user_gpu.c: QEMU vhost-user GPU support > + * > + * Copyright (C) 2019 Red Hat, Inc. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + */ > + > +#include <config.h> > + > +#include "qemu_vhost_user_gpu.h" > +#include "qemu_vhost_user.h" > +#include "qemu_extdevice.h" > + > +#include "conf/domain_conf.h" > +#include "configmake.h" > +#include "vircommand.h" > +#include "viralloc.h" > +#include "virlog.h" > +#include "virutil.h" > +#include "virfile.h" > +#include "virstring.h" > +#include "virtime.h" > +#include "virpidfile.h" > + > +#define VIR_FROM_THIS VIR_FROM_NONE > + > +VIR_LOG_INIT("qemu.vhost_user_gpu"); > + > + > +static char * > +qemuVhostUserGPUCreatePidFilename(const char *stateDir, > + const char *shortName, > + const char *alias) > +{ > + VIR_AUTOFREE(char *) devicename = NULL; > + > + if (virAsprintf(&devicename, "%s-%s-vhost-user-gpu", shortName, alias) < 0) > + return NULL; > + > + return virPidFileBuildPath(stateDir, devicename); > +} > + > + > +/* > + * qemuVhostUserGPUGetPid: > + * @binpath: path of executable associated with the pidfile > + * @stateDir: the directory where vhost-user-gpu writes the pidfile into > + * @shortName: short name of the domain > + * @alias: video device alias > + * @pid: pointer to pid > + * > + * Return -errno upon error, or zero on successful reading of the pidfile. > + * If the PID was not still alive, zero will be returned, and @pid will be > + * set to -1; > + */ > +static int > +qemuVhostUserGPUGetPid(const char *binPath, > + const char *stateDir, > + const char *shortName, > + const char *alias, > + pid_t *pid) > +{ > + VIR_AUTOFREE(char *) pidfile = NULL; > + > + pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias); > + if (!pidfile) > + return -ENOMEM; > + > + return virPidFileReadPathIfAlive(pidfile, pid, binPath); > +} > + > + > +int qemuExtVhostUserGPUPrepareDomain(virQEMUDriverPtr driver, > + virDomainVideoDefPtr video) > +{ > + return qemuVhostUserFillDomainGPU(driver, video); > +} > + > + > +/* > + * qemuExtVhostUserGPUStart: > + * @driver: QEMU driver > + * @vm: the VM domain > + * @video: the video device > + * > + * Start the external vhost-user-gpu process: > + * - open a socketpair for vhost-user communication > + * - have the command line built > + * - start the external process and sync with it before QEMU start > + */ > +int qemuExtVhostUserGPUStart(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainVideoDefPtr video) > +{ > + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); > + VIR_AUTOFREE(char *) shortname = NULL; > + VIR_AUTOFREE(char *) pidfile = NULL; > + VIR_AUTOPTR(virCommand) cmd = NULL; > + int pair[2] = { -1, -1 }; > + int cmdret = 0, rc; > + int exitstatus = 0; > + pid_t pid; > + int ret = -1; > + > + shortname = virDomainDefGetShortName(vm->def); > + if (!shortname) > + goto error; > + > + /* stop any left-over for this VM */ > + qemuExtVhostUserGPUStop(driver, vm, video); > + > + if (!(pidfile = qemuVhostUserGPUCreatePidFilename( > + cfg->stateDir, shortname, video->info.alias))) > + goto error; > + > + if (qemuSecuritySetSocketLabel(driver->securityManager, vm->def) < 0) > + goto error; > + > + if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) { > + virReportSystemError(errno, "%s", _("failed to create socket")); > + goto error; > + } > + > + if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0) > + goto error; > + > + cmd = virCommandNew(video->driver->vhost_user_binary); > + if (!cmd) > + goto error; > + > + virCommandClearCaps(cmd); > + virCommandSetPidFile(cmd, pidfile); > + virCommandDaemonize(cmd); > + > + if (qemuExtDeviceLogCommand(driver, vm, cmd, "vhost-user-gpu") < 0) > + goto error; > + Most, possibly all, of these 'goto error;' usages are overwriting detailed error messages with 'vhost-user-gpu failed to start'. So this needs to be reworked. You can use virGetLastErrorMessage if you wanted to prepend the vhost-user specific bit to the error. Maybe break up this function into chunks, like one to build the Command object

> + virCommandAddArgFormat(cmd, "--fd=%d", pair[0]); > + virCommandPassFD(cmd, pair[0], VIR_COMMAND_PASS_FD_CLOSE_PARENT); > + pair[0] = -1; > + > + if (video->accel) { > + if (video->accel->accel3d) > + virCommandAddArg(cmd, "--virgl"); > + > + if (video->accel->rendernode) > + virCommandAddArgFormat(cmd, "--render-node=%s", video->accel->rendernode); > + } > + > + if (qemuSecurityStartVhostUserGPU(driver, vm, cmd, > + &exitstatus, &cmdret) < 0) > + goto error; > + > + if (cmdret < 0 || exitstatus != 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not start 'vhost-user-gpu'. exitstatus: %d"), exitstatus); > + goto cleanup; > + } > + cmdret < 0 will trigger StartVhostUserGPU to return -1, so we will never hit one condition of the below case Otherwise this looks good. Since the series is pretty close otherwise, these bits can be addressed in a follow up patch IMO Reviewed-by: Cole Robinson <crobinso(a)redhat.com&gt; - Cole -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list