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