[libvirt] [PATCHv2 0/2] macvtap: don't use netlink to save/set mac for macvtap+passthrough+802.1Qbh
These two patches are the v2 of a single patch that I sent last week: https://www.redhat.com/archives/libvir-list/2015-August/msg00889.html At the time I sent that patch, I had no hardware to test it on, but was operating on a misunderstood assumption - someone in the know had told me that libvirt didn't need to save/restore the MAC address of VFs for 802.1Qbh macvtap passthrough, and I extrapolated from there that we didn't need to *set* the MAC address of the VF either. This makes sense, since that is what happens for 802.1Qbh *device* passthrough, but based on my own tests the 802.1Qbh "associate" fails unless libvirt sets the VF's MAC address prior to associate. So this patch takes a different approach from V1 - instead of completely skipping the save/set/restore of MAC address for VFs that are using 802.1Qbh, it still does it, but uses the "old" method of ioctl(SIOCGIFHWADDR) and ioctl(SIOCSIFHWADDR). I *did* have the proper hardware to test this time, and it works (both for a Cisco VMFEX card using 802.1Qbh and for an Intel 82576 card with straight macvtap passthrough). Laine Stump (2): util: make virNetDev(Replace|Restore)MacAddress public functions util: don't use netlink to save/set mac for macvtap+passthrough+802.1Qbh src/libvirt_private.syms | 2 + src/util/virnetdev.c | 172 ++++++++++++++++++++++---------------------- src/util/virnetdev.h | 10 +++ src/util/virnetdevmacvlan.c | 26 +++++-- 4 files changed, 121 insertions(+), 89 deletions(-) -- 2.1.0
These functions were made static as a part of commit cbfe38c since they were no longer called from outside virnetdev.c. We once again need to call them from another file, so this patch makes them once again public. --- src/libvirt_private.syms | 2 + src/util/virnetdev.c | 172 ++++++++++++++++++++++++----------------------- src/util/virnetdev.h | 10 +++ 3 files changed, 99 insertions(+), 85 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d57bf5b..67112ab 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1768,7 +1768,9 @@ virNetDevGetVirtualFunctions; virNetDevGetVLanID; virNetDevIsVirtualFunction; virNetDevLinkDump; +virNetDevReplaceMacAddress; virNetDevReplaceNetConfig; +virNetDevRestoreMacAddress; virNetDevRestoreNetConfig; virNetDevRxFilterFree; virNetDevRxFilterModeTypeFromString; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5fcf805..579ff3b 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -360,6 +360,92 @@ 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: @@ -1838,94 +1924,10 @@ virNetDevSysfsFile(char **pf_sysfs_device_link ATTRIBUTE_UNUSED, return -1; } + #endif /* !__linux__ */ #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX) -/** - * 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 - * - */ -static 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. - * - */ -static 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; -} - static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = { [IFLA_VF_MAC] = { .type = NLA_UNSPEC, diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index fff881c..a27504b 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -114,6 +114,16 @@ int virNetDevGetMAC(const char *ifname, virMacAddrPtr macaddr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevReplaceMacAddress(const char *linkdev, + const virMacAddr *macaddress, + const char *stateDir) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; + +int virNetDevRestoreMacAddress(const char *linkdev, + const char *stateDir) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + int virNetDevSetMTU(const char *ifname, int mtu) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -- 2.1.0
Before libvirt sets the MAC address of the physdev (the physical ethernet device) linked to a macvtap passthrough device, it always saves the previous MAC address to restore when the guest is finished (following a "leave nothing behind" policy). For a long time it accomplished the save/restore with a combination of ioctl(SIOCGIFHWADDR) and ioctl(SIOCSIFHWADDR), but in commit cbfe38c (first in libvirt 1.2.15) this was changed to use netlink RTM_GETLINK and RTM_SETLINK commands sent to the Physical Function (PF) of any device that was detected to be a Virtual Function (VF). We later found out that this caused problems with any devices using the Cisco enic driver (e.g. vmfex cards) because the enic driver hasn't implemented the function that is called to gather the information in the IFLA_VFINFO_LIST attribute of RTM_GETLINK (ndo_get_vf_config() for those keeping score), so we would never get back a useful response. In an ideal world, all drivers would implement all functions, but it turns out that in this case we can work around this omission without any bad side effects - since all macvtap passthrough <interface> definitions pointing to a physdev that uses the enic driver *must* have a <virtualport type='802.1Qbh'>, and since no other type of ethernet devices use 802.1Qbh, libvirt can change its behavior in this case to use the old-style. ioctl(SIOC[GS]IFHWADDR). That's what this patch does. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1257004 --- src/util/virnetdevmacvlan.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 3bc3d73..89985b8 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -778,9 +778,22 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, * This is especially important when using SRIOV capable cards that * emulate their switch in firmware. */ + if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) { - if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0) - return -1; + 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() + */ + if (virNetDevReplaceMacAddress(linkdev, macaddress, stateDir) < 0) + return -1; + } else { + if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0) + return -1; + } } if (tgifname) { @@ -913,8 +926,13 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, int ret = 0; int vf = -1; - if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) - ignore_value(virNetDevRestoreNetConfig(linkdev, vf, stateDir)); + 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, vf, stateDir)); + } if (ifname) { if (virNetDevVPortProfileDisassociate(ifname, -- 2.1.0
On 01.09.2015 20:32, Laine Stump wrote:
These two patches are the v2 of a single patch that I sent last week:
https://www.redhat.com/archives/libvir-list/2015-August/msg00889.html
At the time I sent that patch, I had no hardware to test it on, but was operating on a misunderstood assumption - someone in the know had told me that libvirt didn't need to save/restore the MAC address of VFs for 802.1Qbh macvtap passthrough, and I extrapolated from there that we didn't need to *set* the MAC address of the VF either. This makes sense, since that is what happens for 802.1Qbh *device* passthrough, but based on my own tests the 802.1Qbh "associate" fails unless libvirt sets the VF's MAC address prior to associate.
So this patch takes a different approach from V1 - instead of completely skipping the save/set/restore of MAC address for VFs that are using 802.1Qbh, it still does it, but uses the "old" method of ioctl(SIOCGIFHWADDR) and ioctl(SIOCSIFHWADDR).
I *did* have the proper hardware to test this time, and it works (both for a Cisco VMFEX card using 802.1Qbh and for an Intel 82576 card with straight macvtap passthrough).
Laine Stump (2): util: make virNetDev(Replace|Restore)MacAddress public functions util: don't use netlink to save/set mac for macvtap+passthrough+802.1Qbh
src/libvirt_private.syms | 2 + src/util/virnetdev.c | 172 ++++++++++++++++++++++---------------------- src/util/virnetdev.h | 10 +++ src/util/virnetdevmacvlan.c | 26 +++++-- 4 files changed, 121 insertions(+), 89 deletions(-)
ACK series Michal
participants (2)
-
Laine Stump -
Michal Privoznik