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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux