[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 6/7] qemuDomainGetHostdevPath: Report /dev/vfio/vfio less frequently





On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn 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 redhat com>

Fine approach to me,

Reviewed-by: Marc-André Lureau <marcandre lureau 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 redhat com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]