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

[libvirt] [PATCH] node_device: Fix the improper logic when enumerating SRIOV VF



pciGetVirtualFunctions returns 0 even if there is no "virtfn"
entry under the device sysfs path.

And pciGetVirtualFunctions returns -1 when it tries to get
the PCI config space of the VF, however, with keeping the
the VFs already detected.

That's why udevProcessPCI and gather_pci_cap use logic like:

if (!pciGetVirtualFunctions(syspath,
                            &data->pci_dev.virtual_functions,
                            &data->pci_dev.num_virtual_functions) ||
    data->pci_dev.num_virtual_functions > 0)
    data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;

to tag the PCI device with "virtual_function" cap.

This results in a VF will aslo get "virtual_function" cap.

This patch fixes it by:
  * Ignoring the VF which has failure of getting PCI config space
    of the device (given that the succesfully detected VFs are kept
    , it makes sense to not give up on the failure of one VF too)
    with a warning, so pciGetVirtualFunctions will always return 0
    except out of memory.

  * Free the allocated *virtual_functions when out of memory

And thus the logic can be changed to:

    /* Out of memory */
    if (pciGetVirtualFunctions(syspath, &data->pci_dev.virtual_functions,
                               &data->pci_dev.num_virtual_functions) < 0)
        goto out;
    else if (!pciGetVirtualFunctions(syspath,
                                     &data->pci_dev.virtual_functions,
                                     &data->pci_dev.num_virtual_functions) &&
             (data->pci_dev.num_virtual_functions > 0))
        data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;

This also fixes a memory leak in the old codes (it realloc the
*virtual_functions first, and doesn't shrink it when it fails on
getting the PCI config space).
---
 src/node_device/node_device_hal.c  |   13 +++++++++--
 src/node_device/node_device_udev.c |   11 +++++++--
 src/util/pci.c                     |   37 +++++++++++++++++++++++------------
 3 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
index ff73db0..69e6ebf 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -151,10 +151,17 @@ static int gather_pci_cap(LibHalContext *ctx, const char *udi,
         if (!pciGetPhysicalFunction(sysfs_path, &d->pci_dev.physical_function))
             d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
 
-        if (!pciGetVirtualFunctions(sysfs_path, &d->pci_dev.virtual_functions,
-            &d->pci_dev.num_virtual_functions) ||
-            d->pci_dev.num_virtual_functions > 0)
+        if (!pciGetVirtualFunctions(sysfs_path,
+                                    &d->pci_dev.virtual_functions,
+                                    &d->pci_dev.num_virtual_functions) < 0) {
+            VIR_FREE(sysfs_path);
+            return -1;
+        } else if (!pciGetVirtualFunctions(sysfs_path,
+                                           &d->pci_dev.virtual_functions,
+                                           &d->pci_dev.num_virtual_functions) &&
+                   (d->pci_dev.num_virtual_functions > 0))
             d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
+        }
 
         VIR_FREE(sysfs_path);
     }
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index acd78f2..72b8850 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -484,9 +484,14 @@ static int udevProcessPCI(struct udev_device *device,
     if (!pciGetPhysicalFunction(syspath, &data->pci_dev.physical_function))
         data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
 
-    if (!pciGetVirtualFunctions(syspath, &data->pci_dev.virtual_functions,
-        &data->pci_dev.num_virtual_functions) ||
-        data->pci_dev.num_virtual_functions > 0)
+    /* Out of memory */
+    if (pciGetVirtualFunctions(syspath, &data->pci_dev.virtual_functions,
+                               &data->pci_dev.num_virtual_functions) < 0)
+        goto out;
+    else if (!pciGetVirtualFunctions(syspath,
+                                     &data->pci_dev.virtual_functions,
+                                     &data->pci_dev.num_virtual_functions) &&
+             (data->pci_dev.num_virtual_functions > 0))
         data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
 
     ret = 0;
diff --git a/src/util/pci.c b/src/util/pci.c
index d1ad121..3b4d96a 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -1930,6 +1930,7 @@ pciGetVirtualFunctions(const char *sysfs_path,
                        unsigned int *num_virtual_functions)
 {
     int ret = -1;
+    int i;
     DIR *dir = NULL;
     struct dirent *entry = NULL;
     char *device_link = NULL;
@@ -1952,6 +1953,7 @@ pciGetVirtualFunctions(const char *sysfs_path,
     *num_virtual_functions = 0;
     while ((entry = readdir(dir))) {
         if (STRPREFIX(entry->d_name, "virtfn")) {
+            struct pci_config_address *config_addr = NULL;
 
             if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) {
                 virReportOOMError();
@@ -1960,24 +1962,23 @@ pciGetVirtualFunctions(const char *sysfs_path,
 
             VIR_DEBUG("Number of virtual functions: %d",
                       *num_virtual_functions);
-            if (VIR_REALLOC_N(*virtual_functions,
-                (*num_virtual_functions) + 1) != 0) {
-                virReportOOMError();
-                VIR_FREE(device_link);
-                goto out;
-            }
 
             if (pciGetPciConfigAddressFromSysfsDeviceLink(device_link,
-                &((*virtual_functions)[*num_virtual_functions])) !=
+                                                          &config_addr) !=
                 SRIOV_FOUND) {
-                /* We should not get back SRIOV_NOT_FOUND in this
-                 * case, so if we do, it's an error. */
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("Failed to get SR IOV function from device "
-                               "link '%s'"), device_link);
+                VIR_WARN("Failed to get SRIOV function from device "
+                         "link '%s'", device_link);
                 VIR_FREE(device_link);
-                goto out;
+                continue;
             } else {
+                if (VIR_ALLOC_N(*virtual_functions,
+                                *num_virtual_functions + 1) < 0) {
+                    virReportOOMError();
+                    VIR_FREE(config_addr);
+                    goto out;
+                }
+
+                (*virtual_functions)[*num_virtual_functions] = config_addr;
                 (*num_virtual_functions)++;
             }
             VIR_FREE(device_link);
@@ -1985,8 +1986,18 @@ pciGetVirtualFunctions(const char *sysfs_path,
     }
 
     ret = 0;
+    goto cleanup;
 
 out:
+    if (*virtual_functions) {
+        for (i = 0; i < *num_virtual_functions; i++)
+            VIR_FREE((*virtual_functions)[i]);
+        VIR_FREE(*virtual_functions);
+    }
+
+cleanup:
+    if (device_link)
+        VIR_FREE(device_link);
     if (dir)
         closedir(dir);
 
-- 
1.7.7.6


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