[libvirt] [PATCH v2] qemu: Allow @rednernode for virgl domains

Michal Privoznik posted 1 patch 4 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/f5804280977a8caa5dcfe10ae778a648b8926add.1487580083.git.mprivozn@redhat.com
src/qemu/qemu_cgroup.c | 27 +++++++++++++++++++++++++++
src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)

[libvirt] [PATCH v2] qemu: Allow @rednernode for virgl domains

Posted by Michal Privoznik 4 years, 2 months ago
When enabling virgl, qemu opens /dev/dri/render*. So far, we are
not allowing that in devices cgroup nor creating the file in
domain's namespace and thus requiring users to set the paths in
qemu.conf. This, however, is suboptimal as it allows access to
ALL qemu processes even those which don't have virgl configured.
Now that we have a way to specify render node that qemu will use
we can be more cautious and enable just that.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

Technically, this is v2 of:

https://www.redhat.com/archives/libvir-list/2017-February/msg00497.html

diff to v1:
- now that we have @rendernode for <gl/> which selects just one path (and does
  it in predictable fashion) only that path is enabled in the CGgroups and
  created in the namespace.

 src/qemu/qemu_cgroup.c | 27 +++++++++++++++++++++++++++
 src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 209cbc275..f0729743a 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -335,6 +335,28 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
     return ret;
 }
 
+
+static int
+qemuSetupGraphicsCgroup(virDomainObjPtr vm,
+                        virDomainGraphicsDefPtr gfx)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    const char *rendernode = gfx->data.spice.rendernode;
+    int ret;
+
+    if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE ||
+        gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES ||
+        !rendernode)
+        return 0;
+
+    ret = virCgroupAllowDevicePath(priv->cgroup, rendernode,
+                                   VIR_CGROUP_DEVICE_RW, false);
+    virDomainAuditCgroupPath(vm, priv->cgroup, "allow", rendernode,
+                             "rw", ret == 0);
+    return ret;
+}
+
+
 static int
 qemuSetupBlkioCgroup(virDomainObjPtr vm)
 {
@@ -604,6 +626,11 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
             goto cleanup;
     }
 
