[libvirt] [PATCHv3 0/2] pci: properly handle out-of-order SRIOV virtual functions
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1025397 It has turned from a single patch into a 2 part patch because I wanted to use VIR_APPEND_ELEMENT on the array, and that requires the array size to be held in a size_t rather than unsigned int. Thanks to Martin's suggestion (and similar suggestions from both Jan and "Niilona" (the original reporter of the problem), this version is greatly simplified from either my previous version or even the original code (as evidenced by the diffstat). Laine Stump (2): util: use size_t instead of unsigned int for num_virtual_functions pci: properly handle out-of-order SRIOV virtual functions src/conf/node_device_conf.h | 2 +- src/network/bridge_driver.c | 2 +- src/util/virnetdev.c | 4 +-- src/util/virnetdev.h | 2 +- src/util/virpci.c | 84 +++++++++++++++------------------------------ src/util/virpci.h | 2 +- 6 files changed, 34 insertions(+), 62 deletions(-) -- 1.8.3.1
This is a prerequisite to the fix for the fix to: https://bugzilla.redhat.com/show_bug.cgi?id=1025397 num_virtual_functions needs to be size_t in order to use the VIR_APPEND_ELEMENT macro. --- src/conf/node_device_conf.h | 2 +- src/network/bridge_driver.c | 2 +- src/util/virnetdev.c | 4 ++-- src/util/virnetdev.h | 2 +- src/util/virpci.c | 8 ++++---- src/util/virpci.h | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index af8fedc..9f3abe7 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -110,7 +110,7 @@ struct _virNodeDevCapsDef { char *vendor_name; virPCIDeviceAddressPtr physical_function; virPCIDeviceAddressPtr *virtual_functions; - unsigned int num_virtual_functions; + size_t num_virtual_functions; unsigned int flags; virPCIDeviceAddressPtr *iommuGroupDevices; size_t nIommuGroupDevices; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 41edb97..3423a45 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3046,7 +3046,7 @@ int networkRegister(void) { */ static int networkCreateInterfacePool(virNetworkDefPtr netdef) { - unsigned int num_virt_fns = 0; + size_t num_virt_fns = 0; char **vfname = NULL; virPCIDeviceAddressPtr *virt_fns; int ret = -1; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 441b171..e74fc5f 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1104,7 +1104,7 @@ int virNetDevGetVirtualFunctions(const char *pfname, char ***vfname, virPCIDeviceAddressPtr **virt_fns, - unsigned int *n_vfname) + size_t *n_vfname) { int ret = -1; size_t i; @@ -1291,7 +1291,7 @@ int virNetDevGetVirtualFunctions(const char *pfname ATTRIBUTE_UNUSED, char ***vfname ATTRIBUTE_UNUSED, virPCIDeviceAddressPtr **virt_fns ATTRIBUTE_UNUSED, - unsigned int *n_vfname ATTRIBUTE_UNUSED) + size_t *n_vfname ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Unable to get virtual functions on this platform")); diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 8e9ee2d..0aa5dc7 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -124,7 +124,7 @@ int virNetDevGetPhysicalFunction(const char *ifname, char **pfname) int virNetDevGetVirtualFunctions(const char *pfname, char ***vfname, virPCIDeviceAddressPtr **virt_fns, - unsigned int *n_vfname) + size_t *n_vfname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; diff --git a/src/util/virpci.c b/src/util/virpci.c index 148631f..0fe5544 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2385,7 +2385,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, int virPCIGetVirtualFunctions(const char *sysfs_path, virPCIDeviceAddressPtr **virtual_functions, - unsigned int *num_virtual_functions) + size_t *num_virtual_functions) { int ret = -1; size_t i; @@ -2418,7 +2418,7 @@ virPCIGetVirtualFunctions(const char *sysfs_path, goto error; } - VIR_DEBUG("Number of virtual functions: %d", + VIR_DEBUG("Number of virtual functions: %zu", *num_virtual_functions); if (virPCIGetDeviceAddressFromSysfsLink(device_link, @@ -2489,7 +2489,7 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, { int ret = -1; size_t i; - unsigned int num_virt_fns = 0; + size_t num_virt_fns = 0; virPCIDeviceAddressPtr vf_bdf = NULL; virPCIDeviceAddressPtr *virt_fns = NULL; @@ -2634,7 +2634,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path ATTRIBUTE_UNUSED, int virPCIGetVirtualFunctions(const char *sysfs_path ATTRIBUTE_UNUSED, virPCIDeviceAddressPtr **virtual_functions ATTRIBUTE_UNUSED, - unsigned int *num_virtual_functions ATTRIBUTE_UNUSED) + size_t *num_virtual_functions ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; diff --git a/src/util/virpci.h b/src/util/virpci.h index 0aa6fee..0479f0b 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -138,7 +138,7 @@ int virPCIGetPhysicalFunction(const char *sysfs_path, int virPCIGetVirtualFunctions(const char *sysfs_path, virPCIDeviceAddressPtr **virtual_functions, - unsigned int *num_virtual_functions); + size_t *num_virtual_functions); int virPCIIsVirtualFunction(const char *vf_sysfs_device_link); -- 1.8.3.1
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.) --- src/util/virpci.c | 78 ++++++++++++++++++------------------------------------- 1 file changed, 25 insertions(+), 53 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 0fe5544..e59e69d 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 */ @@ -2388,11 +2385,9 @@ virPCIGetVirtualFunctions(const char *sysfs_path, size_t *num_virtual_functions) { int ret = -1; - size_t i; - DIR *dir = NULL; - struct dirent *entry = NULL; + size_t i = 0; 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.3.1
On 11/08/2013 12:11 PM, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1025397
It has turned from a single patch into a 2 part patch because I wanted to use VIR_APPEND_ELEMENT on the array, and that requires the array size to be held in a size_t rather than unsigned int.
Thanks to Martin's suggestion (and similar suggestions from both Jan and "Niilona" (the original reporter of the problem), this version is greatly simplified from either my previous version or even the original code (as evidenced by the diffstat).
Laine Stump (2): util: use size_t instead of unsigned int for num_virtual_functions pci: properly handle out-of-order SRIOV virtual functions
src/conf/node_device_conf.h | 2 +- src/network/bridge_driver.c | 2 +- src/util/virnetdev.c | 4 +-- src/util/virnetdev.h | 2 +- src/util/virpci.c | 84 +++++++++++++++------------------------------ src/util/virpci.h | 2 +- 6 files changed, 34 insertions(+), 62 deletions(-)
ACK
participants (2)
-
Ján Tomko -
Laine Stump