[libvirt] [PATCH 6/7] qemuDomainGetHostdevPath: Report /dev/vfio/vfio less frequently
Marc-André Lureau
marcandre.lureau at gmail.com
Thu Feb 16 12:32:48 UTC 2017
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn at redhat.com>
wrote:
> So far, qemuDomainGetHostdevPath has no knowledge of the reasong
>
reasong/reason
> it is called and thus reports /dev/vfio/vfio for every VFIO
> backed device. This is suboptimal, as we want it to:
>
> a) report /dev/vfio/vfio on every addition or domain startup
> b) report /dev/vfio/vfio only on last VFIO device being unplugged
>
> If a domain is being stopped then namespace and CGroup die with
> it so no need to worry about that. I mean, even when a domain
> that's exiting has more than one VFIO devices assigned to it,
> this function does not clean /dev/vfio/vfio in CGroup nor in the
> namespace. But that doesn't matter.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>
Fine approach to me,
Reviewed-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
> src/qemu/qemu_cgroup.c | 87
> ++++++++++++--------------------------------------
> src/qemu/qemu_domain.c | 38 ++++++++++++++++------
> src/qemu/qemu_domain.h | 4 ++-
> 3 files changed, 52 insertions(+), 77 deletions(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 944e8dc87..209cbc275 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -52,7 +52,6 @@ const char *const defaultDeviceACL[] = {
> #define DEVICE_PTY_MAJOR 136
> #define DEVICE_SND_MAJOR 116
>
> -#define DEV_VFIO "/dev/vfio/vfio"
>
> static int
> qemuSetupImagePathCgroup(virDomainObjPtr vm,
> @@ -271,7 +270,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
> size_t i, npaths = 0;
> int rv, ret = -1;
>
> - if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
> + if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path,
> &perms) < 0)
> goto cleanup;
>
> for (i = 0; i < npaths; i++) {
> @@ -298,11 +297,10 @@ int
> qemuTeardownHostdevCgroup(virDomainObjPtr vm,
> virDomainHostdevDefPtr dev)
> {
> - int ret = -1;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> - virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
> - virPCIDevicePtr pci = NULL;
> - char *path = NULL;
> + char **path = NULL;
> + size_t i, npaths = 0;
> + int rv, ret = -1;
>
> /* currently this only does something for PCI devices using vfio
> * for device assignment, but it is called for *all* hostdev
> @@ -312,70 +310,27 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
> if (!virCgroupHasController(priv->cgroup,
> VIR_CGROUP_CONTROLLER_DEVICES))
> return 0;
>
> - if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> -
> - switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> - if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> - int rv;
> - size_t i, vfios = 0;
> -
> - pci = virPCIDeviceNew(pcisrc->addr.domain,
> - pcisrc->addr.bus,
> - pcisrc->addr.slot,
> - pcisrc->addr.function);
> - if (!pci)
> - goto cleanup;
> -
> - if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
> - goto cleanup;
> -
> - VIR_DEBUG("Cgroup deny %s for PCI device assignment",
> path);
> - rv = virCgroupDenyDevicePath(priv->cgroup, path,
> - VIR_CGROUP_DEVICE_RWM,
> false);
> - virDomainAuditCgroupPath(vm, priv->cgroup,
> - "deny", path, "rwm", rv == 0);
> - if (rv < 0)
> - goto cleanup;
> -
> - /* If this is the last hostdev with VFIO backend deny
> - * /dev/vfio/vfio too. */
> - for (i = 0; i < vm->def->nhostdevs; i++) {
> - virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
> - if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> - tmp->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> - tmp->source.subsys.u.pci.backend ==
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
> - vfios++;
> - }
> -
> - if (vfios == 0) {
> - VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device
> assignment");
> - rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
> - VIR_CGROUP_DEVICE_RWM,
> false);
> - virDomainAuditCgroupPath(vm, priv->cgroup,
> - "deny", DEV_VFIO, "rwm", rv
> == 0);
> - if (rv < 0)
> - goto cleanup;
> - }
> - }
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> - /* nothing to tear down for USB */
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> - /* nothing to tear down for SCSI */
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
> - /* nothing to tear down for scsi_host */
> - break;
> - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> - break;
> - }
> + if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> + dev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> + dev->source.subsys.u.pci.backend ==
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO &&
> + qemuDomainGetHostdevPath(vm->def, dev, true,
> + &npaths, &path, NULL) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < npaths; i++) {
> + VIR_DEBUG("Cgroup deny %s", path[i]);
> + rv = virCgroupDenyDevicePath(priv->cgroup, path[i],
> + VIR_CGROUP_DEVICE_RWM, false);
> + virDomainAuditCgroupPath(vm, priv->cgroup,
> + "deny", path[i], "rwm", rv == 0);
> + if (rv < 0)
> + goto cleanup;
> }
>
> ret = 0;
> cleanup:
> - virPCIDeviceFree(pci);
> + for (i = 0; i < npaths; i++)
> + VIR_FREE(path[i]);
> VIR_FREE(path);
> return ret;
> }
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 530eced33..515e0052e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6834,7 +6834,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
> video,
>
> /**
> * qemuDomainGetHostdevPath:
> + * @def: domain definition
> * @dev: host device definition
> + * @teardown: true if device will be removed
> * @npaths: number of items in @path and @perms arrays
> * @path: resulting path to @dev
> * @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
> @@ -6849,7 +6851,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
> video,
> * Returns 0 on success, -1 otherwise.
> */
> int
> -qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
> +qemuDomainGetHostdevPath(virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + bool teardown,
> size_t *npaths,
> char ***path,
> int **perms)
> @@ -6890,7 +6894,21 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
> freeTmpPath = true;
>
> perm = VIR_CGROUP_DEVICE_RW;
> - includeVFIO = true;
> + if (teardown) {
> + size_t nvfios = 0;
> + for (i = 0; i < def->nhostdevs; i++) {
> + virDomainHostdevDefPtr tmp = def->hostdevs[i];
> + if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> + tmp->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> + tmp->source.subsys.u.pci.backend ==
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
> + nvfios++;
> + }
> +
> + if (nvfios == 0)
> + includeVFIO = true;
> + } else {
> + includeVFIO = true;
> + }
> }
> break;
>
> @@ -7389,7 +7407,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver
> ATTRIBUTE_UNUSED,
> char **path = NULL;
> size_t i, npaths = 0;
>
> - if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
> + if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, NULL)
> < 0)
> goto cleanup;
>
> for (i = 0; i < npaths; i++) {
> @@ -8024,7 +8042,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr
> driver,
> if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
> return 0;
>
> - if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
> + if (qemuDomainGetHostdevPath(NULL, hostdev, false, &npaths, &path,
> NULL) < 0)
> goto cleanup;
>
> for (i = 0; i < npaths; i++) {
> @@ -8055,14 +8073,14 @@
> qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
> if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
> return 0;
>
> - if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
> + if (qemuDomainGetHostdevPath(vm->def, hostdev, true,
> + &npaths, &path, NULL) < 0)
> goto cleanup;
>
> - /* Don't remove other paths than for the @hostdev itself.
> - * They might be still in use by other devices. */
> - if (npaths > 0 &&
> - qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
> - goto cleanup;
> + for (i = 0; i < npaths; i++) {
> + if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0)
> + goto cleanup;
> + }
>
> ret = 0;
> cleanup:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index e64aa25ba..80de50fbe 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -802,7 +802,9 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
> bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
> virQEMUCapsPtr qemuCaps);
>
> -int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
> +int qemuDomainGetHostdevPath(virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + bool teardown,
> size_t *npaths,
> char ***path,
> int **perms);
> --
> 2.11.0
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170216/2f57d0d6/attachment-0001.htm>
More information about the libvir-list
mailing list