The first arg isn't modified in the function, so it should be const. --- New for V2. src/util/virpci.c | 4 ++-- src/util/virpci.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 2c1b75855..110d9741c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2857,7 +2857,7 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr, * Returns the network device name of a pci device */ int -virPCIGetNetName(char *device_link_sysfs_path, char **netname) +virPCIGetNetName(const char *device_link_sysfs_path, char **netname) { char *pcidev_sysfs_net_path = NULL; int ret = -1; @@ -2991,7 +2991,7 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr dev ATTRIBUTE_UNUSED, } int -virPCIGetNetName(char *device_link_sysfs_path ATTRIBUTE_UNUSED, +virPCIGetNetName(const char *device_link_sysfs_path ATTRIBUTE_UNUSED, char **netname ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); diff --git a/src/util/virpci.h b/src/util/virpci.h index 570684e75..82d4ddc61 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -207,7 +207,7 @@ int virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, int virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr, char **pci_sysfs_device_link); -int virPCIGetNetName(char *device_link_sysfs_path, char **netname); +int virPCIGetNetName(const char *device_link_sysfs_path, char **netname); int virPCIGetSysfsFile(char *virPCIDeviceName, char **pci_sysfs_device_link) -- 2.13.3

This patch updates functions in netdev.c to pay attention to phys_port_id. It uses the new function virNetDevGetPhysPortID() to learn the phys_port_id of a VF or PF, then sends that info to virPCIGetNetName(), which has newly been modified to take an optional phys_port_id. --- Change from V1 - Minor changes due to differences in patch 3. src/util/virnetdev.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index b6ef00e2e..5738a8936 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1231,13 +1231,17 @@ virNetDevGetVirtualFunctions(const char *pfname, char *pf_sysfs_device_link = NULL; char *pci_sysfs_device_link = NULL; char *pciConfigAddr = NULL; + char *pfPhysPortID = NULL; *virt_fns = NULL; *n_vfname = 0; *max_vfs = 0; + if (virNetDevGetPhysPortID(pfname, &pfPhysPortID) < 0) + goto cleanup; + if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) - return ret; + goto cleanup; if (virPCIGetVirtualFunctions(pf_sysfs_device_link, virt_fns, n_vfname, max_vfs) < 0) @@ -1263,7 +1267,7 @@ virNetDevGetVirtualFunctions(const char *pfname, } if (virPCIGetNetName(pci_sysfs_device_link, 0, - NULL, &((*vfname)[i])) < 0) { + pfPhysPortID, &((*vfname)[i])) < 0) { goto cleanup; } @@ -1278,6 +1282,7 @@ virNetDevGetVirtualFunctions(const char *pfname, VIR_FREE(*vfname); VIR_FREE(*virt_fns); } + VIR_FREE(pfPhysPortID); VIR_FREE(pf_sysfs_device_link); VIR_FREE(pci_sysfs_device_link); VIR_FREE(pciConfigAddr); @@ -1359,14 +1364,19 @@ int virNetDevGetPhysicalFunction(const char *ifname, char **pfname) { char *physfn_sysfs_path = NULL; + char *vfPhysPortID = NULL; int ret = -1; + if (virNetDevGetPhysPortID(ifname, &vfPhysPortID) < 0) + goto cleanup; + if (virNetDevSysfsDeviceFile(&physfn_sysfs_path, ifname, "physfn") < 0) - return ret; + goto cleanup; if (virPCIGetNetName(physfn_sysfs_path, 0, - NULL, pfname) < 0) + vfPhysPortID, pfname) < 0) { goto cleanup; + } if (!*pfname) { /* this shouldn't be possible. A VF can't exist unless its @@ -1380,6 +1390,7 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname) ret = 0; cleanup: + VIR_FREE(vfPhysPortID); VIR_FREE(physfn_sysfs_path); return ret; } @@ -1407,8 +1418,16 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname) { char *virtfnName = NULL; char *virtfnSysfsPath = NULL; + char *pfPhysPortID = NULL; int ret = -1; + /* a VF may have multiple "ports", each one having its own netdev, + * and each netdev having a different phys_port_id. Be sure we get + * the VF netdev with a phys_port_id matchine that of pfname + */ + if (virNetDevGetPhysPortID(pfname, &pfPhysPortID) < 0) + goto cleanup; + if (virAsprintf(&virtfnName, "virtfn%d", vf) < 0) goto cleanup; @@ -1425,11 +1444,12 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname) * isn't bound to a netdev driver, it won't have a netdev name, * and vfname will be NULL). */ - ret = virPCIGetNetName(virtfnSysfsPath, 0, NULL, vfname); + ret = virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname); cleanup: VIR_FREE(virtfnName); VIR_FREE(virtfnSysfsPath); + VIR_FREE(pfPhysPortID); return ret; } -- 2.13.3

