[libvirt] [PATCH] virt-aa-helper: generate rules for gl enabled graphics devices
This adds the virt-aa-helper support for gl enabled graphics devices to generate rules for the needed rendernode paths. Example in domain xml: <graphics type='spice'> <gl enable='yes' rendernode='/dev/dri/bar'/> </graphics> results in: "/dev/dri" r, "/dev/dri/bar" rw, Special cases are: - multiple devices with rendernodes -> all are added - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode - rendernode without opengl (in egl-headless for example) -> still add the node Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64a425671d..327a8a0202 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -938,7 +938,7 @@ get_files(vahControl * ctl) size_t i; char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; - bool needsVfio = false, needsvhost = false; + bool needsVfio = false, needsvhost = false, needDefaultRenderNode = false; /* verify uuid is same as what we were given on the command line */ virUUIDFormat(ctl->def->uuid, uuidstr); @@ -1062,6 +1062,10 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->ngraphics; i++) { virDomainGraphicsDefPtr graphics = ctl->def->graphics[i]; size_t n; + const char *rendernode = virDomainGraphicsGetRenderNode(graphics); + + if (rendernode) + vah_add_file(&buf, rendernode, "rw"); for (n = 0; n < graphics->nListens; n++) { virDomainGraphicsListenDef listenObj = graphics->listens[n]; @@ -1071,6 +1075,20 @@ get_files(vahControl * ctl) vah_add_file(&buf, listenObj.socket, "rw")) goto cleanup; } + + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) { + if (!rendernode) + needDefaultRenderNode = true; + } + } + + if (virDomainGraphicsDefHasOpenGL(ctl->def)) + vah_add_file(&buf, "/dev/dri", "r"); + + if (needDefaultRenderNode) { + const char *rendernode = virHostGetDRMRenderNode(); + if (rendernode) + vah_add_file(&buf, virHostGetDRMRenderNode(), "rw"); } if (ctl->def->ngraphics == 1 && -- 2.17.1
On 1/15/19 5:34 PM, Christian Ehrhardt wrote:
This adds the virt-aa-helper support for gl enabled graphics devices to generate rules for the needed rendernode paths.
Example in domain xml: <graphics type='spice'> <gl enable='yes' rendernode='/dev/dri/bar'/> </graphics>
results in: "/dev/dri" r, "/dev/dri/bar" rw,
Special cases are: - multiple devices with rendernodes -> all are added - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode - rendernode without opengl (in egl-headless for example) -> still add the node
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64a425671d..327a8a0202 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -938,7 +938,7 @@ get_files(vahControl * ctl) size_t i; char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; - bool needsVfio = false, needsvhost = false; + bool needsVfio = false, needsvhost = false, needDefaultRenderNode = false;
/* verify uuid is same as what we were given on the command line */ virUUIDFormat(ctl->def->uuid, uuidstr); @@ -1062,6 +1062,10 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->ngraphics; i++) { virDomainGraphicsDefPtr graphics = ctl->def->graphics[i]; size_t n; + const char *rendernode = virDomainGraphicsGetRenderNode(graphics); + + if (rendernode) + vah_add_file(&buf, rendernode, "rw");
for (n = 0; n < graphics->nListens; n++) { virDomainGraphicsListenDef listenObj = graphics->listens[n]; @@ -1071,6 +1075,20 @@ get_files(vahControl * ctl) vah_add_file(&buf, listenObj.socket, "rw")) goto cleanup; } + + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) { + if (!rendernode) + needDefaultRenderNode = true; + } + } + + if (virDomainGraphicsDefHasOpenGL(ctl->def)) + vah_add_file(&buf, "/dev/dri", "r"); + + if (needDefaultRenderNode) { + const char *rendernode = virHostGetDRMRenderNode(); + if (rendernode) + vah_add_file(&buf, virHostGetDRMRenderNode(), "rw");
This leaks @rendernode and also looks a bit strange to call the function twice. ACK with this squashed in: diff --git i/src/security/virt-aa-helper.c w/src/security/virt-aa-helper.c index 327a8a0202..60fea9b174 100644 --- i/src/security/virt-aa-helper.c +++ w/src/security/virt-aa-helper.c @@ -1086,9 +1086,11 @@ get_files(vahControl * ctl) vah_add_file(&buf, "/dev/dri", "r"); if (needDefaultRenderNode) { - const char *rendernode = virHostGetDRMRenderNode(); - if (rendernode) - vah_add_file(&buf, virHostGetDRMRenderNode(), "rw"); + char *rendernode = virHostGetDRMRenderNode(); + if (rendernode) { + vah_add_file(&buf, rendernode, "rw"); + VIR_FREE(rendernode); + } } if (ctl->def->ngraphics == 1 && I believe you have commit access, right? If not let me know and I will push it. Michal
On Wed, Jan 16, 2019 at 10:37 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/15/19 5:34 PM, Christian Ehrhardt wrote:
This adds the virt-aa-helper support for gl enabled graphics devices to generate rules for the needed rendernode paths.
Example in domain xml: <graphics type='spice'> <gl enable='yes' rendernode='/dev/dri/bar'/> </graphics>
results in: "/dev/dri" r, "/dev/dri/bar" rw,
Special cases are: - multiple devices with rendernodes -> all are added - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode - rendernode without opengl (in egl-headless for example) -> still add the node
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64a425671d..327a8a0202 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -938,7 +938,7 @@ get_files(vahControl * ctl) size_t i; char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; - bool needsVfio = false, needsvhost = false; + bool needsVfio = false, needsvhost = false, needDefaultRenderNode = false;
/* verify uuid is same as what we were given on the command line */ virUUIDFormat(ctl->def->uuid, uuidstr); @@ -1062,6 +1062,10 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->ngraphics; i++) { virDomainGraphicsDefPtr graphics = ctl->def->graphics[i]; size_t n; + const char *rendernode = virDomainGraphicsGetRenderNode(graphics); + + if (rendernode) + vah_add_file(&buf, rendernode, "rw");
for (n = 0; n < graphics->nListens; n++) { virDomainGraphicsListenDef listenObj = graphics->listens[n]; @@ -1071,6 +1075,20 @@ get_files(vahControl * ctl) vah_add_file(&buf, listenObj.socket, "rw")) goto cleanup; } + + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) { + if (!rendernode) + needDefaultRenderNode = true; + } + } + + if (virDomainGraphicsDefHasOpenGL(ctl->def)) + vah_add_file(&buf, "/dev/dri", "r"); + + if (needDefaultRenderNode) { + const char *rendernode = virHostGetDRMRenderNode(); + if (rendernode) + vah_add_file(&buf, virHostGetDRMRenderNode(), "rw");
This leaks @rendernode and also looks a bit strange to call the function twice.
Yes you are totally right, thanks for the suggestion.
ACK with this squashed in:
diff --git i/src/security/virt-aa-helper.c w/src/security/virt-aa-helper.c index 327a8a0202..60fea9b174 100644 --- i/src/security/virt-aa-helper.c +++ w/src/security/virt-aa-helper.c @@ -1086,9 +1086,11 @@ get_files(vahControl * ctl) vah_add_file(&buf, "/dev/dri", "r");
if (needDefaultRenderNode) { - const char *rendernode = virHostGetDRMRenderNode(); - if (rendernode) - vah_add_file(&buf, virHostGetDRMRenderNode(), "rw"); + char *rendernode = virHostGetDRMRenderNode(); + if (rendernode) { + vah_add_file(&buf, rendernode, "rw"); + VIR_FREE(rendernode); + } }
if (ctl->def->ngraphics == 1 &&
I believe you have commit access, right? If not let me know and I will push it.
I have access, but let me create a V2 with Eriks comment addressed first.
Michal
-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
On Tue, Jan 15, 2019 at 06:34:41PM +0200, Christian Ehrhardt wrote:
This adds the virt-aa-helper support for gl enabled graphics devices to generate rules for the needed rendernode paths.
Example in domain xml: <graphics type='spice'> <gl enable='yes' rendernode='/dev/dri/bar'/> </graphics>
results in: "/dev/dri" r, "/dev/dri/bar" rw,
Special cases are: - multiple devices with rendernodes -> all are added - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode - rendernode without opengl (in egl-headless for example) -> still add the node
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64a425671d..327a8a0202 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -938,7 +938,7 @@ get_files(vahControl * ctl) size_t i; char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; - bool needsVfio = false, needsvhost = false; + bool needsVfio = false, needsvhost = false, needDefaultRenderNode = false;
/* verify uuid is same as what we were given on the command line */ virUUIDFormat(ctl->def->uuid, uuidstr); @@ -1062,6 +1062,10 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->ngraphics; i++) { virDomainGraphicsDefPtr graphics = ctl->def->graphics[i]; size_t n; + const char *rendernode = virDomainGraphicsGetRenderNode(graphics); + + if (rendernode) + vah_add_file(&buf, rendernode, "rw");
for (n = 0; n < graphics->nListens; n++) { virDomainGraphicsListenDef listenObj = graphics->listens[n]; @@ -1071,6 +1075,20 @@ get_files(vahControl * ctl) vah_add_file(&buf, listenObj.socket, "rw")) goto cleanup; } + + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) { + if (!rendernode) + needDefaultRenderNode = true; + }
^This feels a bit detached from the hunk above. Also, what about EGL_HEADLESS, you will want to add (potentially a default one) rendernode for egl_headless too. You could also optimize this with virDomainGraphicsNeedsAutoRenderNode and cover both graphics types.
+ } + + if (virDomainGraphicsDefHasOpenGL(ctl->def)) + vah_add_file(&buf, "/dev/dri", "r");
Why is ^this needed?
+ + if (needDefaultRenderNode) { + const char *rendernode = virHostGetDRMRenderNode(); + if (rendernode) + vah_add_file(&buf, virHostGetDRMRenderNode(), "rw");
IUC the virDomainGraphicsNeedsAutoRenderNode part would come ^here. I also agree with Michal's suggestion. Erik
On Thu, Jan 17, 2019 at 11:05 AM Erik Skultety <eskultet@redhat.com> wrote:
On Tue, Jan 15, 2019 at 06:34:41PM +0200, Christian Ehrhardt wrote:
This adds the virt-aa-helper support for gl enabled graphics devices to generate rules for the needed rendernode paths.
Example in domain xml: <graphics type='spice'> <gl enable='yes' rendernode='/dev/dri/bar'/> </graphics>
results in: "/dev/dri" r, "/dev/dri/bar" rw,
Special cases are: - multiple devices with rendernodes -> all are added - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode - rendernode without opengl (in egl-headless for example) -> still add the node
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64a425671d..327a8a0202 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -938,7 +938,7 @@ get_files(vahControl * ctl) size_t i; char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; - bool needsVfio = false, needsvhost = false; + bool needsVfio = false, needsvhost = false, needDefaultRenderNode = false;
/* verify uuid is same as what we were given on the command line */ virUUIDFormat(ctl->def->uuid, uuidstr); @@ -1062,6 +1062,10 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->ngraphics; i++) { virDomainGraphicsDefPtr graphics = ctl->def->graphics[i]; size_t n; + const char *rendernode = virDomainGraphicsGetRenderNode(graphics); + + if (rendernode) + vah_add_file(&buf, rendernode, "rw");
for (n = 0; n < graphics->nListens; n++) { virDomainGraphicsListenDef listenObj = graphics->listens[n]; @@ -1071,6 +1075,20 @@ get_files(vahControl * ctl) vah_add_file(&buf, listenObj.socket, "rw")) goto cleanup; } + + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) { + if (!rendernode) + needDefaultRenderNode = true; + }
^This feels a bit detached from the hunk above.
Agreed, better grouped in an upcoming V2
Also, what about EGL_HEADLESS, you will want to add (potentially a default one) rendernode for egl_headless too.
Yes I'd want EGL Headless as well, using the suggested virDomainGraphicsNeedsAutoRenderNode to cover both. As it does the same checks, but better - thanks for the hint!
You could also optimize this with virDomainGraphicsNeedsAutoRenderNode and cover both graphics types.
+ } + + if (virDomainGraphicsDefHasOpenGL(ctl->def)) + vah_add_file(&buf, "/dev/dri", "r");
Why is ^this needed?
I started on this before the virDomainGraphics helpers and before you changed it to always pass a defined value. Back then qemu then would have enumerated /dev/dri, but you are right we - no more - need the base dir since we pass an explicit value. In case I later on find cases this still triggers I can come back with a small patch.
+ + if (needDefaultRenderNode) { + const char *rendernode = virHostGetDRMRenderNode(); + if (rendernode) + vah_add_file(&buf, virHostGetDRMRenderNode(), "rw");
IUC the virDomainGraphicsNeedsAutoRenderNode part would come ^here.
Yeah, since we already iterate on "graphics" elements above and there is no drawback in adding the default virHostGetDRMRenderNode path twice (would only happen in awkward guest XMLs anyway) this can be simplified even further. it essentially becomes: "if it has explicit node add it, otherwise if it needs a rendernode at all add the default"
I also agree with Michal's suggestion.
Erik
-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
participants (3)
-
Christian Ehrhardt -
Erik Skultety -
Michal Privoznik