From: Marc-André Lureau <marcandre.lureau@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@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;
+
+ 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;
+ }
+
+ rc = virPidFileReadPath(pidfile, &pid);
+ if (rc < 0) {
+ virReportSystemError(-rc,
+ _("Unable to read vhost-user-gpu pidfile '%s'"),
+ pidfile);
+ goto cleanup;
+ }
+
+ ret = 0;
+ QEMU_DOMAIN_VIDEO_PRIVATE(video)->vhost_user_fd = pair[1];
+ pair[1] = -1;
+
+ cleanup:
+ VIR_FORCE_CLOSE(pair[0]);
+ VIR_FORCE_CLOSE(pair[1]);
+
+ return ret;
+
+ error:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("vhost-user-gpu failed to start"));
+ goto cleanup;
+}
+
+
+/*
+ * qemuExtVhostUserGPUStop:
+ *
+ * @driver: QEMU driver
+ * @vm: the VM domain
+ * @video: the video device
+ *
+ * Check if vhost-user process pidfile is around, kill the process,
+ * and remove the pidfile.
+ */
+void qemuExtVhostUserGPUStop(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainVideoDefPtr video)
+{
+ VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
+ VIR_AUTOFREE(char *) pidfile = NULL;
+ VIR_AUTOFREE(char *) shortname = NULL;
+ virErrorPtr orig_err;
+
+ shortname = virDomainDefGetShortName(vm->def);
+ if (!(pidfile = qemuVhostUserGPUCreatePidFilename(
+ cfg->stateDir, shortname, video->info.alias))) {
+ VIR_WARN("Unable to construct vhost-user-gpu pidfile path");
+ return;
+ }
+
+ virErrorPreserveLast(&orig_err);
+ if (virPidFileForceCleanupPath(pidfile) < 0) {
+ VIR_WARN("Unable to kill vhost-user-gpu process");
+ } else {
+ if (unlink(pidfile) < 0 &&
+ errno != ENOENT) {
+ virReportSystemError(errno,
+ _("Unable to remove stale pidfile %s"),
+ pidfile);
+ }
+ }
+ virErrorRestore(&orig_err);
+}
+
+
+/*
+ * qemuExtVhostUserGPUSetupCgroup:
+ *
+ * @driver: QEMU driver
+ * @def: domain definition
+ * @video: the video device
+ * @cgroupe: a cgroup
+ *
+ * Add the vhost-user-gpu PID to the given cgroup.
+ */
+int
+qemuExtVhostUserGPUSetupCgroup(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainVideoDefPtr video,
+ virCgroupPtr cgroup)
+{
+ VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
+ VIR_AUTOFREE(char *) pidfile = NULL;
+ VIR_AUTOFREE(char *) shortname = NULL;
+ int rc;
+ pid_t pid;
+
+ shortname = virDomainDefGetShortName(def);
+ if (!shortname)
+ return -1;
+
+ rc = qemuVhostUserGPUGetPid(video->driver->vhost_user_binary,
+ cfg->stateDir, shortname, video->info.alias, &pid);
+ if (rc < 0 || (rc == 0 && pid == (pid_t)-1)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not get process id of vhost-user-gpu"));
+ return -1;
+ }
+ if (virCgroupAddProcess(cgroup, pid) < 0)
+ return -1;
+
+ return 0;
+}
diff --git a/src/qemu/qemu_vhost_user_gpu.h b/src/qemu/qemu_vhost_user_gpu.h
new file mode 100644
index 0000000000..40a8567630
--- /dev/null
+++ b/src/qemu/qemu_vhost_user_gpu.h
@@ -0,0 +1,49 @@
+/*
+ * qemu_vhost_user_gpu.h: 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/>.
+ */
+
+#pragma once
+
+#include "domain_conf.h"
+#include "qemu_domain.h"
+#include "qemu_security.h"
+
+int qemuExtVhostUserGPUPrepareDomain(virQEMUDriverPtr driver,
+ virDomainVideoDefPtr video)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+ ATTRIBUTE_RETURN_CHECK;
+
+int qemuExtVhostUserGPUStart(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainVideoDefPtr video)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
+
+void qemuExtVhostUserGPUStop(virQEMUDriverPtr driver,
+ virDomainObjPtr def,
+ virDomainVideoDefPtr video)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+int
+qemuExtVhostUserGPUSetupCgroup(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainVideoDefPtr video,
+ virCgroupPtr cgroup)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 9/13/19 8:50 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@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@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@redhat.com>
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi
On Sat, Sep 21, 2019 at 1:05 AM Cole Robinson <crobinso@redhat.com> wrote:
>
> On 9/13/19 8:50 AM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@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@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
It doesn't "overwrite" the detailed error, it merely adds another
virReportError(). Or am I missing something?
I'll leave that for now.
>
> > + 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@redhat.com>
>
> - Cole
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 23, 2019 at 02:16:37PM +0400, Marc-André Lureau wrote: >Hi > >On Sat, Sep 21, 2019 at 1:05 AM Cole Robinson <crobinso@redhat.com> wrote: >> >> On 9/13/19 8:50 AM, marcandre.lureau@redhat.com wrote: >> > From: Marc-André Lureau <marcandre.lureau@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@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 >> > [...] >> > + 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 > >It doesn't "overwrite" the detailed error, it merely adds another >virReportError(). Or am I missing something? > virReportError does two things: * logs an error into the configured log * sets the thread-local error variable The error variable can only store one error and this is what will get propagated to the API users. All previous errors logged via virReportError can only be found in the log. So ideally [0] one error path would only set one error. And vice-versa, once you log an error via virReportError, resetting it via virResetLastError will not unlog it, only reset the local error pointer. This function is intended to be used on public APIs entry points to make sure we don't have a leftover error from previous APIs. Jano >I'll leave that for now. > [0] Ideally to match this design. It might be sensible to report more than one error to the API users, but we do not have the infrastructure for that. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Sep 13, 2019 at 04:50:51PM +0400, marcandre.lureau@redhat.com wrote:
>From: Marc-André Lureau <marcandre.lureau@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@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
>
>+/*
>+ * qemuExtVhostUserGPUSetupCgroup:
>+ *
>+ * @driver: QEMU driver
>+ * @def: domain definition
>+ * @video: the video device
>+ * @cgroupe: a cgroup
>+ *
>+ * Add the vhost-user-gpu PID to the given cgroup.
>+ */
>+int
>+qemuExtVhostUserGPUSetupCgroup(virQEMUDriverPtr driver,
>+ virDomainDefPtr def,
>+ virDomainVideoDefPtr video,
>+ virCgroupPtr cgroup)
>+{
>+ VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
>+ VIR_AUTOFREE(char *) pidfile = NULL;
Clang fails to compile this:
qemu/qemu_vhost_user_gpu.c:256:26: error: unused variable 'pidfile' [-Werror,-Wunused-variable]
VIR_AUTOFREE(char *) pidfile = NULL;
^
1 error generated.
>+ VIR_AUTOFREE(char *) shortname = NULL;
>+ int rc;
>+ pid_t pid;
>+
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.