virHostdevRestoreNetConfig() calls virNetDevReadNetConfig() to try and read the "original config" of a netdev, and if that fails, it tries again with a different directory/netdev name. This achieves the desired effect (we end up finding the config wherever it may be), but for each failure, virNetDevReadNetConfig() places a nice error message in the system logs. Experience has shown that false-positive error logs like this lead to erroneous bug reports, and can often mislead those searching for *real* bugs. This patch changes virNetDevReadNetConfig() to explicitly check if the file exists before calling virFileReadAll(); if it doesn't exist, virNetDevReadNetConfig() returns a success, but leaves all the variables holding the results as NULL. (This makes sense if you define the purpose of the function as "read a netdev's config from its config file *if that file exists*). To take advantage of that change, the caller, virHostdevRestoreNetConfig() is modified to fail immediately if virNetDevReadNetConfig() returns an error, and otherwise to try the different directory/netdev name if adminMAC & vlan & MAC are all NULL after the preceding attempt. --- New in V2. src/util/virhostdev.c | 126 ++++++++++++++++++++++++++++---------------- src/util/virnetdev.c | 16 ++++-- src/util/virnetdevmacvlan.c | 5 +- 3 files changed, 96 insertions(+), 51 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 102fd85c1..cca9d81a4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -534,6 +534,10 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, int ret = -1; int vf = -1; bool port_profile_associate = false; + virMacAddrPtr MAC = NULL; + virMacAddrPtr adminMAC = NULL; + virNetDevVlanPtr vlan = NULL; + /* This is only needed for PCI devices that have been defined * using <interface type='hostdev'>. For all others, it is a NOP. @@ -559,62 +563,92 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, NULL, port_profile_associate); } else { - virMacAddrPtr MAC = NULL; - virMacAddrPtr adminMAC = NULL; - virNetDevVlanPtr vlan = NULL; - - ret = virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &vlan, &MAC); - if (ret < 0 && oldStateDir) - ret = virNetDevReadNetConfig(linkdev, vf, oldStateDir, - &adminMAC, &vlan, &MAC); - - if (ret < 0) { - /* see if the config was saved using the PF's "port 2" - * netdev for the file name. - */ - VIR_FREE(linkdev); + /* we need to try 3 different places for the config file: + * 1) ${stateDir}/${PF}_vf${vf} + * This is almost always where the saved config is + * + * 2) ${oldStateDir/${PF}_vf${vf} + * saved config is only here if this machine was running a + * (by now *very*) old version of libvirt that saved the + * file in a different directory + * + * 3) ${stateDir}${PF[1]}_vf${VF} + * PF[1] means "the netdev for port 2 of the PF device", and + * is only valid when the PF is a Mellanox dual port NIC with + * a VF that was created in "single port" mode. + * + * NB: if virNetDevReadNetConfig() returns < 0, then it found + * the file, but there was a problem, so we should + * immediately return an error to our caller. If it returns + * 0, but all of the interesting stuff is NULL, that means + * the file wasn't found, so we can/should check other + * locations for it. + */ - if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) >= 0) { - ret = virNetDevReadNetConfig(linkdev, vf, stateDir, - &adminMAC, &vlan, &MAC); - } + /* 1) standard location */ + if (virNetDevReadNetConfig(linkdev, vf, stateDir, + &adminMAC, &vlan, &MAC) < 0) { + goto cleanup; } - if (ret == 0) { - /* if a MAC was stored for the VF, we should now restore - * that as the adminMAC. We have to do it this way because - * the VF is still not bound to the host's net driver, so - * we can't directly set its MAC (and even after it is - * re-bound to the host net driver, it will still have its - * "administratively set" flag on, and that prohibits the - * VF's net driver from directly setting the MAC - * anyway). But it we set the desired VF MAC as the "admin - * MAC" *now*, then when the VF is re-bound to the host - * net driver (which will happen soon after returning from - * this function), that adminMAC will be set (by the PF) - * as the VF's new initial MAC. - * - * If no MAC was stored for the VF, that means it wasn't - * bound to a net driver before we used it anyway, so the - * adminMAC is all we have, and we can just restore it - * directly. - */ - if (MAC) { - VIR_FREE(adminMAC); - adminMAC = MAC; - MAC = NULL; + /* 2) "old" (pre-1.2.3 circa 2014) location - whenever we get + * to the point that nobody will ever upgrade directly from + * 1.2.3 (or older) directly to current libvirt, we can + * eliminate this clause + **/ + if (!(adminMAC || vlan || MAC) && oldStateDir && + virNetDevReadNetConfig(linkdev, vf, oldStateDir, + &adminMAC, &vlan, &MAC) < 0) { + goto cleanup; + } + + /* 3) try using the PF's "port 2" netdev as the name of the + * config file + */ + if (!(adminMAC || vlan || MAC)) { + VIR_FREE(linkdev); + + if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) < 0 || + virNetDevReadNetConfig(linkdev, vf, stateDir, + &adminMAC, &vlan, &MAC) < 0) { + goto cleanup; } + } - ignore_value(virNetDevSetNetConfig(linkdev, vf, - adminMAC, vlan, MAC, true)); + /* if a MAC was stored for the VF, we should now restore + * that as the adminMAC. We have to do it this way because + * the VF is still not bound to the host's net driver, so + * we can't directly set its MAC (and even after it is + * re-bound to the host net driver, it will still have its + * "administratively set" flag on, and that prohibits the + * VF's net driver from directly setting the MAC + * anyway). But it we set the desired VF MAC as the "admin + * MAC" *now*, then when the VF is re-bound to the host + * net driver (which will happen soon after returning from + * this function), that adminMAC will be set (by the PF) + * as the VF's new initial MAC. + * + * If no MAC was stored for the VF, that means it wasn't + * bound to a net driver before we used it anyway, so the + * adminMAC is all we have, and we can just restore it + * directly. + */ + if (MAC) { + VIR_FREE(adminMAC); + adminMAC = MAC; + MAC = NULL; } - VIR_FREE(MAC); - VIR_FREE(adminMAC); - virNetDevVlanFree(vlan); + ignore_value(virNetDevSetNetConfig(linkdev, vf, + adminMAC, vlan, MAC, true)); } + ret = 0; + cleanup: VIR_FREE(linkdev); + VIR_FREE(MAC); + VIR_FREE(adminMAC); + virNetDevVlanFree(vlan); return ret; } diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5738a8936..8fc643c93 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1971,7 +1971,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf, * @linkdev:@vf from a file in @stateDir. (see virNetDevSaveNetConfig * for details of file name and format). * - * Returns 0 on success, -1 on failure. + * Returns 0 on success, -1 on failure. It is *NOT* considered failure + * if no file is found to read. In that case, adminMAC, vlan, and MAC + * are set to NULL, and success is returned. * * The caller MUST free adminMAC, vlan, and MAC when it is finished * with them (they will be NULL if they weren't found in the file) @@ -2036,8 +2038,8 @@ virNetDevReadNetConfig(const char *linkdev, int vf, if (linkdev && !virFileExists(filePath)) { /* the device may have been stored in a file named for the * VF due to saveVlan == false (or an older version of - * libvirt), so reset filePath so we'll try the other - * filename before failing. + * libvirt), so reset filePath and pfDevName so we'll try + * the other filename. */ VIR_FREE(filePath); pfDevName = NULL; @@ -2049,6 +2051,14 @@ virNetDevReadNetConfig(const char *linkdev, int vf, goto cleanup; } + if (!virFileExists(filePath)) { + /* having no file to read is not necessarily an error, so we + * just return success, but with MAC, adminMAC, and vlan set to NULL + */ + ret = 0; + goto cleanup; + } + if (virFileReadAll(filePath, 128, &fileStr) < 0) goto cleanup; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 97c87701c..fb41bf934 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -1187,8 +1187,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, virMacAddrPtr adminMAC = NULL; virNetDevVlanPtr vlan = NULL; - if (virNetDevReadNetConfig(linkdev, -1, stateDir, - &adminMAC, &vlan, &MAC) == 0) { + if ((virNetDevReadNetConfig(linkdev, -1, stateDir, + &adminMAC, &vlan, &MAC) == 0) && + (adminMAC || vlan || MAC)) { ignore_value(virNetDevSetNetConfig(linkdev, -1, adminMAC, vlan, MAC, !!vlan)); -- 2.13.3

