[libvirt] [PATCH v4 14/20] qemu: add vhost-user-gpu helper unit
Cole Robinson
crobinso at redhat.com
Fri Sep 20 21:05:18 UTC 2019
On 9/13/19 8:50 AM, marcandre.lureau at redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>
> 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 at redhat.com>
> ---
> 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 at redhat.com>
- Cole
More information about the libvir-list
mailing list