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

[John Ferlan]

On 05/30/2018 09:43 AM, Erik Skultety wrote:
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 24 +++++++++++++++-
>  .../hostdev-mdev-display-spice-no-opengl.args      | 32 ++++++++++++++++++++++
>  .../hostdev-mdev-display-spice-opengl.args         | 31 +++++++++++++++++++++
>  .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 ++++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           | 23 ++++++++++++++++
>  5 files changed, 141 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1667b09a8b..8a1a4dc72b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5088,6 +5088,10 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
>      virBufferAdd(&buf, dev_str, -1);
>      virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
>  
> +    if (mdevsrc->display)

> VIR_TRISTATE_SWITCH_ABSENT

> +        virBufferAsprintf(&buf, ",display=%s",
> +                          virTristateSwitchTypeToString(mdevsrc->display));
> +
>      if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
>          goto cleanup;
>  
> @@ -5305,7 +5309,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",
> @@ -5313,6 +5319,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>                                       "supported by this version of QEMU"));
>                      return -1;
>                  }
> +
> +                if (mdevsrc->display &&

!= 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;
> +                }
> +

After reading this, I thought perhaps patch 6 wasn't necessary at least
w/r/t that by setting off we write out to the config file which may not
be desired.

So as a way to avoid this, can we set a "local" @mdev_display value and
pass that qemuBuildHostdevMediatedDevStr which then would then either
ignore or write on/off on the command line.

Thus:

    if (mdevsrc->display > SWITCH_ABSENT && !VFIO_PCI_DISPLAY)
        error as is
    mdev_display = mdevsrc->display
    if (mdev_display == SWITCH_ABSENT && VFIO_PCI_DISPLAY)
        mdev_display == SWITCH_OFF;

And passing mdev_display to building the command line will print "off"
when mdevsrc->display == ABSENT, but we don't write "off" to the config
file.

??? thoughts

>                  break;
>              case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
>                  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
> @@ -5335,6 +5350,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>                  return -1;
>              virCommandAddArg(cmd, devstr);
>              VIR_FREE(devstr);
> +
> +            /* we need to add '-display egl-headless' if 'display=on' for
> +             * vfio-pci and OpenGL is disabled (either not supported or user
> +             * forgot to add 'gl=on' to SPICE or simply wants to use VNC...)
> +             */
> +            if (mdevsrc->display && !virDomainDefHasSpiceGL(def))

Doesn't this only matter if display == SWITCH_ON  ?

Hence the OCD on the "if (mdevsrc->display)" type checks. ;-)


John

[...]