When using a VF from an SRIOV-capable network card in a guest (either in macvtap passthrough mode, or via VFIO PCI device assignment), The associated PF netdev must be online in order for the VF to be usable by the guest. The guest, however, is not able to change the state of the PF. And libvirt *could* set the PF online as needed, but that could lead to the host receiving unexpected IPv6 traffic (since the default for an unconfigured interface is to participate in IPv6 autoconf). For this reason, before assigning a VF to a guest, libvirt verifies that the related PF netdev is online - if it isn't, then we log an error and don't allow the guest startup to continue. Until now, this check was done during virNetDevSetNetConfig(). This works nicely because the same function is called both for macvtap passthrough and for VFIO device assignment. But in the case of VFIO, the VF has already been unbound from its netdev driver by the time we get to virNetDevSetNetConfig(), and in the case of dual port Mellanox NICs that have their VFs setup in single port mode, the *only* way to determine the proper PF netdev to query for online status is via the "phys_port_id" file that is in the VF netdev's sysfs directory. *BUT* if we've unbound the VF from the netdev driver, then it doesn't *have* a netdev sysfs directory. So, in order to check the correct PF netdev for online status, this patch moved the check earlier in the setup, into virNetDevSaveNetConfig(), which is called *before* unbinding the VF from its netdev driver. (Note that this implies that if you are using VFIO device assignment for the VFs of a Mellanox NIC that has the VFs programmed in single port mode, you must let the VFs be bound to their net driver and use "managed='yes'" in the device definition. To be more specific, this is only true if the VFs in single port mode are using port *2* of the PF - if the VFs are using only port 1, then the correct PF netdev will be arrived at by default/chance)) --- New in V2. src/util/virnetdev.c | 59 +++++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 8fc643c93..4ea6d5de2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1878,8 +1878,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf, goto cleanup; linkdev = vfDevOrig; + saveVlan = true; - } else if (saveVlan && virNetDevIsVirtualFunction(linkdev) == 1) { + } else if (virNetDevIsVirtualFunction(linkdev) == 1) { /* when vf is -1, linkdev might be a standard netdevice (not * SRIOV), or it might be an SRIOV VF. If it's a VF, normalize * it to PF + VFname @@ -1894,6 +1895,34 @@ virNetDevSaveNetConfig(const char *linkdev, int vf, goto cleanup; } + if (pfDevName) { + bool pfIsOnline; + + /* Assure that PF is online before trying to use it to set + * anything up for this VF. It *should* be online already, + * but if it isn't online the changes made to the VF via the + * PF won't take effect, yet there will be no error + * reported. In the case that the PF isn't online, we need to + * fail and report the error, rather than automatically + * setting it online, since setting an unconfigured interface + * online automatically turns on IPv6 autoconfig, which may + * not be what the admin expects, so we require them to + * explicitly enable the PF in the host system network config. + */ + if (virNetDevGetOnline(pfDevName, &pfIsOnline) < 0) + goto cleanup; + + if (!pfIsOnline) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to configure VF %d of PF '%s' " + "because the PF is not online. Please " + "change host network config to put the " + "PF online."), + vf, pfDevName); + goto cleanup; + } + } + if (!(configJSON = virJSONValueNewObject())) goto cleanup; @@ -1902,7 +1931,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf, * on the host) */ - if (pfDevName) { + if (pfDevName && saveVlan) { if (virAsprintf(&filePath, "%s/%s_vf%d", stateDir, pfDevName, vf) < 0) goto cleanup; @@ -2251,32 +2280,6 @@ virNetDevSetNetConfig(const char *linkdev, int vf, } } else { - bool pfIsOnline; - - /* Assure that PF is online before trying to use it to set - * anything up for this VF. It *should* be online already, - * but if it isn't online the changes made to the VF via the - * PF won't take effect, yet there will be no error - * reported. In the case that the PF isn't online, we need to - * fail and report the error, rather than automatically - * setting it online, since setting an unconfigured interface - * online automatically turns on IPv6 autoconfig, which may - * not be what the admin expects, so we require them to - * explicitly enable the PF in the host system network config. - */ - if (virNetDevGetOnline(pfDevName, &pfIsOnline) < 0) - goto cleanup; - - if (!pfIsOnline) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to configure VF %d of PF '%s' " - "because the PF is not online. Please " - "change host network config to put the " - "PF online."), - vf, pfDevName); - goto cleanup; - } - if (vlan) { if (vlan->nTags != 1 || vlan->trunk) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", -- 2.13.3