[libvirt] [PATCH v1 00/11] Enable vfio-pci 'property' for mediated device
See the RFC here: https://www.redhat.com/archives/libvir-list/2018-May/msg02218.html Since RFC: - split graphics 'gl' to a standalone structure since it's now SPICE, SDL, *and* VNC that will support it - egl-headless support for VNC, since VNC doesn't support OpenGL natively like SPICE does - added a new attribute 'native' for spice which will instruct libvirt to use '-display egl-headless' instead of libvirt trying to figure this on out by itself, since egl-headless might have other uses besides mdev with VNC - dropped formatting of the 'display' attribute to the domain XML if it had the default value, so it will only be formatted if user explicitly set it (previously patch 6) - minor adjustments like splitting the second patch into 2 (now 2 and 3) - more cleanup Erik Skultety (11): conf: Remove a redundant model/address-type check in mdev post parse qemu: command: Move graphics iteration to its own function qemu: command: Add virReportEnumRangeError to BuildHostdevCommandline conf: Replace error label with cleanup in virDomainGraphicsDefParseVNCXML qemu: command: Fix building of the SDL display command line conf: Make graphics's GL a standalone structure conf: Allow usage of the <gl> element with VNC graphics conf: Introduce new <gl> attribute 'native' for SPICE qemu: caps: Add vfio-pci.display capability conf: Introduce new <hostdev> attribute 'display' qemu: command: Enable formatting vfio-pci.display option onto cmdline docs/formatdomain.html.in | 33 ++- docs/schemas/domaincommon.rng | 28 ++- src/conf/domain_conf.c | 230 ++++++++++++++------- src/conf/domain_conf.h | 14 +- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_cgroup.c | 10 +- src/qemu/qemu_command.c | 189 +++++++++++------ src/qemu/qemu_domain.c | 79 ++++++- src/security/security_dac.c | 7 +- 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 + tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + .../qemuxml2argvdata/graphics-sdl-fullscreen.args | 2 +- tests/qemuxml2argvdata/graphics-sdl.args | 2 +- .../qemuxml2argvdata/graphics-spice-gl-native.args | 26 +++ .../qemuxml2argvdata/graphics-spice-gl-native.xml | 25 +++ .../graphics-spice-gl-non-native.args | 27 +++ .../graphics-spice-gl-non-native.xml | 24 +++ tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml | 37 ++++ tests/qemuxml2argvdata/graphics-vnc-gl.args | 28 +++ tests/qemuxml2argvdata/graphics-vnc-gl.xml | 37 ++++ .../hostdev-mdev-display-missing-graphics.xml | 35 ++++ .../hostdev-mdev-display-spice-egl-headless.args | 32 +++ .../hostdev-mdev-display-spice-egl-headless.xml | 41 ++++ .../hostdev-mdev-display-spice-opengl.args | 31 +++ .../hostdev-mdev-display-spice-opengl.xml | 41 ++++ .../hostdev-mdev-display-vnc-egl-headless.args | 32 +++ .../hostdev-mdev-display-vnc-egl-headless.xml | 41 ++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 +++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml | 39 ++++ tests/qemuxml2argvdata/hostdev-mdev-display.xml | 39 ++++ .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 2 +- tests/qemuxml2argvtest.c | 38 ++++ .../hostdev-mdev-display-active.xml | 47 +++++ .../hostdev-mdev-display-inactive.xml | 47 +++++ .../video-virtio-gpu-spice-gl.xml | 2 +- tests/qemuxml2xmltest.c | 2 + 40 files changed, 1143 insertions(+), 163 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.xml create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.xml create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.args create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-inactive.xml -- 2.14.4
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 d8cb7f37f3..d887df2c3e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4523,20 +4523,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.4
On 06/27/2018 09:34 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(-)
I think the reason for the separate checks was fear of messing with the current check when ccw addressing was added ;-) 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> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 57 +++++++++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4fc3176ad3..78c18440e5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8141,26 +8141,44 @@ 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: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported graphics type '%s'"), + virDomainGraphicsTypeToString(graphics->type)); + return -1; + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsType, graphics->type); + return -1; + } } return 0; @@ -10299,11 +10317,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.4
Adding the 'default' case to our enum-typecasted switches is the current safety trend, so add it here for mdevs too. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 78c18440e5..1e5d7f9ccf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5397,10 +5397,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"); -- 2.14.4
On 06/27/2018 09:34 AM, Erik Skultety wrote:
Adding the 'default' case to our enum-typecasted switches is the current safety trend, so add it here for mdevs too.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d887df2c3e..09d9bac739 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13568,13 +13568,13 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, int ret = -1; if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) - goto error; + goto cleanup; if (port) { if (virStrToLong_i(port, NULL, 10, &def->data.vnc.port) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse vnc port %s"), port); - goto error; + goto cleanup; } /* Legacy compat syntax, used -1 for auto-port */ if (def->data.vnc.port == -1) { @@ -13603,7 +13603,7 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, &def->data.vnc.websocket) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse vnc WebSocket port %s"), websocket); - goto error; + goto cleanup; } } @@ -13615,7 +13615,7 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown vnc display sharing policy '%s'"), sharePolicy); - goto error; + goto cleanup; } else { def->data.vnc.sharePolicy = policy; } @@ -13625,10 +13625,10 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, if (virDomainGraphicsAuthDefParseXML(node, &def->data.vnc.auth, def->type) < 0) - goto error; + goto cleanup; ret = 0; - error: + cleanup: VIR_FREE(port); VIR_FREE(autoport); VIR_FREE(websocket); -- 2.14.4
On 06/27/2018 09:34 AM, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
QEMU uses a shorthand '-sdl' which maps to '-display sdl'. However, if there are any options to be passed to SDL, the full command version must be used. Everything seemingly worked for us until commit 5038b300437 introduced OpenGL support for SDL and added ',gl=on/off' option which as mentioned above could have never worked with the shorthand version of the command. Indeed starting a domain with an SDL display and OpenGL enabled, QEMU produces a rather cryptic error: -sdl: Could not open 'gl=on': No such file or directory This patch provides fixes to both the SDL cmdline generation and the test suite. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 19 +++++++++++-------- tests/qemuxml2argvdata/graphics-sdl-fullscreen.args | 2 +- tests/qemuxml2argvdata/graphics-sdl.args | 2 +- tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 2 +- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1e5d7f9ccf..195d03e373 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7722,7 +7722,6 @@ qemuBuildGraphicsSDLCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, { int ret = -1; virBuffer opt = VIR_BUFFER_INITIALIZER; - const char *optContent; if (graphics->data.sdl.xauth) virCommandAddEnvPair(cmd, "XAUTHORITY", graphics->data.sdl.xauth); @@ -7738,22 +7737,26 @@ qemuBuildGraphicsSDLCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL); - virCommandAddArg(cmd, "-sdl"); + virCommandAddArg(cmd, "-display"); + virBufferAddLit(&opt, "sdl"); - if (graphics->data.sdl.gl == VIR_TRISTATE_BOOL_YES) { + if (graphics->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL_GL)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support SDL OpenGL")); + _("OpenGL for SDL is not supported with this QEMU " + "binary")); goto cleanup; } - virBufferAsprintf(&opt, "gl=%s", + virBufferAsprintf(&opt, ",gl=%s", virTristateSwitchTypeToString(graphics->data.sdl.gl)); + } - optContent = virBufferCurrentContent(&opt); - if (optContent && STRNEQ(optContent, "")) - virCommandAddArgBuffer(cmd, &opt); + if (virBufferCheckError(&opt) < 0) + goto cleanup; + + virCommandAddArgBuffer(cmd, &opt); ret = 0; cleanup: diff --git a/tests/qemuxml2argvdata/graphics-sdl-fullscreen.args b/tests/qemuxml2argvdata/graphics-sdl-fullscreen.args index 2da9d1e327..0741baa039 100644 --- a/tests/qemuxml2argvdata/graphics-sdl-fullscreen.args +++ b/tests/qemuxml2argvdata/graphics-sdl-fullscreen.args @@ -25,5 +25,5 @@ server,nowait \ -drive file=/dev/HostVG/QEMUGuest1,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 \ -full-screen \ --sdl \ +-display sdl \ -vga cirrus diff --git a/tests/qemuxml2argvdata/graphics-sdl.args b/tests/qemuxml2argvdata/graphics-sdl.args index 29a0a3d823..2553679738 100644 --- a/tests/qemuxml2argvdata/graphics-sdl.args +++ b/tests/qemuxml2argvdata/graphics-sdl.args @@ -24,5 +24,5 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,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 \ --sdl \ +-display sdl \ -vga std diff --git a/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args index 4172320ede..f8610e53b0 100644 --- a/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args +++ b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args @@ -23,6 +23,6 @@ server,nowait \ -drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ id=drive-ide0-0-0,cache=none \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --sdl gl=on \ +-display sdl,gl=on \ -device virtio-gpu-pci,id=video0,virgl=on,bus=pci.0,addr=0x2 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -- 2.14.4
On 06/27/2018 09:34 AM, Erik Skultety wrote:
QEMU uses a shorthand '-sdl' which maps to '-display sdl'. However, if there are any options to be passed to SDL, the full command version must be used. Everything seemingly worked for us until commit 5038b300437 introduced OpenGL support for SDL and added ',gl=on/off' option which as mentioned above could have never worked with the shorthand version of the command. Indeed starting a domain with an SDL display and OpenGL enabled, QEMU produces a rather cryptic error:
-sdl: Could not open 'gl=on': No such file or directory
This patch provides fixes to both the SDL cmdline generation and the test suite.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 19 +++++++++++-------- tests/qemuxml2argvdata/graphics-sdl-fullscreen.args | 2 +- tests/qemuxml2argvdata/graphics-sdl.args | 2 +- tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 2 +- 4 files changed, 14 insertions(+), 11 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John Weird on this one... I thought I looked through qemu sources to see if this would work right. Maybe I just didn't look hard enough - cannot recall. Oh well glad you fixed it <sigh>.
Since we support gl with SPICE and SDL with future VNC enablement in sight (egl-headless), let's separate the gl-related attributes into a standalone structure. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 137 +++++++++++++++++++++++++------------------- src/conf/domain_conf.h | 12 +++- src/qemu/qemu_cgroup.c | 10 ++-- src/qemu/qemu_command.c | 66 ++++++++++++--------- src/qemu/qemu_domain.c | 7 ++- src/security/security_dac.c | 7 ++- 6 files changed, 138 insertions(+), 101 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 09d9bac739..6bfa3ca130 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1420,8 +1420,6 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) break; case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: - VIR_FREE(def->data.spice.rendernode); - VIR_FREE(def->data.spice.keymap); virDomainGraphicsAuthDefClear(&def->data.spice.auth); break; @@ -1429,6 +1427,8 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) break; } + virDomainGraphicsGLDefFree(def->gl); + for (i = 0; i < def->nListens; i++) virDomainGraphicsListenDefClear(&def->listens[i]); VIR_FREE(def->listens); @@ -1436,6 +1436,18 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) VIR_FREE(def); } + +void +virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->rendernode); + VIR_FREE(def); +} + + const char *virDomainInputDefGetPath(virDomainInputDefPtr input) { switch ((virDomainInputType) input->type) { @@ -13555,6 +13567,48 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def, } +static int +virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, + xmlNodePtr node) +{ + virDomainGraphicsGLDefPtr gl = NULL; + char *enable = NULL; + int ret = -1; + + if (!node) + return 0; + + if (VIR_ALLOC(gl) < 0) + return -1; + + if (!(enable = virXMLPropString(node, "enable"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'enable' attribute is mandatory for graphics " + "<gl> element")); + goto cleanup; + } + + if ((gl->enable = + virTristateBoolTypeFromString(enable)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown value for attribute enable '%s'"), + enable); + goto cleanup; + } + + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) + gl->rendernode = virXMLPropString(node, "rendernode"); + + VIR_STEAL_PTR(def->gl, gl); + + ret = 0; + cleanup: + VIR_FREE(enable); + virDomainGraphicsGLDefFree(gl); + return ret; +} + + static int virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, xmlNodePtr node, @@ -13644,8 +13698,6 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def, { xmlNodePtr save = ctxt->node; char *enable = NULL; - int enableVal; - xmlNodePtr glNode; char *fullscreen = virXMLPropString(node, "fullscreen"); int ret = -1; @@ -13668,23 +13720,9 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def, def->data.sdl.xauth = virXMLPropString(node, "xauth"); def->data.sdl.display = virXMLPropString(node, "display"); - glNode = virXPathNode("./gl", ctxt); - if (glNode) { - enable = virXMLPropString(glNode, "enable"); - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("sdl gl element missing enable")); - goto cleanup; - } - - enableVal = virTristateBoolTypeFromString(enable); - if (enableVal < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - goto cleanup; - } - def->data.sdl.gl = enableVal; - } + if (virDomainGraphicsGLDefParseXML(def, + virXPathNode("./gl[1]", ctxt)) < 0) + goto cleanup; ret = 0; cleanup: @@ -14026,31 +14064,6 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, VIR_FREE(enable); def->data.spice.filetransfer = enableVal; - } else if (virXMLNodeNameEqual(cur, "gl")) { - char *enable = virXMLPropString(cur, "enable"); - char *rendernode = virXMLPropString(cur, "rendernode"); - int enableVal; - - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice gl element missing enable")); - VIR_FREE(rendernode); - goto error; - } - - if ((enableVal = - virTristateBoolTypeFromString(enable)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - VIR_FREE(enable); - VIR_FREE(rendernode); - goto error; - } - VIR_FREE(enable); - - def->data.spice.gl = enableVal; - def->data.spice.rendernode = rendernode; - } else if (virXMLNodeNameEqual(cur, "mouse")) { char *mode = virXMLPropString(cur, "mode"); int modeVal; @@ -14071,6 +14084,9 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, VIR_FREE(mode); def->data.spice.mousemode = modeVal; + } else if (virXMLNodeNameEqual(cur, "gl")) { + if (virDomainGraphicsGLDefParseXML(def, cur) < 0) + goto error; } } cur = cur->next; @@ -26148,18 +26164,25 @@ virDomainGraphicsListenDefFormatAddr(virBufferPtr buf, virBufferAsprintf(buf, " listen='%s'", glisten->address); } + static void -virDomainSpiceGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def) +virDomainGraphicsGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def) { - if (def->data.spice.gl == VIR_TRISTATE_BOOL_ABSENT) + virDomainGraphicsGLDefPtr gl = def->gl; + + if (!gl) return; virBufferAsprintf(buf, "<gl enable='%s'", - virTristateBoolTypeToString(def->data.spice.gl)); - virBufferEscapeString(buf, " rendernode='%s'", def->data.spice.rendernode); + virTristateBoolTypeToString(gl->enable)); + + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) + virBufferEscapeString(buf, " rendernode='%s'", gl->rendernode); + virBufferAddLit(buf, "/>\n"); } + static int virDomainGraphicsDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def, @@ -26247,18 +26270,12 @@ virDomainGraphicsDefFormat(virBufferPtr buf, if (def->data.sdl.fullscreen) virBufferAddLit(buf, " fullscreen='yes'"); - if (!children && def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) { + if (!children && def->gl) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); children = true; } - if (def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) { - virBufferAsprintf(buf, "<gl enable='%s'", - virTristateBoolTypeToString(def->data.sdl.gl)); - virBufferAddLit(buf, "/>\n"); - } - break; case VIR_DOMAIN_GRAPHICS_TYPE_RDP: @@ -26405,8 +26422,7 @@ virDomainGraphicsDefFormat(virBufferPtr buf, if (!children && (def->data.spice.image || def->data.spice.jpeg || def->data.spice.zlib || def->data.spice.playback || def->data.spice.streaming || def->data.spice.copypaste || - def->data.spice.mousemode || def->data.spice.filetransfer || - def->data.spice.gl)) { + def->data.spice.mousemode || def->data.spice.filetransfer)) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); children = true; @@ -26436,9 +26452,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", virTristateBoolTypeToString(def->data.spice.filetransfer)); - virDomainSpiceGLDefFormat(buf, def); } + virDomainGraphicsGLDefFormat(buf, def); + if (children) { virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</graphics>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0924fc4f3c..20dc1334c4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1598,6 +1598,13 @@ struct _virDomainGraphicsListenDef { bool autoGenerated; }; +typedef struct _virDomainGraphicsGLDef virDomainGraphicsGLDef; +typedef virDomainGraphicsGLDef *virDomainGraphicsGLDefPtr; +struct _virDomainGraphicsGLDef { + virTristateBool enable; + char *rendernode; /* SPICE only */ +}; + struct _virDomainGraphicsDef { /* Port value discipline: * Value -1 is legacy syntax indicating that it should be auto-allocated. @@ -1620,7 +1627,6 @@ struct _virDomainGraphicsDef { char *display; char *xauth; bool fullscreen; - virTristateBool gl; } sdl; struct { int port; @@ -1650,8 +1656,6 @@ struct _virDomainGraphicsDef { int streaming; virTristateBool copypaste; virTristateBool filetransfer; - virTristateBool gl; - char *rendernode; } spice; } data; /* nListens, listens, and *port are only useful if type is vnc, @@ -1659,6 +1663,7 @@ struct _virDomainGraphicsDef { * simplify parsing code.*/ size_t nListens; virDomainGraphicsListenDefPtr listens; + virDomainGraphicsGLDefPtr gl; }; typedef enum { @@ -2837,6 +2842,7 @@ int virDomainObjWaitUntil(virDomainObjPtr vm, void virDomainPanicDefFree(virDomainPanicDefPtr panic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); +void virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def); const char *virDomainInputDefGetPath(virDomainInputDefPtr input); void virDomainInputDefFree(virDomainInputDefPtr def); virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index c8fba7f9e6..81e86fa973 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -489,20 +489,20 @@ qemuSetupGraphicsCgroup(virDomainObjPtr vm, virDomainGraphicsDefPtr gfx) { qemuDomainObjPrivatePtr priv = vm->privateData; - const char *rendernode = gfx->data.spice.rendernode; int ret; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE || - gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES || - !rendernode) + !gfx->gl || + gfx->gl->enable != VIR_TRISTATE_BOOL_YES || + !gfx->gl->rendernode) return 0; - ret = virCgroupAllowDevicePath(priv->cgroup, rendernode, + ret = virCgroupAllowDevicePath(priv->cgroup, gfx->gl->rendernode, VIR_CGROUP_DEVICE_RW, false); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", rendernode, + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", gfx->gl->rendernode, "rw", ret); return ret; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 195d03e373..ef0be95b0f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7740,7 +7740,8 @@ qemuBuildGraphicsSDLCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virCommandAddArg(cmd, "-display"); virBufferAddLit(&opt, "sdl"); - if (graphics->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) { + if (graphics->gl && + graphics->gl->enable != VIR_TRISTATE_BOOL_ABSENT) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL_GL)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("OpenGL for SDL is not supported with this QEMU " @@ -7749,7 +7750,7 @@ qemuBuildGraphicsSDLCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, } virBufferAsprintf(&opt, ",gl=%s", - virTristateSwitchTypeToString(graphics->data.sdl.gl)); + virTristateSwitchTypeToString(graphics->gl->enable)); } @@ -7886,6 +7887,35 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, } +static int +qemuBuildGraphicsSPICEGLCommandLine(virDomainGraphicsGLDefPtr gl, + virBufferPtr opt, + virQEMUCapsPtr qemuCaps) +{ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL")); + return -1; + } + + virBufferAddLit(opt, "gl=on,"); + + if (gl->rendernode) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL rendernode")); + return -1; + } + + virBufferAddLit(opt, "rendernode="); + virQEMUBuildBufferEscapeComma(opt, gl->rendernode); + virBufferAddLit(opt, ","); + } + + return 0; +} + + static int qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, virCommandPtr cmd, @@ -8089,31 +8119,6 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } } - if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support spice OpenGL")); - goto error; - } - - /* spice.gl is a TristateBool, but qemu expects on/off: use - * TristateSwitch helper */ - virBufferAsprintf(&opt, "gl=%s,", - virTristateSwitchTypeToString(graphics->data.spice.gl)); - - if (graphics->data.spice.rendernode) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support spice OpenGL rendernode")); - goto error; - } - - virBufferAddLit(&opt, "rendernode="); - virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice.rendernode); - virBufferAddLit(&opt, ","); - } - } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { /* If qemu supports seamless migration turn it * unconditionally on. If migration destination @@ -8122,10 +8127,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, virBufferAddLit(&opt, "seamless-migration=on,"); } + /* OpenGL magic */ + if (graphics->gl && + graphics->gl->enable == VIR_TRISTATE_BOOL_YES && + qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, &opt, qemuCaps) < 0) + goto error; + virBufferTrim(&opt, ",", -1); virCommandAddArg(cmd, "-spice"); virCommandAddArgBuffer(cmd, &opt); + if (graphics->data.spice.keymap) virCommandAddArgList(cmd, "-k", graphics->data.spice.keymap, NULL); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f8a662f747..948b9b7fd0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11290,11 +11290,12 @@ qemuDomainSetupGraphics(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virDomainGraphicsDefPtr gfx, const struct qemuDomainCreateDeviceData *data) { - const char *rendernode = gfx->data.spice.rendernode; + const char *rendernode = NULL; if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE || - gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES || - !rendernode) + !gfx->gl || + gfx->gl->enable != VIR_TRISTATE_BOOL_YES || + !gfx->gl->rendernode) return 0; return qemuDomainCreateDevice(rendernode, data, false); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b623dcf39..e8757f04e8 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1431,10 +1431,11 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr, return -1; if (gfx->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && - gfx->data.spice.gl == VIR_TRISTATE_BOOL_YES && - gfx->data.spice.rendernode) { + gfx->gl && + gfx->gl->enable == VIR_TRISTATE_BOOL_YES && + gfx->gl->rendernode) { if (virSecurityDACSetOwnership(priv, NULL, - gfx->data.spice.rendernode, + gfx->gl->rendernode, user, group) < 0) return -1; } -- 2.14.4
...
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f8a662f747..948b9b7fd0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11290,11 +11290,12 @@ qemuDomainSetupGraphics(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virDomainGraphicsDefPtr gfx, const struct qemuDomainCreateDeviceData *data) { - const char *rendernode = gfx->data.spice.rendernode; + const char *rendernode = NULL;
if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE || - gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES || - !rendernode) + !gfx->gl || + gfx->gl->enable != VIR_TRISTATE_BOOL_YES || + !gfx->gl->rendernode) return 0;
return qemuDomainCreateDevice(rendernode, data, false);
Consider the following hunk squashed in: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 39920da442..98fc9a128b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11360,15 +11360,13 @@ qemuDomainSetupGraphics(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virDomainGraphicsDefPtr gfx, const struct qemuDomainCreateDeviceData *data) { - const char *rendernode = NULL; - if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE || !gfx->gl || gfx->gl->enable != VIR_TRISTATE_BOOL_YES || !gfx->gl->rendernode) return 0; - return qemuDomainCreateDevice(rendernode, data, false); + return qemuDomainCreateDevice(gfx->gl->rendernode, data, false); } Erik
On 06/27/2018 09:34 AM, Erik Skultety wrote:
Since we support gl with SPICE and SDL with future VNC enablement in sight (egl-headless), let's separate the gl-related attributes into a standalone structure.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 137 +++++++++++++++++++++++++------------------- src/conf/domain_conf.h | 12 +++- src/qemu/qemu_cgroup.c | 10 ++-- src/qemu/qemu_command.c | 66 ++++++++++++--------- src/qemu/qemu_domain.c | 7 ++- src/security/security_dac.c | 7 ++- 6 files changed, 138 insertions(+), 101 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 09d9bac739..6bfa3ca130 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1420,8 +1420,6 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: - VIR_FREE(def->data.spice.rendernode); - VIR_FREE(def->data.spice.keymap);
Perhaps a bit too aggressive here? Restore the keymap FREE - it's allocated in virDomainGraphicsDefParseXMLSpice
virDomainGraphicsAuthDefClear(&def->data.spice.auth); break;
@@ -1429,6 +1427,8 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) break; }
+ virDomainGraphicsGLDefFree(def->gl); + for (i = 0; i < def->nListens; i++) virDomainGraphicsListenDefClear(&def->listens[i]); VIR_FREE(def->listens); @@ -1436,6 +1436,18 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) VIR_FREE(def); }
+ +void +virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def)
The only caller is above - even after the last patch, thus this should be static to this function and above the previous.
+{ + if (!def) + return; + + VIR_FREE(def->rendernode); + VIR_FREE(def); +} + + const char *virDomainInputDefGetPath(virDomainInputDefPtr input) { switch ((virDomainInputType) input->type) { @@ -13555,6 +13567,48 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def, }
+static int +virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, + xmlNodePtr node) +{ + virDomainGraphicsGLDefPtr gl = NULL; + char *enable = NULL; + int ret = -1; + + if (!node) + return 0; + + if (VIR_ALLOC(gl) < 0) + return -1; + + if (!(enable = virXMLPropString(node, "enable"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'enable' attribute is mandatory for graphics " + "<gl> element")); + goto cleanup; + } + + if ((gl->enable = + virTristateBoolTypeFromString(enable)) <= 0) {
This should go on the previous line - it fits, I checked. The <= changes from the previous code which had: - enableVal = virTristateBoolTypeFromString(enable); - if (enableVal < 0) { hopefully there's no "default"'s in the wild. But it is fine given the previous usage model of absent being the default and being checked.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown value for attribute enable '%s'"), + enable); + goto cleanup; + } + + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) + gl->rendernode = virXMLPropString(node, "rendernode"); + + VIR_STEAL_PTR(def->gl, gl); + + ret = 0; + cleanup: + VIR_FREE(enable); + virDomainGraphicsGLDefFree(gl); + return ret; +} + + static int virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, xmlNodePtr node, @@ -13644,8 +13698,6 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def, { xmlNodePtr save = ctxt->node; char *enable = NULL; - int enableVal; - xmlNodePtr glNode; char *fullscreen = virXMLPropString(node, "fullscreen"); int ret = -1;
@@ -13668,23 +13720,9 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def, def->data.sdl.xauth = virXMLPropString(node, "xauth"); def->data.sdl.display = virXMLPropString(node, "display");
- glNode = virXPathNode("./gl", ctxt); - if (glNode) { - enable = virXMLPropString(glNode, "enable"); - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("sdl gl element missing enable")); - goto cleanup; - } - - enableVal = virTristateBoolTypeFromString(enable); - if (enableVal < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - goto cleanup; - } - def->data.sdl.gl = enableVal; - } + if (virDomainGraphicsGLDefParseXML(def, + virXPathNode("./gl[1]", ctxt)) < 0)
Move to the previous line, it fits, I checked.
+ goto cleanup;
ret = 0; cleanup: @@ -14026,31 +14064,6 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, VIR_FREE(enable);
def->data.spice.filetransfer = enableVal; - } else if (virXMLNodeNameEqual(cur, "gl")) { - char *enable = virXMLPropString(cur, "enable"); - char *rendernode = virXMLPropString(cur, "rendernode"); - int enableVal; - - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice gl element missing enable")); - VIR_FREE(rendernode); - goto error; - } - - if ((enableVal = - virTristateBoolTypeFromString(enable)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - VIR_FREE(enable); - VIR_FREE(rendernode); - goto error; - } - VIR_FREE(enable); - - def->data.spice.gl = enableVal; - def->data.spice.rendernode = rendernode; -
Was moving to the last else if intentional? IDC either way, but it is strange.
} else if (virXMLNodeNameEqual(cur, "mouse")) { char *mode = virXMLPropString(cur, "mode"); int modeVal; @@ -14071,6 +14084,9 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,
[...]
static int virDomainGraphicsDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def, @@ -26247,18 +26270,12 @@ virDomainGraphicsDefFormat(virBufferPtr buf, if (def->data.sdl.fullscreen) virBufferAddLit(buf, " fullscreen='yes'");
- if (!children && def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) { + if (!children && def->gl) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); children = true; }
- if (def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) { - virBufferAsprintf(buf, "<gl enable='%s'", - virTristateBoolTypeToString(def->data.sdl.gl)); - virBufferAddLit(buf, "/>\n"); - } - break;
case VIR_DOMAIN_GRAPHICS_TYPE_RDP: @@ -26405,8 +26422,7 @@ virDomainGraphicsDefFormat(virBufferPtr buf, if (!children && (def->data.spice.image || def->data.spice.jpeg || def->data.spice.zlib || def->data.spice.playback || def->data.spice.streaming || def->data.spice.copypaste || - def->data.spice.mousemode || def->data.spice.filetransfer || - def->data.spice.gl)) { + def->data.spice.mousemode || def->data.spice.filetransfer)) {
Interesting, it's a bit sneaky but even when not provided the above format for <listen type...> will format to 'none' meaning we don't have to worry about def->gl here (even if autoport isn't provided for <graphics...> Hopefully that remains "true" going forward in time.
virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); children = true; @@ -26436,9 +26452,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", virTristateBoolTypeToString(def->data.spice.filetransfer));
- virDomainSpiceGLDefFormat(buf, def); }
+ virDomainGraphicsGLDefFormat(buf, def); + if (children) { virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</graphics>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0924fc4f3c..20dc1334c4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h
[...]
typedef enum { @@ -2837,6 +2842,7 @@ int virDomainObjWaitUntil(virDomainObjPtr vm, void virDomainPanicDefFree(virDomainPanicDefPtr panic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); +void virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def);
This won't be necessary.... The "tip off" is that the private syms file wasn't touched...
const char *virDomainInputDefGetPath(virDomainInputDefPtr input); void virDomainInputDefFree(virDomainInputDefPtr def); virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt);
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 195d03e373..ef0be95b0f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7740,7 +7740,8 @@ qemuBuildGraphicsSDLCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virCommandAddArg(cmd, "-display"); virBufferAddLit(&opt, "sdl");
- if (graphics->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) { + if (graphics->gl && + graphics->gl->enable != VIR_TRISTATE_BOOL_ABSENT) {
Ironically, because the <= exists in the Parse code, that means that if ->gl is true, then we don't have to check ABSENT I would think. I also see other/future patches seem to use enable == YES a lot...
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL_GL)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("OpenGL for SDL is not supported with this QEMU "
[...]
@@ -8122,10 +8127,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, virBufferAddLit(&opt, "seamless-migration=on,"); }
+ /* OpenGL magic */ + if (graphics->gl && + graphics->gl->enable == VIR_TRISTATE_BOOL_YES && + qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, &opt, qemuCaps) < 0) + goto error; +
Not that it matters, but this is a change of order w/ "seamless-migration=on,"... No code uses both now and I don't imagine order matters. With recommended adjustments, Reviewed-by: John Ferlan <jferlan@redhat.com> John
virBufferTrim(&opt, ",", -1);
virCommandAddArg(cmd, "-spice"); virCommandAddArgBuffer(cmd, &opt); + if (graphics->data.spice.keymap) virCommandAddArgList(cmd, "-k", graphics->data.spice.keymap, NULL);
[...]
VNC doesn't support OpenGL natively, but can run with non-native egl-headless support, so enable that. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 6 ++++ docs/schemas/domaincommon.rng | 7 ++++ src/conf/domain_conf.c | 8 +++++ src/qemu/qemu_command.c | 7 ++++ tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.args | 28 ++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 8 files changed, 131 insertions(+) create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.args create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0d68596991..aa0d6b26df 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6350,6 +6350,12 @@ qemu-kvm -net nic,model=? /dev/null auto-allocation and <code>autoport</code> having no effect due to security reasons) <span class="since">Since 1.0.6</span>. </p> + <p> + <span class="since">Since 4.6.0</span> it's possible to use the + <code>gl</code> element with <code>enable='yes'</code> to enable + OpenGL support using QEMU's egl-headless display, since VNC + doesn't support OpenGL natively like SPICE does. + </p> </dd> <dt><code>spice</code> <span class="since">Since 0.8.6</span></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f46145cf9b..20649c5f6f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3135,6 +3135,13 @@ </attribute> </optional> <ref name="listenElements"/> + <optional> + <element name="gl"> + <attribute name="enable"> + <ref name="virYesNo"/> + </attribute> + </element> + </optional> </group> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6bfa3ca130..2ccd9e124f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13619,8 +13619,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, char *websocket = virXMLPropString(node, "websocket"); char *sharePolicy = virXMLPropString(node, "sharePolicy"); char *autoport = virXMLPropString(node, "autoport"); + xmlNodePtr save = ctxt->node; int ret = -1; + ctxt->node = node; + if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) goto cleanup; @@ -13681,12 +13684,17 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, def->type) < 0) goto cleanup; + if (virDomainGraphicsGLDefParseXML(def, + virXPathNode("./gl[1]", ctxt)) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(port); VIR_FREE(autoport); VIR_FREE(websocket); VIR_FREE(sharePolicy); + ctxt->node = save; return ret; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ef0be95b0f..89a8408df6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7879,6 +7879,13 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, else virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); + /* OpenGL support */ + if (graphics->gl && + graphics->gl->enable == VIR_TRISTATE_BOOL_YES) { + virCommandAddArg(cmd, "-display"); + virCommandAddArg(cmd, "egl-headless"); + } + return 0; error: diff --git a/tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml b/tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml new file mode 100644 index 0000000000..0f34791046 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</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/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no'> + <listen type='none'/> + <gl enable='yes' native='yes' rendernode='/dev/dri/foo'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/graphics-vnc-gl.args b/tests/qemuxml2argvdata/graphics-vnc-gl.args new file mode 100644 index 0000000000..2d2b3cf0fb --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-vnc-gl.args @@ -0,0 +1,28 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-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-QEMUGuest1/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/QEMUGuest1,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 '[2001:1:2:3:4:5:1234:1234]:3' \ +-display egl-headless \ +-vga cirrus diff --git a/tests/qemuxml2argvdata/graphics-vnc-gl.xml b/tests/qemuxml2argvdata/graphics-vnc-gl.xml new file mode 100644 index 0000000000..fea2caf843 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-vnc-gl.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</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/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no' listen='2001:1:2:3:4:5:1234:1234'> + <listen type='address' address='2001:1:2:3:4:5:1234:1234'/> + <gl enable='yes'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c279ac4975..c310349b57 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1187,6 +1187,7 @@ mymain(void) DO_TEST("graphics-vnc-none", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("graphics-vnc-socket-new-cmdline", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_VNC_MULTI_SERVERS); + DO_TEST("graphics-vnc-gl", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); driver.config->vncSASL = 1; VIR_FREE(driver.config->vncSASLdir); -- 2.14.4
On 06/27/2018 09:34 AM, Erik Skultety wrote:
VNC doesn't support OpenGL natively, but can run with non-native egl-headless support, so enable that.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 6 ++++ docs/schemas/domaincommon.rng | 7 ++++ src/conf/domain_conf.c | 8 +++++ src/qemu/qemu_command.c | 7 ++++ tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.args | 28 ++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 8 files changed, 131 insertions(+) create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.args create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.xml
hmmm... I recall some discussion about egl-headless in the review of the RFC: https://www.redhat.com/archives/libvir-list/2018-June/msg00461.html So, what's the point? Well I was going to ask why no capability for egl-headless, but the above link (and a couple followups) discusses the issue. Anyway, I think either a commit log message or a comment in the code when egl-headless is added may be appropriate. Another option is following what commit 3278a7bb does and adding the 2.10 check for the capability that is added.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0d68596991..aa0d6b26df 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6350,6 +6350,12 @@ qemu-kvm -net nic,model=? /dev/null auto-allocation and <code>autoport</code> having no effect due to security reasons) <span class="since">Since 1.0.6</span>. </p> + <p> + <span class="since">Since 4.6.0</span> it's possible to use the + <code>gl</code> element with <code>enable='yes'</code> to enable + OpenGL support using QEMU's egl-headless display, since VNC + doesn't support OpenGL natively like SPICE does. + </p> </dd> <dt><code>spice</code> <span class="since">Since 0.8.6</span></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f46145cf9b..20649c5f6f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3135,6 +3135,13 @@ </attribute> </optional> <ref name="listenElements"/> + <optional> + <element name="gl"> + <attribute name="enable"> + <ref name="virYesNo"/> + </attribute> + </element> + </optional> </group> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6bfa3ca130..2ccd9e124f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13619,8 +13619,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, char *websocket = virXMLPropString(node, "websocket"); char *sharePolicy = virXMLPropString(node, "sharePolicy"); char *autoport = virXMLPropString(node, "autoport"); + xmlNodePtr save = ctxt->node; int ret = -1;
+ ctxt->node = node; +
Not used here - but virDomainGraphicsListensParseXML does the same thing... I think you can remove it here or just pass node to virDomainGraphicsListensParseXML - your call.
if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) goto cleanup;
@@ -13681,12 +13684,17 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, def->type) < 0) goto cleanup;
+ if (virDomainGraphicsGLDefParseXML(def, + virXPathNode("./gl[1]", ctxt)) < 0)
Fits on previous line
+ goto cleanup; + ret = 0; cleanup: VIR_FREE(port); VIR_FREE(autoport); VIR_FREE(websocket); VIR_FREE(sharePolicy); + ctxt->node = save; return ret; }
With a couple of cleanups and addressing the egl-headless conundrum, I think we're good... John
On Thu, Jun 28, 2018 at 05:18:46PM -0400, John Ferlan wrote:
On 06/27/2018 09:34 AM, Erik Skultety wrote:
VNC doesn't support OpenGL natively, but can run with non-native egl-headless support, so enable that.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 6 ++++ docs/schemas/domaincommon.rng | 7 ++++ src/conf/domain_conf.c | 8 +++++ src/qemu/qemu_command.c | 7 ++++ tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.args | 28 ++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 8 files changed, 131 insertions(+) create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.args create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.xml
hmmm... I recall some discussion about egl-headless in the review of the RFC:
https://www.redhat.com/archives/libvir-list/2018-June/msg00461.html
So, what's the point? Well I was going to ask why no capability for egl-headless, but the above link (and a couple followups) discusses the issue.
So the point behind egl-headless is that you'll get accelerated output, even for VNC which especially in this case is an advantage, since you can use rendernodes to do the rendering instead of doing it in software. SPICE can benefit from this when used remotely, because native OpenGL support works only locally at the moment. The output is going to be rendered into a dma-buf which apps like VNC can get access to and transfer it to your screen.
Anyway, I think either a commit log message or a comment in the code when egl-headless is added may be appropriate. Another option is following what commit 3278a7bb does and adding the 2.10 check for the capability that is added.
Hmm, thanks for the pointer, we could increase the safety a bit by doing this, the thing with egl-headless is that the enums that qemu defines are always available, even when no SPICE/VNC/whatever display support is compiled into it, but that should be fine with libvirt as there are capabilities for those and we check for them at appropriate places already. Erik
On Thu, Jun 28, 2018 at 05:18:46PM -0400, John Ferlan wrote:
On 06/27/2018 09:34 AM, Erik Skultety wrote:
VNC doesn't support OpenGL natively, but can run with non-native egl-headless support, so enable that.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 6 ++++ docs/schemas/domaincommon.rng | 7 ++++ src/conf/domain_conf.c | 8 +++++ src/qemu/qemu_command.c | 7 ++++ tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.args | 28 ++++++++++++++++ tests/qemuxml2argvdata/graphics-vnc-gl.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 8 files changed, 131 insertions(+) create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl-invalid.xml create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.args create mode 100644 tests/qemuxml2argvdata/graphics-vnc-gl.xml
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6bfa3ca130..2ccd9e124f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13619,8 +13619,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, char *websocket = virXMLPropString(node, "websocket"); char *sharePolicy = virXMLPropString(node, "sharePolicy"); char *autoport = virXMLPropString(node, "autoport"); + xmlNodePtr save = ctxt->node; int ret = -1;
+ ctxt->node = node; +
Not used here - but virDomainGraphicsListensParseXML does the same thing... I think you can remove it here or just pass node to virDomainGraphicsListensParseXML - your call.
I actually need to set it so that virXPathNode(./gl) below would behave as expected, it looks this way only because in the SPICE code, we're traversing node children, so I already get a "gl" node, here I have to query it first. But you're right, that's terrible, I'm going to move the spice bit out of the children loop, thus virDomainGraphicsGLDefParse will behave similarly to virDomainGraphicsListenParseXML which replaces the context node with the current one, restoring it back later. Consider the above hunk moved into virDomainGraphicsGLDefParse. Erik
Since QEMU 2.10, there's a new cmdline option '-display egl-headless' which enables OpenGL support for cases where we can't render on a local display (i.e. <listen> is set to either 'address' or 'network'). This is to work around the current restriction on the local node with native OpenGL SPICE support. However, compared to native support, egl-headless has overhead, because the rendered framebuffer has to be copied back to QEMU's display area which is then accessed by either SPICE or VNC and sent to the remote side. This patch enables this configuration feature by introducing a new SPICE-only attribute 'native' which helps libvirt to determine whether to use '-spice gl=on' or '-display egl-headless' on the cmdline. If the attribute wasn't specified but OpenGL is to be enabled, then given the nature of the <listen> element, libvirt will determine the default value. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 8 ++++ docs/schemas/domaincommon.rng | 16 +++++-- src/conf/domain_conf.c | 49 ++++++++++++++++++++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 43 ++++++++++++------- .../qemuxml2argvdata/graphics-spice-gl-native.args | 26 ++++++++++++ .../qemuxml2argvdata/graphics-spice-gl-native.xml | 25 +++++++++++ .../graphics-spice-gl-non-native.args | 27 ++++++++++++ .../graphics-spice-gl-non-native.xml | 24 +++++++++++ tests/qemuxml2argvtest.c | 8 ++++ .../video-virtio-gpu-spice-gl.xml | 2 +- 11 files changed, 206 insertions(+), 23 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.xml create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index aa0d6b26df..c6ebd39bd9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6461,6 +6461,14 @@ qemu-kvm -net nic,model=? /dev/null You can enable or disable OpenGL support explicitly with the <code>gl</code> element, by setting the <code>enable</code> property. (QEMU only, <span class="since">since 1.3.3</span>). + Additionally, attribute <code>native</code> + (<span class="since">Since 4.6.0</span>) can be used to specify + whether native OpenGL support should be used with SPICE or + egl-headless should be used instead. Note that the native OpenGL + support is only limited to the local setup (<code>listen</code> is + either 'none' or 'socket') and for remote access egl-headless + needs to be used. The supported values for this attribute are 'on' + and 'off'. </p> <p> By default, QEMU will pick the first available GPU DRM render node. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 20649c5f6f..4ad53d976b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3066,6 +3066,11 @@ <attribute name="enable"> <ref name="virYesNo"/> </attribute> + <optional> + <attribute name="native"> + <ref name="virYesNo"/> + </attribute> + </optional> <empty/> </element> </optional> @@ -3322,9 +3327,14 @@ <ref name="absFilePath"/> </attribute> </optional> - <empty/> - </element> - </optional> + <optional> + <attribute name="native"> + <ref name="virYesNo"/> + </attribute> + </optional> + <empty/> + </element> + </optional> </interleave> </group> <group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2ccd9e124f..6d399a198e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4280,6 +4280,7 @@ virDomainDefPostParseGraphics(virDomainDef *def) * same. */ if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { virDomainGraphicsListenDefPtr glisten = &graphics->listens[0]; + virDomainGraphicsGLDefPtr gl = graphics->gl; if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS && graphics->data.spice.port == 0 && @@ -4288,6 +4289,28 @@ virDomainDefPostParseGraphics(virDomainDef *def) VIR_FREE(glisten->address); glisten->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE; } + + /* Next we need to figure out how to properly configure the OpenGL + * if that is enabled and the 'native' attribute is missing. + * The cases are: + * 1) Listen type is either 'socket' or 'none' - SPICE native + * OpenGL support (,gl=on) should be used because we're + * rendering on a local display. + * + * 2) Listen is either network or address - SPICE can't use + * the native OpenGL support remotely yet, so we use + * native='no' and format '-display egl-headless' onto the + * cmdline. + */ + if (graphics->gl && + graphics->gl->enable == VIR_TRISTATE_BOOL_YES && + graphics->gl->native == VIR_TRISTATE_BOOL_ABSENT) { + gl->native = VIR_TRISTATE_BOOL_NO; + + if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE || + glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET) + gl->native = VIR_TRISTATE_BOOL_YES; + } } } } @@ -13573,6 +13596,7 @@ virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, { virDomainGraphicsGLDefPtr gl = NULL; char *enable = NULL; + char *native = NULL; int ret = -1; if (!node) @@ -13596,14 +13620,26 @@ virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, goto cleanup; } - if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) - gl->rendernode = virXMLPropString(node, "rendernode"); + /* SPICE recognizes a few more attributes */ + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + gl->rendernode = virXMLPropString(node, "rendernode"); + + native = virXMLPropString(node, "native"); + if (native && + (gl->native = virTristateBoolTypeFromString(native)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown value for attribute enable '%s'"), + enable); + goto cleanup; + } + } VIR_STEAL_PTR(def->gl, gl); ret = 0; cleanup: VIR_FREE(enable); + VIR_FREE(native); virDomainGraphicsGLDefFree(gl); return ret; } @@ -26184,8 +26220,13 @@ virDomainGraphicsGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def) virBufferAsprintf(buf, "<gl enable='%s'", virTristateBoolTypeToString(gl->enable)); - if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) - virBufferEscapeString(buf, " rendernode='%s'", gl->rendernode); + if (gl->enable == VIR_TRISTATE_BOOL_YES) { + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + virBufferAsprintf(buf, " native='%s'", + virTristateBoolTypeToString(gl->native)); + virBufferEscapeString(buf, " rendernode='%s'", gl->rendernode); + } + } virBufferAddLit(buf, "/>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 20dc1334c4..99b5896391 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1602,6 +1602,7 @@ typedef struct _virDomainGraphicsGLDef virDomainGraphicsGLDef; typedef virDomainGraphicsGLDef *virDomainGraphicsGLDefPtr; struct _virDomainGraphicsGLDef { virTristateBool enable; + virTristateBool native; /* -spice gl=on vs -display egl-headless for QEMU */ char *rendernode; /* SPICE only */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 89a8408df6..fc80a6c3a6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7896,27 +7896,39 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, static int qemuBuildGraphicsSPICEGLCommandLine(virDomainGraphicsGLDefPtr gl, + virCommandPtr cmd, virBufferPtr opt, virQEMUCapsPtr qemuCaps) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support spice OpenGL")); - return -1; - } - - virBufferAddLit(opt, "gl=on,"); - - if (gl->rendernode) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) { + if (gl->native == VIR_TRISTATE_BOOL_NO) { + /* For non-native OpenGL, we need to add egl-headless to the cmdline. + * + * NB: QEMU defaults to '-spice gl=off', so we don't have to add that + * explicitly, especially since we're not testing for GL capability + * presence. + */ + virCommandAddArg(cmd, "-display"); + virCommandAddArg(cmd, "egl-headless"); + } else { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support spice OpenGL rendernode")); + _("This QEMU doesn't support spice OpenGL")); return -1; } - virBufferAddLit(opt, "rendernode="); - virQEMUBuildBufferEscapeComma(opt, gl->rendernode); - virBufferAddLit(opt, ","); + virBufferAddLit(opt, "gl=on,"); + + if (gl->rendernode) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL rendernode")); + return -1; + } + + virBufferAddLit(opt, "rendernode="); + virQEMUBuildBufferEscapeComma(opt, gl->rendernode); + virBufferAddLit(opt, ","); + } } return 0; @@ -8137,7 +8149,8 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, /* OpenGL magic */ if (graphics->gl && graphics->gl->enable == VIR_TRISTATE_BOOL_YES && - qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, &opt, qemuCaps) < 0) + qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, cmd, + &opt, qemuCaps) < 0) goto error; virBufferTrim(&opt, ",", -1); diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-native.args b/tests/qemuxml2argvdata/graphics-spice-gl-native.args new file mode 100644 index 0000000000..70d6694022 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-gl-native.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-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-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-usb \ +-spice port=0,gl=on,rendernode=/dev/dri/foo \ +-vga cirrus \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-native.xml b/tests/qemuxml2argvdata/graphics-spice-gl-native.xml new file mode 100644 index 0000000000..abfe628a9c --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-gl-native.xml @@ -0,0 +1,25 @@ +<domain type='qemu'> + <name>QEMUGuest1</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> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice' autoport='no'> + <listen type='none'/> + <gl enable='yes' native='yes' rendernode='/dev/dri/foo'/> + </graphics> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-non-native.args b/tests/qemuxml2argvdata/graphics-spice-gl-non-native.args new file mode 100644 index 0000000000..9833cd6c0f --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-gl-non-native.args @@ -0,0 +1,27 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-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-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-usb \ +-display egl-headless \ +-spice port=0 \ +-vga cirrus \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-non-native.xml b/tests/qemuxml2argvdata/graphics-spice-gl-non-native.xml new file mode 100644 index 0000000000..003f2406ac --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-gl-non-native.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>QEMUGuest1</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> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice' autoport='no'> + <gl enable='yes' native='no'/> + </graphics> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c310349b57..e82496d403 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1256,6 +1256,14 @@ mymain(void) QEMU_CAPS_SPICE_UNIX, QEMU_CAPS_DEVICE_CIRRUS_VGA); driver.config->spiceAutoUnixSocket = false; + DO_TEST("graphics-spice-gl-native", + QEMU_CAPS_SPICE, + QEMU_CAPS_SPICE_GL, + QEMU_CAPS_SPICE_RENDERNODE, + QEMU_CAPS_DEVICE_CIRRUS_VGA); + DO_TEST("graphics-spice-gl-non-native", + QEMU_CAPS_SPICE, + QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("input-usbmouse", NONE); DO_TEST("input-usbtablet", NONE); diff --git a/tests/qemuxml2xmloutdata/video-virtio-gpu-spice-gl.xml b/tests/qemuxml2xmloutdata/video-virtio-gpu-spice-gl.xml index 720d362230..578341e09b 100644 --- a/tests/qemuxml2xmloutdata/video-virtio-gpu-spice-gl.xml +++ b/tests/qemuxml2xmloutdata/video-virtio-gpu-spice-gl.xml @@ -31,7 +31,7 @@ <input type='keyboard' bus='ps2'/> <graphics type='spice'> <listen type='none'/> - <gl enable='yes' rendernode='/dev/dri/foo'/> + <gl enable='yes' native='yes' rendernode='/dev/dri/foo'/> </graphics> <video> <model type='virtio' heads='1' primary='yes'> -- 2.14.4
On 06/27/2018 09:34 AM, Erik Skultety wrote:
Since QEMU 2.10, there's a new cmdline option '-display egl-headless' which enables OpenGL support for cases where we can't render on a local display (i.e. <listen> is set to either 'address' or 'network'). This is to work around the current restriction on the local node with native OpenGL SPICE support. However, compared to native support, egl-headless has overhead, because the rendered framebuffer has to be copied back to QEMU's display area which is then accessed by either SPICE or VNC and sent to the remote side.
blank line for readability
This patch enables this configuration feature by introducing a new SPICE-only attribute 'native' which helps libvirt to determine whether to use '-spice gl=on' or '-display egl-headless' on the cmdline. If the attribute wasn't specified but OpenGL is to be enabled, then given the nature of the <listen> element, libvirt will determine the default value.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 8 ++++ docs/schemas/domaincommon.rng | 16 +++++-- src/conf/domain_conf.c | 49 ++++++++++++++++++++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 43 ++++++++++++------- .../qemuxml2argvdata/graphics-spice-gl-native.args | 26 ++++++++++++ .../qemuxml2argvdata/graphics-spice-gl-native.xml | 25 +++++++++++ .../graphics-spice-gl-non-native.args | 27 ++++++++++++ .../graphics-spice-gl-non-native.xml | 24 +++++++++++ tests/qemuxml2argvtest.c | 8 ++++ .../video-virtio-gpu-spice-gl.xml | 2 +- 11 files changed, 206 insertions(+), 23 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.xml create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index aa0d6b26df..c6ebd39bd9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6461,6 +6461,14 @@ qemu-kvm -net nic,model=? /dev/null You can enable or disable OpenGL support explicitly with the <code>gl</code> element, by setting the <code>enable</code> property. (QEMU only, <span class="since">since 1.3.3</span>). + Additionally, attribute <code>native</code> + (<span class="since">Since 4.6.0</span>) can be used to specify + whether native OpenGL support should be used with SPICE or + egl-headless should be used instead. Note that the native OpenGL + support is only limited to the local setup (<code>listen</code> is + either 'none' or 'socket') and for remote access egl-headless + needs to be used. The supported values for this attribute are 'on' + and 'off'. </p> <p> By default, QEMU will pick the first available GPU DRM render node. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 20649c5f6f..4ad53d976b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3066,6 +3066,11 @@ <attribute name="enable"> <ref name="virYesNo"/> </attribute> + <optional> + <attribute name="native"> + <ref name="virYesNo"/> + </attribute> + </optional> <empty/> </element> </optional> @@ -3322,9 +3327,14 @@ <ref name="absFilePath"/> </attribute> </optional> - <empty/> - </element> - </optional> + <optional> + <attribute name="native"> + <ref name="virYesNo"/> + </attribute> + </optional> + <empty/> + </element> + </optional> </interleave> </group> <group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2ccd9e124f..6d399a198e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4280,6 +4280,7 @@ virDomainDefPostParseGraphics(virDomainDef *def) * same. */ if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { virDomainGraphicsListenDefPtr glisten = &graphics->listens[0]; + virDomainGraphicsGLDefPtr gl = graphics->gl;
if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS && graphics->data.spice.port == 0 && @@ -4288,6 +4289,28 @@ virDomainDefPostParseGraphics(virDomainDef *def) VIR_FREE(glisten->address); glisten->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE; } + + /* Next we need to figure out how to properly configure the OpenGL + * if that is enabled and the 'native' attribute is missing. + * The cases are: + * 1) Listen type is either 'socket' or 'none' - SPICE native + * OpenGL support (,gl=on) should be used because we're + * rendering on a local display. + * + * 2) Listen is either network or address - SPICE can't use + * the native OpenGL support remotely yet, so we use + * native='no' and format '-display egl-headless' onto the + * cmdline. + */ + if (graphics->gl && + graphics->gl->enable == VIR_TRISTATE_BOOL_YES && + graphics->gl->native == VIR_TRISTATE_BOOL_ABSENT) { + gl->native = VIR_TRISTATE_BOOL_NO; + + if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE || + glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET) + gl->native = VIR_TRISTATE_BOOL_YES; + }
So the upside/downside to setting something in the XML that wasn't previously set by the consumer is that "we're stuck with it" going forward, right? Additionally, we cannot ascertain whether we set native to NO or YES or if the consumer did. So the "other" side of this is this is being done in a domain_conf function instead of some qemu device post parse function since if I'm reading the RFC comments right, the desire is not to somehow automagically add it, but rather force some usage. Still, I wonder if what's been done follows the spirit of Gerd's comment: "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." could be turned into a "headless='yes|no|absent'" instead of native and described as being necessary for specific conditions. Then perhaps a qemu domain validation check to "force" the issue.
} } } @@ -13573,6 +13596,7 @@ virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, { virDomainGraphicsGLDefPtr gl = NULL; char *enable = NULL; + char *native = NULL; int ret = -1;
if (!node) @@ -13596,14 +13620,26 @@ virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, goto cleanup; }
- if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) - gl->rendernode = virXMLPropString(node, "rendernode"); + /* SPICE recognizes a few more attributes */ + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + gl->rendernode = virXMLPropString(node, "rendernode"); + + native = virXMLPropString(node, "native"); + if (native && + (gl->native = virTristateBoolTypeFromString(native)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown value for attribute enable '%s'"), + enable);
I believe you wanted "s/enable/native" for 2 instances, copy pasta error
+ goto cleanup; + } + }
VIR_STEAL_PTR(def->gl, gl);
ret = 0; cleanup: VIR_FREE(enable); + VIR_FREE(native); virDomainGraphicsGLDefFree(gl); return ret; } @@ -26184,8 +26220,13 @@ virDomainGraphicsGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def) virBufferAsprintf(buf, "<gl enable='%s'", virTristateBoolTypeToString(gl->enable));
- if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) - virBufferEscapeString(buf, " rendernode='%s'", gl->rendernode); + if (gl->enable == VIR_TRISTATE_BOOL_YES) {
Not clear why the YES guard was put in place. Not that "enable=no and rendernode=xxx" means anything, but that would be dropped with this change. Since rng/grammar using interleave, order of attributes shouldn't matter should it?
+ if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + virBufferAsprintf(buf, " native='%s'", + virTristateBoolTypeToString(gl->native)); + virBufferEscapeString(buf, " rendernode='%s'", gl->rendernode); + } + }
virBufferAddLit(buf, "/>\n"); }
Do you think we need ABI checks for this? Could be challenging given the post parse setting of the value.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 20dc1334c4..99b5896391 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1602,6 +1602,7 @@ typedef struct _virDomainGraphicsGLDef virDomainGraphicsGLDef; typedef virDomainGraphicsGLDef *virDomainGraphicsGLDefPtr; struct _virDomainGraphicsGLDef { virTristateBool enable; + virTristateBool native; /* -spice gl=on vs -display egl-headless for QEMU */ char *rendernode; /* SPICE only */ };
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 89a8408df6..fc80a6c3a6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7896,27 +7896,39 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
static int qemuBuildGraphicsSPICEGLCommandLine(virDomainGraphicsGLDefPtr gl, + virCommandPtr cmd, virBufferPtr opt, virQEMUCapsPtr qemuCaps) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support spice OpenGL")); - return -1; - } - - virBufferAddLit(opt, "gl=on,"); - - if (gl->rendernode) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) { + if (gl->native == VIR_TRISTATE_BOOL_NO) { + /* For non-native OpenGL, we need to add egl-headless to the cmdline. + * + * NB: QEMU defaults to '-spice gl=off', so we don't have to add that + * explicitly, especially since we're not testing for GL capability + * presence. + */ + virCommandAddArg(cmd, "-display"); + virCommandAddArg(cmd, "egl-headless");
Similar thoughts as previous patch related to egl-headless and need for capability and/or details related to what's being used to gate it's presence on the command line. Honestly it's confusing to read "when" it's supposed to be used between the new code and what was written in the RFC. There's a comment regarding nvidia, non-opengl config for vfio, where it's not clear to me whether headless could be used (IOW: some gl=no condition). John
+ } else { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support spice OpenGL rendernode")); + _("This QEMU doesn't support spice OpenGL")); return -1; }
- virBufferAddLit(opt, "rendernode="); - virQEMUBuildBufferEscapeComma(opt, gl->rendernode); - virBufferAddLit(opt, ","); + virBufferAddLit(opt, "gl=on,"); + + if (gl->rendernode) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL rendernode")); + return -1; + } + + virBufferAddLit(opt, "rendernode="); + virQEMUBuildBufferEscapeComma(opt, gl->rendernode); + virBufferAddLit(opt, ","); + } }
return 0; @@ -8137,7 +8149,8 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, /* OpenGL magic */ if (graphics->gl && graphics->gl->enable == VIR_TRISTATE_BOOL_YES && - qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, &opt, qemuCaps) < 0) + qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, cmd, + &opt, qemuCaps) < 0) goto error;
virBufferTrim(&opt, ",", -1);
[...]
On Thu, Jun 28, 2018 at 06:34:56PM -0400, John Ferlan wrote:
On 06/27/2018 09:34 AM, Erik Skultety wrote:
Since QEMU 2.10, there's a new cmdline option '-display egl-headless' which enables OpenGL support for cases where we can't render on a local display (i.e. <listen> is set to either 'address' or 'network'). This is to work around the current restriction on the local node with native OpenGL SPICE support. However, compared to native support, egl-headless has overhead, because the rendered framebuffer has to be copied back to QEMU's display area which is then accessed by either SPICE or VNC and sent to the remote side.
blank line for readability
This patch enables this configuration feature by introducing a new SPICE-only attribute 'native' which helps libvirt to determine whether to use '-spice gl=on' or '-display egl-headless' on the cmdline. If the attribute wasn't specified but OpenGL is to be enabled, then given the nature of the <listen> element, libvirt will determine the default value.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 8 ++++ docs/schemas/domaincommon.rng | 16 +++++-- src/conf/domain_conf.c | 49 ++++++++++++++++++++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 43 ++++++++++++------- .../qemuxml2argvdata/graphics-spice-gl-native.args | 26 ++++++++++++ .../qemuxml2argvdata/graphics-spice-gl-native.xml | 25 +++++++++++ .../graphics-spice-gl-non-native.args | 27 ++++++++++++ .../graphics-spice-gl-non-native.xml | 24 +++++++++++ tests/qemuxml2argvtest.c | 8 ++++ .../video-virtio-gpu-spice-gl.xml | 2 +- 11 files changed, 206 insertions(+), 23 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-native.xml create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-non-native.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index aa0d6b26df..c6ebd39bd9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6461,6 +6461,14 @@ qemu-kvm -net nic,model=? /dev/null You can enable or disable OpenGL support explicitly with the <code>gl</code> element, by setting the <code>enable</code> property. (QEMU only, <span class="since">since 1.3.3</span>). + Additionally, attribute <code>native</code> + (<span class="since">Since 4.6.0</span>) can be used to specify + whether native OpenGL support should be used with SPICE or + egl-headless should be used instead. Note that the native OpenGL + support is only limited to the local setup (<code>listen</code> is + either 'none' or 'socket') and for remote access egl-headless + needs to be used. The supported values for this attribute are 'on' + and 'off'. </p> <p> By default, QEMU will pick the first available GPU DRM render node. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 20649c5f6f..4ad53d976b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3066,6 +3066,11 @@ <attribute name="enable"> <ref name="virYesNo"/> </attribute> + <optional> + <attribute name="native"> + <ref name="virYesNo"/> + </attribute> + </optional> <empty/> </element> </optional> @@ -3322,9 +3327,14 @@ <ref name="absFilePath"/> </attribute> </optional> - <empty/> - </element> - </optional> + <optional> + <attribute name="native"> + <ref name="virYesNo"/> + </attribute> + </optional> + <empty/> + </element> + </optional> </interleave> </group> <group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2ccd9e124f..6d399a198e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4280,6 +4280,7 @@ virDomainDefPostParseGraphics(virDomainDef *def) * same. */ if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { virDomainGraphicsListenDefPtr glisten = &graphics->listens[0]; + virDomainGraphicsGLDefPtr gl = graphics->gl;
if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS && graphics->data.spice.port == 0 && @@ -4288,6 +4289,28 @@ virDomainDefPostParseGraphics(virDomainDef *def) VIR_FREE(glisten->address); glisten->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE; } + + /* Next we need to figure out how to properly configure the OpenGL + * if that is enabled and the 'native' attribute is missing. + * The cases are: + * 1) Listen type is either 'socket' or 'none' - SPICE native + * OpenGL support (,gl=on) should be used because we're + * rendering on a local display. + * + * 2) Listen is either network or address - SPICE can't use + * the native OpenGL support remotely yet, so we use + * native='no' and format '-display egl-headless' onto the + * cmdline. + */ + if (graphics->gl && + graphics->gl->enable == VIR_TRISTATE_BOOL_YES && + graphics->gl->native == VIR_TRISTATE_BOOL_ABSENT) { + gl->native = VIR_TRISTATE_BOOL_NO; + + if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE || + glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET) + gl->native = VIR_TRISTATE_BOOL_YES; + }
So the upside/downside to setting something in the XML that wasn't previously set by the consumer is that "we're stuck with it" going forward, right? Additionally, we cannot ascertain whether we set native to NO or YES or if the consumer did.
Does it matter? If libvirt set it, then we set it in a way that is safe for us, IOW we can ensure the vm will start using that setting. If the consumer set it, then we evaluate the setting and either fail or proceed with starting up the vm.
So the "other" side of this is this is being done in a domain_conf function instead of some qemu device post parse function since if I'm reading the RFC comments right, the desire is not to somehow automagically add it, but rather force some usage.
Correct, although, it's not that our "automagic" was wrong (in fact it was safe), but it wasn't very flexible and 'egl-headless' can find use cases even without mdevs, that was the main driver to expose it.
Still, I wonder if what's been done follows the spirit of Gerd's comment:
"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."
As I already explained in the cover letter reply, to me the facts that a standalone graphics type='headless' doesn't make sense, is basically just some kind of OpenGL switch and can't be combined with some of the other types felt like a wrong way of doing things, that's why I merged it with the 'gl' element. But okay, I'll give it a try on a separate branch and see how 'making it a graphics type' goes. The one thing that won't be necessary and worth doing anymore is separating the 'gl' data into a separate element, but I'm curious how much code will be saved (if any) if done this way.
could be turned into a "headless='yes|no|absent'" instead of native and described as being necessary for specific conditions. Then perhaps a qemu domain validation check to "force" the issue.
} } } @@ -13573,6 +13596,7 @@ virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, { virDomainGraphicsGLDefPtr gl = NULL; char *enable = NULL; + char *native = NULL; int ret = -1;
if (!node) @@ -13596,14 +13620,26 @@ virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def, goto cleanup; }
- if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) - gl->rendernode = virXMLPropString(node, "rendernode"); + /* SPICE recognizes a few more attributes */ + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + gl->rendernode = virXMLPropString(node, "rendernode"); + + native = virXMLPropString(node, "native"); + if (native && + (gl->native = virTristateBoolTypeFromString(native)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown value for attribute enable '%s'"), + enable);
I believe you wanted "s/enable/native" for 2 instances, copy pasta error
+ goto cleanup; + } + }
VIR_STEAL_PTR(def->gl, gl);
ret = 0; cleanup: VIR_FREE(enable); + VIR_FREE(native); virDomainGraphicsGLDefFree(gl); return ret; } @@ -26184,8 +26220,13 @@ virDomainGraphicsGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def) virBufferAsprintf(buf, "<gl enable='%s'", virTristateBoolTypeToString(gl->enable));
- if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) - virBufferEscapeString(buf, " rendernode='%s'", gl->rendernode); + if (gl->enable == VIR_TRISTATE_BOOL_YES) {
Not clear why the YES guard was put in place. Not that "enable=no and rendernode=xxx" means anything, but that would be dropped with this change.
Since rng/grammar using interleave, order of attributes shouldn't matter should it?
+ if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + virBufferAsprintf(buf, " native='%s'", + virTristateBoolTypeToString(gl->native)); + virBufferEscapeString(buf, " rendernode='%s'", gl->rendernode); + } + }
virBufferAddLit(buf, "/>\n"); }
Do you think we need ABI checks for this? Could be challenging given the post parse setting of the value.
I don't see an ABI problem here, old libvirt doesn't know what to do with it (and VNC will just ignore the gl element) and new enough libvirt would ensure that -spice gl=on and -display egl-headless will not appear on the cmdline. Everything else is something that's been working for a very long time. Erik
[...]
+ /* Next we need to figure out how to properly configure the OpenGL + * if that is enabled and the 'native' attribute is missing. + * The cases are: + * 1) Listen type is either 'socket' or 'none' - SPICE native + * OpenGL support (,gl=on) should be used because we're + * rendering on a local display. + * + * 2) Listen is either network or address - SPICE can't use + * the native OpenGL support remotely yet, so we use + * native='no' and format '-display egl-headless' onto the + * cmdline. + */ + if (graphics->gl && + graphics->gl->enable == VIR_TRISTATE_BOOL_YES && + graphics->gl->native == VIR_TRISTATE_BOOL_ABSENT) { + gl->native = VIR_TRISTATE_BOOL_NO; + + if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE || + glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET) + gl->native = VIR_TRISTATE_BOOL_YES; + }
So the upside/downside to setting something in the XML that wasn't previously set by the consumer is that "we're stuck with it" going forward, right? Additionally, we cannot ascertain whether we set native to NO or YES or if the consumer did.
Does it matter? If libvirt set it, then we set it in a way that is safe for us, IOW we can ensure the vm will start using that setting. If the consumer set it, then we evaluate the setting and either fail or proceed with starting up the vm.
To me in particular, no it doesn't matter, but I note it because setting a value however reasonable is something "of note". Does the domain "work" without us setting that or does it fail to start? IOW: Are things failing today/now without this being set?
So the "other" side of this is this is being done in a domain_conf function instead of some qemu device post parse function since if I'm reading the RFC comments right, the desire is not to somehow automagically add it, but rather force some usage.
Correct, although, it's not that our "automagic" was wrong (in fact it was safe), but it wasn't very flexible and 'egl-headless' can find use cases even without mdevs, that was the main driver to expose it.
The relationship to mdev/hostdev is what I assumed in the long run this was primarily geared towards. Having it without mdev would seem to be sub-optimal especially w/ the required network component. It's taking me a bit to figure this stuff out - you've been working with it, so I'm trying to catch up.
Still, I wonder if what's been done follows the spirit of Gerd's comment:
"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."
As I already explained in the cover letter reply, to me the facts that a standalone graphics type='headless' doesn't make sense, is basically just some kind of OpenGL switch and can't be combined with some of the other types felt like a wrong way of doing things, that's why I merged it with the 'gl' element. But okay, I'll give it a try on a separate branch and see how 'making it a graphics type' goes. The one thing that won't be necessary and worth doing anymore is separating the 'gl' data into a separate element, but I'm curious how much code will be saved (if any) if done this way.
Is it "fair" or "right" to say there's two ways to do gl support - one where Spice/VNC is connected locally and one where it's connected remotely. Then remotely is split into networked connection and (eventually in subsequent patches) vGPU connection? So you chose "native" to provide that functionality which gets some automagic setting because we've determined there to be a need. Whether that's because a domain fails to start or just is (well) brain dead is what isn't clear to me. Since we're not co-located - this is "my way" of talking through things to "learn" and make sure what you've developed matches what someone reading it "expected" as functionality. Automagic setting isn't inherently wrong, but sometimes it needs to be carefully looked at to make sure the reasons for doing it are "understandable". [...]
Do you think we need ABI checks for this? Could be challenging given the post parse setting of the value.
I don't see an ABI problem here, old libvirt doesn't know what to do with it (and VNC will just ignore the gl element) and new enough libvirt would ensure that -spice gl=on and -display egl-headless will not appear on the cmdline. Everything else is something that's been working for a very long time.
I'm thinking about going forward. Once this is "in", if someone changes the setting, then is that a problem ABI wise. You have a technology with a network or vGPU connection that could change if the native changes. Then of course there's the automagic setting. Of course there's no ABI checking done yet on graphics - so I could be off in the weeds. 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 + tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + 7 files changed, 8 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 37c8fbe3d3..b9cdee4ed8 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -501,6 +501,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 310 */ "sev-guest", "machine.pseries.cap-hpt-max-page-size", + "vfio-pci.display", ); @@ -1194,6 +1195,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 df9bf49abb..aeaed2343e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -485,6 +485,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 310 */ QEMU_CAPS_SEV_GUEST, /* -object sev-guest,... */ QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE, /* -machine pseries.cap-hpt-max-page-size */ + 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 ecc029f403..021db06b8e 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -169,6 +169,7 @@ <flag name='vhost-vsock'/> <flag name='chardev-fd-pass'/> <flag name='tpm-emulator'/> + <flag name='vfio-pci.display'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>347550</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 2ee582f343..5d702789bd 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -166,6 +166,7 @@ <flag name='vhost-vsock'/> <flag name='chardev-fd-pass'/> <flag name='tpm-emulator'/> + <flag name='vfio-pci.display'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>428334</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 87d189e58d..75121a7dc1 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -133,6 +133,7 @@ <flag name='vhost-vsock'/> <flag name='chardev-fd-pass'/> <flag name='tpm-emulator'/> + <flag name='vfio-pci.display'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>375999</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 9c1f6c327c..2bfdb8c047 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -211,6 +211,7 @@ <flag name='mch'/> <flag name='mch.extended-tseg-mbytes'/> <flag name='sev-guest'/> + <flag name='vfio-pci.display'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>416196</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml index 7e958b2efc..8ee4201d0f 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml @@ -166,6 +166,7 @@ <flag name='chardev-fd-pass'/> <flag name='tpm-emulator'/> <flag name='machine.pseries.cap-hpt-max-page-size'/> + <flag name='vfio-pci.display'/> <version>2012050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>446771</microcodeVersion> -- 2.14.4
On 06/27/2018 09:34 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 + tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + 7 files changed, 8 insertions(+)
Looks like you included the commit comment from the subsequent patch here.... All that needs to be indicated is add a check for the field. Reviewed-by: John Ferlan <jferlan@redhat.com> John
QEMU 2.12 introduced a new type of display for mediated devices using vfio-pci backend which allows a mediated device to be used as a VGA compatible device as an alternative to an emulated video device. QEMU exposes this feature via a vfio device property 'display' with supported values 'on/off/auto' (default is 'off'). This patch adds the necessary bits to domain config handling in order to expose this feature. Since there's no convenient way for libvirt to come up with usable defaults for the display setting, simply because libvirt is not able to figure out which of the display implementations - dma-buf which requires OpenGL support vs vfio regions which doesn't need OpenGL (works with OpenGL enabled too) - the underlying mdev uses. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 19 +++++- docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 18 +++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 71 +++++++++++++++++++++- .../hostdev-mdev-display-missing-graphics.xml | 35 +++++++++++ tests/qemuxml2argvdata/hostdev-mdev-display.xml | 39 ++++++++++++ .../hostdev-mdev-display-active.xml | 47 ++++++++++++++ .../hostdev-mdev-display-inactive.xml | 47 ++++++++++++++ tests/qemuxml2xmltest.c | 2 + 10 files changed, 279 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-inactive.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c6ebd39bd9..f45eee6812 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4502,9 +4502,22 @@ 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.6.0 (QEMU 2.12)</span> an optional + <code>display</code> attribute may be used 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'). It is required to use a + <a href="#elementsGraphics">graphical framebuffer</a> in order to + use this attribute, currently only supported with VNC and Spice + graphics devices. + <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 4ad53d976b..1df479cda2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4590,6 +4590,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 6d399a198e..23d646cb63 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7651,6 +7651,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; @@ -7670,6 +7671,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 @@ -7757,6 +7759,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) { @@ -26574,9 +26585,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 > VIR_TRISTATE_SWITCH_ABSENT) + 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 99b5896391..6b21d2bf4c 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 948b9b7fd0..d624383c61 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6127,6 +6127,72 @@ qemuDomainVsockDefPostParse(virDomainVsockDefPtr vsock) } +static int +qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def) +{ + if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT && + mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("<hostdev> attribute 'display' is only supported" + " with model='vfio-pci'")); + + return -1; + } + + if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + virDomainGraphicsDefPtr graphics = def->graphics[0]; + + if (def->ngraphics == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("graphics device is needed for attribute value " + "'display=on' in <hostdev>")); + return -1; + } + + /* We're not able to tell whether an mdev needs OpenGL or not at the + * moment, so print a warning that an extra <gl> element might be + * necessary to be added. + */ + if (!graphics->gl || + graphics->gl->enable != VIR_TRISTATE_BOOL_YES) { + VIR_WARN("<hostdev> attribute 'display' may need the OpenGL to " + "be enabled"); + } + } + + return 0; +} + + +static int +qemuDomainHostdevDefPostParse(const virDomainHostdevDef *hostdev, + const virDomainDef *def) +{ + const virDomainHostdevSubsysMediatedDev *mdevsrc; + + 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 qemuDomainHostdevMdevDefPostParse(mdevsrc, def); + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSubsysType, + hostdev->source.subsys.type); + return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -6177,11 +6243,14 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, ret = qemuDomainVsockDefPostParse(dev->data.vsock); break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + ret = qemuDomainHostdevDefPostParse(dev->data.hostdev, def); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: - case VIR_DOMAIN_DEVICE_HOSTDEV: case VIR_DOMAIN_DEVICE_WATCHDOG: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: 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.xml b/tests/qemuxml2argvdata/hostdev-mdev-display.xml new file mode 100644 index 0000000000..f5b3575c04 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display.xml @@ -0,0 +1,39 @@ +<domain type='qemu' id='1'> + <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-active.xml b/tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml new file mode 100644 index 0000000000..63a1a00278 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml @@ -0,0 +1,47 @@ +<domain type='qemu' id='1'> + <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/qemuxml2xmloutdata/hostdev-mdev-display-inactive.xml b/tests/qemuxml2xmloutdata/hostdev-mdev-display-inactive.xml new file mode 100644 index 0000000000..95a692aee5 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-mdev-display-inactive.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'> + <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 eac6d5b073..0095c27cf6 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -477,6 +477,8 @@ mymain(void) DO_TEST("hostdev-pci-address", NONE); DO_TEST("hostdev-vfio", NONE); DO_TEST("hostdev-mdev-precreated", NONE); + DO_TEST_FULL("hostdev-mdev-display", WHEN_ACTIVE, GIC_NONE, + 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.4
On 06/27/2018 09:34 AM, Erik Skultety wrote:
QEMU 2.12 introduced a new type of display for mediated devices using vfio-pci backend which allows a mediated device to be used as a VGA compatible device as an alternative to an emulated video device. QEMU exposes this feature via a vfio device property 'display' with supported values 'on/off/auto' (default is 'off').
This patch adds the necessary bits to domain config handling in order to expose this feature. Since there's no convenient way for libvirt to come up with usable defaults for the display setting, simply because libvirt is not able to figure out which of the display implementations - dma-buf which requires OpenGL support vs vfio regions which doesn't need OpenGL (works with OpenGL enabled too) - the underlying mdev uses.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 19 +++++- docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 18 +++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 71 +++++++++++++++++++++- .../hostdev-mdev-display-missing-graphics.xml | 35 +++++++++++ tests/qemuxml2argvdata/hostdev-mdev-display.xml | 39 ++++++++++++ .../hostdev-mdev-display-active.xml | 47 ++++++++++++++ .../hostdev-mdev-display-inactive.xml | 47 ++++++++++++++ tests/qemuxml2xmltest.c | 2 + 10 files changed, 279 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-inactive.xml
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d399a198e..23d646cb63 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7651,6 +7651,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; @@ -7670,6 +7671,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 @@ -7757,6 +7759,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) { @@ -26574,9 +26585,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 > VIR_TRISTATE_SWITCH_ABSENT)
Usually just != instead of >, but whatever.
+ virBufferAsprintf(buf, " display='%s'", + virTristateSwitchTypeToString(mdevsrc->display)); + } + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2);
Similar as before (and sorry if I asked the last time too), any ABI concerns? [...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 948b9b7fd0..d624383c61 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6127,6 +6127,72 @@ qemuDomainVsockDefPostParse(virDomainVsockDefPtr vsock) }
+static int +qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def) +{ + if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT &&
Hmmm... usually this only matters for display == YES (or != ABSENT), but then in the subsequent patch we generate NO by default. John
+ mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("<hostdev> attribute 'display' is only supported" + " with model='vfio-pci'")); + + return -1; + } + + if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + virDomainGraphicsDefPtr graphics = def->graphics[0]; + + if (def->ngraphics == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("graphics device is needed for attribute value " + "'display=on' in <hostdev>")); + return -1; + } + + /* We're not able to tell whether an mdev needs OpenGL or not at the + * moment, so print a warning that an extra <gl> element might be + * necessary to be added. + */ + if (!graphics->gl || + graphics->gl->enable != VIR_TRISTATE_BOOL_YES) { + VIR_WARN("<hostdev> attribute 'display' may need the OpenGL to " + "be enabled"); + } + } + + return 0; +} +
[...]
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 23 +++++++++++- src/qemu/qemu_domain.c | 3 +- .../hostdev-mdev-display-spice-egl-headless.args | 32 +++++++++++++++++ .../hostdev-mdev-display-spice-egl-headless.xml | 41 ++++++++++++++++++++++ .../hostdev-mdev-display-spice-opengl.args | 31 ++++++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 41 ++++++++++++++++++++++ .../hostdev-mdev-display-vnc-egl-headless.args | 32 +++++++++++++++++ .../hostdev-mdev-display-vnc-egl-headless.xml | 41 ++++++++++++++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 ++++++++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml | 39 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 29 +++++++++++++++ 11 files changed, 341 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fc80a6c3a6..a2a27b5b9b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5162,6 +5162,16 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def, virBufferAdd(&buf, dev_str, -1); virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath); + /* QEMU 2.12 added support for vfio-pci display type, we default to + * 'display=off' 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; + + if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT) + virBufferAsprintf(&buf, ",display=%s", + virTristateSwitchTypeToString(mdevsrc->display)); + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto cleanup; @@ -5379,7 +5389,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", @@ -5387,6 +5399,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, "supported by this version of QEMU")); return -1; } + + if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("display property of device vfio-pci is " + "not supported by this version of QEMU")); + return -1; + } + break; case VIR_MDEV_MODEL_TYPE_VFIO_CCW: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d624383c61..39920da442 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6141,7 +6141,7 @@ qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevs } if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { - virDomainGraphicsDefPtr graphics = def->graphics[0]; + virDomainGraphicsDefPtr graphics = NULL; if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -6154,6 +6154,7 @@ qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevs * moment, so print a warning that an extra <gl> element might be * necessary to be added. */ + graphics = def->graphics[0]; if (!graphics->gl || graphics->gl->enable != VIR_TRISTATE_BOOL_YES) { VIR_WARN("<hostdev> attribute 'display' may need the OpenGL to " diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args new file mode 100644 index 0000000000..8df306e229 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.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 \ +-display egl-headless \ +-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 diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml new file mode 100644 index 0000000000..3bf67e8b1b --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml @@ -0,0 +1,41 @@ +<domain type='qemu' id='1'> + <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' native='no'/> + </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.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-spice-opengl.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml new file mode 100644 index 0000000000..816930e2cf --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml @@ -0,0 +1,41 @@ +<domain type='qemu' id='1'> + <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' native='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-egl-headless.args b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.args new file mode 100644 index 0000000000..439e741b0d --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.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 \ +-display egl-headless \ +-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-egl-headless.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml new file mode 100644 index 0000000000..0fe79f9a28 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml @@ -0,0 +1,41 @@ +<domain type='qemu' id='1'> + <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'> + <gl enable='yes'/> + </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.args b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args new file mode 100644 index 0000000000..d1fa3e7b8c --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args @@ -0,0 +1,31 @@ +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 diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml new file mode 100644 index 0000000000..f5b3575c04 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml @@ -0,0 +1,39 @@ +<domain type='qemu' id='1'> + <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/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e82496d403..8293be949d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1582,6 +1582,35 @@ 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-egl-headless", + 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("hostdev-mdev-display-vnc-egl-headless", + 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.4
On 06/27/2018 09:34 AM, Erik Skultety wrote: Kinda sparse for what I assume is a new "feature" (and I won't comment about you're missing a news.xml change :-P)
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 23 +++++++++++- src/qemu/qemu_domain.c | 3 +- .../hostdev-mdev-display-spice-egl-headless.args | 32 +++++++++++++++++ .../hostdev-mdev-display-spice-egl-headless.xml | 41 ++++++++++++++++++++++ .../hostdev-mdev-display-spice-opengl.args | 31 ++++++++++++++++ .../hostdev-mdev-display-spice-opengl.xml | 41 ++++++++++++++++++++++ .../hostdev-mdev-display-vnc-egl-headless.args | 32 +++++++++++++++++ .../hostdev-mdev-display-vnc-egl-headless.xml | 41 ++++++++++++++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 ++++++++++++++++ .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml | 39 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 29 +++++++++++++++ 11 files changed, 341 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fc80a6c3a6..a2a27b5b9b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5162,6 +5162,16 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def, virBufferAdd(&buf, dev_str, -1); virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
+ /* QEMU 2.12 added support for vfio-pci display type, we default to + * 'display=off' to stay safe from future changes */
I have a vague recollection of a longer explanation for this during the previous review - maybe put some more details here why we had to use display=off, especially since the commit message is sparse.
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY) && + mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + mdevsrc->display = VIR_TRISTATE_SWITCH_OFF; + + if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT) + virBufferAsprintf(&buf, ",display=%s", + virTristateSwitchTypeToString(mdevsrc->display)); + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto cleanup;
@@ -5379,7 +5389,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", @@ -5387,6 +5399,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, "supported by this version of QEMU")); return -1; } + + if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("display property of device vfio-pci is " + "not supported by this version of QEMU")); + return -1;
s/ // Too many spaces for return -1
+ } + break; case VIR_MDEV_MODEL_TYPE_VFIO_CCW: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d624383c61..39920da442 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6141,7 +6141,7 @@ qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevs }
if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { - virDomainGraphicsDefPtr graphics = def->graphics[0]; + virDomainGraphicsDefPtr graphics = NULL;
if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -6154,6 +6154,7 @@ qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevs * moment, so print a warning that an extra <gl> element might be * necessary to be added. */ + graphics = def->graphics[0]; if (!graphics->gl || graphics->gl->enable != VIR_TRISTATE_BOOL_YES) { VIR_WARN("<hostdev> attribute 'display' may need the OpenGL to "
Thus hunk belongs in the previous patch... John [...] FYI: The examples certainly help point out usage of gl enable=yes conditions and when egl-headless comes into play, I'm still confused over the RFC comments, but that shouldn't really surprise anyone ;-)
On Wed, Jun 27, 2018 at 03:34:37PM +0200, Erik Skultety wrote:
See the RFC here: https://www.redhat.com/archives/libvir-list/2018-May/msg02218.html
Since RFC: - split graphics 'gl' to a standalone structure since it's now SPICE, SDL, *and* VNC that will support it
- egl-headless support for VNC, since VNC doesn't support OpenGL natively like SPICE does
- added a new attribute 'native' for spice which will instruct libvirt to use '-display egl-headless' instead of libvirt trying to figure this on out by itself, since egl-headless might have other uses besides mdev with VNC
Can you elaborate on this part, as it feels a bit odd to me. Currently -display accepts vnc or spice or sdl or gtk or none Each of these -display options corresponds to a different <graphics type=XXX/> element in libvirt XML, except 'none' which corresponds to no <graphics> at all. We already allow multiple <graphics> elements to turn on say both SPICE and VNC at once. So why would we want to treat egl-headless differently as an attribute under the SPICE graphics, as opposed to just having <graphics type=egl-headless> as a thing in its own right. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jun 27, 2018 at 02:53:28PM +0100, Daniel P. Berrangé wrote:
On Wed, Jun 27, 2018 at 03:34:37PM +0200, Erik Skultety wrote:
See the RFC here: https://www.redhat.com/archives/libvir-list/2018-May/msg02218.html
Since RFC: - split graphics 'gl' to a standalone structure since it's now SPICE, SDL, *and* VNC that will support it
- egl-headless support for VNC, since VNC doesn't support OpenGL natively like SPICE does
- added a new attribute 'native' for spice which will instruct libvirt to use '-display egl-headless' instead of libvirt trying to figure this on out by itself, since egl-headless might have other uses besides mdev with VNC
Can you elaborate on this part, as it feels a bit odd to me.
Currently -display accepts vnc or spice or sdl or gtk or none
Each of these -display options corresponds to a different <graphics type=XXX/> element in libvirt XML, except 'none' which corresponds to no <graphics> at all.
We already allow multiple <graphics> elements to turn on say both SPICE and VNC at once.
So why would we want to treat egl-headless differently as an attribute under the SPICE graphics, as opposed to just having <graphics type=egl-headless> as a thing in its own right.
Egl-headless doesn't make sense on its own, neither does it make sense with anything else but SPICE and VNC. And since we're talking about dri render nodes, I figured it would be better to design this as an attribute to the gl element rather than a standalone element. Erik
Hi,
So why would we want to treat egl-headless differently as an attribute under the SPICE graphics, as opposed to just having <graphics type=egl-headless> as a thing in its own right.
Egl-headless doesn't make sense on its own, neither does it make sense with anything else but SPICE and VNC.
Well, you can run a vm with egl-headless and without vnc/spice just fine. Might even make sense to do that for automated QA runs as you can get the display content using the screendump monitor command. So the "<graphics type=egl-headless>" suggestion looks good to me. cheers, Gerd
On Mon, Jul 02, 2018 at 01:34:15PM +0200, Gerd Hoffmann wrote:
Hi,
So why would we want to treat egl-headless differently as an attribute under the SPICE graphics, as opposed to just having <graphics type=egl-headless> as a thing in its own right.
Egl-headless doesn't make sense on its own, neither does it make sense with anything else but SPICE and VNC.
Well, you can run a vm with egl-headless and without vnc/spice just fine. Might even make sense to do that for automated QA runs as you can get the display content using the screendump monitor command.
So the "<graphics type=egl-headless>" suggestion looks good to me.
cheers, Gerd
Hi, so I incorporated the suggestions and sent a v2 where egl-headless is a standalone graphics element. Erik
participants (4)
-
Daniel P. Berrangé -
Erik Skultety -
Gerd Hoffmann -
John Ferlan