[libvirt] [PATCH v2 11/16] qemu: add vhost-user-gpu helper unit

[Cole Robinson]

On 8/23/19 12:21 PM, Cole Robinson wrote:
> From: Marc-André Lureau <marcandre.lureau at 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 at redhat.com>
> Signed-off-by: Cole Robinson <crobinso at 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