[libvirt] [PATCH v3 6/8] conf: Introduce new <hostdev> attribute 'display'

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:58:26PM +0200, Erik Skultety wrote:
>QEMU 2.12 introduced a new type of display for mediated devices using
>vfio-pci backend which allows a mediated device to be used as a VGA
>compatible device as an alternative to an emulated video device. QEMU
>exposes this feature via a vfio device property 'display' with supported
>values 'on/off/auto' (default is 'off').
>
>This patch adds the necessary bits to domain config handling in order to
>expose this feature. Since there's no convenient way for libvirt to come
>up with usable defaults for the display setting, simply because libvirt
>is not able to figure out which of the display implementations - dma-buf
>which requires OpenGL support vs vfio regions which doesn't need OpenGL
>(works with OpenGL enabled too) - the underlying mdev uses.
>
>Signed-off-by: Erik Skultety <eskultet at redhat.com>
>---
> docs/formatdomain.html.in                          | 20 ++++++--
> docs/schemas/domaincommon.rng                      |  5 ++
> src/conf/domain_conf.c                             | 19 +++++++-
> src/conf/domain_conf.h                             |  1 +
> src/qemu/qemu_domain.c                             | 55 ++++++++++++++++++++++
> tests/qemuxml2argvdata/hostdev-mdev-display.xml    | 39 +++++++++++++++
> .../hostdev-mdev-display-active.xml                | 47 ++++++++++++++++++
> tests/qemuxml2xmltest.c                            |  2 +
> 8 files changed, 184 insertions(+), 4 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display.xml
> create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml
>
>@@ -7752,6 +7754,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>                            model);
>             goto cleanup;
>         }
>+
>+        if (display &&
>+            (mdevsrc->display = virTristateSwitchTypeFromString(display)) <= 0) {
>+            virReportError(VIR_ERR_XML_ERROR,
>+                           _("unknown value '%s' for <hostdev> attribute "
>+                             "'display'"),
>+                           display);
>+            goto cleanup;
>+        }
>     }
>
>     switch (def->source.subsys.type) {

>diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>index 3deda1d978..8ca9558ceb 100644
>--- a/src/conf/domain_conf.h
>+++ b/src/conf/domain_conf.h
>@@ -382,6 +382,7 @@ typedef struct _virDomainHostdevSubsysMediatedDev virDomainHostdevSubsysMediated
> typedef virDomainHostdevSubsysMediatedDev *virDomainHostdevSubsysMediatedDevPtr;
> struct _virDomainHostdevSubsysMediatedDev {
>     int model;                          /* enum virMediatedDeviceModelType */
>+    int display; /* virTristateSwitch */

I was convinced that using the enum type here is the right way, but I've
had my illusions shattered.

If you decide to go that way anyway, don't forget to use a temporary int
variable in the parser, since the compiler can choose to make
virTristateSwitch unsinged and it won't be able to hold the negative
return value from virTristateSwitchTypeFromString.

>     char uuidstr[VIR_UUID_STRING_BUFLEN];   /* mediated device's uuid string */
> };
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 5da8c8bfcc..a03b1fb029 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -4450,10 +4450,47 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net)
> }
>
>
>+static int
>+qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
>+                          const virDomainDef *def)
>+{
>+    if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT &&
>+        mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
>+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                       _("<hostdev> attribute 'display' is only supported"
>+                         " with model='vfio-pci'"));
>+
>+        return -1;
>+    }
>+
>+    if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
>+        if (def->ngraphics == 0) {
>+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                           _("graphics device is needed for attribute value "
>+                             "'display=on' in <hostdev>"));
>+            return -1;
>+        }
>+
>+        /* We're not able to tell whether an mdev needs OpenGL or not at the
>+         * moment, so print a warning that an extra <gl> element or
>+         * <graphics type='egl-headless/>' might be necessary to be added,
>+         * depending on whether we're running with SPICE or VNC respectively.
>+         */
>+        if (!virDomainGraphicsDefHasOpenGL(def))
>+            VIR_WARN("<hostdev> attribute 'display' may need the OpenGL to "
>+                     "be enabled");

I'd suggest dropping the warning - nobody reads warnings.

>+    }
>+
>+    return 0;
>+}
>+
>+
> static int

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180713/ea15c872/attachment-0001.sig>