[libvirt] [RFC PATCH 0/7] Enable vfio-pci 'property' for mediated devices
Since QEMU 2.12 there's a new vfio-pci device property 'display' with values on/off/auto. This special kind of display allows using a mediated device which is a VGA compatible device for a display output. There are 2 different implementations of how the device output is handled, referred to as dmabuf and vfio region mapping (currently NVIDIA uses the latter, while Intel relies on the former). From libvirt's perspective the important difference is that dmabuf requires OpenGL support whereas vfio regions don't (it will of course work even with OpenGL enabled). There's a catch though - because of several constraints in the vendor drivers (as discussed here [1]), there currently isn't a reasonable way for libvirt (other than spawning a dummy QEMU instance) to probe such mediated devices for the display mode they use. This the nr.1 reason why libvirt is not going to support the value 'auto' with QEMU and will default to 'off' instead in all cases to stay safe the least and therefore is going to rely on users being able to configure this properly otherwise they'll get an error. Once there's a way for libvirt to probe the nature of the display-capable mediated devices, we can consider adding support for 'auto' value meaning that libvirt is going to take care of adding an appropriate Spice/VNC graphics device depending on the system if these are missing in the config, otherwise the user's choice is always favoured. TL;DR: - we have a new attribute value for vfio-pci mediated devices called 'display' -> devices can now format this new 'display=on/off' property to the cmdline - if user enables the vfio display (display=on) but doesn't enable OpenGL for Spice, we automatically assume the usage of '-display egl-headless' (uses local drm nodes) which works both for Spice and VNC -> if OpenGL is enabled, then '-display egl-headless' is not necessary Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1475770 [1] https://www.redhat.com/archives/libvir-list/2018-May/msg00243.html Erik Skultety (7): conf: Remove a redundant model/address-type check in mdev post parse qemu: command: Move graphics iteration to its own function conf: Introduce virDomainDefHasSpiceGL helper conf: Introduce new <hostdev> attribute 'display' qemu: caps: Add vfio-pci.display capability qemu: domain: Set default vfio-pci display value depending on capability qemu: command: Enable formatting vfio-pci.display option onto cmdline docs/formatdomain.html.in | 16 ++++- docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 56 +++++++++++---- src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 84 +++++++++++++++------- src/qemu/qemu_domain.c | 75 +++++++++++++++++++ tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + .../hostdev-mdev-display-missing-graphics.xml | 35 +++++++++ .../hostdev-mdev-display-spice-no-opengl.args | 32 +++++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 41 +++++++++++ .../hostdev-mdev-display-spice-opengl.args | 31 ++++++++ .../hostdev-mdev-display-spice-opengl.xml | 41 +++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 +++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml | 39 ++++++++++ tests/qemuxml2argvtest.c | 23 ++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 47 ++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 48 +++++++++++++ .../hostdev-mdev-display-vnc.xml | 47 ++++++++++++ tests/qemuxml2xmltest.c | 3 + 25 files changed, 626 insertions(+), 41 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.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.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-no-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-vnc.xml -- 2.14.3
It's pointless to check the same thing multiple times. Fix the indentation along the way too. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 544f63a2a9..62bd17739c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4513,20 +4513,10 @@ virDomainHostdevDefPostParse(virDomainHostdevDefPtr dev, if (dev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) return 0; - if (model == VIR_MDEV_MODEL_TYPE_VFIO_PCI && - dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_XML_ERROR, - _("Unsupported address type '%s' with mediated " - "device model '%s'"), - virDomainDeviceAddressTypeToString(dev->info->type), - virMediatedDeviceModelTypeToString(model)); - return -1; - } - if ((model == VIR_MDEV_MODEL_TYPE_VFIO_PCI && - dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) || + dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) || (model == VIR_MDEV_MODEL_TYPE_VFIO_CCW && - dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) { + dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) { virReportError(VIR_ERR_XML_ERROR, _("Unsupported address type '%s' with mediated " "device model '%s'"), -- 2.14.3
On 05/30/2018 09:42 AM, Erik Skultety wrote:
It's pointless to check the same thing multiple times. Fix the indentation along the way too.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
It should be the command line helper who takes care of the iteration rather than the caller. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 60 +++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0b5ec4f2ba..1667b09a8b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5323,10 +5323,10 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, } break; case VIR_MDEV_MODEL_TYPE_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected vfio type '%d'"), subsys->u.mdev.model); + default: + virReportEnumRangeError(virMediatedDeviceModelType, + subsys->u.mdev.model); return -1; - break; } virCommandAddArg(cmd, "-device"); @@ -8042,26 +8042,41 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, static int qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virCommandPtr cmd, - virQEMUCapsPtr qemuCaps, - virDomainGraphicsDefPtr graphics) + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) { - switch (graphics->type) { - case VIR_DOMAIN_GRAPHICS_TYPE_SDL: - return qemuBuildGraphicsSDLCommandLine(cfg, cmd, qemuCaps, graphics); + size_t i; - case VIR_DOMAIN_GRAPHICS_TYPE_VNC: - return qemuBuildGraphicsVNCCommandLine(cfg, cmd, qemuCaps, graphics); + for (i = 0; i < def->ngraphics; i++) { + virDomainGraphicsDefPtr graphics = def->graphics[i]; - case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: - return qemuBuildGraphicsSPICECommandLine(cfg, cmd, qemuCaps, graphics); + switch (graphics->type) { + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + if (qemuBuildGraphicsSDLCommandLine(cfg, cmd, + qemuCaps, graphics) < 0) + return -1; - case VIR_DOMAIN_GRAPHICS_TYPE_RDP: - case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: - case VIR_DOMAIN_GRAPHICS_TYPE_LAST: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported graphics type '%s'"), - virDomainGraphicsTypeToString(graphics->type)); - return -1; + break; + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: + if (qemuBuildGraphicsVNCCommandLine(cfg, cmd, + qemuCaps, graphics) < 0) + return -1; + + break; + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + if (qemuBuildGraphicsSPICECommandLine(cfg, cmd, + qemuCaps, graphics) < 0) + return -1; + + break; + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + break; + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsType, graphics->type); + return -1; + } } return 0; @@ -10131,11 +10146,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildInputCommandLine(cmd, def, qemuCaps) < 0) goto error; - for (i = 0; i < def->ngraphics; ++i) { - if (qemuBuildGraphicsCommandLine(cfg, cmd, qemuCaps, - def->graphics[i]) < 0) - goto error; - } + if (qemuBuildGraphicsCommandLine(cfg, cmd, def, qemuCaps) < 0) + goto error; if (qemuBuildVideoCommandLine(cmd, def, qemuCaps) < 0) goto error; -- 2.14.3
On 05/30/2018 09:42 AM, Erik Skultety wrote:
It should be the command line helper who takes care of the iteration rather than the caller.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 60 +++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0b5ec4f2ba..1667b09a8b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5323,10 +5323,10 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, } break; case VIR_MDEV_MODEL_TYPE_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected vfio type '%d'"), subsys->u.mdev.model); + default: + virReportEnumRangeError(virMediatedDeviceModelType, + subsys->u.mdev.model); return -1; - break;
Separate and unrelated hunk... Reviewed-by: John Ferlan <jferlan@redhat.com> for a separated patch...
}
virCommandAddArg(cmd, "-device"); @@ -8042,26 +8042,41 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, static int qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virCommandPtr cmd, - virQEMUCapsPtr qemuCaps, - virDomainGraphicsDefPtr graphics) + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) { - switch (graphics->type) { - case VIR_DOMAIN_GRAPHICS_TYPE_SDL: - return qemuBuildGraphicsSDLCommandLine(cfg, cmd, qemuCaps, graphics); + size_t i;
- case VIR_DOMAIN_GRAPHICS_TYPE_VNC: - return qemuBuildGraphicsVNCCommandLine(cfg, cmd, qemuCaps, graphics); + for (i = 0; i < def->ngraphics; i++) { + virDomainGraphicsDefPtr graphics = def->graphics[i];
- case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: - return qemuBuildGraphicsSPICECommandLine(cfg, cmd, qemuCaps, graphics); + switch (graphics->type) { + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + if (qemuBuildGraphicsSDLCommandLine(cfg, cmd, + qemuCaps, graphics) < 0) + return -1;
- case VIR_DOMAIN_GRAPHICS_TYPE_RDP: - case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: - case VIR_DOMAIN_GRAPHICS_TYPE_LAST: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported graphics type '%s'"), - virDomainGraphicsTypeToString(graphics->type)); - return -1; + break; + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: + if (qemuBuildGraphicsVNCCommandLine(cfg, cmd, + qemuCaps, graphics) < 0) + return -1; + + break; + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + if (qemuBuildGraphicsSPICECommandLine(cfg, cmd, + qemuCaps, graphics) < 0) + return -1; + + break; + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + break;
Slight change from previous, but probably still worthy of the: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported graphics type '%s'"), virDomainGraphicsTypeToString(graphics->type)); return -1; since both are VirtualBox based and not QEMU. With that change, Reviewed-by: John Ferlan <jferlan@redhat.com> For this hunk. John
+ case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsType, graphics->type); + return -1; + } }
return 0; @@ -10131,11 +10146,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildInputCommandLine(cmd, def, qemuCaps) < 0) goto error;
- for (i = 0; i < def->ngraphics; ++i) { - if (qemuBuildGraphicsCommandLine(cfg, cmd, qemuCaps, - def->graphics[i]) < 0) - goto error; - } + if (qemuBuildGraphicsCommandLine(cfg, cmd, def, qemuCaps) < 0) + goto error;
if (qemuBuildVideoCommandLine(cmd, def, qemuCaps) < 0) goto error;
This helper will later help us to make corresponding changes when building QEMU cmdline, depending on what implementation of vfio-pci display should be used - dmabuf (requires OpenGL) vs vfio region mapping (doesn't need OpenGL). Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 28 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 62bd17739c..c868d8de08 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29927,3 +29927,27 @@ virDomainDefHasManagedPR(const virDomainDef *def) return false; } + + +/** + * virDomainDefHasSpiceGL: + * @def: domain definition + * + * Returns true if we have a SPICE graphic frame buffer with OpenGL enabled, + * false otherwise + */ +bool +virDomainDefHasSpiceGL(const virDomainDef *def) +{ + size_t i; + + for (i = 0; i < def->ngraphics; i++) { + virDomainGraphicsDefPtr gfx = def->graphics[i]; + + if (gfx->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && + gfx->data.spice.gl == VIR_TRISTATE_BOOL_YES) + return true; + } + + return false; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6cc8f8a29b..8493dfdd76 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3592,4 +3592,7 @@ virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard, bool virDomainDefHasManagedPR(const virDomainDef *def); +bool +virDomainDefHasSpiceGL(const virDomainDef *def); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6001635916..b18f2fd5ea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -278,6 +278,7 @@ virDomainDefHasDeviceAddress; virDomainDefHasManagedPR; virDomainDefHasMemballoon; virDomainDefHasMemoryHotplug; +virDomainDefHasSpiceGL; virDomainDefHasUSB; virDomainDefHasVcpusOffline; virDomainDefLifecycleActionAllowed; -- 2.14.3
On 05/30/2018 09:42 AM, Erik Skultety wrote:
This helper will later help us to make corresponding changes when building QEMU cmdline, depending on what implementation of vfio-pci display should be used - dmabuf (requires OpenGL) vs vfio region mapping (doesn't need OpenGL).
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 28 insertions(+)
Although this could move to the patch before it used (e.g. patch 7). Reviewed-by: John Ferlan <jferlan@redhat.com> John
QEMU introduced a new type of display for mediated devices using vfio-pci backend which controls whether a mediated device can be used as a native rendering device as an alternative to an emulated video device. This patch adds the necessary bits to domain config handling in order to expose this feature. It's worth noting that even though QEMU supports and defaults to the value 'auto', this patch only introduces values 'on' and 'off' defaulting to 'off' (for safety), since there's no convenient way for libvirt to come up with relevant defaults based on the fact, that libvirt doesn't know whether the underlying device uses dma-buf or vfio region mapping. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 16 ++++++-- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 18 +++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 43 +++++++++++++++++++ .../hostdev-mdev-display-missing-graphics.xml | 35 ++++++++++++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 41 ++++++++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 41 ++++++++++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml | 39 ++++++++++++++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 47 +++++++++++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 48 ++++++++++++++++++++++ .../hostdev-mdev-display-vnc.xml | 47 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 3 ++ 13 files changed, 380 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-no-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-vnc.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b5a6e33bfe..deecc3166a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4437,9 +4437,19 @@ guest. Currently, <code>model='vfio-pci'</code> and <code>model='vfio-ccw'</code> (<span class="since">Since 4.4.0</span>) is supported. Refer <a href="drvnodedev.html#MDEV">MDEV</a> to create - a mediated device on the host. There are also some implications on the - usage of guest's address type depending on the <code>model</code> - attribute, see the <code>address</code> element below. + a mediated device on the host. + <span class="since">Since 4.4.0 (QEMU 2.12)</span> libvirt added + a new optional <code>display</code> attribute to enable or disable + support for an accelerated remote desktop backed by a mediated + device (such as NVIDIA vGPU or Intel GVT-g) as an alternative to + emulated <a href="#elementsVideo">video devices</a>. This attribute + is limited to <code>model='vfio-pci'</code> only. Supported values + are either 'on' or 'off' (default is 'off'). + <p> + Note: There are also some implications on the usage of guest's + address type depending on the <code>model</code> attribute, + see the <code>address</code> element below. + </p> </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 703a1bb6f8..345bd3c757 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4507,6 +4507,11 @@ <value>vfio-ccw</value> </choice> </attribute> + <optional> + <attribute name="display"> + <ref name="virOnOff"/> + </attribute> + </optional> <element name="source"> <ref name="mdevaddress"/> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c868d8de08..7844b6d272 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7606,6 +7606,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, char *rawio = NULL; char *backendStr = NULL; char *model = NULL; + char *display = NULL; int backend; int ret = -1; virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci; @@ -7625,6 +7626,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, sgio = virXMLPropString(node, "sgio"); rawio = virXMLPropString(node, "rawio"); model = virXMLPropString(node, "model"); + display = virXMLPropString(node, "display"); /* @type is passed in from the caller rather than read from the * xml document, because it is specified in different places for @@ -7712,6 +7714,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, model); goto error; } + + if (display && + (mdevsrc->display = virTristateSwitchTypeFromString(display)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown value '%s' for <hostdev> attribute " + "'display'"), + display); + goto error; + } } switch (def->source.subsys.type) { @@ -26297,9 +26308,14 @@ virDomainHostdevDefFormat(virBufferPtr buf, virTristateBoolTypeToString(scsisrc->rawio)); } - if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) { virBufferAsprintf(buf, " model='%s'", virMediatedDeviceModelTypeToString(mdevsrc->model)); + if (mdevsrc->display) + virBufferAsprintf(buf, " display='%s'", + virTristateSwitchTypeToString(mdevsrc->display)); + } + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8493dfdd76..123a676ab9 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; /* virTristateSwitchType */ 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 2c51e4c0d8..27088d4456 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4277,10 +4277,35 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) } +static int +qemuDomainDeviceDefValidateHostdevMdev(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def) +{ + if (mdevsrc->display) { + if (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; + } else if (def->ngraphics == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("graphics device is needed for attribute value " + "'display=on' in <hostdev>")); + return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def) { + const virDomainHostdevSubsysMediatedDev *mdevsrc; + /* forbid capabilities mode hostdev in this kind of hypervisor */ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4290,6 +4315,24 @@ qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, return -1; } + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + mdevsrc = &hostdev->source.subsys.u.mdev; + return qemuDomainDeviceDefValidateHostdevMdev(mdevsrc, def); + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSubsysType, + hostdev->source.subsys.type); + return -1; + } + } + return 0; } diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml new file mode 100644 index 0000000000..ea559a6444 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + </controller> + <hostdev mode='subsystem' type='mdev' model='vfio-pci' display='on'> + <source> + <address uuid='53764d0e-85a0-42b4-af5c-2046b460b1dc'/> + </source> + </hostdev> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.xml new file mode 100644 index 0000000000..64a9d7b70c --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + </controller> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <hostdev mode='subsystem' type='mdev' model='vfio-pci' display='on'> + <source> + <address uuid='53764d0e-85a0-42b4-af5c-2046b460b1dc'/> + </source> + </hostdev> + <video> + <model type='qxl' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml new file mode 100644 index 0000000000..b7f2fdf63e --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + </controller> + <graphics type='spice'> + <gl enable='yes' rendernode='/dev/dri/foo'/> + </graphics> + <hostdev mode='subsystem' type='mdev' model='vfio-pci' display='on'> + <source> + <address uuid='53764d0e-85a0-42b4-af5c-2046b460b1dc'/> + </source> + </hostdev> + <video> + <model type='qxl' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml new file mode 100644 index 0000000000..f37e08e1b9 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + </controller> + <graphics type='vnc'/> + <hostdev mode='subsystem' type='mdev' model='vfio-pci' display='on'> + <source> + <address uuid='53764d0e-85a0-42b4-af5c-2046b460b1dc'/> + </source> + </hostdev> + <video> + <model type='qxl' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-no-opengl.xml b/tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-no-opengl.xml new file mode 100644 index 0000000000..ce56724629 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-no-opengl.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <video> + <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-pci' display='on'> + <source> + <address uuid='53764d0e-85a0-42b4-af5c-2046b460b1dc'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </hostdev> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-opengl.xml b/tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-opengl.xml new file mode 100644 index 0000000000..6848213a1c --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-opengl.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice'> + <listen type='none'/> + <gl enable='yes' rendernode='/dev/dri/foo'/> + </graphics> + <video> + <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-pci' display='on'> + <source> + <address uuid='53764d0e-85a0-42b4-af5c-2046b460b1dc'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </hostdev> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-mdev-display-vnc.xml b/tests/qemuxml2xmloutdata/hostdev-mdev-display-vnc.xml new file mode 100644 index 0000000000..94c11b1199 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-mdev-display-vnc.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes'> + <listen type='address'/> + </graphics> + <video> + <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-pci' display='on'> + <source> + <address uuid='53764d0e-85a0-42b4-af5c-2046b460b1dc'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </hostdev> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 5671114de0..f9a9967a7c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -477,6 +477,9 @@ mymain(void) DO_TEST("hostdev-pci-address", NONE); DO_TEST("hostdev-vfio", NONE); DO_TEST("hostdev-mdev-precreated", NONE); + DO_TEST("hostdev-mdev-display-spice-opengl", NONE); + DO_TEST("hostdev-mdev-display-spice-no-opengl", NONE); + DO_TEST("hostdev-mdev-display-vnc", NONE); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE); -- 2.14.3
On 05/30/2018 09:42 AM, Erik Skultety wrote:
QEMU introduced a new type of display for mediated devices using vfio-pci backend which controls whether a mediated device can be used as a native rendering device as an alternative to an emulated video device. This patch adds the necessary bits to domain config handling in order to expose this feature.
Add blank line between paragraphs
It's worth noting that even though QEMU supports and defaults to the value 'auto', this patch only introduces values 'on' and 'off' defaulting to 'off' (for safety), since there's no convenient way for libvirt to come up with relevant defaults based on the fact, that libvirt doesn't know whether the underlying device uses dma-buf or vfio region mapping.
Such a strange feature - almost seems like qemu tries to enable by default by using auto and now it's up to libvirt to say, no, set the default to off and if someone then turns it on make sure the "expected" graphics is available in order to avoid issues. Perhaps if you drag in some of the comments from the cover letter it may clear up a few things... The TL;DR in particular and part of the paragraph just above it. Looking at the QEMU code it seems there is the expectation of essentially "on" or "off" - auto just seems to be a way to have a default value and try to force usage of on or off... It seems to me that vfio_realize will check the value != ON_OFF_AUTO_OFF(2), then do the probe. The "default" for the property is set as ON_OFF_AUTO_AUTO (or 0), so perhaps since 2.12 there's always an attempt to do this. Of course I could be reading things wrong. The assumption in that code being that the "user" of the qemu api would "know" to add the right graphics device. Mind boggling.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 16 ++++++-- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 18 +++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 43 +++++++++++++++++++ .../hostdev-mdev-display-missing-graphics.xml | 35 ++++++++++++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 41 ++++++++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 41 ++++++++++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml | 39 ++++++++++++++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 47 +++++++++++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 48 ++++++++++++++++++++++ .../hostdev-mdev-display-vnc.xml | 47 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 3 ++ 13 files changed, 380 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-no-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-vnc.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b5a6e33bfe..deecc3166a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4437,9 +4437,19 @@ guest. Currently, <code>model='vfio-pci'</code> and <code>model='vfio-ccw'</code> (<span class="since">Since 4.4.0</span>) is supported. Refer <a href="drvnodedev.html#MDEV">MDEV</a> to create - a mediated device on the host. There are also some implications on the - usage of guest's address type depending on the <code>model</code> - attribute, see the <code>address</code> element below. + a mediated device on the host. + <span class="since">Since 4.4.0 (QEMU 2.12)</span> libvirt added
4.5.0 s/libvirt added a new/an/ s/attribute/attribute may be used/
+ a new optional <code>display</code> attribute to enable or disable + support for an accelerated remote desktop backed by a mediated + device (such as NVIDIA vGPU or Intel GVT-g) as an alternative to + emulated <a href="#elementsVideo">video devices</a>. This attribute + is limited to <code>model='vfio-pci'</code> only. Supported values + are either 'on' or 'off' (default is 'off').
Usage of this option requires ???which types/styles??? of graphics devices to be defined for the domain.
+ <p> + Note: There are also some implications on the usage of guest's + address type depending on the <code>model</code> attribute, + see the <code>address</code> element below. + </p> </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 703a1bb6f8..345bd3c757 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4507,6 +4507,11 @@ <value>vfio-ccw</value> </choice> </attribute> + <optional> + <attribute name="display"> + <ref name="virOnOff"/> + </attribute> + </optional> <element name="source"> <ref name="mdevaddress"/> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c868d8de08..7844b6d272 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7606,6 +7606,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, char *rawio = NULL; char *backendStr = NULL; char *model = NULL; + char *display = NULL; int backend; int ret = -1; virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci; @@ -7625,6 +7626,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, sgio = virXMLPropString(node, "sgio"); rawio = virXMLPropString(node, "rawio"); model = virXMLPropString(node, "model"); + display = virXMLPropString(node, "display");
/* @type is passed in from the caller rather than read from the * xml document, because it is specified in different places for @@ -7712,6 +7714,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, model); goto error; } + + if (display && + (mdevsrc->display = virTristateSwitchTypeFromString(display)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown value '%s' for <hostdev> attribute " + "'display'"), + display); + goto error; + } }
switch (def->source.subsys.type) { @@ -26297,9 +26308,14 @@ virDomainHostdevDefFormat(virBufferPtr buf, virTristateBoolTypeToString(scsisrc->rawio)); }
- if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) { virBufferAsprintf(buf, " model='%s'", virMediatedDeviceModelTypeToString(mdevsrc->model)); + if (mdevsrc->display)
->display > VIR_TRISTATE_SWITCH_ABSENT
+ virBufferAsprintf(buf, " display='%s'", + virTristateSwitchTypeToString(mdevsrc->display)); + } + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2);
What about a virDomainHostdevDefCheckABIStability check that display type didn't change?
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8493dfdd76..123a676ab9 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; /* virTristateSwitchType */ 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 2c51e4c0d8..27088d4456 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4277,10 +4277,35 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) }
+static int +qemuDomainDeviceDefValidateHostdevMdev(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def) +{ + if (mdevsrc->display) {
VIR_TRISTATE_SWITCH_ABSENT
+ if (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; + } else if (def->ngraphics == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("graphics device is needed for attribute value " + "'display=on' in <hostdev>"));
Hmmm.. not sure we noted that in the formatdomain - probably should. Also if this were a PostParse check, then would the xml2xml tests fail if no graphics were defined (e.g. hostdev-mdev-display-missing-graphics.xml)
+ return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def) { + const virDomainHostdevSubsysMediatedDev *mdevsrc; + /* forbid capabilities mode hostdev in this kind of hypervisor */ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4290,6 +4315,24 @@ qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, return -1; }
+ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + mdevsrc = &hostdev->source.subsys.u.mdev; + return qemuDomainDeviceDefValidateHostdevMdev(mdevsrc, def); + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSubsysType, + hostdev->source.subsys.type); + return -1; + } + } + return 0; }
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml new file mode 100644 index 0000000000..ea559a6444 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
This one is not used until the last patch... It could be moved there or as I noted above, would moving the no graphics check to PostParse allow xml2xml failure of some sort (I cannot remember any more and it's too late for me to dig). The rest of the tests seem OK to me... [...] I think for the most part this is OK - just curious about the ABI check, the test or PostParse check, and making sure I read things right before the R-b.. John
On Mon, Jun 04, 2018 at 07:39:00PM -0400, John Ferlan wrote:
On 05/30/2018 09:42 AM, Erik Skultety wrote:
QEMU introduced a new type of display for mediated devices using vfio-pci backend which controls whether a mediated device can be used as a native rendering device as an alternative to an emulated video device. This patch adds the necessary bits to domain config handling in order to expose this feature.
Add blank line between paragraphs
It's worth noting that even though QEMU supports and defaults to the value 'auto', this patch only introduces values 'on' and 'off' defaulting to 'off' (for safety), since there's no convenient way for libvirt to come up with relevant defaults based on the fact, that libvirt doesn't know whether the underlying device uses dma-buf or vfio region mapping.
Such a strange feature - almost seems like qemu tries to enable by default by using auto and now it's up to libvirt to say, no, set the default to off and if someone then turns it on make sure the "expected" graphics is available in order to avoid issues.
QEMU has already realized that and they're trying to change it to 'off' too, see [1].
Perhaps if you drag in some of the comments from the cover letter it may clear up a few things... The TL;DR in particular and part of the paragraph just above it.
Noted, I will do that.
Looking at the QEMU code it seems there is the expectation of essentially "on" or "off" - auto just seems to be a way to have a default value and try to force usage of on or off... It seems to me that vfio_realize will check the value != ON_OFF_AUTO_OFF(2), then do the probe. The "default" for the property is set as ON_OFF_AUTO_AUTO (or 0), so perhaps since 2.12 there's always an attempt to do this. Of course I could be reading things wrong. The assumption in that code being that the "user" of the qemu api would "know" to add the right graphics device. Mind boggling.
It doesn't really work reliably even if QEMU has much more info about the device that libvirt could possibly have and there's still the factor that you can't predict user's intention with the device, so 'auto' is not really the smartest default option in this case (given the 3rd party vendors and 2 different approaches), hence the 'off' default for libvirt, 'auto' would be IMHO too tricky and dangerous in terms of guaranteeing backwards compatibility. ...
+ a new optional <code>display</code> attribute to enable or disable + support for an accelerated remote desktop backed by a mediated + device (such as NVIDIA vGPU or Intel GVT-g) as an alternative to + emulated <a href="#elementsVideo">video devices</a>. This attribute + is limited to <code>model='vfio-pci'</code> only. Supported values + are either 'on' or 'off' (default is 'off').
Usage of this option requires ???which types/styles??? of graphics devices to be defined for the domain.
Right, worth mentioning that currently only works for QEMU+vnc/spice combo, thanks. ...
+ <p> + Note: There are also some implications on the usage of guest's + address type depending on the <code>model</code> attribute, + see the <code>address</code> element below. + </p> </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 703a1bb6f8..345bd3c757 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4507,6 +4507,11 @@ <value>vfio-ccw</value> </choice> </attribute> + <optional> + <attribute name="display"> + <ref name="virOnOff"/> + </attribute> + </optional> <element name="source"> <ref name="mdevaddress"/> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c868d8de08..7844b6d272 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7606,6 +7606,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, char *rawio = NULL; char *backendStr = NULL; char *model = NULL; + char *display = NULL; int backend; int ret = -1; virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci; @@ -7625,6 +7626,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, sgio = virXMLPropString(node, "sgio"); rawio = virXMLPropString(node, "rawio"); model = virXMLPropString(node, "model"); + display = virXMLPropString(node, "display");
/* @type is passed in from the caller rather than read from the * xml document, because it is specified in different places for @@ -7712,6 +7714,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, model); goto error; } + + if (display && + (mdevsrc->display = virTristateSwitchTypeFromString(display)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown value '%s' for <hostdev> attribute " + "'display'"), + display); + goto error; + } }
switch (def->source.subsys.type) { @@ -26297,9 +26308,14 @@ virDomainHostdevDefFormat(virBufferPtr buf, virTristateBoolTypeToString(scsisrc->rawio)); }
- if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) { virBufferAsprintf(buf, " model='%s'", virMediatedDeviceModelTypeToString(mdevsrc->model)); + if (mdevsrc->display)
->display > VIR_TRISTATE_SWITCH_ABSENT
+ virBufferAsprintf(buf, " display='%s'", + virTristateSwitchTypeToString(mdevsrc->display)); + } + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2);
What about a virDomainHostdevDefCheckABIStability check that display type didn't change?
I'm sorry, I'm failing to see how it could change, the way I designed it is that whenever QEMU supports the capability we always default to 'off' which means that it'll always get formatted explicitly as 'off', even if the attribute was missing in the source XML. The only problem would an older libvirt who doesn't know what to do with that attribute, so it would ignore it which could cause issues with a newer QEMU without the patch linked below, but the ABI stability check wouldn't help anyway.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8493dfdd76..123a676ab9 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; /* virTristateSwitchType */ 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 2c51e4c0d8..27088d4456 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4277,10 +4277,35 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) }
+static int +qemuDomainDeviceDefValidateHostdevMdev(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def) +{ + if (mdevsrc->display) {
VIR_TRISTATE_SWITCH_ABSENT
+ if (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; + } else if (def->ngraphics == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("graphics device is needed for attribute value " + "'display=on' in <hostdev>"));
Hmmm.. not sure we noted that in the formatdomain - probably should.
Also if this were a PostParse check, then would the xml2xml tests fail if no graphics were defined (e.g. hostdev-mdev-display-missing-graphics.xml)
Right, good point, I preferred validation instead of post parse so as not to risk 'loosing' a domain, but it doesn't make any sense wanting a display and not having any graphical framebuffer, I'll change that.
+ return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def) { + const virDomainHostdevSubsysMediatedDev *mdevsrc; + /* forbid capabilities mode hostdev in this kind of hypervisor */ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4290,6 +4315,24 @@ qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, return -1; }
+ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + mdevsrc = &hostdev->source.subsys.u.mdev; + return qemuDomainDeviceDefValidateHostdevMdev(mdevsrc, def); + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSubsysType, + hostdev->source.subsys.type); + return -1; + } + } + return 0; }
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml new file mode 100644 index 0000000000..ea559a6444 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
This one is not used until the last patch... It could be moved there or as I noted above, would moving the no graphics check to PostParse allow xml2xml failure of some sort (I cannot remember any more and it's too late for me to dig).
Sure, I'll make the PostParse change. [1] https://www.redhat.com/archives/libvir-list/2018-May/msg02128.html Erik
[...] if (mdevsrc->display)
->display > VIR_TRISTATE_SWITCH_ABSENT
+ virBufferAsprintf(buf, " display='%s'", + virTristateSwitchTypeToString(mdevsrc->display)); + } + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2);
What about a virDomainHostdevDefCheckABIStability check that display type didn't change?
I'm sorry, I'm failing to see how it could change, the way I designed it is that whenever QEMU supports the capability we always default to 'off' which means that it'll always get formatted explicitly as 'off', even if the attribute was missing in the source XML. The only problem would an older libvirt who doesn't know what to do with that attribute, so it would ignore it which could cause issues with a newer QEMU without the patch linked below, but the ABI stability check wouldn't help anyway.
OK - The ABI stuff is sometimes forgotten and it's just one of those things I like to make sure about. Seems like we're good without it.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8493dfdd76..123a676ab9 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; /* virTristateSwitchType */ 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 2c51e4c0d8..27088d4456 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4277,10 +4277,35 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) }
+static int +qemuDomainDeviceDefValidateHostdevMdev(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def) +{ + if (mdevsrc->display) {
VIR_TRISTATE_SWITCH_ABSENT
+ if (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; + } else if (def->ngraphics == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("graphics device is needed for attribute value " + "'display=on' in <hostdev>"));
Hmmm.. not sure we noted that in the formatdomain - probably should.
Also if this were a PostParse check, then would the xml2xml tests fail if no graphics were defined (e.g. hostdev-mdev-display-missing-graphics.xml)
Right, good point, I preferred validation instead of post parse so as not to risk 'loosing' a domain, but it doesn't make any sense wanting a display and not having any graphical framebuffer, I'll change that.
When you're adding new options, going with PostParse I think is fine. The Validate pass was added to catch those cases where we found something after introduction of an option but forgot to check validity so we have to check "later" so we don't have a domain go missing. John [...]
QEMU 2.12 introduced a new vfio-pci device option 'display=on/off/auto'. Initially, libvirt is only going to support values on/off only, as we don't want to predict what the intended usage of the mediated device will be and most importantly, what kind of vfio display implementation to use - dmabuf vs vfio region mapping as there's no convenient way for us to find out what implementation the device's underlying driver supports. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + 6 files changed, 7 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e2e76e4dd8..7c08991103 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -493,6 +493,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 305 */ "vhost-vsock", + "vfio-pci.display", ); @@ -1179,6 +1180,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsPCIAssign[] = { static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVfioPCI[] = { { "bootindex", QEMU_CAPS_VFIO_PCI_BOOTINDEX }, + { "display", QEMU_CAPS_VFIO_PCI_DISPLAY }, }; static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsSCSIDisk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f2aecefb9b..07cbe58789 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -477,6 +477,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 305 */ QEMU_CAPS_DEVICE_VHOST_VSOCK, /* -device vhost-vsock-* */ + QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index f511bcb58c..3dba3e3b27 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -167,6 +167,7 @@ <flag name='hda-output'/> <flag name='blockdev-del'/> <flag name='vhost-vsock'/> + <flag name='vfio-pci.display'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>343099</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 37813ad0b1..13634db45f 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -164,6 +164,7 @@ <flag name='hda-output'/> <flag name='blockdev-del'/> <flag name='vhost-vsock'/> + <flag name='vfio-pci.display'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>419968</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 3191e9ba95..5ff96afc86 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -131,6 +131,7 @@ <flag name='screendump_device'/> <flag name='blockdev-del'/> <flag name='vhost-vsock'/> + <flag name='vfio-pci.display'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>371055</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 964b9e8fa6..5aa6b2bc3d 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -206,6 +206,7 @@ <flag name='blockdev-del'/> <flag name='vmgenid'/> <flag name='vhost-vsock'/> + <flag name='vfio-pci.display'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>390813</microcodeVersion> -- 2.14.3
On 05/30/2018 09:42 AM, Erik Skultety wrote:
QEMU 2.12 introduced a new vfio-pci device option 'display=on/off/auto'. Initially, libvirt is only going to support values on/off only, as we don't want to predict what the intended usage of the mediated device will be and most importantly, what kind of vfio display implementation to use - dmabuf vs vfio region mapping as there's no convenient way for us to find out what implementation the device's underlying driver supports.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + 6 files changed, 7 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
If QEMU supports vfio-pci.display option, we default to 'off' as we're not trying to guess what a user's intentions with the mdev are. Perform this decision as part of driver-specific post parse callback. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 27088d4456..b9f152022b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5887,6 +5887,35 @@ qemuDomainVsockDefPostParse(virDomainVsockDefPtr vsock) } +static int +qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc, + virQEMUCapsPtr qemuCaps) +{ + /* QEMU 2.12 added support for vfio-pci display type, we default to + * 'display=off' to stay safe from future changes */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY) && + mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + mdevsrc->display = VIR_TRISTATE_SWITCH_OFF; + + return 0; +} + + +static int +qemuDomainHostdevDefPostParse(virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps) +{ + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV && + qemuDomainHostdevDefMdevPostParse(&subsys->u.mdev, qemuCaps) < 0) + return -1; + + return 0; +} + + static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5942,6 +5971,9 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_HOSTDEV: + ret = qemuDomainHostdevDefPostParse(dev->data.hostdev, qemuCaps); + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: -- 2.14.3
On 05/30/2018 09:43 AM, Erik Skultety wrote:
If QEMU supports vfio-pci.display option, we default to 'off' as we're not trying to guess what a user's intentions with the mdev are. Perform this decision as part of driver-specific post parse callback.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 27088d4456..b9f152022b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5887,6 +5887,35 @@ qemuDomainVsockDefPostParse(virDomainVsockDefPtr vsock) }
+static int +qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc, + virQEMUCapsPtr qemuCaps) +{ + /* QEMU 2.12 added support for vfio-pci display type, we default to + * 'display=off' to stay safe from future changes */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY) && + mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + mdevsrc->display = VIR_TRISTATE_SWITCH_OFF;
This seems to affirm my reading of the QEMU code from my patch 4 comments where display defaulted to auto which called vfio_realize for every domain... Still this then writes "off" out to any configuration that had/used mdev, right? But if it writes "off", then the parameter has a quasi optional state that we can never change to auto in the future since we'll find off and won't know it it's off because someone set it off or if it's off because we set it that way. So I think there's a way to do what you want - see comments in patch 7... John
+ + return 0; +} + + +static int +qemuDomainHostdevDefPostParse(virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps) +{ + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV && + qemuDomainHostdevDefMdevPostParse(&subsys->u.mdev, qemuCaps) < 0) + return -1; + + return 0; +} + + static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5942,6 +5971,9 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_HOSTDEV: + ret = qemuDomainHostdevDefPostParse(dev->data.hostdev, qemuCaps); + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB:
Signed-off-by: Erik Skultety <eskultet@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) + 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 && + !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; + } + 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)) + virCommandAddArgList(cmd, "-display", "egl-headless", NULL); } } diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args new file mode 100644 index 0000000000..b3fe29cbf4 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args @@ -0,0 +1,32 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest2 \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-spice port=0 \ +-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,\ +addr=0x2 \ +-device vfio-pci,id=hostdev0,\ +sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,display=on,\ +bus=pci.0,addr=0x3 \ +-display egl-headless diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args new file mode 100644 index 0000000000..170b4e4ed0 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest2 \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-spice port=0,gl=on,rendernode=/dev/dri/foo \ +-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,\ +addr=0x2 \ +-device vfio-pci,id=hostdev0,\ +sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,display=on,\ +bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args new file mode 100644 index 0000000000..126ac8e6a6 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args @@ -0,0 +1,32 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest2 \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-vnc 127.0.0.1:0 \ +-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,\ +addr=0x2 \ +-device vfio-pci,id=hostdev0,\ +sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,display=on,\ +bus=pci.0,addr=0x3 \ +-display egl-headless diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ddd2b88c0a..9cd92fbfbe 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1557,6 +1557,29 @@ mymain(void) QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST_PARSE_ERROR("hostdev-mdev-invalid-target-address", QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST("hostdev-mdev-display-spice-opengl", + QEMU_CAPS_SPICE, + QEMU_CAPS_SPICE_GL, + QEMU_CAPS_SPICE_RENDERNODE, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_VFIO_PCI_DISPLAY); + DO_TEST("hostdev-mdev-display-spice-no-opengl", + QEMU_CAPS_SPICE, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_VFIO_PCI_DISPLAY); + DO_TEST("hostdev-mdev-display-vnc", + QEMU_CAPS_VNC, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_VFIO_PCI_DISPLAY); + DO_TEST_PARSE_ERROR("hostdev-mdev-display-missing-graphics", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_VFIO_PCI_DISPLAY); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE); -- 2.14.3
On 05/30/2018 09:43 AM, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@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 [...]
On Mon, Jun 04, 2018 at 07:40:42PM -0400, John Ferlan wrote:
On 05/30/2018 09:43 AM, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@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
So, w/r/t comments in patch 6, I understand that we'd keep an open door to changing the default (no display attr means use 'auto' as it would in most cases in libvirt). However, I don't believe that we'd want to make 'auto' a default ever because you can't predict whether the user wants to use the device as GPGPU or graphics, if they leave the display attr unset and we'd default to auto in the future, we might risk that they know they can't use it for graphics and they didn't even intend to, but we tried and failed because of some underlying reasons. And I'm not even sure we'd ever introduce 'auto' to libvirt for reasons I just mentioned + the cover letter and the discussion that happened upstream regarding the VNC console. What I know though, is that libvirt is never going to use QEMU's 'auto', since it doesn't make sense, either we take care of everything or the user does, regardless of QEMU. Anyway, the way I imagine 'auto' could work in libvirt is that user references an mdev, but doesn't care (or doesn't know) what technology it uses underneath and explicitly says "libvirt please take care of that on my behalf, I want to use the display, but have 0 clue on what to enable". That's a different kind of 'auto' compared to when we'd try to read user's mind. I tried to play it very safe here, as the vgpu display thing can be very tricky, not even thinking about future migrations. But if there's a strong disagreement with this kind of approach, then I'll of course need to incorporate your comments in patch 6 and this one to get this successfully merged. Erik
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. ;-)
Will fix this. Thanks, Erik
On 06/08/2018 07:09 AM, Erik Skultety wrote:
On Mon, Jun 04, 2018 at 07:40:42PM -0400, John Ferlan wrote:
On 05/30/2018 09:43 AM, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@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
So, w/r/t comments in patch 6, I understand that we'd keep an open door to changing the default (no display attr means use 'auto' as it would in most cases in libvirt). However, I don't believe that we'd want to make 'auto' a default ever because you can't predict whether the user wants to use the device as GPGPU or graphics, if they leave the display attr unset and we'd default to auto in the future, we might risk that they know they can't use it for graphics and they didn't even intend to, but we tried and failed because of some underlying reasons. And I'm not even sure we'd ever introduce 'auto' to libvirt for reasons I just mentioned + the cover letter and the discussion that happened upstream regarding the VNC console. What I know though, is that libvirt is never going to use QEMU's 'auto', since it doesn't make sense, either we take care of everything or the user does, regardless of QEMU.
Anyway, the way I imagine 'auto' could work in libvirt is that user references an mdev, but doesn't care (or doesn't know) what technology it uses underneath and explicitly says "libvirt please take care of that on my behalf, I want to use the display, but have 0 clue on what to enable". That's a different kind of 'auto' compared to when we'd try to read user's mind. I tried to play it very safe here, as the vgpu display thing can be very tricky, not even thinking about future migrations. But if there's a strong disagreement with this kind of approach, then I'll of course need to incorporate your comments in patch 6 and this one to get this successfully merged.
Erik
I didn't think I was advocating for adding AUTO - just blabbering how insane the code is ;-). IIRC, if "off" isn't written then auto is assumed. What I thought I was suggesting here though was to not write the "off" into the libvirt XML, but rather only into the .args. IOW: If the option the option is available in qemu, but not supplied in libvirt XML, then write "off". If someone changes the XML to have "off" or "on", then that's fine - go with what they have and keep it. I just was trying to be cautious about writing "off" into the output XML (especially the *config* one). John
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. ;-)
Will fix this.
Thanks, Erik
On Fri, Jun 08, 2018 at 12:00:50PM -0400, John Ferlan wrote:
On 06/08/2018 07:09 AM, Erik Skultety wrote:
On Mon, Jun 04, 2018 at 07:40:42PM -0400, John Ferlan wrote:
On 05/30/2018 09:43 AM, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@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
So, w/r/t comments in patch 6, I understand that we'd keep an open door to changing the default (no display attr means use 'auto' as it would in most cases in libvirt). However, I don't believe that we'd want to make 'auto' a default ever because you can't predict whether the user wants to use the device as GPGPU or graphics, if they leave the display attr unset and we'd default to auto in the future, we might risk that they know they can't use it for graphics and they didn't even intend to, but we tried and failed because of some underlying reasons. And I'm not even sure we'd ever introduce 'auto' to libvirt for reasons I just mentioned + the cover letter and the discussion that happened upstream regarding the VNC console. What I know though, is that libvirt is never going to use QEMU's 'auto', since it doesn't make sense, either we take care of everything or the user does, regardless of QEMU.
Anyway, the way I imagine 'auto' could work in libvirt is that user references an mdev, but doesn't care (or doesn't know) what technology it uses underneath and explicitly says "libvirt please take care of that on my behalf, I want to use the display, but have 0 clue on what to enable". That's a different kind of 'auto' compared to when we'd try to read user's mind. I tried to play it very safe here, as the vgpu display thing can be very tricky, not even thinking about future migrations. But if there's a strong disagreement with this kind of approach, then I'll of course need to incorporate your comments in patch 6 and this one to get this successfully merged.
Erik
I didn't think I was advocating for adding AUTO - just blabbering how insane the code is ;-). IIRC, if "off" isn't written then auto is assumed. What I thought I was suggesting here though was to not write the "off" into the libvirt XML, but rather only into the .args. IOW: If the option the option is available in qemu, but not supplied in libvirt XML, then write "off". If someone changes the XML to have "off" or "on", then that's fine - go with what they have and keep it. I just was trying to be cautious about writing "off" into the output XML (especially the *config* one).
Alright, we could skip the "offline" XML that's a fair argument, but we should format something into the "live" one, as that one serves as the base for the "migration" XML, I'd rather us not sending a missing display argument to a newer libvirt on the destination for safety reasons, I believe that turning something 'off' rather than 'on' by default is good. Erik
CC'ing Gerd to comment since he implemented the feature in QEMU. Erik On Wed, May 30, 2018 at 03:42:54PM +0200, Erik Skultety wrote:
Since QEMU 2.12 there's a new vfio-pci device property 'display' with values on/off/auto. This special kind of display allows using a mediated device which is a VGA compatible device for a display output. There are 2 different implementations of how the device output is handled, referred to as dmabuf and vfio region mapping (currently NVIDIA uses the latter, while Intel relies on the former). From libvirt's perspective the important difference is that dmabuf requires OpenGL support whereas vfio regions don't (it will of course work even with OpenGL enabled). There's a catch though - because of several constraints in the vendor drivers (as discussed here [1]), there currently isn't a reasonable way for libvirt (other than spawning a dummy QEMU instance) to probe such mediated devices for the display mode they use. This the nr.1 reason why libvirt is not going to support the value 'auto' with QEMU and will default to 'off' instead in all cases to stay safe the least and therefore is going to rely on users being able to configure this properly otherwise they'll get an error.
Once there's a way for libvirt to probe the nature of the display-capable mediated devices, we can consider adding support for 'auto' value meaning that libvirt is going to take care of adding an appropriate Spice/VNC graphics device depending on the system if these are missing in the config, otherwise the user's choice is always favoured.
TL;DR: - we have a new attribute value for vfio-pci mediated devices called 'display' -> devices can now format this new 'display=on/off' property to the cmdline
- if user enables the vfio display (display=on) but doesn't enable OpenGL for Spice, we automatically assume the usage of '-display egl-headless' (uses local drm nodes) which works both for Spice and VNC -> if OpenGL is enabled, then '-display egl-headless' is not necessary
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1475770 [1] https://www.redhat.com/archives/libvir-list/2018-May/msg00243.html
Erik Skultety (7): conf: Remove a redundant model/address-type check in mdev post parse qemu: command: Move graphics iteration to its own function conf: Introduce virDomainDefHasSpiceGL helper conf: Introduce new <hostdev> attribute 'display' qemu: caps: Add vfio-pci.display capability qemu: domain: Set default vfio-pci display value depending on capability qemu: command: Enable formatting vfio-pci.display option onto cmdline
docs/formatdomain.html.in | 16 ++++- docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 56 +++++++++++---- src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 84 +++++++++++++++------- src/qemu/qemu_domain.c | 75 +++++++++++++++++++ tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + .../hostdev-mdev-display-missing-graphics.xml | 35 +++++++++ .../hostdev-mdev-display-spice-no-opengl.args | 32 +++++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 41 +++++++++++ .../hostdev-mdev-display-spice-opengl.args | 31 ++++++++ .../hostdev-mdev-display-spice-opengl.xml | 41 +++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 +++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml | 39 ++++++++++ tests/qemuxml2argvtest.c | 23 ++++++ .../hostdev-mdev-display-spice-no-opengl.xml | 47 ++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 48 +++++++++++++ .../hostdev-mdev-display-vnc.xml | 47 ++++++++++++ tests/qemuxml2xmltest.c | 3 + 25 files changed, 626 insertions(+), 41 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.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.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-no-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-vnc.xml
-- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi,
TL;DR: - we have a new attribute value for vfio-pci mediated devices called 'display' -> devices can now format this new 'display=on/off' property to the cmdline
Good.
- if user enables the vfio display (display=on) but doesn't enable OpenGL for Spice, we automatically assume the usage of '-display egl-headless'
Hmm, I think it would be better to have egl-headless explicitly configured in the domain xml instead of doing it automagically depending on configuration. First, I'd prefer to allow non-opengl configuration for vfio displays. nvidia has its own non-standard ways of doing things, which causes problems now and then, so I'd like to have an easy way out in case using egl-headless on nvidia fails to work properly. It's a bit ugly that we have to depend on qemu throwing errors in case the vfio display uses dma-bufs (and therefore requires opengl) then because libvirt can't easily probe the vfio display type. Second, you can use egl-headless for other use cases too, for example to use virgl with vnc. cheers, Gerd
On Tue, Jun 05, 2018 at 11:52:03AM +0200, Gerd Hoffmann wrote:
Hi,
TL;DR: - we have a new attribute value for vfio-pci mediated devices called 'display' -> devices can now format this new 'display=on/off' property to the cmdline
Good.
- if user enables the vfio display (display=on) but doesn't enable OpenGL for Spice, we automatically assume the usage of '-display egl-headless'
Hmm, I think it would be better to have egl-headless explicitly configured in the domain xml instead of doing it automagically depending on configuration.
I was thinking about that earlier, then we had a private conversation where I asked why there's no documentation on egl-headless in QEMU to which you replied that the future of egl-headless being uncertain, especially once there's remote opengl renderer support (as opposed to local nowadays). So I took a bit of a safe path here and only made the changes to the command line, thus leaving us with a way of easily ditching that if QEMU decides to deprecate egl-headless completely. So, if QEMU can guarantee supporting egl-headless (of course you can discourage usage of it...) then we surely can expose it through the XML, but I'd really wouldn't want to end up with an XML setting which we won't be able to satisfy at some point in the future. More importantly though, egl-headless was introduced in 2.10 IIRC, but I don't see any way for libvirt to use any kind of introspection here, there's no capability for egl-headless, I see it defined as a value for DisplayType in the qapi scheme (ui.json), but I don't see any query command. The way I handled it here is easy, I'm querying vfio display support which was introduced in 2.12 which is newer, but I'm not sure about the virgl + vnc case below, because that one doesn't need vfio display=on/off support.
First, I'd prefer to allow non-opengl configuration for vfio displays. nvidia has its own non-standard ways of doing things, which causes problems now and then, so I'd like to have an easy way out in case using egl-headless on nvidia fails to work properly.
It's a bit ugly that we have to depend on qemu throwing errors in case the vfio display uses dma-bufs (and therefore requires opengl) then because libvirt can't easily probe the vfio display type.
Second, you can use egl-headless for other use cases too, for example to use virgl with vnc.
Yep, that makes sense for us to support. Erik
On Wed, Jun 06, 2018 at 02:49:24PM +0200, Erik Skultety wrote:
On Tue, Jun 05, 2018 at 11:52:03AM +0200, Gerd Hoffmann wrote:
Hi,
TL;DR: - we have a new attribute value for vfio-pci mediated devices called 'display' -> devices can now format this new 'display=on/off' property to the cmdline
Good.
- if user enables the vfio display (display=on) but doesn't enable OpenGL for Spice, we automatically assume the usage of '-display egl-headless'
Hmm, I think it would be better to have egl-headless explicitly configured in the domain xml instead of doing it automagically depending on configuration.
I was thinking about that earlier, then we had a private conversation where I asked why there's no documentation on egl-headless in QEMU to which you replied that the future of egl-headless being uncertain, especially once there's remote opengl renderer support (as opposed to local nowadays). So I took a bit of a safe path here and only made the changes to the command line, thus leaving us with a way of easily ditching that if QEMU decides to deprecate egl-headless completely. So, if QEMU can guarantee supporting egl-headless (of course you can discourage usage of it...) then we surely can expose it through the XML,
Well, I guess if libvirt uses it it pretty much has to stay anyway ... It isn't much of a burden code-wise, most of the underlying egl/opengl code is needed for -spice gl=on too. It is just that once spice got remote support for gl=on mode it is alot less useful because native spice support clearly is the better route performance-wise.
egl-headless was introduced in 2.10 IIRC, but I don't see any way for libvirt to use any kind of introspection here, there's no capability for egl-headless,
Yea, right, isn't there.
I see it defined as a value for DisplayType in the qapi scheme (ui.json), but I don't see any query command.
Also the enum is alwayws present, no matter whenever qemu was compiled with opengl support or not. So even in case you could query it it wouldn't buy you anything. How is spice/vnc support probed for btw? cheers, Gerd
On Thu, Jun 07, 2018 at 08:01:23AM +0200, Gerd Hoffmann wrote:
On Wed, Jun 06, 2018 at 02:49:24PM +0200, Erik Skultety wrote:
On Tue, Jun 05, 2018 at 11:52:03AM +0200, Gerd Hoffmann wrote:
Hi,
TL;DR: - we have a new attribute value for vfio-pci mediated devices called 'display' -> devices can now format this new 'display=on/off' property to the cmdline
Good.
- if user enables the vfio display (display=on) but doesn't enable OpenGL for Spice, we automatically assume the usage of '-display egl-headless'
Hmm, I think it would be better to have egl-headless explicitly configured in the domain xml instead of doing it automagically depending on configuration.
I was thinking about that earlier, then we had a private conversation where I asked why there's no documentation on egl-headless in QEMU to which you replied that the future of egl-headless being uncertain, especially once there's remote opengl renderer support (as opposed to local nowadays). So I took a bit of a safe path here and only made the changes to the command line, thus leaving us with a way of easily ditching that if QEMU decides to deprecate egl-headless completely. So, if QEMU can guarantee supporting egl-headless (of course you can discourage usage of it...) then we surely can expose it through the XML,
Well, I guess if libvirt uses it it pretty much has to stay anyway ...
It would be beneficial to add it to the man page and/or --help output then.
It isn't much of a burden code-wise, most of the underlying egl/opengl code is needed for -spice gl=on too. It is just that once spice got remote support for gl=on mode it is alot less useful because native spice support clearly is the better route performance-wise.
egl-headless was introduced in 2.10 IIRC, but I don't see any way for libvirt to use any kind of introspection here, there's no capability for egl-headless,
Yea, right, isn't there.
I see it defined as a value for DisplayType in the qapi scheme (ui.json), but I don't see any query command.
Also the enum is alwayws present, no matter whenever qemu was compiled with opengl support or not. So even in case you could query it it wouldn't buy you anything.
In which case the only thing we can do is let qemu fail with an internal error rather than failing in libvirt because of a missing capability.
How is spice/vnc support probed for btw?
we execute 'query-commands' and then looking for in "query-spice" and "query-vnc" literals in the list of returned commands. Erik
participants (3)
-
Erik Skultety -
Gerd Hoffmann -
John Ferlan