[libvirt] [PATCH v2 00/11] Fix saving/setting/restoring SR-IOV VF MAC address
I pushed the 1st 10 patches of this series. The rest of the series was ACKed as well, except patch 11, which is patch 01/11 in this repost. Michal had asked that the rewrite of the saving/restoring of MAC and vlan tag data use a more sane format for the data file (it has been just an unformatted plaintext file since the beginning, which has always bothered me, but not enough to change it.) The resulting JSON format file is *much* nicer, and will be easier to expand if we ever need to. Patches 2 - 11 were already ACKed and haven't been modified, but feel free to go over them again if you want :-) Laine Stump (11): util: new functions virNetDev(Save|Read|Set)NetConfig() util: use new virNetDev*NetConfig() functions for macvtap setup/teardown util: use new virNetDev*NetConfig() functions for hostdev setup/teardown util: replace virHostdevNetConfigReplace with ...(Save|Set)NetConfig() util: save hostdev network device config before unbinding from host driver util: after hostdev assignment, restore VF MAC address via setting admin MAC util: remove unused functions from virnetdev.c util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00 util: try *really* hard to set the MAC address of an SRIOV VF util: log all setting of MAC addresses and vlan tags util: rename virHostdevNetConfigRestore() to virHostdevRestoreNetConfig() src/libvirt_private.syms | 7 +- src/util/virhostdev.c | 147 ++++++-- src/util/virnetdev.c | 855 +++++++++++++++++++++++++++++++------------- src/util/virnetdev.h | 29 +- src/util/virnetdevmacvlan.c | 40 ++- 5 files changed, 778 insertions(+), 300 deletions(-) -- 2.9.3
These three functions are destined to replace virNetDev(Replace|Restore)NetConfig() and virNetDev(Replace|Restore)MacAddress(), which both do the save and set together as a single step. We need to separate the save, read, and set steps because there will be situations where we need to do something else in between (in particular, we will need to rebind a VF's driver after save but before set). This patch creates the new functions, but doesn't call them - that will come in a subsequent patch. Note that the new functions to read/write the file that stores the original network config now uses JSON rather than plaintext (it still recognizes the old format as well though, so it won't get confused during an upgrade). --- Change from V1: use JSON for the data file, at mprivozn's suggestion (and with his assistance writing the code :-) src/libvirt_private.syms | 3 + src/util/virnetdev.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/virnetdev.h | 22 ++ 3 files changed, 613 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e99bdb0..7e9334a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2011,6 +2011,7 @@ virNetDevIfStateTypeFromString; virNetDevIfStateTypeToString; virNetDevIsVirtualFunction; virNetDevPFGetVF; +virNetDevReadNetConfig; virNetDevReplaceMacAddress; virNetDevReplaceNetConfig; virNetDevRestoreMacAddress; @@ -2020,11 +2021,13 @@ virNetDevRxFilterFree; virNetDevRxFilterModeTypeFromString; virNetDevRxFilterModeTypeToString; virNetDevRxFilterNew; +virNetDevSaveNetConfig; virNetDevSetMAC; virNetDevSetMTU; virNetDevSetMTUFromDevice; virNetDevSetName; virNetDevSetNamespace; +virNetDevSetNetConfig; virNetDevSetOnline; virNetDevSetPromiscuous; virNetDevSetRcvAllMulti; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index b9e9cbe..c2d7a2f 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -33,6 +33,7 @@ #include "virlog.h" #include "virstring.h" #include "virutil.h" +#include "virjson.h" #include <sys/ioctl.h> #include <net/if.h> @@ -1971,7 +1972,551 @@ virNetDevRestoreNetConfig(const char *linkdev, int vf, const char *stateDir) return ret; } -#else /* defined(__linux__) && defined(HAVE_LIBNL) */ +# define VIR_NETDEV_KEYNAME_ADMIN_MAC "adminMac" +# define VIR_NETDEV_KEYNAME_VLAN_TAG "vlanTag" +# define VIR_NETDEV_KEYNAME_MAC "mac" + +/** + * virNetDevSaveNetConfig: + * @linkdev: name of the interface + * @vf: vf index if linkdev is a pf + * @stateDir: directory to store old net config + * @saveVlan: false if we shouldn't attempt to save vlan tag info + * (eg for interfaces using 802.1Qbg, since it handles + * vlan tags internally) + * + * Save current MAC address and (if linkdev itself is a VF, or if @vf + * >= 0) the "admin MAC address" and vlan tag the device described by + * @linkdev:@vf to @stateDir. (the "admin MAC address" is stored in + * the PF, and is what the VF MAC will be initialized to the next time + * its driver is reloaded (either on host or guest). + * + * File Format: + * + * The file is in json format and will contain 1 or more of the + * following values: + * + * "mac" - VF MAC address (or missing if VF has no host net driver) + * "vlanTag" - a single vlan tag id + * "adminMac" - admin MAC address (stored in the PF) + * + * For example: + * + * {"mac": "9A:11:22:33:44:55", + * "vlanTag": "42", + * "adminMac": "00:00:00:00:00:00" + * } + * + * File Name: + * + * If the device is a VF and we're allowed to save vlan tag info, the + * file will be named ${pfDevName_vf#{vf} (e.g. "enp2s0f0_vf5") and + * will contain at least "adminMac" and "vlanTag" (if the device was bound + * to a net driver on the host prior to use, it will also have "mac".. + * If the device isn't a VF, or we're not allowed to save vlan tag + * info, the file will be named ${linkdev} (e.g. "enp3s0f0") and will + * contain just linkdev's MAC address. + * + * Returns 0 on success, -1 on failure + * + */ +int +virNetDevSaveNetConfig(const char *linkdev, int vf, + const char *stateDir, + bool saveVlan) +{ + int ret = -1; + const char *pfDevName = NULL; + char *pfDevOrig = NULL; + char *vfDevOrig = NULL; + virMacAddr oldMAC; + char MACStr[VIR_MAC_STRING_BUFLEN]; + int oldVlanTag = -1; + char *filePath = NULL; + char *fileStr = NULL; + virJSONValuePtr configJSON = NULL; + + if (vf >= 0) { + /* linkdev is the PF */ + pfDevName = linkdev; + + /* linkdev should get the VF's netdev name (or NULL if none) */ + if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0) + goto cleanup; + + linkdev = vfDevOrig; + + } else if (saveVlan && 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 + */ + + if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0) + goto cleanup; + + pfDevName = pfDevOrig; + + if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0) + goto cleanup; + } + + if (!(configJSON = virJSONValueNewObject())) + goto cleanup; + + /* if there is a PF, it's now in pfDevName, and linkdev is either + * the VF's name, or NULL (if the VF isn't bound to a net driver + * on the host) + */ + + if (pfDevName) { + if (virAsprintf(&filePath, "%s/%s_vf%d", stateDir, pfDevName, vf) < 0) + goto cleanup; + + /* get admin MAC and vlan tag */ + if (virNetDevGetVfConfig(pfDevName, vf, &oldMAC, + saveVlan ? &oldVlanTag : NULL) < 0) { + goto cleanup; + } + + if (virJSONValueObjectAppendString(configJSON, + VIR_NETDEV_KEYNAME_ADMIN_MAC, + virMacAddrFormat(&oldMAC, MACStr)) < 0 || + virJSONValueObjectAppendNumberInt(configJSON, + VIR_NETDEV_KEYNAME_VLAN_TAG, + oldVlanTag) < 0) { + goto cleanup; + } + + } else { + if (virAsprintf(&filePath, "%s/%s", stateDir, linkdev) < 0) + goto cleanup; + } + + if (linkdev) { + if (virNetDevGetMAC(linkdev, &oldMAC) < 0) + goto cleanup; + + /* for interfaces with no pfDevName (i.e. not a VF, this will + * be the only value in the file. + */ + if (virJSONValueObjectAppendString(configJSON, VIR_NETDEV_KEYNAME_MAC, + virMacAddrFormat(&oldMAC, MACStr)) < 0) + goto cleanup; + } + + if (!(fileStr = virJSONValueToString(configJSON, true))) + goto cleanup; + + if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) { + virReportSystemError(errno, _("Unable to preserve mac/vlan tag " + "for device = %s, vf = %d"), linkdev, vf); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(pfDevOrig); + VIR_FREE(vfDevOrig); + VIR_FREE(filePath); + VIR_FREE(fileStr); + virJSONValueFree(configJSON); + return ret; +} + + +/** + * virNetDevReadNetConfig: + * @linkdev: name of the interface + * @vf: vf index if linkdev is a pf + * @stateDir: directory where net config is stored + * @adminMAC: returns admin MAC to store in the PF (if this is a VF) + * @MAC: returns MAC to set on device immediately + * + * Read saved MAC address and (if linkdev itself is a VF, or if @vf >= + * 0) "admin MAC address" and vlan tag of the device described by + * @linkdev:@vf from a file in @stateDir. (see virNetDevSaveNetConfig + * for details of file name and format). + * + * Returns 0 on success, -1 on failure. + * + * 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) + * + */ +int +virNetDevReadNetConfig(const char *linkdev, int vf, + const char *stateDir, + virMacAddrPtr *adminMAC, + virNetDevVlanPtr *vlan, + virMacAddrPtr *MAC) +{ + int ret = -1; + const char *pfDevName = NULL; + char *pfDevOrig = NULL; + char *vfDevOrig = NULL; + char *filePath = NULL; + char *fileStr = NULL; + virJSONValuePtr configJSON = NULL; + const char *MACStr = NULL; + const char *adminMACStr = NULL; + int vlanTag = -1; + + *adminMAC = NULL; + *vlan = NULL; + *MAC = NULL; + + if (vf >= 0) { + /* linkdev is the PF */ + pfDevName = linkdev; + + /* linkdev should get the VF's netdev name (or NULL if none) */ + if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0) + goto cleanup; + + linkdev = vfDevOrig; + + } 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 + */ + + if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0) + goto cleanup; + + pfDevName = pfDevOrig; + + if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0) + goto cleanup; + } + + /* if there is a PF, it's now in pfDevName, and linkdev is either + * the VF's name, or NULL (if the VF isn't bound to a net driver + * on the host) + */ + + if (pfDevName) { + if (virAsprintf(&filePath, "%s/%s_vf%d", stateDir, pfDevName, vf) < 0) + goto cleanup; + + 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. + */ + VIR_FREE(filePath); + pfDevName = NULL; + } + } + + if (!pfDevName) { + if (virAsprintf(&filePath, "%s/%s", stateDir, linkdev) < 0) + goto cleanup; + } + + if (virFileReadAll(filePath, 128, &fileStr) < 0) + goto cleanup; + + if (strchr("0123456789abcdefABCDEF", fileStr[0])) { + const char *vlanStr = NULL; + + /* old version of file - just two lines of text. Line 1 is the + * MAC address (or if line 2 is present, line 1 is adminMAC), + * and line 2 (if present) is the vlan tag + */ + + if ((vlanStr = strchr(fileStr, '\n'))) { + char *endptr; + + /* if there are 2 lines, the first is adminMAC */ + adminMACStr = fileStr; + vlanStr++; + + if ((virStrToLong_i(vlanStr, &endptr, 10, &vlanTag) < 0) || + (endptr && *endptr != '\n' && *endptr != 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse vlan tag '%s' from file '%s'"), + vlanStr, filePath); + goto cleanup; + } + } else { + /* if there is only one line, it is MAC */ + MACStr = fileStr; + } + } else { + /* if it doesn't start with a hex digit, it is a modern + * version of the config file - JSON format as described in + * preamble to virNetDevSaveNetConfig() + */ + if (!(configJSON = virJSONValueFromString(fileStr))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid json in net device saved " + "config file '%s': '%.60s'"), + filePath, fileStr); + goto cleanup; + } + + MACStr = virJSONValueObjectGetString(configJSON, + VIR_NETDEV_KEYNAME_MAC); + adminMACStr = virJSONValueObjectGetString(configJSON, + VIR_NETDEV_KEYNAME_ADMIN_MAC); + ignore_value(virJSONValueObjectGetNumberInt(configJSON, + VIR_NETDEV_KEYNAME_VLAN_TAG, + &vlanTag)); + + if (!(MACStr || adminMACStr)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("network device saved config file '%s' " + "has unexpected contents, missing both " + "'MAC' and 'adminMAC': '%.60s'"), + filePath, fileStr); + goto cleanup; + } + } + + if (MACStr) { + if (VIR_ALLOC(*MAC) < 0) + goto cleanup; + + if (virMacAddrParse(MACStr, *MAC) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse MAC address '%s' from file '%s'"), + MACStr, filePath); + goto cleanup; + } + } + + if (adminMACStr) { + if (VIR_ALLOC(*adminMAC) < 0) + goto cleanup; + + if (virMacAddrParse(adminMACStr, *adminMAC) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse MAC address '%s' from file '%s'"), + adminMACStr, filePath); + goto cleanup; + } + } + + if (vlanTag != -1) { + /* construct a simple virNetDevVlan object with a single tag */ + if (VIR_ALLOC(*vlan) < 0) + goto cleanup; + if (VIR_ALLOC((*vlan)->tag) < 0) + goto cleanup; + (*vlan)->nTags = 1; + (*vlan)->tag[0] = vlanTag; + } + + /* we won't need the file again */ + ignore_value(unlink(filePath)); + + ret = 0; + cleanup: + if (ret < 0) { + VIR_FREE(*adminMAC); + VIR_FREE(*MAC); + VIR_FREE(*vlan); + } + + VIR_FREE(pfDevOrig); + VIR_FREE(vfDevOrig); + VIR_FREE(filePath); + VIR_FREE(fileStr); + virJSONValueFree(configJSON); + return ret; +} + + +/** + * virNetDevSetNetConfig: + * @linkdev: name of the interface + * @vf: vf index if linkdev is a PF + * @adminMAC: new admin MAC address (will be stored in PF and + * used for next initialization of VF driver) + * @vlan: new vlan tag info (or NULL) + * @MAC: new MAC address to set on the device immediately + * @setVlan: true to enable setting vlan tag (even if @vlan is NULL, + * the interface vlan tag will be set to 0). + * + * + * Set new MAC address and (optionally) admin MAC and vlan tag of + * @linkdev VF# @vf. + * + * Returns 0 on success, -1 on failure + * + */ +int +virNetDevSetNetConfig(const char *linkdev, int vf, + const virMacAddr *adminMAC, + virNetDevVlanPtr vlan, + const virMacAddr *MAC, + bool setVlan) +{ + int ret = -1; + char MACStr[VIR_MAC_STRING_BUFLEN]; + const char *pfDevName = NULL; + char *pfDevOrig = NULL; + char *vfDevOrig = NULL; + int vlanTag = -1; + + if (vf >= 0) { + /* linkdev is the PF */ + pfDevName = linkdev; + + /* linkdev should get the VF's netdev name (or NULL if none) */ + if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0) + goto cleanup; + + linkdev = vfDevOrig; + + } 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 + */ + + if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0) + goto cleanup; + + pfDevName = pfDevOrig; + + if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0) + goto cleanup; + } + + + if (!pfDevName) { + /* if it's not SRIOV, then we can't set the admin MAC address + * or vlan tag + */ + if (adminMAC) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("admin MAC can only be set for SR-IOV VFs, but " + "%s is not a VF"), linkdev); + goto cleanup; + } + + if (vlan) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("vlan can only be set for SR-IOV VFs, but " + "%s is not a VF"), linkdev); + goto cleanup; + } + + } 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", + _("vlan trunking is not supported " + "by SR-IOV network devices")); + goto cleanup; + } + + if (!setVlan) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vlan tag set for interface %s but " + "caller requested it not be set")); + goto cleanup; + } + + vlanTag = vlan->tag[0]; + + } else if (setVlan) { + vlanTag = 0; /* assure any existing vlan tag is reset */ + } + } + + if (MAC) { + if (!linkdev) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("VF %d of PF '%s' is not bound to a net driver, " + "so its MAC address cannot be set to %s"), + vf, pfDevName, virMacAddrFormat(MAC, MACStr)); + goto cleanup; + } + + if (virNetDevSetMAC(linkdev, MAC) < 0) { + /* This may have failed due to the "administratively + * set" flag being set in the PF for this VF. For now + * we will just fail, but in the future we should + * attempt to set the VF MAC via the PF. + */ + goto cleanup; + } + if (pfDevOrig) { + /* if pfDevOrig is set, it means that the caller was + * *really* only interested in setting the MAC of the VF + * itself, *not* the admin MAC via the PF. In those cases, + * the adminMAC was only provided in case we need to set + * the VF's MAC by temporarily unbinding/rebinding the + * VF's net driver with the admin MAC set to the desired + * MAC, and then want to restore the admin MAC to its + * original setting when we're finished. We would only + * need to do that if the virNetDevSetMAC() above had + * failed; since it didn't, we don't need to set the + * adminMAC, so we are NULLing it out here to avoid that + * below. + + * (NB: since setting the admin MAC sets the + * "administratively set" flag for the VF in the PF's + * driver, which prevents any future changes to the VF's + * MAC address, we want to avoid setting the admin MAC as + * much as possible.) + */ + adminMAC = NULL; + } + } + + if (adminMAC || vlanTag >= 0) { + /* Set vlanTag and admin MAC using an RTM_SETLINK request sent to + * PFdevname+VF#, if mac != NULL this will set the "admin MAC" via + * the PF, *not* the actual VF MAC - the admin MAC only takes + * effect the next time the VF's driver is initialized (either in + * guest or host). if there is a vlanTag to set, it will take + * effect immediately though. + */ + if (virNetDevSetVfConfig(pfDevName, vf, adminMAC, vlanTag) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(pfDevOrig); + VIR_FREE(vfDevOrig); + return ret; +} + + +#else /* defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX) */ int virNetDevReplaceNetConfig(const char *linkdev ATTRIBUTE_UNUSED, @@ -1996,7 +2541,48 @@ virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED, return -1; } -#endif /* defined(__linux__) && defined(HAVE_LIBNL) */ + +int +virNetDevSaveNetConfig(const char *linkdev ATTRIBUTE_UNUSED, + int vf ATTRIBUTE_UNUSED, + const char *stateDir ATTRIBUTE_UNUSED, + bool saveVlan ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to save net device config on this platform")); + return -1; +} + + +int +virNetDevReadNetConfig(const char *linkdev ATTRIBUTE_UNUSED, + int vf ATTRIBUTE_UNUSED, + const char *stateDir ATTRIBUTE_UNUSED, + virMacAddrPtr *adminMAC ATTRIBUTE_UNUSED, + virNetDevVLanPtr *vlan ATTRIBUTE_UNUSED, + virMacAddrPtr *MAC ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to read net device config on this platform")); + return -1; +} + + +int +virNetDevSetNetConfig(const char *linkdev ATTRIBUTE_UNUSED, + int vf ATTRIBUTE_UNUSED, + const virMacAddr *adminMAC ATTRIBUTE_UNUSED, + virNetDevVlanPtr vlan ATTRIBUTE_UNUSED, + const virMacAddr *MAC ATTRIBUTE_UNUSED, + bool setVlan ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to set net device config on this platform")); + return -1; +} + + +#endif /* defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX) */ VIR_ENUM_IMPL(virNetDevIfState, VIR_NETDEV_IF_STATE_LAST, diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 01f6d8b..00f94b7 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -194,6 +194,28 @@ int virNetDevGetVirtualFunctions(const char *pfname, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; +int virNetDevSaveNetConfig(const char *linkdev, int vf, + const char *stateDir, + bool saveVlan) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; + +int +virNetDevReadNetConfig(const char *linkdev, int vf, + const char *stateDir, + virMacAddrPtr *adminMAC, + virNetDevVlanPtr *vlan, + virMacAddrPtr *MAC) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK; + +int +virNetDevSetNetConfig(const char *linkdev, int vf, + const virMacAddr *adminMAC, + virNetDevVlanPtr vlan, + const virMacAddr *MAC, + bool setVLan) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + int virNetDevReplaceNetConfig(const char *linkdev, int vf, const virMacAddr *macaddress, virNetDevVlanPtr vlan, -- 2.9.3
This patch modifies the macvtap passthrough setup to use virNetDevSaveNetConfig()+virNetDevSetConfig() instead of virNetDevReplaceNetConfig() or virNetDevReplaceMacAddress(), and the teardown to use virNetDevReadNetConfig()+virNetDevSetConfig() instead of virNetDevRestoreNetConfig() or virNetDevRestoreMacAddress(). Since the older functions only saved/restored the admin MAC and vlan tag (which is incorrect) and the new functions save/restore the VF's own MAC address and vlan tag (correct), this actually fixes a bug (which was introduced by commit cb3fe38c7, which was itself supposed to be a fix for https://bugzilla.redhat.com/1113474 ). The downside to this patch is that it causes an *apparent* regression in that bug (because there will once again be an error reported if the interface had previously been used for VFIO device assignment), but in reality, the code hasn't been working for *any* case before this current patch (at least not with any recent kernel). Anyway, that "regression" will be fixed with an upcoming patch that fixes it the *right* way. --- src/util/virnetdevmacvlan.c | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index be9dff6..7222b0f 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -1010,20 +1010,22 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, */ if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) { + bool setVlan = true; + if (virtPortProfile && virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBH) { - /* The Cisco enic driver (the only card that uses - * 802.1Qbh) doesn't support IFLA_VFINFO_LIST, which is - * required for virNetDevReplaceNetConfig(), so we must - * use this function (which uses ioctl(SIOCGIFHWADDR) - * instead or virNetDevReplaceNetConfig() + /* The Cisco enic driver (the only SRIOV-capable card that + * uses 802.1Qbh) doesn't support IFLA_VFINFO_LIST, which + * is required to get/set the vlan tag of a VF. */ - if (virNetDevReplaceMacAddress(linkdev, macaddress, stateDir) < 0) - return -1; - } else { - if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, vlan, stateDir) < 0) - return -1; + setVlan = false; } + + if (virNetDevSaveNetConfig(linkdev, -1, stateDir, setVlan) < 0) + return -1; + + if (virNetDevSetNetConfig(linkdev, -1, NULL, vlan, macaddress, setVlan) < 0) + return -1; } if (ifnameRequested) { @@ -1194,11 +1196,19 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, } if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) { - if (virtPortProfile && - virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBH) - ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir)); - else - ignore_value(virNetDevRestoreNetConfig(linkdev, -1, stateDir)); + virMacAddrPtr MAC = NULL; + virMacAddrPtr adminMAC = NULL; + virNetDevVlanPtr vlan = NULL; + + if (virNetDevReadNetConfig(linkdev, -1, stateDir, + &adminMAC, &vlan, &MAC) == 0) { + + ignore_value(virNetDevSetNetConfig(linkdev, -1, + adminMAC, vlan, MAC, !!vlan)); + VIR_FREE(MAC); + VIR_FREE(adminMAC); + virNetDevVlanFree(vlan); + } } virNetlinkEventRemoveClient(0, macaddr, NETLINK_ROUTE); -- 2.9.3
virHostdevNetConfigReplace() and virHostdevNetConfigRestore() are modified to use the new virNetDev*NetConfig() functions. Note that due to the VF's original MAC addresses being saved after it has already been un-bound from the host net driver, the actual current VF MAC address won't be saved (because it no longer exists) - only the "admin MAC" will be saved. This reflects existing behavior that will be fixed in an upcoming patch. --- src/util/virhostdev.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index faa46aa..0f0a1ee 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -446,10 +446,13 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, goto cleanup; } } else { - /* Set only mac and vlan */ - if (virNetDevReplaceNetConfig(linkdev, vf, - &hostdev->parent.data.net->mac, - vlan, stateDir) < 0) { + /* Save/Set only mac and vlan */ + + if (virNetDevSaveNetConfig(linkdev, vf, stateDir, true) < 0) + goto cleanup; + + if (virNetDevSetNetConfig(linkdev, vf, &hostdev->parent.data.net->mac, + vlan, NULL, true) < 0) { goto cleanup; } } @@ -502,9 +505,23 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, NULL, port_profile_associate); } else { - ret = virNetDevRestoreNetConfig(linkdev, vf, stateDir); - if (ret < 0 && oldStateDir != NULL) - ret = virNetDevRestoreNetConfig(linkdev, vf, oldStateDir); + 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) { + ignore_value(virNetDevSetNetConfig(linkdev, vf, + adminMAC, vlan, MAC, true)); + } + + VIR_FREE(MAC); + VIR_FREE(adminMAC); + virNetDevVlanFree(vlan); } VIR_FREE(linkdev); -- 2.9.3
These two operations will need to be separated so that saving of the original config is done before detaching the host net driver, and setting the new config is done after attaching vfio-pci. This patch splits the single function into two, but for now calls them together (to make bisecting easier if there is a regression). --- src/util/virhostdev.c | 89 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 21 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 0f0a1ee..31896e8 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -408,17 +408,30 @@ virHostdevNetConfigVirtPortProfile(const char *linkdev, int vf, } +/** + * virHostdevSaveNetConfig: + * @hostdev: config object describing a hostdev device + * @stateDir: directory to save device state into + * + * If the given hostdev device is an SRIOV network VF and *does not* + * have a <virtualport> element (ie, it isn't being configured via + * 802.11Qbh), determine its PF+VF#, and use that to save its current + * "admin" MAC address and VF tag (the ones saved in the PF + * driver). + * + * Returns 0 on success, -1 on failure. + */ static int -virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, - const unsigned char *uuid, - const char *stateDir) +virHostdevSaveNetConfig(virDomainHostdevDefPtr hostdev, + const char *stateDir) { - char *linkdev = NULL; - virNetDevVlanPtr vlan; - virNetDevVPortProfilePtr virtPort; int ret = -1; + char *linkdev = NULL; int vf = -1; - bool port_profile_associate = true; + + if (!virHostdevIsPCINetDevice(hostdev) || + virDomainNetGetActualVirtPortProfile(hostdev->parent.data.net)) + return 0; if (virHostdevIsVirtualFunction(hostdev) != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -430,6 +443,44 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, if (virHostdevNetDevice(hostdev, &linkdev, &vf) < 0) goto cleanup; + if (virNetDevSaveNetConfig(linkdev, vf, stateDir, true) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(linkdev); + return ret; +} + + +/** + * virHostdevSetNetConfig: + * @hostdev: config object describing a hostdev device + * @uuid: uuid of the domain + * + * If the given hostdev device is an SRIOV network VF, determine its + * PF+VF#, and use that to set the "admin" MAC address and VF tag (the + * ones saved in the PF driver).xs + * + * Returns 0 on success, -1 on failure. + */ +static int +virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev, + const unsigned char *uuid) +{ + char *linkdev = NULL; + virNetDevVlanPtr vlan; + virNetDevVPortProfilePtr virtPort; + int ret = -1; + int vf = -1; + bool port_profile_associate = true; + + if (!virHostdevIsPCINetDevice(hostdev)) + return 0; + + if (virHostdevNetDevice(hostdev, &linkdev, &vf) < 0) + goto cleanup; + vlan = virDomainNetGetActualVlan(hostdev->parent.data.net); virtPort = virDomainNetGetActualVirtPortProfile(hostdev->parent.data.net); if (virtPort) { @@ -446,11 +497,6 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, goto cleanup; } } else { - /* Save/Set only mac and vlan */ - - if (virNetDevSaveNetConfig(linkdev, vf, stateDir, true) < 0) - goto cleanup; - if (virNetDevSetNetConfig(linkdev, vf, &hostdev->parent.data.net->mac, vlan, NULL, true) < 0) { goto cleanup; @@ -463,6 +509,7 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, return ret; } + /* @oldStateDir: * For upgrade purpose: * To an existing VM on QEMU, the hostdev netconfig file is originally stored @@ -689,16 +736,16 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, } /* Step 4: For SRIOV network devices, Now that we have detached the - * the network device, set the netdev config */ + * the network device, set the new netdev config */ for (i = 0; i < nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = hostdevs[i]; - if (!virHostdevIsPCINetDevice(hostdev)) - continue; - if (virHostdevNetConfigReplace(hostdev, uuid, - mgr->stateDir) < 0) { - goto resetvfnetconfig; - } - last_processed_hostdev_vf = i; + + if (virHostdevSaveNetConfig(hostdevs[i], mgr->stateDir) < 0) + goto resetvfnetconfig; + + if (virHostdevSetNetConfig(hostdevs[i], uuid) < 0) + goto resetvfnetconfig; + + last_processed_hostdev_vf = i; } /* Step 5: Move devices from the inactive list to the active list */ -- 2.9.3
In order to properly restore the original state of an SRIOV VF when we're finished with it, we need to save the MAC address of the VF itself (not just the admin MAC address for the VF that is stored in the PF). But that can only be done when the VF is still bound to the host's netdev driver, and we have always done the saving of device config after the VF is already bound to vfio-pci. This patch prepares us for adding a save of the VF's MAC by calling the function that saves netconfig earlier in the device preparation, before we've unbound it from the host netdev driver. --- src/util/virhostdev.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 31896e8..ad9ba65 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -649,6 +649,14 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, } } + /* Step 1.5: For non-802.11Qbh SRIOV network devices, save the + * current device config + */ + for (i = 0; i < nhostdevs; i++) { + if (virHostdevSaveNetConfig(hostdevs[i], mgr->stateDir) < 0) + goto cleanup; + } + /* Step 2: detach managed devices and make sure unmanaged devices * have already been taken care of */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { @@ -739,9 +747,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, * the network device, set the new netdev config */ for (i = 0; i < nhostdevs; i++) { - if (virHostdevSaveNetConfig(hostdevs[i], mgr->stateDir) < 0) - goto resetvfnetconfig; - if (virHostdevSetNetConfig(hostdevs[i], uuid) < 0) goto resetvfnetconfig; -- 2.9.3
It takes longer to explain this than to fix it... In the past we weren't able to save the VF's own MAC address *at all* when using it for hostdev assignment, because we had already unbound the VF from the host net driver prior to saving its config. With the previous patch, that problem has been solved, so we now have the VF's MAC address saved and can move on to the *next* problem, which is twofold: 1) during teardown we restore the config before we've re-bound, so the VF doesn't have a net driver, and thus we can't set its MAC address directly. 2) even if we delay restoring the config until the VF is bound to a net driver, the request to set its MAC address would fail, since (during device setup) we had set the "admin MAC" for the VF via an RTM_SETLINK to the PF - once you've set the admin MAC for a VF, the VF driver (either on host or on guest) is not allowed to change the VF's MAC address "forever" (well, until you reload the PF driver, but that requires destroying and recreating every single VF, which isn't something you can require). The solution is to keep the restoration of config at the same place, but to set the *admin MAC* to the address you want the VF to have - when the VF net driver is later initialized (as a part of re-binding to the VF net driver) its MAC will be initialized to the current value of the admin MAC. --- src/util/virhostdev.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index ad9ba65..cf57354 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -562,6 +562,30 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, &adminMAC, &vlan, &MAC); 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; + } + ignore_value(virNetDevSetNetConfig(linkdev, vf, adminMAC, vlan, MAC, true)); } -- 2.9.3
The global functions virNetDevReplaceMacAddress(), virNetDevReplaceNetConfig(), virNetDevRestoreMacAddress(), and virNetDevRestoreNetConfig() are no longer used, as their functionality has been replaced by virNetDev(Save|Read|Set)NetConfig(). The static functions virNetDevReplaceVfConfig() and virNetDevRestoreVfConfig() were only used by the above-named global functions that were removed. --- src/libvirt_private.syms | 4 - src/util/virnetdev.c | 332 ----------------------------------------------- src/util/virnetdev.h | 9 -- 3 files changed, 345 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7e9334a..69f38c7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2012,10 +2012,6 @@ virNetDevIfStateTypeToString; virNetDevIsVirtualFunction; virNetDevPFGetVF; virNetDevReadNetConfig; -virNetDevReplaceMacAddress; -virNetDevReplaceNetConfig; -virNetDevRestoreMacAddress; -virNetDevRestoreNetConfig; virNetDevRunEthernetScript; virNetDevRxFilterFree; virNetDevRxFilterModeTypeFromString; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c2d7a2f..6dafc87 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -391,92 +391,6 @@ int virNetDevGetMAC(const char *ifname, #endif -/** - * virNetDevReplaceMacAddress: - * @macaddress: new MAC address for interface - * @linkdev: name of interface - * @stateDir: directory to store old MAC address - * - * Returns 0 on success, -1 on failure - * - */ -int -virNetDevReplaceMacAddress(const char *linkdev, - const virMacAddr *macaddress, - const char *stateDir) -{ - virMacAddr oldmac; - char *path = NULL; - char macstr[VIR_MAC_STRING_BUFLEN]; - int ret = -1; - - if (virNetDevGetMAC(linkdev, &oldmac) < 0) - return -1; - - if (virAsprintf(&path, "%s/%s", - stateDir, - linkdev) < 0) - return -1; - virMacAddrFormat(&oldmac, macstr); - if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) { - virReportSystemError(errno, _("Unable to preserve mac for %s"), - linkdev); - goto cleanup; - } - - if (virNetDevSetMAC(linkdev, macaddress) < 0) - goto cleanup; - - ret = 0; - cleanup: - VIR_FREE(path); - return ret; -} - -/** - * virNetDevRestoreMacAddress: - * @linkdev: name of interface - * @stateDir: directory containing old MAC address - * - * Returns 0 on success, -errno on failure. - * - */ -int -virNetDevRestoreMacAddress(const char *linkdev, - const char *stateDir) -{ - int rc = -1; - char *oldmacname = NULL; - char *macstr = NULL; - char *path = NULL; - virMacAddr oldmac; - - if (virAsprintf(&path, "%s/%s", - stateDir, - linkdev) < 0) - return -1; - - if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0) - goto cleanup; - - if (virMacAddrParse(macstr, &oldmac) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse MAC address from '%s'"), - oldmacname); - goto cleanup; - } - - /*reset mac and remove file-ignore results*/ - rc = virNetDevSetMAC(linkdev, &oldmac); - ignore_value(unlink(path)); - - cleanup: - VIR_FREE(macstr); - VIR_FREE(path); - return rc; -} - - #if defined(SIOCGIFMTU) && defined(HAVE_STRUCT_IFREQ) /** * virNetDevGetMTU: @@ -1748,229 +1662,6 @@ virNetDevGetVfConfig(const char *ifname, int vf, virMacAddrPtr mac, return rc; } -static int -virNetDevReplaceVfConfig(const char *pflinkdev, int vf, - const virMacAddr *macaddress, - int vlanid, - const char *stateDir) -{ - int ret = -1; - virMacAddr oldmac; - int oldvlanid = -1; - char *path = NULL; - char macstr[VIR_MAC_STRING_BUFLEN]; - char *fileData = NULL; - bool pfIsOnline; - - /* Assure that PF is online prior to twiddling with the VF. It - * *should* be, but if the PF 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 it isn't online, fail and report the - * error, since setting an unconfigured interface online - * automatically turns on IPv6 autoconfig, which may not be what - * the admin expects, so we want them to explicitly enable the PF - * in the host system network config. - */ - if (virNetDevGetOnline(pflinkdev, &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, pflinkdev); - goto cleanup; - } - - if (virNetDevGetVfConfig(pflinkdev, vf, &oldmac, &oldvlanid) < 0) - goto cleanup; - - if (virAsprintf(&path, "%s/%s_vf%d", - stateDir, pflinkdev, vf) < 0) - goto cleanup; - - if (virAsprintf(&fileData, "%s\n%d\n", - virMacAddrFormat(&oldmac, macstr), oldvlanid) < 0) - goto cleanup; - if (virFileWriteStr(path, fileData, O_CREAT|O_TRUNC|O_WRONLY) < 0) { - virReportSystemError(errno, _("Unable to preserve mac/vlan tag " - "for pf = %s, vf = %d"), pflinkdev, vf); - goto cleanup; - } - - ret = virNetDevSetVfConfig(pflinkdev, vf, macaddress, vlanid); - - cleanup: - VIR_FREE(path); - VIR_FREE(fileData); - return ret; -} - -static int -virNetDevRestoreVfConfig(const char *pflinkdev, - int vf, const char *vflinkdev, - const char *stateDir) -{ - int rc = -1; - char *path = NULL; - char *fileData = NULL; - char *vlan = NULL; - virMacAddr oldmac; - int vlanid = -1; - - if (virAsprintf(&path, "%s/%s_vf%d", - stateDir, pflinkdev, vf) < 0) - return rc; - - if (vflinkdev && !virFileExists(path)) { - /* this VF's config may have been stored with - * virNetDevReplaceMacAddress while running an older version - * of libvirt. If so, the ${pf}_vf${id} file won't exist. In - * that case, try to restore using the older method with the - * VF's name directly. - */ - rc = virNetDevRestoreMacAddress(vflinkdev, stateDir); - goto cleanup; - } - - if (virFileReadAll(path, 128, &fileData) < 0) - goto cleanup; - - if ((vlan = strchr(fileData, '\n'))) { - char *endptr; - - *vlan++ = 0; /* NULL terminate the mac address */ - if (*vlan) { - if ((virStrToLong_i(vlan, &endptr, 10, &vlanid) < 0) || - (endptr && *endptr != '\n' && *endptr != 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse vlan tag from '%s'"), - vlan); - goto cleanup; - } - } - } - - if (virMacAddrParse(fileData, &oldmac) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse MAC address from '%s'"), - fileData); - goto cleanup; - } - - /*reset mac and remove file-ignore results*/ - rc = virNetDevSetVfConfig(pflinkdev, vf, &oldmac, vlanid); - ignore_value(unlink(path)); - - cleanup: - VIR_FREE(path); - VIR_FREE(fileData); - - return rc; -} - -/** - * virNetDevReplaceNetConfig: - * @linkdev: name of the interface - * @vf: vf index if linkdev is a pf - * @macaddress: new MAC address for interface - * @vlanid: new vlanid - * @stateDir: directory to store old net config - * - * Returns 0 on success, -1 on failure - * - */ -int -virNetDevReplaceNetConfig(const char *linkdev, int vf, - const virMacAddr *macaddress, - virNetDevVlanPtr vlan, - const char *stateDir) -{ - int ret = -1; - char *pfdevname = NULL; - - if (vf == -1 && virNetDevIsVirtualFunction(linkdev) == 1) { - /* If this really *is* a VF and the caller just didn't know - * it, we should set the MAC address via PF+vf# instead of - * setting directly via VF, because the latter will be - * rejected any time after the former has been done. - */ - if (virNetDevGetPhysicalFunction(linkdev, &pfdevname) < 0) - goto cleanup; - if (virNetDevGetVirtualFunctionIndex(pfdevname, linkdev, &vf) < 0) - goto cleanup; - linkdev = pfdevname; - } - - if (vf == -1) { - if (vlan) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("vlan can only be set for SR-IOV VFs, but " - "%s is not a VF"), linkdev); - goto cleanup; - } - ret = virNetDevReplaceMacAddress(linkdev, macaddress, stateDir); - } else { - int vlanid = 0; /* assure any current vlan tag is reset */ - - if (vlan) { - if (vlan->nTags != 1 || vlan->trunk) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("vlan trunking is not supported " - "by SR-IOV network devices")); - goto cleanup; - } - vlanid = vlan->tag[0]; - } - ret = virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid, - stateDir); - } - - cleanup: - VIR_FREE(pfdevname); - return ret; -} - -/** - * virNetDevRestoreNetConfig: - * @linkdev: name of the interface - * @vf: vf index if linkdev is a pf - * @stateDir: directory containing old net config - * - * Returns 0 on success, -errno on failure. - * - */ -int -virNetDevRestoreNetConfig(const char *linkdev, int vf, const char *stateDir) -{ - int ret = -1; - char *pfdevname = NULL; - const char *vfdevname = NULL; - - if (vf == -1 && virNetDevIsVirtualFunction(linkdev) == 1) { - /* If this really *is* a VF and the caller just didn't know - * it, we should set the MAC address via PF+vf# instead of - * setting directly via VF, because the latter will be - * rejected any time after the former has been done. - */ - if (virNetDevGetPhysicalFunction(linkdev, &pfdevname) < 0) - goto cleanup; - if (virNetDevGetVirtualFunctionIndex(pfdevname, linkdev, &vf) < 0) - goto cleanup; - vfdevname = linkdev; - linkdev = pfdevname; - } - - if (vf == -1) - ret = virNetDevRestoreMacAddress(linkdev, stateDir); - else - ret = virNetDevRestoreVfConfig(linkdev, vf, vfdevname, stateDir); - - cleanup: - VIR_FREE(pfdevname); - return ret; -} # define VIR_NETDEV_KEYNAME_ADMIN_MAC "adminMac" # define VIR_NETDEV_KEYNAME_VLAN_TAG "vlanTag" @@ -2518,29 +2209,6 @@ virNetDevSetNetConfig(const char *linkdev, int vf, #else /* defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX) */ -int -virNetDevReplaceNetConfig(const char *linkdev ATTRIBUTE_UNUSED, - int vf ATTRIBUTE_UNUSED, - const virMacAddr *macaddress ATTRIBUTE_UNUSED, - virNetDevVlanPtr vlan ATTRIBUTE_UNUSED, - const char *stateDir ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENOSYS, "%s", - _("Unable to replace net config on this platform")); - return -1; - -} - -int -virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED, - int vf ATTRIBUTE_UNUSED, - const char *stateDir ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENOSYS, "%s", - _("Unable to restore net config on this platform")); - return -1; -} - int virNetDevSaveNetConfig(const char *linkdev ATTRIBUTE_UNUSED, diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 00f94b7..437a776 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -216,15 +216,6 @@ virNetDevSetNetConfig(const char *linkdev, int vf, bool setVLan) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int virNetDevReplaceNetConfig(const char *linkdev, int vf, - const virMacAddr *macaddress, - virNetDevVlanPtr vlan, - const char *stateDir) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); - -int virNetDevRestoreNetConfig(const char *linkdev, int vf, const char *stateDir) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); - int virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, int *vf) ATTRIBUTE_NONNULL(1); -- 2.9.3
Some PF drivers allow setting the admin MAC (that is the MAC address that the VF will be initialized to the next time the VF's driver is loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers initialize the admin MACs to all 0, but don't allow setting it to that very same value. It has been an uphill battle convincing the driver people that it's reasonable to expect The argument that's used is that an all 0 device MAC address on a device is invalid; however, from an outsider's point of view, when the admin MAC is set to 0 at the time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a random non-0 value. But that's beside the point - even if I could convince one or two SRIOV driver maintainers to permit setting the admin MAC to 0, there are still several other drivers. So rather than fighting that losing battle, this patch checks for a failure to set the admin MAC due to an all 0 value, and retries it with 02:00:00:00:00:00. That won't result in a random value being set in the VF MAC at next VF driver init, but that's okay, because we always want to set a specific value anyway. Rather, the "almost 0" setting makes it easy to visually detect from the output of "ip link show" which VFs are currently in use and which are free. --- src/util/virnetdev.c | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6dafc87..eb2c679 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1460,6 +1460,14 @@ virNetDevSysfsFile(char **pf_sysfs_device_link ATTRIBUTE_UNUSED, #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX) +static virMacAddr zeroMAC = { .addr = { 0, 0, 0, 0, 0, 0 } }; + +/* if a net driver doesn't allow setting MAC to all 0, try setting + * to this (the only bit that is set is the "locally administered" bit") + */ +static virMacAddr altZeroMAC = { .addr = { 2, 0, 0, 0, 0, 0 } }; + + static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = { [IFLA_VF_MAC] = { .type = NLA_UNSPEC, .maxlen = sizeof(struct ifla_vf_mac) }, @@ -1470,7 +1478,8 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = { static int virNetDevSetVfConfig(const char *ifname, int vf, - const virMacAddr *macaddr, int vlanid) + const virMacAddr *macaddr, int vlanid, + bool *allowRetry) { int rc = -1; struct nlmsghdr *resp = NULL; @@ -1547,7 +1556,15 @@ virNetDevSetVfConfig(const char *ifname, int vf, if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) goto malformed_resp; - if (err->error) { + /* if allowRetry is true and the error was EINVAL, then + * silently return a failure so the caller can retry with a + * different MAC address + */ + if (err->error == -EINVAL && *allowRetry && + macaddr && !virMacAddrCmp(macaddr, &zeroMAC)) { + goto cleanup; + } else if (err->error) { + /* other errors are permanent */ char macstr[VIR_MAC_STRING_BUFLEN]; virReportSystemError(-err->error, @@ -1559,6 +1576,7 @@ virNetDevSetVfConfig(const char *ifname, int vf, vlanid, ifname ? ifname : "(unspecified)", vf); + *allowRetry = false; /* no use retrying */ goto cleanup; } break; @@ -2195,8 +2213,24 @@ virNetDevSetNetConfig(const char *linkdev, int vf, * guest or host). if there is a vlanTag to set, it will take * effect immediately though. */ - if (virNetDevSetVfConfig(pfDevName, vf, adminMAC, vlanTag) < 0) - goto cleanup; + bool allowRetry = true; + + if (virNetDevSetVfConfig(pfDevName, vf, + adminMAC, vlanTag, &allowRetry) < 0) { + /* allowRetry will still be true if the failure was due to + * trying to set the MAC address to all 0. In that case, + * we can retry with "altZeroMAC", which is just an all-0 MAC + * with the "locally administered" bit set. + */ + if (!allowRetry) + goto cleanup; + + allowRetry = false; + if (virNetDevSetVfConfig(pfDevName, vf, + &altZeroMAC, vlanTag, &allowRetry) < 0) { + goto cleanup; + } + } } ret = 0; -- 2.9.3
If an SRIOV VF has previously been used for VFIO device assignment, the "admin MAC" that is stored in the PF driver's table of VF info will have been set to the MAC address that the virtual machine wanted the device to have. Setting the admin MAC for a VF also sets a flag in the PF that is loosely called the "administratively set" flag. Once that flag is set, it is no longer possible for the net driver of the VF (either on the host or in a virtual machine) to directly set the VF's MAC again; this flag isn't reset until the *PF* driver is restarted, and that requires taking *all* VFs offline, so it's not really feasible to do. If the same SRIOV VF is later used for macvtap passthrough mode, the VF's MAC address must be set, but normally we don't unbind the VF from its host net driver (since we actually need the host net driver in this case). Since setting the VF MAC directly will fail, in the past "we" ("I") had tried to fix the problem by simply setting the admin MAC (via the PF) instead. This *appeared* to work (and might have at one time, due to promiscuous mode being turned on somewhere or something), but it currently creates a non-working interface because only the value for admin MAC is set to the desired value, *not* the actual MAC that the VF is using. Earlier patches in this series reverted that behavior, so that we once again set the MAC of the VF itself for macvtap passthrough operation, not the admin MAC. But that brings back the original bug - if the interface has been used for VFIO device assignment, you can no longer use it for macvtap passthrough. This patch solves that problem by noticing when virNetDevSetMAC() fails for a VF, and in that case it sets the desired MAC to the admin MAC via the PF, then "bounces" the VF driver (by unbinding and the immediately rebinding it to the VF). This causes the VF's MAC to be reinitialized from the admin MAC, and everybody is happy (until the *next* time someone wants to set the VF's MAC address, since the "administratively set" bit is still turned on). --- src/util/virnetdev.c | 102 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 83 insertions(+), 19 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index eb2c679..0169e1c 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1123,6 +1123,32 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname, return 0; } + +static virPCIDevicePtr +virNetDevGetPCIDevice(const char *devName) +{ + char *vfSysfsDevicePath = NULL; + virPCIDeviceAddressPtr vfPCIAddr = NULL; + virPCIDevicePtr vfPCIDevice = NULL; + + if (virNetDevSysfsFile(&vfSysfsDevicePath, devName, "device") < 0) + goto cleanup; + + vfPCIAddr = virPCIGetDeviceAddressFromSysfsLink(vfSysfsDevicePath); + if (!vfPCIAddr) + goto cleanup; + + vfPCIDevice = virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus, + vfPCIAddr->slot, vfPCIAddr->function); + + cleanup: + VIR_FREE(vfSysfsDevicePath); + VIR_FREE(vfPCIAddr); + + return vfPCIDevice; +} + + /** * virNetDevGetVirtualFunctions: * @@ -2070,6 +2096,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf, char *pfDevOrig = NULL; char *vfDevOrig = NULL; int vlanTag = -1; + virPCIDevicePtr vfPCIDevice = NULL; if (vf >= 0) { /* linkdev is the PF */ @@ -2165,6 +2192,8 @@ virNetDevSetNetConfig(const char *linkdev, int vf, } if (MAC) { + int setMACrc; + if (!linkdev) { virReportError(VIR_ERR_INTERNAL_ERROR, _("VF %d of PF '%s' is not bound to a net driver, " @@ -2173,27 +2202,61 @@ virNetDevSetNetConfig(const char *linkdev, int vf, goto cleanup; } - if (virNetDevSetMAC(linkdev, MAC) < 0) { - /* This may have failed due to the "administratively - * set" flag being set in the PF for this VF. For now - * we will just fail, but in the future we should - * attempt to set the VF MAC via the PF. + setMACrc = virNetDevSetMACInternal(linkdev, MAC, !!pfDevOrig); + if (setMACrc < 0) { + bool allowRetry = false; + int retries = 100; + + /* if pfDevOrig == NULL, this isn't a VF, so we've failed */ + if (!pfDevOrig || errno != EADDRNOTAVAIL) + goto cleanup; + + /* Otherwise this is a VF, and virNetDevSetMAC failed with + * EADDRNOTAVAIL, which could be due to the + * "administratively set" flag being set in the PF for + * this VF. When this happens, we can attempt to use an + * alternate method to set the VF MAC: first set it into + * the admin MAC for this VF in the PF, then unbind/rebind + * the VF from its net driver. This causes the VF's MAC to + * be initialized to whatever was stored in the admin MAC. */ - goto cleanup; + + if (virNetDevSetVfConfig(pfDevName, vf, + MAC, vlanTag, &allowRetry) < 0) { + goto cleanup; + } + + /* admin MAC is set, now we need to construct a virPCIDevice + * object so we can call virPCIDeviceRebind() + */ + if (!(vfPCIDevice = virNetDevGetPCIDevice(linkdev))) + goto cleanup; + + /* Rebind the device. This should set the proper MAC address */ + if (virPCIDeviceRebind(vfPCIDevice) < 0) + goto cleanup; + + /* Wait until virNetDevGetIndex for the VF netdev returns success. + * This indicates that the device is ready to be used. If we don't + * wait, then upcoming operations on the VF may fail. + */ + while (retries-- > 0 && !virNetDevExists(linkdev)) + usleep(1000); } - if (pfDevOrig) { - /* if pfDevOrig is set, it means that the caller was - * *really* only interested in setting the MAC of the VF - * itself, *not* the admin MAC via the PF. In those cases, - * the adminMAC was only provided in case we need to set - * the VF's MAC by temporarily unbinding/rebinding the - * VF's net driver with the admin MAC set to the desired - * MAC, and then want to restore the admin MAC to its - * original setting when we're finished. We would only - * need to do that if the virNetDevSetMAC() above had - * failed; since it didn't, we don't need to set the - * adminMAC, so we are NULLing it out here to avoid that - * below. + + if (pfDevOrig && setMACrc == 0) { + /* if pfDevOrig is set, it that the caller was *really* + * only interested in setting the MAC of the VF itself, + * *not* the admin MAC via the PF. In those cases, the + * adminMAC was only provided in case we need to set the + * VF's MAC by temporarily unbinding/rebinding the VF's + * net driver with the admin MAC set to the desired MAC, + * and then want to restore the admin MAC to its original + * setting when we're finished. We would only need to do + * that if the virNetDevSetMAC() above had failed; since + * setMACrc == 0, we know it didn't fail and we don't need + * to set the adminMAC, so we are NULLing it out here to + * avoid that below. * (NB: since setting the admin MAC sets the * "administratively set" flag for the VF in the PF's @@ -2237,6 +2300,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf, cleanup: VIR_FREE(pfDevOrig); VIR_FREE(vfDevOrig); + virPCIDeviceFree(vfPCIDevice); return ret; } -- 2.9.3
Having this information available will make it easier to determine the culprit when MAC or vlan tag appear to not be set, eg.: https://bugzilla.redhat.com/1364073 (This patch doesn't fix that bug, just makes it easier to diagnose) --- src/util/virnetdev.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 0169e1c..31eca2d 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -241,6 +241,7 @@ virNetDevSetMACInternal(const char *ifname, int fd = -1; int ret = -1; struct ifreq ifr; + char macstr[VIR_MAC_STRING_BUFLEN]; if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -256,7 +257,6 @@ virNetDevSetMACInternal(const char *ifname, virMacAddrGetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data); if (ioctl(fd, SIOCSIFHWADDR, &ifr) < 0) { - char macstr[VIR_MAC_STRING_BUFLEN]; if (quiet && errno == EADDRNOTAVAIL) goto cleanup; @@ -270,6 +270,10 @@ virNetDevSetMACInternal(const char *ifname, ret = 0; cleanup: + VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - %s", + ifname, virMacAddrFormat(macaddr, macstr), + ret < 0 ? "Fail" : "Success"); + VIR_FORCE_CLOSE(fd); return ret; } @@ -312,6 +316,9 @@ virNetDevSetMACInternal(const char *ifname, ret = 0; cleanup: + VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - %s", ifname, mac + 1), + ret < 0 ? "Fail" : "Success"); + VIR_FORCE_CLOSE(s); return ret; @@ -1508,6 +1515,7 @@ virNetDevSetVfConfig(const char *ifname, int vf, bool *allowRetry) { int rc = -1; + char macstr[VIR_MAC_STRING_BUFLEN]; struct nlmsghdr *resp = NULL; struct nlmsgerr *err; unsigned int recvbuflen = 0; @@ -1591,8 +1599,6 @@ virNetDevSetVfConfig(const char *ifname, int vf, goto cleanup; } else if (err->error) { /* other errors are permanent */ - char macstr[VIR_MAC_STRING_BUFLEN]; - virReportSystemError(-err->error, _("Cannot set interface MAC/vlanid to %s/%d " "for ifname %s vf %d"), @@ -1616,6 +1622,11 @@ virNetDevSetVfConfig(const char *ifname, int vf, rc = 0; cleanup: + VIR_DEBUG("RTM_SETLINK %s vf %d MAC=%s vlanid=%d - %s", + ifname, vf, + macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)", + vlanid, rc < 0 ? "Fail" : "Success"); + nlmsg_free(nl_msg); VIR_FREE(resp); return rc; -- 2.9.3
--- src/util/virhostdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index cf57354..468f079 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -518,7 +518,7 @@ virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev, * case, try to find in the old state dir. */ static int -virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, +virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, const char *stateDir, const char *oldStateDir) { @@ -868,7 +868,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, resetvfnetconfig: if (last_processed_hostdev_vf >= 0) { for (i = 0; i <= last_processed_hostdev_vf; i++) - virHostdevNetConfigRestore(hostdevs[i], mgr->stateDir, NULL); + virHostdevRestoreNetConfig(hostdevs[i], mgr->stateDir, NULL); } reattachdevs: @@ -929,7 +929,7 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, } /* @oldStateDir: - * For upgrade purpose: see virHostdevNetConfigRestore + * For upgrade purpose: see virHostdevRestoreNetConfig */ void virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, @@ -1030,7 +1030,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, if (actual) { VIR_DEBUG("Restoring network configuration of PCI device %s", virPCIDeviceGetName(actual)); - virHostdevNetConfigRestore(hostdev, mgr->stateDir, + virHostdevRestoreNetConfig(hostdev, mgr->stateDir, oldStateDir); } } -- 2.9.3
On 24.03.2017 06:02, Laine Stump wrote:
I pushed the 1st 10 patches of this series. The rest of the series was ACKed as well, except patch 11, which is patch 01/11 in this repost.
Michal had asked that the rewrite of the saving/restoring of MAC and vlan tag data use a more sane format for the data file (it has been just an unformatted plaintext file since the beginning, which has always bothered me, but not enough to change it.) The resulting JSON format file is *much* nicer, and will be easier to expand if we ever need to.
Patches 2 - 11 were already ACKed and haven't been modified, but feel free to go over them again if you want :-)
Laine Stump (11): util: new functions virNetDev(Save|Read|Set)NetConfig() util: use new virNetDev*NetConfig() functions for macvtap setup/teardown util: use new virNetDev*NetConfig() functions for hostdev setup/teardown util: replace virHostdevNetConfigReplace with ...(Save|Set)NetConfig() util: save hostdev network device config before unbinding from host driver util: after hostdev assignment, restore VF MAC address via setting admin MAC util: remove unused functions from virnetdev.c util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00 util: try *really* hard to set the MAC address of an SRIOV VF util: log all setting of MAC addresses and vlan tags util: rename virHostdevNetConfigRestore() to virHostdevRestoreNetConfig()
src/libvirt_private.syms | 7 +- src/util/virhostdev.c | 147 ++++++-- src/util/virnetdev.c | 855 +++++++++++++++++++++++++++++++------------- src/util/virnetdev.h | 29 +- src/util/virnetdevmacvlan.c | 40 ++- 5 files changed, 778 insertions(+), 300 deletions(-)
ACK series Michal
participants (2)
-
Laine Stump -
Michal Privoznik