On 8/23/19 12:21 PM, Cole Robinson 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.
>
> 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@xxxxxxxxxx>
> Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
