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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
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@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux