Message ID | 556480400e3d312cc3310fe27d04253d4d98f024.1566576129.git.crobinso@redhat.com |
---|---|
State | New |
Headers | show |
Series | Add vhost-user-gpu support | expand |
On 8/23/19 12:21 PM, Cole Robinson 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. > > Since the vhost-user connection fd isn't necessarily specific to QEMU, > it was easier to add it to virDomainDeviceInfo, although a reasonable > place could be qemuDomainObjPrivate (with an associate hashtable > device-info -> qemu-device-info for example). > Yeah stuffing those two fields in DeviceInfo is no good IMO but your idea sounds good to me. I think you can hash on info->alias which should be assigned at this point and will be unique for every device. > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Cole Robinson <crobinso@redhat.com> > --- > src/conf/device_conf.h | 1 + > src/qemu/Makefile.inc.am | 2 + > src/qemu/qemu_vhost_user_gpu.c | 305 +++++++++++++++++++++++++++++++++ > src/qemu/qemu_vhost_user_gpu.h | 50 ++++++ > 4 files changed, 358 insertions(+) > create mode 100644 src/qemu/qemu_vhost_user_gpu.c > create mode 100644 src/qemu/qemu_vhost_user_gpu.h > > diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h > index 0b0c525ad8..23cc2e9ba6 100644 > --- a/src/conf/device_conf.h > +++ b/src/conf/device_conf.h > @@ -180,6 +180,7 @@ struct _virDomainDeviceInfo { > * locking the isolation group */ > bool isolationGroupLocked; > char *vhost_user_binary; > + int vhost_user_fd; > }; > > void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); > diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am > index 18a9220d01..d394eab957 100644 > --- a/src/qemu/Makefile.inc.am > +++ b/src/qemu/Makefile.inc.am > @@ -62,6 +62,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_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c > new file mode 100644 > index 0000000000..0735af1473 > --- /dev/null > +++ b/src/qemu/qemu_vhost_user_gpu.c > @@ -0,0 +1,305 @@ > +/* > + * qemu_vhost_user_gpu.c: QEMU vhost-user GPU support > + * > + * Copyright (C) 2018 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"); > + For consistency, these should be underscores: qemu.vhost_user_gpu The code looks like a mix of qemu_tpm and scsi prmanager handling from qemu_process.c. There's definitely opportunities for code reuse but that can come after this series IMO rather than requiring it up front and slowing things down further. However you can convert a few things here to VIR_AUTOFREE and VIR_AUTOUNREF fairly easily. This and the tpm code use virCgroupAddProcess but the prmanager code uses virCgroupAddMachineProcess ... I don't really understand the distinction, they seem to be similar usecases, a helper process per VM. Something to explore maybe Giving some the public functions here an Ext prefix seems weird since that's not present in the filename at all. I realize that's what the TPM code does but I don't really get it there either Otherwise this seems to be following established patterns so no major comments from me - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Aug 23, 2019 at 12:21:55PM -0400, Cole Robinson 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. > >Since the vhost-user connection fd isn't necessarily specific to QEMU, >it was easier to add it to virDomainDeviceInfo, although a reasonable >place could be qemuDomainObjPrivate (with an associate hashtable >device-info -> qemu-device-info for example). > >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >Signed-off-by: Cole Robinson <crobinso@redhat.com> >--- > src/conf/device_conf.h | 1 + > src/qemu/Makefile.inc.am | 2 + > src/qemu/qemu_vhost_user_gpu.c | 305 +++++++++++++++++++++++++++++++++ > src/qemu/qemu_vhost_user_gpu.h | 50 ++++++ > 4 files changed, 358 insertions(+) > create mode 100644 src/qemu/qemu_vhost_user_gpu.c > create mode 100644 src/qemu/qemu_vhost_user_gpu.h > >diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h >index 0b0c525ad8..23cc2e9ba6 100644 >--- a/src/conf/device_conf.h >+++ b/src/conf/device_conf.h >@@ -180,6 +180,7 @@ struct _virDomainDeviceInfo { > * locking the isolation group */ > bool isolationGroupLocked; > char *vhost_user_binary; >+ int vhost_user_fd; > }; > > void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); >diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am >index 18a9220d01..d394eab957 100644 >--- a/src/qemu/Makefile.inc.am >+++ b/src/qemu/Makefile.inc.am >@@ -62,6 +62,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_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c >new file mode 100644 >index 0000000000..0735af1473 >--- /dev/null >+++ b/src/qemu/qemu_vhost_user_gpu.c >@@ -0,0 +1,305 @@ >+/* >+ * qemu_vhost_user_gpu.c: QEMU vhost-user GPU support >+ * >+ * Copyright (C) 2018 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) >+{ >+ char *pidfile = NULL; >+ char *devicename = NULL; >+ >+ if (virAsprintf(&devicename, "%s-%s-vhost-user-gpu", shortName, alias) < 0) >+ return NULL; >+ >+ pidfile = virPidFileBuildPath(stateDir, devicename); >+ >+ VIR_FREE(devicename); >+ >+ return pidfile; >+} >+ >+ >+/* >+ * 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) >+{ >+ int ret; >+ char *pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias); >+ if (!pidfile) >+ return -ENOMEM; >+ >+ ret = virPidFileReadPathIfAlive(pidfile, pid, binPath); >+ >+ VIR_FREE(pidfile); >+ >+ return ret; >+} >+ >+ >+int qemuExtVhostUserGPUPrepareDomain(virQEMUDriverPtr driver, >+ virDomainVideoDefPtr video) >+{ >+ return qemuVhostUserFillDomainGPU(driver, video); >+} >+ >+ >+/* >+ * qemuExtVhostUserGPUStart: >+ * @driver: QEMU driver >+ * @vm: the VM domain >+ * @video: the video device >+ * @logCtxt: log context >+ * >+ * 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, >+ qemuDomainLogContextPtr logCtxt) >+{ >+ int ret = -1; >+ virCommandPtr cmd = NULL; >+ int exitstatus = 0; >+ char *errbuf = NULL; >+ virQEMUDriverConfigPtr cfg; >+ char *pidfile = NULL, *shortName = virDomainDefGetShortName(vm->def); These should be on separate lines and shortName needs to be freed. Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 0b0c525ad8..23cc2e9ba6 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -180,6 +180,7 @@ struct _virDomainDeviceInfo { * locking the isolation group */ bool isolationGroupLocked; char *vhost_user_binary; + int vhost_user_fd; }; void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am index 18a9220d01..d394eab957 100644 --- a/src/qemu/Makefile.inc.am +++ b/src/qemu/Makefile.inc.am @@ -62,6 +62,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_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c new file mode 100644 index 0000000000..0735af1473 --- /dev/null +++ b/src/qemu/qemu_vhost_user_gpu.c @@ -0,0 +1,305 @@ +/* + * qemu_vhost_user_gpu.c: QEMU vhost-user GPU support + * + * Copyright (C) 2018 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) +{ + char *pidfile = NULL; + char *devicename = NULL; + + if (virAsprintf(&devicename, "%s-%s-vhost-user-gpu", shortName, alias) < 0) + return NULL; + + pidfile = virPidFileBuildPath(stateDir, devicename); + + VIR_FREE(devicename); + + return pidfile; +} + + +/* + * 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) +{ + int ret; + char *pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias); + if (!pidfile) + return -ENOMEM; + + ret = virPidFileReadPathIfAlive(pidfile, pid, binPath); + + VIR_FREE(pidfile); + + return ret; +} + + +int qemuExtVhostUserGPUPrepareDomain(virQEMUDriverPtr driver, + virDomainVideoDefPtr video) +{ + return qemuVhostUserFillDomainGPU(driver, video); +} + + +/* + * qemuExtVhostUserGPUStart: + * @driver: QEMU driver + * @vm: the VM domain + * @video: the video device + * @logCtxt: log context + * + * 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, + qemuDomainLogContextPtr logCtxt) +{ + int ret = -1; + virCommandPtr cmd = NULL; + int exitstatus = 0; + char *errbuf = NULL; + virQEMUDriverConfigPtr cfg; + char *pidfile = NULL, *shortName = virDomainDefGetShortName(vm->def); + virTimeBackOffVar timebackoff; + const unsigned long long timeout = 500 * 1000; /* ms */ + int cmdret = 0, rc; + int pair[2] = { -1, -1 }; + + pid_t pid; + + if (!shortName) + return -1; + + cfg = virQEMUDriverGetConfig(driver); + + /* 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 cleanup; + + 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 cleanup; + + cmd = virCommandNew(video->info.vhost_user_binary); + if (!cmd) + goto error; + + virCommandClearCaps(cmd); + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + + if (qemuExtDeviceLogCommand(logCtxt, 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, " + "error: %s"), exitstatus, errbuf); + goto cleanup; + } + + /* check that the helper has written its pid into the file */ + if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0) + goto error; + while (virTimeBackOffWait(&timebackoff)) { + rc = qemuVhostUserGPUGetPid(video->info.vhost_user_binary, + cfg->stateDir, shortName, video->info.alias, &pid); + if (rc < 0) + continue; + if (rc == 0 && pid == (pid_t)-1) + goto error; + break; + } + + ret = 0; + video->info.vhost_user_fd = pair[1]; + pair[1] = -1; + + cleanup: + VIR_FORCE_CLOSE(pair[0]); + VIR_FORCE_CLOSE(pair[1]); + virObjectUnref(cfg); + VIR_FREE(pidfile); + virCommandFree(cmd); + + 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) +{ + virErrorPtr orig_err; + char *pidfile, *shortName = virDomainDefGetShortName(vm->def); + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + 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); + + VIR_FREE(pidfile); +} + + +/* + * 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) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + char *pidfile = NULL; + char *shortName = NULL; + int ret = -1, rc; + pid_t pid; + + shortName = virDomainDefGetShortName(def); + if (!shortName) + goto cleanup; + + rc = qemuVhostUserGPUGetPid(video->info.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")); + goto cleanup; + } + if (virCgroupAddProcess(cgroup, pid) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(pidfile); + VIR_FREE(shortName); + virObjectUnref(cfg); + + return ret; +} diff --git a/src/qemu/qemu_vhost_user_gpu.h b/src/qemu/qemu_vhost_user_gpu.h new file mode 100644 index 0000000000..85054894fd --- /dev/null +++ b/src/qemu/qemu_vhost_user_gpu.h @@ -0,0 +1,50 @@ +/* + * qemu_vhost_user_gpu.h: QEMU vhost-user GPU support + * + * Copyright (C) 2018 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, + qemuDomainLogContextPtr logCtxt) + 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;