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@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