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@xxxxxxxxxx>
> ---
> src/qemu/qemu_cgroup.c | 48 +++++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_domain.c | 36 +++++++++++++++++++++++++++++++
> 2 files changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 4d6f0c33cd..f110b49d16 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -25,6 +25,7 @@
> #include "qemu_domain.h"
> #include "qemu_process.h"
> #include "qemu_extdevice.h"
> +#include "qemu_hostdev.h"
> #include "vircgroup.h"
> #include "virlog.h"
> #include "viralloc.h"
> @@ -360,6 +361,17 @@ qemuTeardownInputCgroup(virDomainObjPtr vm,
> }
>
>
> +/**
> + * qemuSetupHostdevCgroup:
> + * vm: domain object
> + * @dev: device to allow
> + *
> + * For given host device @dev allow access to in Cgroups.
> + * Note, @dev must not be in @vm's definition.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise.
> + */
> int
> qemuSetupHostdevCgroup(virDomainObjPtr vm,
> virDomainHostdevDefPtr dev)
> @@ -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.
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
