src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.