+    for (i = 0; i < vm->def->ngraphics; i++) {
+        if (qemuSetupGraphicsCgroup(vm, vm->def->graphics[i]) < 0)
+            goto cleanup;
+    }
+
     for (i = 0; i < vm->def->ninputs; i++) {
         if (qemuSetupInputCgroup(vm, vm->def->inputs[i]) < 0)
             goto cleanup;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 92a9a105c..ea4b28288 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7526,6 +7526,42 @@ qemuDomainSetupTPM(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
 }
 
 
+static int
+qemuDomainSetupGraphics(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+                        virDomainGraphicsDefPtr gfx,
+                        const char *devPath)
+{
+    const char *rendernode = gfx->data.spice.rendernode;
+
+    if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE ||
+        gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES ||
+        !rendernode)
+        return 0;
+
+    return qemuDomainCreateDevice(rendernode, devPath, false);
+}
+
+
+static int
+qemuDomainSetupAllGraphics(virQEMUDriverPtr driver,
+                           virDomainObjPtr vm,
+                           const char *devPath)
+{
+    size_t i;
+
+    VIR_DEBUG("Setting up graphics");
+    for (i = 0; i < vm->def->ngraphics; i++) {
+        if (qemuDomainSetupGraphics(driver,
+                                    vm->def->graphics[i],
+                                    devPath) < 0)
+            return -1;
+    }
+
+    VIR_DEBUG("Setup all graphics");
+    return 0;
+}
+
+
 static int
 qemuDomainSetupInput(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
                      virDomainInputDefPtr input,
@@ -7679,6 +7715,9 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver,
     if (qemuDomainSetupTPM(driver, vm, devPath) < 0)
         goto cleanup;
 
+    if (qemuDomainSetupAllGraphics(driver, vm, devPath) < 0)
+        goto cleanup;
+
     if (qemuDomainSetupAllInputs(driver, vm, devPath) < 0)
         goto cleanup;
 
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemu: Allow @rednernode for virgl domains

Posted by Marc-André Lureau 4 years, 2 months ago
Hi


Fix the title @rednernode -> @rendernode

----- Original Message -----
> When enabling virgl, qemu opens /dev/dri/render*. So far, we are
> not allowing that in devices cgroup nor creating the file in
> domain's namespace and thus requiring users to set the paths in
> qemu.conf. This, however, is suboptimal as it allows access to
> ALL qemu processes even those which don't have virgl configured.
> Now that we have a way to specify render node that qemu will use
> we can be more cautious and enable just that.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> Technically, this is v2 of:
> 
> https://www.redhat.com/archives/libvir-list/2017-February/msg00497.html
> 
> diff to v1:
> - now that we have @rendernode for <gl/> which selects just one path (and
> does
>   it in predictable fashion) only that path is enabled in the CGgroups and
>   created in the namespace.

That means in practice we are not compatible with older qemu releases, and we make rendernode attribute somehow mandatory for qemu:///system (no automatic selection).

I'd suggest to let all /dev/dri/render* if rendernode is not specified, but this can be discussed and done in a seperate patch.

> 
>  src/qemu/qemu_cgroup.c | 27 +++++++++++++++++++++++++++
>  src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 209cbc275..f0729743a 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -335,6 +335,28 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
>      return ret;
>  }
>  
> +
> +static int
> +qemuSetupGraphicsCgroup(virDomainObjPtr vm,
> +                        virDomainGraphicsDefPtr gfx)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    const char *rendernode = gfx->data.spice.rendernode;
> +    int ret;
> +
> +    if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE ||
> +        gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES ||
> +        !rendernode)
> +        return 0;
> +
> +    ret = virCgroupAllowDevicePath(priv->cgroup, rendernode,
> +                                   VIR_CGROUP_DEVICE_RW, false);
> +    virDomainAuditCgroupPath(vm, priv->cgroup, "allow", rendernode,
> +                             "rw", ret == 0);
> +    return ret;
> +}
> +
> +
>  static int
>  qemuSetupBlkioCgroup(virDomainObjPtr vm)
>  {
> @@ -604,6 +626,11 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
>              goto cleanup;
>      }
>  
> +    for (i = 0; i < vm->def->ngraphics; i++) {
> +        if (qemuSetupGraphicsCgroup(vm, vm->def->graphics[i]) < 0)
> +            goto cleanup;
> +    }
> +
>      for (i = 0; i < vm->def->ninputs; i++) {
>          if (qemuSetupInputCgroup(vm, vm->def->inputs[i]) < 0)
>              goto cleanup;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 92a9a105c..ea4b28288 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7526,6 +7526,42 @@ qemuDomainSetupTPM(virQEMUDriverPtr driver
> ATTRIBUTE_UNUSED,
>  }
>  
>  
> +static int
> +qemuDomainSetupGraphics(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +                        virDomainGraphicsDefPtr gfx,
> +                        const char *devPath)
> +{
> +    const char *rendernode = gfx->data.spice.rendernode;
> +
> +    if (gfx->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE ||
> +        gfx->data.spice.gl != VIR_TRISTATE_BOOL_YES ||
> +        !rendernode)
> +        return 0;
> +
> +    return qemuDomainCreateDevice(rendernode, devPath, false);
> +}
> +
> +
> +static int
> +qemuDomainSetupAllGraphics(virQEMUDriverPtr driver,
> +                           virDomainObjPtr vm,
> +                           const char *devPath)
> +{
> +    size_t i;
> +
> +    VIR_DEBUG("Setting up graphics");
> +    for (i = 0; i < vm->def->ngraphics; i++) {
> +        if (qemuDomainSetupGraphics(driver,
> +                                    vm->def->graphics[i],
> +                                    devPath) < 0)
> +            return -1;
> +    }
> +
> +    VIR_DEBUG("Setup all graphics");
> +    return 0;
> +}
> +
> +
>  static int
>  qemuDomainSetupInput(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>                       virDomainInputDefPtr input,
> @@ -7679,6 +7715,9 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver,
>      if (qemuDomainSetupTPM(driver, vm, devPath) < 0)
>          goto cleanup;
>  
> +    if (qemuDomainSetupAllGraphics(driver, vm, devPath) < 0)
> +        goto cleanup;
> +
>      if (qemuDomainSetupAllInputs(driver, vm, devPath) < 0)
>          goto cleanup;
>  

Looks good,

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> --
> 2.11.0
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemu: Allow @rednernode for virgl domains

Posted by Michal Privoznik 4 years, 2 months ago
On 20.02.2017 09:49, Marc-André Lureau wrote:
> ----- Original Message -----
>> When enabling virgl, qemu opens /dev/dri/render*. So far, we are
>> not allowing that in devices cgroup nor creating the file in
>> domain's namespace and thus requiring users to set the paths in
>> qemu.conf. This, however, is suboptimal as it allows access to
>> ALL qemu processes even those which don't have virgl configured.
>> Now that we have a way to specify render node that qemu will use
>> we can be more cautious and enable just that.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> Technically, this is v2 of:
>>
>> https://www.redhat.com/archives/libvir-list/2017-February/msg00497.html
>>
>> diff to v1:
>> - now that we have @rendernode for <gl/> which selects just one path (and
>> does
>>   it in predictable fashion) only that path is enabled in the CGgroups and
>>   created in the namespace.
> 
> That means in practice we are not compatible with older qemu releases, and we make rendernode attribute somehow mandatory for qemu:///system (no automatic selection).

Depends on your definition of compatibility :-) Until Friday afternoon
(when I pushed your patches), there was no way how to tell qemu which
render node to use. Thus users using virgl domains needed to adjust
cgroup_device_acl list in qemu.conf anyway and enable whatever device
qemu needed for virgl.

With my patch, if they select the device at domain XML level, libvirt
can do that for them. I think this is in fact the correct solution. The
reason why we have that variable in qemu.conf is that libvirt can't
possibly know all the paths qemu will ever touch (even though we try
hard to do so), simply because the matrix of combinations of paths and
possible device configs is endless.

> 
> I'd suggest to let all /dev/dri/render* if rendernode is not specified, but this can be discussed and done in a seperate patch.


Frankly, I like this approach better. I mean, the one implemented here.
The whole point of devices CGroup is to limit devices that a process
(qemu) has access to. And if we don't know simply shrug and allow
everything is not ideal IMO.

Michal

> 
> Looks good,
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks, fixed the typo in the subject and pushed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list