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

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.

> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > 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.