[libvirt] [PATCH v3 00/11] Autoselect a DRM node for egl-headless add it to cmdline
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628892. The problem is that we didn't put the DRI device into the namespace for QEMU to access, but that was only a part of the issue. The other part of the issue is that QEMU doesn't support specifying 'rendernode' for egl-headless yet (patches are already in upstream) Instead, QEMU's been autoselecting the DRI device on its own. There's no compelling reason for libvirt not doing that instead and thus prevent any permission-related issues. Since v1: - updated capabilities to 3.1.0-rc2 containing the necessary QEMU patches - provided more test cases as requested - added a new XML sub-element <gl> for egl-headless graphics type Since v2: - The cmdline code wouldn't play nicely with old QEMU where I'd pick a rendernode automatically and then fail if we didn't have the corresponding capability, so this was fixed for v3 - v2 converted some tests to CAPS_LATEST only which would also be wrong because QEMU can still pick a DRM node so that's a valid use case (not a practical one though) - the 3.1.0 capabilities patch was merged separately Erik Skultety (11): util: Introduce virHostGetDRMRenderNode helper conf: Introduce virDomainGraphics-related helpers qemu: process: spice: Pick the first available DRM render node qemu: command: Introduce qemuBuildGraphicsEGLHeadlessCommandLine helper qemu: caps: Introduce QEMU_EGL_HEADLESS_RENDERNODE capability conf: gfx: Add egl-headless as a member to virDomainGraphicsDef struct conf: gfx: egl-headless: Introduce a new <gl> subelement qemu: domain: egl-headless: Add the DRI device into the namespace qemu: cgroup: gfx: egl-headless: Add the DRI device into the cgroup list security: dac: gfx: egl-headless: Relabel the DRI device qemu: command: gfx: egl-headless: Add 'rendernode' option to the cmdline docs/formatdomain.html.in | 11 ++- docs/schemas/domaincommon.rng | 17 +++- src/conf/domain_conf.c | 84 +++++++++++++++++++ src/conf/domain_conf.h | 12 +++ src/libvirt_private.syms | 4 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_cgroup.c | 10 +-- src/qemu/qemu_command.c | 42 +++++++++- src/qemu/qemu_domain.c | 9 +- src/qemu/qemu_process.c | 32 ++++++- src/security/security_dac.c | 15 ++-- src/util/virutil.c | 53 ++++++++++++ src/util/virutil.h | 2 + .../caps_3.1.0.x86_64.xml | 1 + ...egl-headless-rendernode.x86_64-latest.args | 31 +++++++ .../graphics-egl-headless-rendernode.xml | 33 ++++++++ .../graphics-egl-headless.x86_64-latest.args | 31 +++++++ .../graphics-spice-gl-no-rendernode.args | 25 ++++++ .../graphics-spice-gl-no-rendernode.xml | 24 ++++++ ...play-spice-egl-headless.x86_64-latest.args | 2 +- ...isplay-vnc-egl-headless.x86_64-latest.args | 2 +- tests/qemuxml2argvmock.c | 9 ++ tests/qemuxml2argvtest.c | 2 + .../graphics-egl-headless-rendernode.xml | 41 +++++++++ tests/qemuxml2xmltest.c | 2 + 26 files changed, 464 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless-rendernode.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless-rendernode.xml create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml create mode 100644 tests/qemuxml2xmloutdata/graphics-egl-headless-rendernode.xml -- 2.19.1
This is the first step towards libvirt picking the first available render node instead of QEMU. It also makes sense for us to be able to do that, since we allow specifying the node directly for SPICE, so if there's no render node specified by the user, we should pick the first available one. The algorithm used for that is essentially the same as the one QEMU uses. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 3 files changed, 56 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5018a13e9c..c7f08f9620 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3151,6 +3151,7 @@ virGetUserName; virGetUserRuntimeDirectory; virGetUserShell; virHexToBin; +virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; diff --git a/src/util/virutil.c b/src/util/virutil.c index 974cffc2ee..da12a11e04 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2145,3 +2145,56 @@ virHostHasIOMMU(void) VIR_DIR_CLOSE(iommuDir); return ret; } + + +/** + * virHostGetDRMRenderNode: + * + * Picks the first DRM render node available. Missing DRI or missing DRM render + * nodes in the system results in an error. + * + * Returns an absolute path to the first render node available or NULL in case + * of an error with the error being reported. + * Caller is responsible for freeing the result string. + * + */ +char * +virHostGetDRMRenderNode(void) +{ + char *ret = NULL; + DIR *driDir = NULL; + const char *driPath = "/dev/dri"; + struct dirent *ent = NULL; + int dirErr = 0; + bool have_rendernode = false; + + if (virDirOpen(&driDir, driPath) < 0) + return NULL; + + while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) { + if (ent->d_type != DT_CHR) + continue; + + if (STRPREFIX(ent->d_name, "renderD")) { + have_rendernode = true; + break; + } + } + + if (dirErr < 0) + goto cleanup; + + /* even if /dev/dri exists, there might be no renderDX nodes available */ + if (!have_rendernode) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("No DRM render nodes available")); + goto cleanup; + } + + if (virAsprintf(&ret, "%s/%s", driPath, ent->d_name) < 0) + goto cleanup; + + cleanup: + VIR_DIR_CLOSE(driDir); + return ret; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index e0ab0da0f2..89bd21b148 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -222,6 +222,8 @@ unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE; bool virHostHasIOMMU(void); +char *virHostGetDRMRenderNode(void); + /** * VIR_ASSIGN_IS_OVERFLOW: * @rvalue: value that is checked (evaluated twice) -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
This is the first step towards libvirt picking the first available render node instead of QEMU. It also makes sense for us to be able to do that, since we allow specifying the node directly for SPICE, so if there's no render node specified by the user, we should pick the first available one. The algorithm used for that is essentially the same as the one QEMU uses.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 3 files changed, 56 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
This is the first step towards libvirt picking the first available render node instead of QEMU. It also makes sense for us to be able to do that, since we allow specifying the node directly for SPICE, so if there's no render node specified by the user, we should pick the first available one. The algorithm used for that is essentially the same as the one QEMU uses.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 3 files changed, 56 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5018a13e9c..c7f08f9620 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3151,6 +3151,7 @@ virGetUserName; virGetUserRuntimeDirectory; virGetUserShell; virHexToBin; +virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; diff --git a/src/util/virutil.c b/src/util/virutil.c index 974cffc2ee..da12a11e04 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2145,3 +2145,56 @@ virHostHasIOMMU(void) VIR_DIR_CLOSE(iommuDir); return ret; } + + +/** + * virHostGetDRMRenderNode: + * + * Picks the first DRM render node available. Missing DRI or missing DRM render + * nodes in the system results in an error. + * + * Returns an absolute path to the first render node available or NULL in case + * of an error with the error being reported. + * Caller is responsible for freeing the result string. + * + */ +char * +virHostGetDRMRenderNode(void) +{ + char *ret = NULL; + DIR *driDir = NULL; + const char *driPath = "/dev/dri"; + struct dirent *ent = NULL; + int dirErr = 0; + bool have_rendernode = false; + + if (virDirOpen(&driDir, driPath) < 0) + return NULL; + + while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) { + if (ent->d_type != DT_CHR) + continue;
The 'd_type' field is a Linux-ism, so this has broken the build Is it even needed ? ie are then any files in /dev/dri that have a prefix of "renderD" but which are not character devices ?
+ + if (STRPREFIX(ent->d_name, "renderD")) { + have_rendernode = true; + break; + } + }
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 Mon, Dec 03, 2018 at 03:18:47PM +0000, Daniel P. Berrangé wrote:
On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
This is the first step towards libvirt picking the first available render node instead of QEMU. It also makes sense for us to be able to do that, since we allow specifying the node directly for SPICE, so if there's no render node specified by the user, we should pick the first available one. The algorithm used for that is essentially the same as the one QEMU uses.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 3 files changed, 56 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5018a13e9c..c7f08f9620 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3151,6 +3151,7 @@ virGetUserName; virGetUserRuntimeDirectory; virGetUserShell; virHexToBin; +virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; diff --git a/src/util/virutil.c b/src/util/virutil.c index 974cffc2ee..da12a11e04 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2145,3 +2145,56 @@ virHostHasIOMMU(void) VIR_DIR_CLOSE(iommuDir); return ret; } + + +/** + * virHostGetDRMRenderNode: + * + * Picks the first DRM render node available. Missing DRI or missing DRM render + * nodes in the system results in an error. + * + * Returns an absolute path to the first render node available or NULL in case + * of an error with the error being reported. + * Caller is responsible for freeing the result string. + * + */ +char * +virHostGetDRMRenderNode(void) +{ + char *ret = NULL; + DIR *driDir = NULL; + const char *driPath = "/dev/dri"; + struct dirent *ent = NULL; + int dirErr = 0; + bool have_rendernode = false; + + if (virDirOpen(&driDir, driPath) < 0) + return NULL; + + while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) { + if (ent->d_type != DT_CHR) + continue;
The 'd_type' field is a Linux-ism, so this has broken the build
Is it even needed ? ie are then any files in /dev/dri that have a prefix of "renderD" but which are not character devices ?
Can't say it's impossible, but I haven't bumped into a setup like that - I had different permissions, I had DRI missing, I had renderD missing, but I haven't seen a non-character DRI node...You're right in the thinking, the reason why I put it in there is to spare a few cycles wasted on string comparison, so either I can shrug my shoulders, remove the check, waste the cycles and be happy or I can have a look whether gnulib can help us out here. Erik
On Mon, Dec 03, 2018 at 04:24:18PM +0100, Erik Skultety wrote:
On Mon, Dec 03, 2018 at 03:18:47PM +0000, Daniel P. Berrangé wrote:
On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
This is the first step towards libvirt picking the first available render node instead of QEMU. It also makes sense for us to be able to do that, since we allow specifying the node directly for SPICE, so if there's no render node specified by the user, we should pick the first available one. The algorithm used for that is essentially the same as the one QEMU uses.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 3 files changed, 56 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5018a13e9c..c7f08f9620 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3151,6 +3151,7 @@ virGetUserName; virGetUserRuntimeDirectory; virGetUserShell; virHexToBin; +virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; diff --git a/src/util/virutil.c b/src/util/virutil.c index 974cffc2ee..da12a11e04 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2145,3 +2145,56 @@ virHostHasIOMMU(void) VIR_DIR_CLOSE(iommuDir); return ret; } + + +/** + * virHostGetDRMRenderNode: + * + * Picks the first DRM render node available. Missing DRI or missing DRM render + * nodes in the system results in an error. + * + * Returns an absolute path to the first render node available or NULL in case + * of an error with the error being reported. + * Caller is responsible for freeing the result string. + * + */ +char * +virHostGetDRMRenderNode(void) +{ + char *ret = NULL; + DIR *driDir = NULL; + const char *driPath = "/dev/dri"; + struct dirent *ent = NULL; + int dirErr = 0; + bool have_rendernode = false; + + if (virDirOpen(&driDir, driPath) < 0) + return NULL; + + while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) { + if (ent->d_type != DT_CHR) + continue;
The 'd_type' field is a Linux-ism, so this has broken the build
Is it even needed ? ie are then any files in /dev/dri that have a prefix of "renderD" but which are not character devices ?
Can't say it's impossible, but I haven't bumped into a setup like that - I had different permissions, I had DRI missing, I had renderD missing, but I haven't seen a non-character DRI node...You're right in the thinking, the reason why I put it in there is to spare a few cycles wasted on string comparison, so either I can shrug my shoulders, remove the check, waste the cycles and be happy or I can have a look whether gnulib can help us out here.
gnulib can't fix this unfortunately. Without d_type being reported by the kernel, the only way gnulib could "fix" it would be to call stat() on every dirent result which would be horrifically inefficient for users who don't care about the d_type. 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 :|
A few simple helpers that allow us to determine whether a graphics can and will need to make use of a DRM render node. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 41 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++++ src/libvirt_private.syms | 3 +++ 3 files changed, 53 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a7ab3e26f1..6ae98435cc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30933,3 +30933,44 @@ virDomainGraphicsDefHasOpenGL(const virDomainDef *def) return false; } + + +bool +virDomainGraphicsSupportsRenderNode(const virDomainGraphicsDef *graphics) +{ + return graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE; +} + + +const char * +virDomainGraphicsGetRenderNode(const virDomainGraphicsDef *graphics) +{ + const char *ret = NULL; + + switch (graphics->type) { + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) + ret = graphics->data.spice.rendernode; + break; + case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS: + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + break; + } + + return ret; +} + + +bool +virDomainGraphicsNeedsRenderNode(const virDomainGraphicsDef *graphics) +{ + if (!virDomainGraphicsSupportsRenderNode(graphics) || + virDomainGraphicsGetRenderNode(graphics)) + return false; + + return true; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7a724fbc6f..39dc33db8f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3668,4 +3668,13 @@ virDomainDefHasManagedPR(const virDomainDef *def); bool virDomainGraphicsDefHasOpenGL(const virDomainDef *def); +bool +virDomainGraphicsSupportsRenderNode(const virDomainGraphicsDef *graphics); + +const char * +virDomainGraphicsGetRenderNode(const virDomainGraphicsDef *graphics); + +bool +virDomainGraphicsNeedsRenderNode(const virDomainGraphicsDef *graphics); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c7f08f9620..d34a387485 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -372,8 +372,10 @@ virDomainGraphicsAuthConnectedTypeToString; virDomainGraphicsDefFree; virDomainGraphicsDefHasOpenGL; virDomainGraphicsGetListen; +virDomainGraphicsGetRenderNode; virDomainGraphicsListenAppendAddress; virDomainGraphicsListenAppendSocket; +virDomainGraphicsNeedsRenderNode; virDomainGraphicsSpiceChannelModeTypeFromString; virDomainGraphicsSpiceChannelModeTypeToString; virDomainGraphicsSpiceChannelNameTypeFromString; @@ -388,6 +390,7 @@ virDomainGraphicsSpiceStreamingModeTypeFromString; virDomainGraphicsSpiceStreamingModeTypeToString; virDomainGraphicsSpiceZlibCompressionTypeFromString; virDomainGraphicsSpiceZlibCompressionTypeToString; +virDomainGraphicsSupportsRenderNode; virDomainGraphicsTypeFromString; virDomainGraphicsTypeToString; virDomainGraphicsVNCSharePolicyTypeFromString; -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:12PM +0100, Erik Skultety wrote:
A few simple helpers that allow us to determine whether a graphics can and will need to make use of a DRM render node.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 41 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++++ src/libvirt_private.syms | 3 +++ 3 files changed, 53 insertions(+)
+bool +virDomainGraphicsNeedsRenderNode(const virDomainGraphicsDef *graphics)
Consider: NeedsAutoRenderNode (in a way graphics with a rendernode already specified also needs it)
+{ + if (!virDomainGraphicsSupportsRenderNode(graphics) || + virDomainGraphicsGetRenderNode(graphics)) + return false;
Personally I'd decouple these two conditions.
+ + return true; +}
Regardless: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Up until now, we formatted 'rendernode=' onto QEMU cmdline only if the user specified it in the XML, otherwise we let QEMU do it for us. This causes permission issues because by default the /dev/dri/renderDX permissions are as follows: crw-rw----. 1 root video There's literally no reason why it shouldn't be libvirt picking the DRM render node instead of QEMU, that way (and because we're using namespaces by default), we can safely relabel the device within the namespace. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_process.c | 22 +++++++++++++++- src/util/virutil.h | 2 +- .../graphics-spice-gl-no-rendernode.args | 25 +++++++++++++++++++ .../graphics-spice-gl-no-rendernode.xml | 24 ++++++++++++++++++ tests/qemuxml2argvmock.c | 9 +++++++ 5 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 12d1fca0d4..feebdc7fdc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4784,9 +4784,25 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver, } +static int +qemuProcessGraphicsSetupRenderNode(virDomainGraphicsDefPtr graphics, + virQEMUCapsPtr qemuCaps) +{ + /* Don't bother picking a DRM node if QEMU doesn't support it. */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) + return 0; + + if (!(graphics->data.spice.rendernode = virHostGetDRMRenderNode())) + return -1; + + return 0; +} + + static int qemuProcessSetupGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, + virQEMUCapsPtr qemuCaps, unsigned int flags) { virDomainGraphicsDefPtr graphics; @@ -4797,6 +4813,10 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ngraphics; i++) { graphics = vm->def->graphics[i]; + if (virDomainGraphicsNeedsRenderNode(graphics) && + qemuProcessGraphicsSetupRenderNode(graphics, qemuCaps) < 0) + goto cleanup; + if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0) goto cleanup; } @@ -5953,7 +5973,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, goto cleanup; VIR_DEBUG("Setting graphics devices"); - if (qemuProcessSetupGraphics(driver, vm, flags) < 0) + if (qemuProcessSetupGraphics(driver, vm, priv->qemuCaps, flags) < 0) goto cleanup; VIR_DEBUG("Create domain masterKey"); diff --git a/src/util/virutil.h b/src/util/virutil.h index 89bd21b148..588d779d10 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -222,7 +222,7 @@ unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE; bool virHostHasIOMMU(void); -char *virHostGetDRMRenderNode(void); +char *virHostGetDRMRenderNode(void) ATTRIBUTE_NOINLINE; /** * VIR_ASSIGN_IS_OVERFLOW: diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args new file mode 100644 index 0000000000..1b08811692 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args @@ -0,0 +1,25 @@ +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 \ +-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-no-rendernode.xml b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml new file mode 100644 index 0000000000..b48e7bc94e --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.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'/> + </graphics> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 79152d928e..a64cd955c4 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -184,6 +184,15 @@ virNetDevRunEthernetScript(const char *ifname ATTRIBUTE_UNUSED, return 0; } +char * +virHostGetDRMRenderNode(void) +{ + char *dst = NULL; + + ignore_value(VIR_STRDUP(dst, "/dev/dri/foo")); + return dst; +} + static void (*real_virCommandPassFD)(virCommandPtr cmd, int fd, unsigned int flags); static const int testCommandPassSafeFDs[] = { 1730, 1731 }; -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:13PM +0100, Erik Skultety wrote:
Up until now, we formatted 'rendernode=' onto QEMU cmdline only if the user specified it in the XML, otherwise we let QEMU do it for us. This causes permission issues because by default the /dev/dri/renderDX permissions are as follows:
crw-rw----. 1 root video
There's literally no reason why it shouldn't be libvirt picking the DRM render node instead of QEMU, that way (and because we're using namespaces by default), we can safely relabel the device within the namespace.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_process.c | 22 +++++++++++++++- src/util/virutil.h | 2 +- .../graphics-spice-gl-no-rendernode.args | 25 +++++++++++++++++++ .../graphics-spice-gl-no-rendernode.xml | 24 ++++++++++++++++++ tests/qemuxml2argvmock.c | 9 +++++++ 5 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 12d1fca0d4..feebdc7fdc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4784,9 +4784,25 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver, }
+static int +qemuProcessGraphicsSetupRenderNode(virDomainGraphicsDefPtr graphics, + virQEMUCapsPtr qemuCaps) +{ + /* Don't bother picking a DRM node if QEMU doesn't support it. */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) + return 0; + + if (!(graphics->data.spice.rendernode = virHostGetDRMRenderNode())) + return -1; + + return 0; +} + + static int qemuProcessSetupGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, + virQEMUCapsPtr qemuCaps, unsigned int flags) { virDomainGraphicsDefPtr graphics; @@ -4797,6 +4813,10 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ngraphics; i++) { graphics = vm->def->graphics[i];
+ if (virDomainGraphicsNeedsRenderNode(graphics) &&
Just like the call below is called even for graphics with no listen, you can call qemuProcessGraphicsSetupRenderNode unconditionally and move the virDomainGraphicsNeedsRenderNode condition inside.
+ qemuProcessGraphicsSetupRenderNode(graphics, qemuCaps) < 0) + goto cleanup; + if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0) goto cleanup; } @@ -5953,7 +5973,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, goto cleanup;
VIR_DEBUG("Setting graphics devices"); - if (qemuProcessSetupGraphics(driver, vm, flags) < 0) + if (qemuProcessSetupGraphics(driver, vm, priv->qemuCaps, flags) < 0) goto cleanup;
VIR_DEBUG("Create domain masterKey"); diff --git a/src/util/virutil.h b/src/util/virutil.h index 89bd21b148..588d779d10 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -222,7 +222,7 @@ unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
bool virHostHasIOMMU(void);
-char *virHostGetDRMRenderNode(void); +char *virHostGetDRMRenderNode(void) ATTRIBUTE_NOINLINE;
/** * VIR_ASSIGN_IS_OVERFLOW: diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args new file mode 100644 index 0000000000..1b08811692 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args
A test called '-no-rendernode'... [...]
+-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-usb \ +-spice port=0,gl=on,rendernode=/dev/dri/foo \
... with a rendernode on the command line. How about '-auto-rendernode'?
+-vga cirrus \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
The test cases are not used anywhere. Please use DO_TEST_CAPS_LATEST when adding them. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We're going to need a bit more logic for egl-headless down the road so prepare a helper just like for the other display types. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 315419c71b..34c8ad751b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8307,6 +8307,19 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } +static int +qemuBuildGraphicsEGLHeadlessCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, + virCommandPtr cmd, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, + virDomainGraphicsDefPtr graphics ATTRIBUTE_UNUSED) +{ + virCommandAddArg(cmd, "-display"); + virCommandAddArg(cmd, "egl-headless"); + + return 0; +} + + static int qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virCommandPtr cmd, @@ -8338,8 +8351,9 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, break; case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS: - virCommandAddArg(cmd, "-display"); - virCommandAddArg(cmd, "egl-headless"); + if (qemuBuildGraphicsEGLHeadlessCommandLine(cfg, cmd, + qemuCaps, graphics) < 0) + return -1; break; case VIR_DOMAIN_GRAPHICS_TYPE_RDP: -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:14PM +0100, Erik Skultety wrote:
We're going to need a bit more logic for egl-headless down the road so prepare a helper just like for the other display types.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Now that we have QAPI introspection of display types in QEMU upstream, we can check whether the 'rendernode' option is supported with egl-headless display type. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 20a1a0c201..820cddfe04 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -516,6 +516,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "memory-backend-memfd.hugetlb", "iothread.poll-max-ns", "machine.pseries.cap-nested-hv", + "egl-headless.rendernode" ); @@ -1249,6 +1250,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "screendump/arg-type/device", QEMU_CAPS_SCREENDUMP_DEVICE }, { "block-commit/arg-type/*top", QEMU_CAPS_ACTIVE_COMMIT }, { "query-iothreads/ret-type/poll-max-ns", QEMU_CAPS_IOTHREAD_POLLING }, + { "query-display-options/ret-type/+egl-headless/rendernode", QEMU_CAPS_EGL_HEADLESS_RENDERNODE }, }; typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b1990b6bb8..c109887c0c 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -500,6 +500,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_OBJECT_MEMORY_MEMFD_HUGETLB, /* -object memory-backend-memfd.hugetlb */ QEMU_CAPS_IOTHREAD_POLLING, /* -object iothread.poll-max-ns */ QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV, /* -machine pseries.cap-nested-hv */ + QEMU_CAPS_EGL_HEADLESS_RENDERNODE, /* -display egl-headless,rendernode= */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml index cbebc095bd..6c9c0c6612 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml @@ -208,6 +208,7 @@ <flag name='memory-backend-memfd'/> <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> + <flag name='egl-headless.rendernode'/> <version>3000092</version> <kvmVersion>0</kvmVersion> <microcodeVersion>440395</microcodeVersion> -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:15PM +0100, Erik Skultety wrote:
Now that we have QAPI introspection of display types in QEMU upstream, we can check whether the 'rendernode' option is supported with egl-headless display type.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Since we need to specify the rendernode option onto QEMU cmdline, we need this union member to retain consistency in how we build the cmdline. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 39dc33db8f..9ddfef478b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1659,6 +1659,9 @@ struct _virDomainGraphicsDef { virTristateBool gl; char *rendernode; } spice; + struct { + char *rendernode; + } egl_headless; } data; /* nListens, listens, and *port are only useful if type is vnc, * rdp, or spice. They've been extracted from the union only to -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:16PM +0100, Erik Skultety wrote:
Since we need to specify the rendernode option onto QEMU cmdline, we need this union member to retain consistency in how we build the cmdline.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.h | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Unlike with SPICE and SDL which use the <gl> subelement to enable OpenGL acceleration, specifying egl-headless graphics in the XML has essentially the same meaning, thus in case of egl-headless we don't have a need for the 'enable' element attribute and we'll only be interested in the 'rendernode' one further down the road. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 11 +++-- docs/schemas/domaincommon.rng | 17 +++++-- src/conf/domain_conf.c | 45 ++++++++++++++++++- src/qemu/qemu_process.c | 16 +++++-- .../graphics-egl-headless-rendernode.xml | 33 ++++++++++++++ .../graphics-egl-headless-rendernode.xml | 41 +++++++++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 155 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless-rendernode.xml create mode 100644 tests/qemuxml2xmloutdata/graphics-egl-headless-rendernode.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 84259c45e4..428b0e8bb5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6704,12 +6704,17 @@ qemu-kvm -net nic,model=? /dev/null the other types, for practical reasons it should be paired with either <code>vnc</code> or <code>spice</code> graphics types. This display type is only supported by QEMU domains - (needs QEMU <span class="since">2.10</span> or newer) and doesn't - accept any attributes. + (needs QEMU <span class="since">2.10</span> or newer). + <span class="Since">5.0.0</span> this element accepts a + <code><gl/></code> sub-element with an optional attribute + <code>rendernode</code> which can be used to specify an absolute + path to a host's DRI device to be used for OpenGL rendering. </p> <pre> <graphics type='spice' autoport='yes'/> -<graphics type='egl-headless'/> +<graphics type='egl-headless'> + <gl rendernode='/dev/dri/renderD128'/> +</graphics> </pre> </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cb2ca5a20a..5a6c48f1aa 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3418,9 +3418,20 @@ </attribute> </optional> </group> - <attribute name="type"> - <value>egl-headless</value> - </attribute> + <group> + <attribute name="type"> + <value>egl-headless</value> + </attribute> + <optional> + <element name="gl"> + <optional> + <attribute name="rendernode"> + <ref name="absFilePath"/> + </attribute> + </optional> + </element> + </optional> + </group> </choice> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6ae98435cc..86917596c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14074,6 +14074,24 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, } +static int +virDomainGraphicsDefParseXMLEGLHeadless(virDomainGraphicsDefPtr def, + xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr save = ctxt->node; + xmlNodePtr glNode; + + ctxt->node = node; + + if ((glNode = virXPathNode("./gl", ctxt))) + def->data.egl_headless.rendernode = virXMLPropString(glNode, + "rendernode"); + ctxt->node = save; + return 0; +} + + /* Parse the XML definition for a graphics device */ static virDomainGraphicsDefPtr virDomainGraphicsDefParseXML(xmlNodePtr node, @@ -14123,6 +14141,9 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, goto error; break; case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS: + if (virDomainGraphicsDefParseXMLEGLHeadless(def, node, ctxt) < 0) + goto error; + break; case VIR_DOMAIN_GRAPHICS_TYPE_LAST: break; } @@ -26852,6 +26873,20 @@ virDomainGraphicsDefFormat(virBufferPtr buf, break; case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS: + if (!def->data.egl_headless.rendernode) + break; + + if (!children) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + children = true; + } + + virBufferAddLit(buf, "<gl"); + virBufferEscapeString(buf, " rendernode='%s'", + def->data.egl_headless.rendernode); + virBufferAddLit(buf, "/>\n"); + break; case VIR_DOMAIN_GRAPHICS_TYPE_LAST: break; } @@ -30938,7 +30973,13 @@ virDomainGraphicsDefHasOpenGL(const virDomainDef *def) bool virDomainGraphicsSupportsRenderNode(const virDomainGraphicsDef *graphics) { - return graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE; + bool ret = false; + + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE || + graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS) + ret = true; + + return ret; } @@ -30953,6 +30994,8 @@ virDomainGraphicsGetRenderNode(const virDomainGraphicsDef *graphics) ret = graphics->data.spice.rendernode; break; case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS: + ret = graphics->data.egl_headless.rendernode; + break; case VIR_DOMAIN_GRAPHICS_TYPE_SDL: case VIR_DOMAIN_GRAPHICS_TYPE_VNC: case VIR_DOMAIN_GRAPHICS_TYPE_RDP: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index feebdc7fdc..5e33c63c73 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4788,11 +4788,21 @@ static int qemuProcessGraphicsSetupRenderNode(virDomainGraphicsDefPtr graphics, virQEMUCapsPtr qemuCaps) { + char **rendernode = NULL; /* Don't bother picking a DRM node if QEMU doesn't support it. */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) - return 0; + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) + return 0; - if (!(graphics->data.spice.rendernode = virHostGetDRMRenderNode())) + rendernode = &graphics->data.spice.rendernode; + } else { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_EGL_HEADLESS_RENDERNODE)) + return 0; + + rendernode = &graphics->data.egl_headless.rendernode; + } + + if (!(*rendernode = virHostGetDRMRenderNode())) return -1; return 0; diff --git a/tests/qemuxml2argvdata/graphics-egl-headless-rendernode.xml b/tests/qemuxml2argvdata/graphics-egl-headless-rendernode.xml new file mode 100644 index 0000000000..a8d54e75da --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-egl-headless-rendernode.xml @@ -0,0 +1,33 @@ +<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='egl-headless'> + <gl rendernode='/dev/dri/foo'/> + </graphics> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/graphics-egl-headless-rendernode.xml b/tests/qemuxml2xmloutdata/graphics-egl-headless-rendernode.xml new file mode 100644 index 0000000000..9b7ac89928 --- /dev/null +++ b/tests/qemuxml2xmloutdata/graphics-egl-headless-rendernode.xml @@ -0,0 +1,41 @@ +<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'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='egl-headless'> + <gl rendernode='/dev/dri/foo'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2527497675..25ab990a4b 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -413,6 +413,8 @@ mymain(void) cfg->spiceAutoUnixSocket = false; DO_TEST("graphics-spice-egl-headless", NONE); + DO_TEST("graphics-egl-headless-rendernode", NONE); + DO_TEST("input-usbmouse", NONE); DO_TEST("input-usbtablet", NONE); DO_TEST("misc-acpi", NONE); -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:17PM +0100, Erik Skultety wrote:
Unlike with SPICE and SDL which use the <gl> subelement to enable OpenGL acceleration, specifying egl-headless graphics in the XML has essentially the same meaning, thus in case of egl-headless we don't have a need for the 'enable' element attribute and we'll only be interested in the 'rendernode' one further down the road.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/formatdomain.html.in | 11 +++-- docs/schemas/domaincommon.rng | 17 +++++-- src/conf/domain_conf.c | 45 ++++++++++++++++++- src/qemu/qemu_process.c | 16 +++++-- .../graphics-egl-headless-rendernode.xml | 33 ++++++++++++++ .../graphics-egl-headless-rendernode.xml | 41 +++++++++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 155 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless-rendernode.xml create mode 100644 tests/qemuxml2xmloutdata/graphics-egl-headless-rendernode.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Just like for SPICE, we need to put the DRI device into the namespace, otherwise it will be left out from the DAC relabeling process. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ee61caa823..dd4f77193a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11866,14 +11866,9 @@ qemuDomainSetupGraphics(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virDomainGraphicsDefPtr gfx, const struct qemuDomainCreateDeviceData *data) { - const char *rendernode = gfx->data.spice.rendernode; + const char *rendernode = virDomainGraphicsGetRenderNode(gfx); - if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE || - gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES || - !rendernode) - return 0; - - return qemuDomainCreateDevice(rendernode, data, false); + return !rendernode ? 0 : qemuDomainCreateDevice(rendernode, data, false); } -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:18PM +0100, Erik Skultety wrote:
Just like for SPICE, we need to put the DRI device into the namespace, otherwise it will be left out from the DAC relabeling process.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ee61caa823..dd4f77193a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11866,14 +11866,9 @@ qemuDomainSetupGraphics(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virDomainGraphicsDefPtr gfx, const struct qemuDomainCreateDeviceData *data) { - const char *rendernode = gfx->data.spice.rendernode; + const char *rendernode = virDomainGraphicsGetRenderNode(gfx);
- if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE || - gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES || - !rendernode) - return 0;
I think: if (!rendernode) return 0; is more readable than using ?:
- - return qemuDomainCreateDevice(rendernode, data, false); + return !rendernode ? 0 : qemuDomainCreateDevice(rendernode, data, false);
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Just like for SPICE, we need to put the render node DRI device into the device cgroup list so that users don't need to add it manually via qemu.conf file. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_cgroup.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 43e17d786e..3a6efdc4f1 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -491,15 +491,11 @@ qemuSetupGraphicsCgroup(virDomainObjPtr vm, virDomainGraphicsDefPtr gfx) { qemuDomainObjPrivatePtr priv = vm->privateData; - const char *rendernode = gfx->data.spice.rendernode; + const char *rendernode = virDomainGraphicsGetRenderNode(gfx); 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) + if (!rendernode || + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; ret = virCgroupAllowDevicePath(priv->cgroup, rendernode, -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:19PM +0100, Erik Skultety wrote:
Just like for SPICE, we need to put the render node DRI device into the device cgroup list so that users don't need to add it manually via qemu.conf file.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_cgroup.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Just like for SPICE, we need to change the permissions on the DRI device used as the @rendernode for egl-headless graphics type. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/security/security_dac.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 6b64d2c07a..4bdc6ed213 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1492,11 +1492,16 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr, virDomainGraphicsDefPtr gfx) { + const char *rendernode = virDomainGraphicsGetRenderNode(gfx); virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr seclabel; uid_t user; gid_t group; + /* There's nothing to relabel */ + if (!rendernode) + return 0; + /* Skip chowning the shared render file if namespaces are disabled */ if (!priv->mountNamespace) return 0; @@ -1508,14 +1513,8 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - if (gfx->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && - gfx->data.spice.gl == VIR_TRISTATE_BOOL_YES && - gfx->data.spice.rendernode) { - if (virSecurityDACSetOwnership(mgr, NULL, - gfx->data.spice.rendernode, - user, group) < 0) - return -1; - } + if (virSecurityDACSetOwnership(mgr, NULL, rendernode, user, group) < 0) + return -1; return 0; } -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:20PM +0100, Erik Skultety wrote:
Just like for SPICE, we need to change the permissions on the DRI device used as the @rendernode for egl-headless graphics type.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/security/security_dac.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Depending on whether QEMU actually supports the option, we can put the 'rendernode' on the '-display egl-headless' cmdline. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 32 ++++++++++++++++--- ...egl-headless-rendernode.x86_64-latest.args | 31 ++++++++++++++++++ .../graphics-egl-headless.x86_64-latest.args | 31 ++++++++++++++++++ ...play-spice-egl-headless.x86_64-latest.args | 2 +- ...isplay-vnc-egl-headless.x86_64-latest.args | 2 +- tests/qemuxml2argvtest.c | 2 ++ 6 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless-rendernode.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless.x86_64-latest.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 34c8ad751b..07b7082df8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8310,13 +8310,37 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, static int qemuBuildGraphicsEGLHeadlessCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, virCommandPtr cmd, - virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, - virDomainGraphicsDefPtr graphics ATTRIBUTE_UNUSED) + virQEMUCapsPtr qemuCaps, + virDomainGraphicsDefPtr graphics) { + int ret = -1; + virBuffer opt = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&opt, "egl-headless"); + + if (graphics->data.egl_headless.rendernode) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_EGL_HEADLESS_RENDERNODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support OpenGL rendernode " + "with egl-headless graphics type")); + goto cleanup; + } + + virBufferAddLit(&opt, ",rendernode="); + virQEMUBuildBufferEscapeComma(&opt, + graphics->data.egl_headless.rendernode); + } + + if (virBufferCheckError(&opt) < 0) + goto cleanup; + virCommandAddArg(cmd, "-display"); - virCommandAddArg(cmd, "egl-headless"); + virCommandAddArgBuffer(cmd, &opt); - return 0; + ret = 0; + cleanup: + virBufferFreeAndReset(&opt); + return ret; } diff --git a/tests/qemuxml2argvdata/graphics-egl-headless-rendernode.x86_64-latest.args b/tests/qemuxml2argvdata/graphics-egl-headless-rendernode.x86_64-latest.args new file mode 100644 index 0000000000..ad9079d912 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-egl-headless-rendernode.x86_64-latest.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +/usr/bin/qemu-system-i686 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ +-display egl-headless,rendernode=/dev/dri/foo \ +-device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/graphics-egl-headless.x86_64-latest.args b/tests/qemuxml2argvdata/graphics-egl-headless.x86_64-latest.args new file mode 100644 index 0000000000..ad9079d912 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-egl-headless.x86_64-latest.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +/usr/bin/qemu-system-i686 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ +-display egl-headless,rendernode=/dev/dri/foo \ +-device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.x86_64-latest.args index b84869264e..f5229936fc 100644 --- a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.x86_64-latest.args @@ -24,7 +24,7 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -spice port=0,seamless-migration=on \ --display egl-headless \ +-display egl-headless,rendernode=/dev/dri/foo \ -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,\ vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2 \ -device vfio-pci,id=hostdev0,\ diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.x86_64-latest.args index 91708d7663..9eaaf89eef 100644 --- a/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.x86_64-latest.args @@ -24,7 +24,7 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -vnc 127.0.0.1:0 \ --display egl-headless \ +-display egl-headless,rendernode=/dev/dri/foo \ -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,\ vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2 \ -device vfio-pci,id=hostdev0,\ diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index eae2b7edf7..5601b356e2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1246,6 +1246,8 @@ mymain(void) DO_TEST("graphics-egl-headless", QEMU_CAPS_EGL_HEADLESS, QEMU_CAPS_DEVICE_CIRRUS_VGA); + DO_TEST_CAPS_LATEST("graphics-egl-headless"); + DO_TEST_CAPS_LATEST("graphics-egl-headless-rendernode"); DO_TEST("graphics-vnc", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); -- 2.19.1
On Thu, Nov 29, 2018 at 03:20:21PM +0100, Erik Skultety wrote:
Depending on whether QEMU actually supports the option, we can put the 'rendernode' on the '-display egl-headless' cmdline.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 32 ++++++++++++++++--- ...egl-headless-rendernode.x86_64-latest.args | 31 ++++++++++++++++++ .../graphics-egl-headless.x86_64-latest.args | 31 ++++++++++++++++++ ...play-spice-egl-headless.x86_64-latest.args | 2 +- ...isplay-vnc-egl-headless.x86_64-latest.args | 2 +- tests/qemuxml2argvtest.c | 2 ++ 6 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless-rendernode.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless.x86_64-latest.args
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Mon, Dec 03, 2018 at 09:49:28AM +0100, Ján Tomko wrote:
On Thu, Nov 29, 2018 at 03:20:21PM +0100, Erik Skultety wrote:
Depending on whether QEMU actually supports the option, we can put the 'rendernode' on the '-display egl-headless' cmdline.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_command.c | 32 ++++++++++++++++--- ...egl-headless-rendernode.x86_64-latest.args | 31 ++++++++++++++++++ .../graphics-egl-headless.x86_64-latest.args | 31 ++++++++++++++++++ ...play-spice-egl-headless.x86_64-latest.args | 2 +- ...isplay-vnc-egl-headless.x86_64-latest.args | 2 +- tests/qemuxml2argvtest.c | 2 ++ 6 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless-rendernode.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless.x86_64-latest.args
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Fixed all the issues and pushed the series, thanks. Erik
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Ján Tomko