[libvirt] [PATCH v3 00/11] Autoselect a DRM node for egl-headless add it to cmdline

[Erik Skultety]

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

[Erik Skultety]

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 <***@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)

[Ján Tomko]

Post by Erik Skultety
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.
---
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 <***@redhat.com>

Jano

[Daniel P. Berrangé]

Post by Erik Skultety
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.
---
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;
}
+
+
+/**
+ *
+ * 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 ?

Post by Erik Skultety
+
+ if (STRPREFIX(ent->d_name, "renderD")) {
+ have_rendernode = true;
+ break;
+ }
+ }

Regards,
Daniel

[Erik Skultety]

Post by Daniel P. Berrangé

Post by Erik Skultety
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.
---
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;
}
+
+
+/**
+ *
+ * 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

[Daniel P. Berrangé]

Post by Erik Skultety

Post by Daniel P. Berrangé

Post by Erik Skultety
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.
---
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;
}
+
+
+/**
+ *
+ * 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

[Erik Skultety]

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 <***@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;

[Ján Tomko]

Post by Erik Skultety
A few simple helpers that allow us to determine whether a graphics can
and will need to make use of a DRM render node.
---
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)

Post by Erik Skultety
+{
+ if (!virDomainGraphicsSupportsRenderNode(graphics) ||
+ virDomainGraphicsGetRenderNode(graphics))
+ return false;

Personally I'd decouple these two conditions.

Post by Erik Skultety
+
+ return true;
+}

Regardless:
Reviewed-by: Ján Tomko <***@redhat.com>

Jano

[Erik Skultety]

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

[Ján Tomko]

Post by Erik Skultety
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.
---
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 <***@redhat.com>

Jano

[Erik Skultety]

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

[Ján Tomko]

Post by Erik Skultety
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.
---
src/conf/domain_conf.h | 3 +++
1 file changed, 3 insertions(+)

Reviewed-by: Ján Tomko <***@redhat.com>

Jano

[Erik Skultety]

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 <***@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 };

[Ján Tomko]

Post by Erik Skultety
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
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.
---
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.

Post by Erik Skultety
+ 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;
/**
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'...

[...]

Post by Erik Skultety
+-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'?

Post by Erik Skultety
+-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 <***@redhat.com>

Jano

[Erik Skultety]

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 <***@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>&lt;gl/&gt;</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>
&lt;graphics type='spice' autoport='yes'/&gt;
-&lt;graphics type='egl-headless'/&gt;
+&lt;graphics type='egl-headless'&gt;
+ &lt;gl rendernode='/dev/dri/renderD128'/&gt;
+&lt;/graphics&gt;
</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);

[Ján Tomko]

Post by Erik Skultety
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.
---
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 <***@redhat.com>

Jano

[Erik Skultety]

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 <***@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;
}

[Ján Tomko]

Post by Erik Skultety
Just like for SPICE, we need to change the permissions on the DRI device
---
src/security/security_dac.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

Reviewed-by: Ján Tomko <***@redhat.com>

Jano

[Erik Skultety]

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 <***@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:

[Ján Tomko]

Post by Erik Skultety
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.
---
src/qemu/qemu_command.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

Reviewed-by: Ján Tomko <***@redhat.com>

Jano

[Erik Skultety]

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 <***@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);
}

[Ján Tomko]

Post by Erik Skultety
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.
---
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;

Post by Erik Skultety
-
- return qemuDomainCreateDevice(rendernode, data, false);
+ return !rendernode ? 0 : qemuDomainCreateDevice(rendernode, data, false);

Reviewed-by: Ján Tomko <***@redhat.com>

Jano

[Erik Skultety]

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 <***@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,

[Ján Tomko]

Post by Erik Skultety
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.
---
src/qemu/qemu_cgroup.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)

Reviewed-by: Ján Tomko <***@redhat.com>

Jano

[Erik Skultety]

Depending on whether QEMU actually supports the option, we can put the
'rendernode' on the '-display egl-headless' cmdline.

Signed-off-by: Erik Skultety <***@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);

[Ján Tomko]

Post by Erik Skultety
Depending on whether QEMU actually supports the option, we can put the
'rendernode' on the '-display egl-headless' cmdline.
---
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 <***@redhat.com>

Jano

[Erik Skultety]

Post by Erik Skultety
Depending on whether QEMU actually supports the option, we can put the
'rendernode' on the '-display egl-headless' cmdline.
---
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

Fixed all the issues and pushed the series, thanks.

Erik