[libvirt] [PATCH v2 6/7] qemu: command: Enable formatting vfio-pci.display option onto cmdline

[Michal Privoznik]

On 07/09/2018 06:24 PM, Erik Skultety wrote:
> Since QEMU 2.12, QEMU understands a new vfio-pci device option 'display'
> which can be used to turn on display capabilities on vgpu-enabled
> mediated devices, IOW emulated GPU devices like QXL will no longer be
> needed with vgpu-enable mdevs.
> Since QEMU 2.12 defaults to 'auto' for the 'display' attribute, which
> often doesn't work as reliably, we need to play it safe here and
> explicitly format display='off' if this attribute wasn't provided in the
> XML explicitly.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 24 ++++++++++++-
>  .../hostdev-mdev-display-spice-egl-headless.args   | 32 +++++++++++++++++
>  .../hostdev-mdev-display-spice-egl-headless.xml    | 40 +++++++++++++++++++++
>  .../hostdev-mdev-display-spice-opengl.args         | 31 ++++++++++++++++
>  .../hostdev-mdev-display-spice-opengl.xml          | 41 ++++++++++++++++++++++
>  .../hostdev-mdev-display-vnc-egl-headless.args     | 32 +++++++++++++++++
>  .../hostdev-mdev-display-vnc-egl-headless.xml      | 40 +++++++++++++++++++++
>  .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 ++++++++++++++++
>  .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml  | 39 ++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           | 31 ++++++++++++++++
>  10 files changed, 340 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8026efbe87..6192253a9c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5178,6 +5178,17 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
>      virBufferAdd(&buf, dev_str, -1);
>      virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
>  
> +    /* QEMU 2.12 added support for vfio-pci display type, we default to
> +     * 'display=off', since QEMU defaults to 'auto' which is unreliable and
> +     * we don't want to risk any breakages */
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY) &&

Perhaps you forgot to include a commit that introduces this capability
into the series?

> +        mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
> +        mdevsrc->display = VIR_TRISTATE_SWITCH_OFF;

Isn't if ironic that @def is 'const virDomainDef*' but we are actually
changing it? I'm starting to understand why 'const' is pointless in C.
(read as not your fault, I just wanted to point this out)

> +
> +    if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT)

Err, s/>/!=/

> +        virBufferAsprintf(&buf, ",display=%s",
> +                          virTristateSwitchTypeToString(mdevsrc->display));
> +
>      if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
>          goto cleanup;
>  
> @@ -5395,7 +5406,9 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>  
>          /* MDEV */
>          if (virHostdevIsMdevDevice(hostdev)) {
> -            switch ((virMediatedDeviceModelType) subsys->u.mdev.model) {
> +            virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev;
> +
> +            switch ((virMediatedDeviceModelType) mdevsrc->model) {
>              case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
>                  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
>                      virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -5403,6 +5416,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>                                       "supported by this version of QEMU"));
>                      return -1;
>                  }
> +
> +                if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT &&
> +                    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                                   _("display property of device vfio-pci is "
> +                                     "not supported by this version of QEMU"));
> +                    return -1;
> +                }

Any reason why this is not checked for in
qemuBuildHostdevMediatedDevStr? Because with this check it is possible
to work around it - through hotplug code.

> +
>                  break;
>              case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
>                  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {

Michal