[libvirt] [PATCHv2] pci: properly handle out-of-order SRIOV virtual functions
Laine Stump
laine at laine.org
Thu Nov 7 11:10:39 UTC 2013
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1025397
When libvirt creates the list of an SRIOV Physical Function's (PF)
Virtual Functions (VF), it assumes that the order of "virtfn*" links
returned by readdir() from the PF's sysfs directory is already in the
correct order. Experience has shown that this is not always the case.
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). See the bugzilla report for an
example of this improper behavior.
The solution provided by this patch is for virPCIGetVirtualFunctions
to first gather all the "virtfn*" names from the PFs sysfs directory,
then allocate a virtual_functions array large enough to hold all
entries, and finally to create a device entry for each virtfn* name
and place it into the virtual_functions array at the proper index
(based on interpreting the value following "virtfn" in the name).
Checks are introduced to ensure that 1) the virtfn list given in sysfs
is not sparse (ie, there are no indexes larger than the length of the
list), and that no two virtfn* entries decode to the same index.
---
Difference from V1: Based on jtomko's review, truncate trailing NULLs
in virtual_functions array, and actually check for NULLs in the middle
(a sparse array) and generate an error in that case, as we already
claimed to do in the commit message.
(Sorry for the lack of inter-diff, but the only changes from V1 are
the addition of the lines starting with "truncate any trailing nulls"
and continuing until the next VIR_DEBUG.)
src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 90 insertions(+), 25 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 148631f..f744c8b 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2379,6 +2379,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path,
return ret;
}
+static const char *virtfnPrefix = "virtfn";
+
/*
* Returns virtual functions of a physical function
*/
@@ -2393,6 +2395,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
struct dirent *entry = NULL;
char *device_link = NULL;
char errbuf[64];
+ char **virtfnNames = NULL;
+ size_t nVirtfnNames = 0;
VIR_DEBUG("Attempting to get SR IOV virtual functions for device"
"with sysfs path '%s'", sysfs_path);
@@ -2411,51 +2415,112 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
while ((entry = readdir(dir))) {
if (STRPREFIX(entry->d_name, "virtfn")) {
- virPCIDeviceAddress *config_addr = NULL;
+ char *tempName;
- if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) {
- virReportOOMError();
+ /* save these to sort into virtual_functions array */
+ if (VIR_STRDUP(tempName, entry->d_name) < 0)
+ goto error;
+ if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) {
+ VIR_FREE(tempName);
goto error;
}
+ }
+ }
- VIR_DEBUG("Number of virtual functions: %d",
- *num_virtual_functions);
+ /* pre-allocate because we will be filling in out of order */
+ if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0)
+ goto error;
+ *num_virtual_functions = nVirtfnNames;
- 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;
- }
+ for (i = 0; i < nVirtfnNames; i++) {
+ virPCIDeviceAddress *config_addr = NULL;
+ unsigned int virtfnIndex;
- if (VIR_REALLOC_N(*virtual_functions,
- *num_virtual_functions + 1) < 0) {
- VIR_FREE(config_addr);
- goto error;
- }
+ if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+
+ if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix),
+ 0, 10, &virtfnIndex) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid virtual function link name '%s' "
+ "in physical function directory '%s'"),
+ virtfnNames[i], sysfs_path);
+ goto error;
+ }
+
+ if (virtfnIndex >= nVirtfnNames) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Virtual function link name '%s' larger than "
+ "total count of virtual functions %zu "
+ "in physical function directory '%s'"),
+ virtfnNames[i], nVirtfnNames, sysfs_path);
+ goto error;
+ }
+
+ if ((*virtual_functions)[virtfnIndex]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Virtual function link name '%s' "
+ "generates duplicate index %zu "
+ "in physical function directory '%s'"),
+ virtfnNames[i], nVirtfnNames, sysfs_path);
+ goto error;
+ }
- (*virtual_functions)[*num_virtual_functions] = config_addr;
- (*num_virtual_functions)++;
+ 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;
}
+
+ VIR_DEBUG("Found virtual function %d", virtfnIndex);
+ (*virtual_functions)[virtfnIndex] = config_addr;
+ VIR_FREE(device_link);
}
+ /* truncate any trailing nulls due to files having names starting
+ * with "virtfn" which weren't actually a link to a virtual
+ * function.
+ */
+ while (*num_virtual_functions &&
+ !(*virtual_functions)[(*num_virtual_functions) - 1])
+ (*num_virtual_functions)--;
+
+ /* failure of realloc when shrinking is safe */
+ ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions));
+
+ /* if there are any NULLs left in the array, fail and log an error
+ * - the virtual function array cannot be sparse.
+ */
+ for (i = 0; i < *num_virtual_functions; i++) {
+ if (!(*virtual_functions)[*num_virtual_functions]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Missing virtual function link for VF %zu "
+ "in physical function directory '%s'"),
+ i, sysfs_path);
+ goto error;
+ }
+ }
+
+ VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions);
ret = 0;
cleanup:
+ for (i = 0; i < nVirtfnNames; i++)
+ VIR_FREE(virtfnNames[i]);
+ VIR_FREE(virtfnNames);
VIR_FREE(device_link);
if (dir)
closedir(dir);
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;
}
--
1.8.3.1
More information about the libvir-list
mailing list