From 025c32f0b7f6f968e1b33442b68c2e65e11bc43f Mon Sep 17 00:00:00 2001
Message-Id: <025c32f0b7f6f968e1b33442b68c2e65e11bc43f.1383922565.git.jdenemar@redhat.com>
From: Laine Stump <laine@laine.org>
Date: Fri, 8 Nov 2013 05:42:33 -0700
Subject: [PATCH] pci: properly handle out-of-order SRIOV virtual functions

This resolves:

  https://bugzilla.redhat.com/show_bug.cgi?id=1025397

When virPCIGetVirtualFunctions created the list of an SRIOV Physical
Function's (PF) Virtual Functions (VF), it had assumed that the order
of "virtfn*" links returned by readdir() from the PF's sysfs directory
was already in the correct order. Experience has shown that this is
not always the case - it can be in alphabetical order (which would
e.g. place virtfn11 before virtfn2) or even some seemingly random
order (see the example in the bugzilla report)

This results in 1) incorrect assumptions made by consumers of the
output of the virt_functions list of virsh nodedev-dumpxml, and 2)
setting MAC address and vlan tag on the wrong VF (since libvirt uses
netlink to set mac address and vlan tag, netlink requires the VF#, and
the function virPCIGetVirtualFunctionIndex() returns the wrong index
due to the improperly ordered VF list).

The solution provided by this patch is for virPCIGetVirtualFunctions
to no longer scan the entire device directory in its natural order,
but instead to check for links individually by name "virtfn%d" where
%d starts at 0 and increases with each success. Since VFs are created
contiguously by the kernel, this will guarantee that all VFs are
found, and placed in the arry in the correct order.

One note of use to the uninitiated is that VIR_APPEND_ELEMENT always
either increments *num_virtual_functions or fails, so no this isn't an
endless loop.

(NB: the SRIOV_* defines at the top of virpci.c were removed
because they are unnecessary and/or not used.)

(cherry picked from commit 88c1fcd5e7b6568fdb1a06e0d9001c37d5e1cb78)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/util/virpci.c | 76 ++++++++++++++++++-------------------------------------
 1 file changed, 24 insertions(+), 52 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 87ea81b..39bbac8 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -49,10 +49,6 @@
 #define PCI_ID_LEN 10   /* "XXXX XXXX" */
 #define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */
 
-#define SRIOV_FOUND 0
-#define SRIOV_NOT_FOUND 1
-#define SRIOV_ERROR -1
-
 struct _virPCIDevice {
     unsigned int  domain;
     unsigned int  bus;
@@ -2379,6 +2375,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path,
     return ret;
 }
 
+
 /*
  * Returns virtual functions of a physical function
  */
@@ -2389,10 +2386,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
 {
     int ret = -1;
     size_t i;
-    DIR *dir = NULL;
-    struct dirent *entry = NULL;
     char *device_link = NULL;
-    char errbuf[64];
+    virPCIDeviceAddress *config_addr = NULL;
 
     VIR_DEBUG("Attempting to get SR IOV virtual functions for device"
               "with sysfs path '%s'", sysfs_path);
@@ -2400,65 +2395,42 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
     *virtual_functions = NULL;
     *num_virtual_functions = 0;
 
-    dir = opendir(sysfs_path);
-    if (dir == NULL) {
-        memset(errbuf, '\0', sizeof(errbuf));
-        virReportSystemError(errno,
-                             _("Failed to open dir '%s'"),
-                             sysfs_path);
-        return ret;
-    }
-
-    while ((entry = readdir(dir))) {
-        if (STRPREFIX(entry->d_name, "virtfn")) {
-            virPCIDeviceAddress *config_addr = NULL;
-
-            if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) {
-                virReportOOMError();
-                goto error;
-            }
+    do {
+        /* look for virtfn%d links until one isn't found */
+        if (virAsprintf(&device_link, "%s/virtfn%zu", sysfs_path, *num_virtual_functions) < 0)
+            goto error;
 
-            VIR_DEBUG("Number of virtual functions: %zu",
-                      *num_virtual_functions);
+        if (!virFileExists(device_link))
+            break;
 
-            if (virPCIGetDeviceAddressFromSysfsLink(device_link,
-                                                    &config_addr) !=
-                SRIOV_FOUND) {
-                VIR_WARN("Failed to get SRIOV function from device "
-                         "link '%s'", device_link);
-                VIR_FREE(device_link);
-                continue;
-            }
+        if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Failed to get SRIOV function from device link '%s'"),
+                           device_link);
+            goto error;
+        }
 
-            if (VIR_REALLOC_N(*virtual_functions,
-                              *num_virtual_functions + 1) < 0) {
-                VIR_FREE(config_addr);
-                goto error;
-            }
+        VIR_DEBUG("Found virtual function %zu", *num_virtual_functions);
+        if (VIR_APPEND_ELEMENT(*virtual_functions, *num_virtual_functions, config_addr) < 0)
+            goto error;
+        VIR_FREE(device_link);
 
-            (*virtual_functions)[*num_virtual_functions] = config_addr;
-            (*num_virtual_functions)++;
-            VIR_FREE(device_link);
-        }
-    }
+    } while (1);
 
     ret = 0;
-
 cleanup:
     VIR_FREE(device_link);
-    if (dir)
-        closedir(dir);
+    VIR_FREE(config_addr);
     return ret;
 
 error:
-    if (*virtual_functions) {
-        for (i = 0; i < *num_virtual_functions; i++)
-            VIR_FREE((*virtual_functions)[i]);
-        VIR_FREE(*virtual_functions);
-    }
+    for (i = 0; i < *num_virtual_functions; i++)
+        VIR_FREE((*virtual_functions)[i]);
+    VIR_FREE(*virtual_functions);
     goto cleanup;
 }
 
+
 /*
  * Returns 1 if vf device is a virtual function, 0 if not, -1 on error
  */
-- 
1.8.4.2