[libvirt] [PATCH v2 07/39] qemu: Explicitly add/remove /dev/vfio/vfio to/from NS/CGroups

[Michal Privoznik]

On 10/18/19 12:10 AM, Cole Robinson wrote:
> On 9/26/19 12:12 PM, Michal Privoznik wrote:
>> In near future, the decision what to do with /dev/vfio/vfio with
>> respect to domain namespace and CGroup is going to be moved out
>> of qemuDomainGetHostdevPath() because there will be some other
>> types of devices than hostdevs that need access to VFIO.
>>
>> All functions that I'm changing assume that hostdev we are
>> adding/removing to VM is not in the definition yet (because of
>> how qemuDomainNeedsVFIO() is written). Fortunately, this
>> assumption is true.
>>
> 
> qemuProcessLaunch ->
>    qemuSetupCgroup ->
>      qemuSetupDevicesCgroup ->
> 
> has
> 
>      for (i = 0; i < vm->def->nhostdevs; i++) {
> 
>          if (qemuSetupHostdevCgroup(vm, vm->def->hostdevs[i]) < 0)
> 
>              goto cleanup;
> 
>      }
> 
> So that above paragraph doesn't seem correct. If I apply up to patch
> #17, this breaks VM startup with a PCI passthrough device, but caveat
> only with cgroupv1. Apparently cgroupv2 doesn't have any notion of
> allowDevice ? or at least there's no impl there.

Yeah, cgroupv2 doesn't implement devices controller just yet.

> 
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_cgroup.c | 48 +++++++++++++++++++++++++++++++++++++++++-
>>   src/qemu/qemu_domain.c | 36 +++++++++++++++++++++++++++++++
>>   2 files changed, 83 insertions(+), 1 deletion(-)
>>

>> @@ -386,6 +398,17 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>>               goto cleanup;
>>       }
>>   
>> +    if (qemuHostdevNeedsVFIO(dev) &&
>> +        !qemuDomainNeedsVFIO(vm->def)) {
>> +        VIR_DEBUG("Cgroup allow %s perms=%d", QEMU_DEV_VFIO, VIR_CGROUP_DEVICE_RW);
>> +        rv = virCgroupAllowDevicePath(priv->cgroup, QEMU_DEV_VFIO,
>> +                                      VIR_CGROUP_DEVICE_RW, false);
>> +        virDomainAuditCgroupPath(vm, priv->cgroup, "allow",
>> +                                 QEMU_DEV_VFIO, "rw", rv);
>> +        if (rv < 0)
>> +            goto cleanup;
>> +    }
>> +
>>       ret = 0;
>>
> 
> So on VM startup this code path isn't hit, because dev is already in
> vm->def, so the if() condition will never be true.
> 
> However this patch itself doesn't break things, because
> qemuDomainGetHostdevPath will also return /dev/vfio/vfio if the device
> needs it. I guess later patches undo that somehow but I didn't look into
> yet why that is.
> 
> Is the !qemuDomainNeedsVFIO even necessary? The existing code will
> already call virCgroupAllowDevicePath(/dev/vfio/vfio) multiple times if
> the device has multiple VFIO devices attached so apparently that's not
> problematic.

Ah, good catch. It's not necessary. Will fix and repost.

Michal