Hi
On Sat, Sep 21, 2019 at 1:05 AM Cole Robinson <crobinso@xxxxxxxxxx> wrote:
>
> On 9/13/19 8:50 AM, marcandre.lureau@xxxxxxxxxx wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> - Cole
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
