[libvirt] [PATCH 0/2] nodedev: fix virtual_functions element
Moshe Levi pointed out yesterday in an email to libvir-list that there was no way to tell from the node device API that a PCI was capable of having SRIOV virtual functions (VFs) if it was currently setup to not have any: https://www.redhat.com/archives/libvir-list/2015-November/msg00894.html These two patches modify the node device XML in two ways: 1) if the device has a file called sriov_totalvfs, the value contained in that file will be included in the capabilities element as the attribute "maxCount". 2) in this case, even if there are currently no VFs active/visible for the device, the virtual_functions capability will still be reported. Laine Stump (2): conf: support reporting maxCount attribute for virtual_functions cap nodedev: report maxCount for virtual_functions capability docs/formatnode.html.in | 11 ++++++++++- src/conf/node_device_conf.c | 32 +++++++++++++++++++------------ src/conf/node_device_conf.h | 1 + src/network/bridge_driver.c | 5 +++-- src/node_device/node_device_linux_sysfs.c | 7 +++++-- src/util/virnetdev.c | 9 ++++++--- src/util/virnetdev.h | 5 +++-- src/util/virpci.c | 30 +++++++++++++++++++++++++---- src/util/virpci.h | 5 +++-- 9 files changed, 77 insertions(+), 28 deletions(-) -- 2.4.3
Report the maximum possible number of VFs for an SRIOV PF, like this: <capability type='virtual_functions' maxCount='7'> ... </capability> I've just discovered that the virtual_functions and physical_functions capabilities are not supported in the virNodeDeviceParse functions, only in virNodeDeviceFormat (I suppose because they are only reported, not set from XML). This should probably be remedied, but is less immediately useful than the current patch. --- docs/formatnode.html.in | 11 ++++++++++- src/conf/node_device_conf.c | 32 ++++++++++++++++++++------------ src/conf/node_device_conf.h | 1 + 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index ed00af5..79e2448 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -108,7 +108,16 @@ the type is <code>virtual_functions</code>, then this device is an SRIOV PF, and the capability element will have a list of <code>address</code> subelements, one - for each VF on this PF. + for each VF on this PF. If the host system supports + reporting it (via the "sriov_maxvfs" file in the + device's sysfs directory) the capability element will + also have an attribute named <code>maxCount</code> + which is the maximum number of SRIOV VFs supported by + this device, which could be higher than the number of + VFs that are curently active <span class="since">since + 1.2.22</span>; in this case, even if there are + currently no active VFs the virtual_functions + capabililty will still be shown. </dd> <dt><code>numa</code></dt> <dd> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index e6f3f27..c04739f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -361,19 +361,27 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferAddLit(&buf, "</capability>\n"); } if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION) { - virBufferAddLit(&buf, "<capability type='virt_functions'>\n"); - virBufferAdjustIndent(&buf, 2); - for (i = 0; i < data->pci_dev.num_virtual_functions; i++) { - virBufferAsprintf(&buf, - "<address domain='0x%.4x' bus='0x%.2x' " - "slot='0x%.2x' function='0x%.1x'/>\n", - data->pci_dev.virtual_functions[i]->domain, - data->pci_dev.virtual_functions[i]->bus, - data->pci_dev.virtual_functions[i]->slot, - data->pci_dev.virtual_functions[i]->function); + virBufferAddLit(&buf, "<capability type='virt_functions'"); + if (data->pci_dev.max_virtual_functions) + virBufferAsprintf(&buf, " maxCount='%u'", + data->pci_dev.max_virtual_functions); + if (data->pci_dev.num_virtual_functions == 0) { + virBufferAddLit(&buf, "/>\n"); + } else { + virBufferAddLit(&buf, ">\n"); + virBufferAdjustIndent(&buf, 2); + for (i = 0; i < data->pci_dev.num_virtual_functions; i++) { + virBufferAsprintf(&buf, + "<address domain='0x%.4x' bus='0x%.2x' " + "slot='0x%.2x' function='0x%.1x'/>\n", + data->pci_dev.virtual_functions[i]->domain, + data->pci_dev.virtual_functions[i]->bus, + data->pci_dev.virtual_functions[i]->slot, + data->pci_dev.virtual_functions[i]->function); + } + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</capability>\n"); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</capability>\n"); } if (data->pci_dev.nIommuGroupDevices) { virBufferAsprintf(&buf, "<iommuGroup number='%d'>\n", diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 7dd39ca..d007186 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -112,6 +112,7 @@ typedef struct _virNodeDevCapData { virPCIDeviceAddressPtr physical_function; virPCIDeviceAddressPtr *virtual_functions; size_t num_virtual_functions; + unsigned int max_virtual_functions; unsigned int flags; virPCIDeviceAddressPtr *iommuGroupDevices; size_t nIommuGroupDevices; -- 2.4.3
On Mon, Nov 23, 2015 at 04:50:13PM -0500, Laine Stump wrote:
Report the maximum possible number of VFs for an SRIOV PF, like this:
<capability type='virtual_functions' maxCount='7'> ... </capability>
I've just discovered that the virtual_functions and physical_functions capabilities are not supported in the virNodeDeviceParse functions, only in virNodeDeviceFormat (I suppose because they are only reported, not set from XML). This should probably be remedied, but is less immediately useful than the current patch. --- docs/formatnode.html.in | 11 ++++++++++- src/conf/node_device_conf.c | 32 ++++++++++++++++++++------------ src/conf/node_device_conf.h | 1 + 3 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index ed00af5..79e2448 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -108,7 +108,16 @@ the type is <code>virtual_functions</code>, then this device is an SRIOV PF, and the capability element will have a list of <code>address</code> subelements, one - for each VF on this PF. + for each VF on this PF. If the host system supports + reporting it (via the "sriov_maxvfs" file in the + device's sysfs directory) the capability element will + also have an attribute named <code>maxCount</code> + which is the maximum number of SRIOV VFs supported by + this device, which could be higher than the number of + VFs that are curently active <span class="since">since + 1.2.22</span>; in this case, even if there are
The new version will be 1.3.0
+ currently no active VFs the virtual_functions + capabililty will still be shown. </dd> <dt><code>numa</code></dt> <dd>
[...] ACK with that change. Pavel
A PCI device may have the capability to setup virtual functions (VFs) but have them currently all disabled. Prior to this patch, if that was the case the the node device XML for the device wouldn't report any virtual_functions capability. With this patch, if a file called "sriov_totalvfs" is found in the device's sysfs directory, its contents will be interpreted as a decimal number, and that value will be reported as "maxCount" in a capability element of the device's XML, e.g.: <capability type='virtual_functions' maxCount='7'/> This will be reported regardless of whether or not any VFs are currently enabled for the device. NB: sriov_numvfs (the number of VFs currently active) is also available in sysfs, but that value is implied by the number of items in the list that is inside the capability element, so there is no reason to explicitly provide it as an attribute. sriov_totalvfs and sriov_numvfs are available in kernels at least as far back as the 2.6.32 that is in RHEL6.7, but in the case that they simply aren't there, libvirt will behave as it did prior to this patch - no maxCount will be displayed, and the virtual_functions capability will be absent from the device's XML when 0 VFs are enabled. --- src/network/bridge_driver.c | 5 +++-- src/node_device/node_device_linux_sysfs.c | 7 +++++-- src/util/virnetdev.c | 9 ++++++--- src/util/virnetdev.h | 5 +++-- src/util/virpci.c | 30 ++++++++++++++++++++++++++---- src/util/virpci.h | 5 +++-- 6 files changed, 46 insertions(+), 15 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f838671..a0f6494 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2334,6 +2334,7 @@ static int networkCreateInterfacePool(virNetworkDefPtr netdef) { size_t numVirtFns = 0; + unsigned int maxVirtFns = 0; char **vfNames = NULL; virPCIDeviceAddressPtr *virtFns; @@ -2343,8 +2344,8 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) if (netdef->forward.npfs == 0 || netdef->forward.nifs > 0) return 0; - if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev, - &vfNames, &virtFns, &numVirtFns)) < 0) { + if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev, &vfNames, + &virtFns, &numVirtFns, &maxVirtFns)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not get Virtual functions on %s"), netdef->forward.pfs->dev); diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 6d5a406..431f471 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -150,6 +150,7 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, VIR_FREE(data->pci_dev.virtual_functions[i]); VIR_FREE(data->pci_dev.virtual_functions); data->pci_dev.num_virtual_functions = 0; + data->pci_dev.max_virtual_functions = 0; data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; @@ -157,11 +158,13 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; ret = virPCIGetVirtualFunctions(sysfsPath, &data->pci_dev.virtual_functions, - &data->pci_dev.num_virtual_functions); + &data->pci_dev.num_virtual_functions, + &data->pci_dev.max_virtual_functions); if (ret < 0) return ret; - if (data->pci_dev.num_virtual_functions > 0) + if (data->pci_dev.num_virtual_functions > 0 || + data->pci_dev.max_virtual_functions > 0) data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; return ret; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index ade9afa..e7f44cd 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1802,7 +1802,8 @@ int virNetDevGetVirtualFunctions(const char *pfname, char ***vfname, virPCIDeviceAddressPtr **virt_fns, - size_t *n_vfname) + size_t *n_vfname, + unsigned int *max_vfs) { int ret = -1; size_t i; @@ -1812,12 +1813,13 @@ virNetDevGetVirtualFunctions(const char *pfname, *virt_fns = NULL; *n_vfname = 0; + *max_vfs = 0; if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) return ret; if (virPCIGetVirtualFunctions(pf_sysfs_device_link, virt_fns, - n_vfname) < 0) + n_vfname, max_vfs) < 0) goto cleanup; if (VIR_ALLOC_N(*vfname, *n_vfname) < 0) @@ -1987,7 +1989,8 @@ int virNetDevGetVirtualFunctions(const char *pfname ATTRIBUTE_UNUSED, char ***vfname ATTRIBUTE_UNUSED, virPCIDeviceAddressPtr **virt_fns ATTRIBUTE_UNUSED, - size_t *n_vfname ATTRIBUTE_UNUSED) + size_t *n_vfname ATTRIBUTE_UNUSED, + unsigned int *max_vfs 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 772ae75..e7719d5 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -163,9 +163,10 @@ int virNetDevGetPhysicalFunction(const char *ifname, char **pfname) int virNetDevGetVirtualFunctions(const char *pfname, char ***vfname, virPCIDeviceAddressPtr **virt_fns, - size_t *n_vfname) + size_t *n_vfname, + unsigned int *max_vfs) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) - ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; int virNetDevLinkDump(const char *ifname, int ifindex, void **nlData, struct nlattr **tb, diff --git a/src/util/virpci.c b/src/util/virpci.c index 35b1459..bececb5 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1,7 +1,7 @@ /* * virpci.c: helper APIs for managing host PCI devices * - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -2514,18 +2514,36 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, int virPCIGetVirtualFunctions(const char *sysfs_path, virPCIDeviceAddressPtr **virtual_functions, - size_t *num_virtual_functions) + size_t *num_virtual_functions, + unsigned int *max_virtual_functions) { int ret = -1; size_t i; char *device_link = NULL; virPCIDeviceAddress *config_addr = NULL; + char *totalvfs_file = NULL, *totalvfs_str = NULL; VIR_DEBUG("Attempting to get SR IOV virtual functions for device" "with sysfs path '%s'", sysfs_path); *virtual_functions = NULL; *num_virtual_functions = 0; + *max_virtual_functions = 0; + + if (virAsprintf(&totalvfs_file, "%s/sriov_totalvfs", sysfs_path) < 0) + goto error; + if (virFileExists(totalvfs_file)) { + char *end = NULL; /* so that terminating \n doesn't create error */ + + if (virFileReadAll(totalvfs_file, 16, &totalvfs_str) < 0) + goto error; + if (virStrToLong_ui(totalvfs_str, &end, 10, max_virtual_functions) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unrecognized value in %s: %s"), + totalvfs_file, totalvfs_str); + goto error; + } + } do { /* look for virtfn%d links until one isn't found */ @@ -2553,6 +2571,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path, cleanup: VIR_FREE(device_link); VIR_FREE(config_addr); + VIR_FREE(totalvfs_file); + VIR_FREE(totalvfs_str); return ret; error: @@ -2594,6 +2614,7 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, int ret = -1; size_t i; size_t num_virt_fns = 0; + unsigned int max_virt_fns = 0; virPCIDeviceAddressPtr vf_bdf = NULL; virPCIDeviceAddressPtr *virt_fns = NULL; @@ -2602,7 +2623,7 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, return ret; if (virPCIGetVirtualFunctions(pf_sysfs_device_link, &virt_fns, - &num_virt_fns) < 0) { + &num_virt_fns, &max_virt_fns) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Error getting physical function's '%s' " "virtual_functions"), pf_sysfs_device_link); @@ -2738,7 +2759,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path ATTRIBUTE_UNUSED, int virPCIGetVirtualFunctions(const char *sysfs_path ATTRIBUTE_UNUSED, virPCIDeviceAddressPtr **virtual_functions ATTRIBUTE_UNUSED, - size_t *num_virtual_functions ATTRIBUTE_UNUSED) + size_t *num_virtual_functions ATTRIBUTE_UNUSED, + unsigned int *max_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 64b9e96..6516f05 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -1,7 +1,7 @@ /* * virpci.h: helper APIs for managing host PCI devices * - * Copyright (C) 2009, 2011-2014 Red Hat, Inc. + * Copyright (C) 2009, 2011-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -171,7 +171,8 @@ int virPCIGetPhysicalFunction(const char *sysfs_path, int virPCIGetVirtualFunctions(const char *sysfs_path, virPCIDeviceAddressPtr **virtual_functions, - size_t *num_virtual_functions); + size_t *num_virtual_functions, + unsigned int *max_virtual_functions); int virPCIIsVirtualFunction(const char *vf_sysfs_device_link); -- 2.4.3
On Mon, Nov 23, 2015 at 04:50:14PM -0500, Laine Stump wrote:
A PCI device may have the capability to setup virtual functions (VFs) but have them currently all disabled. Prior to this patch, if that was the case the the node device XML for the device wouldn't report any
s/the the/the/
virtual_functions capability.
With this patch, if a file called "sriov_totalvfs" is found in the device's sysfs directory, its contents will be interpreted as a decimal number, and that value will be reported as "maxCount" in a capability element of the device's XML, e.g.:
<capability type='virtual_functions' maxCount='7'/>
This will be reported regardless of whether or not any VFs are currently enabled for the device.
NB: sriov_numvfs (the number of VFs currently active) is also available in sysfs, but that value is implied by the number of items in the list that is inside the capability element, so there is no reason to explicitly provide it as an attribute.
sriov_totalvfs and sriov_numvfs are available in kernels at least as far back as the 2.6.32 that is in RHEL6.7, but in the case that they simply aren't there, libvirt will behave as it did prior to this patch - no maxCount will be displayed, and the virtual_functions capability will be absent from the device's XML when 0 VFs are enabled. --- src/network/bridge_driver.c | 5 +++-- src/node_device/node_device_linux_sysfs.c | 7 +++++-- src/util/virnetdev.c | 9 ++++++--- src/util/virnetdev.h | 5 +++-- src/util/virpci.c | 30 ++++++++++++++++++++++++++---- src/util/virpci.h | 5 +++-- 6 files changed, 46 insertions(+), 15 deletions(-)
ACK Pavel
participants (2)
-
Laine Stump -
Pavel Hrdina