Re: [libvirt] [PATCH v2 11/16] qemu: add vhost-user-gpu helper unit
On 8/23/19 12:21 PM, Cole Robinson wrote:
From: Marc-André Lureau <marcandre.lureau(a)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(a)redhat.com>
Signed-off-by: Cole Robinson <crobinso(a)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