From d336362cea31d3f2aa2f884b5886919768dc542c Mon Sep 17 00:00:00 2001 From: Laine Stump Date: Fri, 10 Apr 2015 12:19:49 -0400 Subject: [PATCH] util: set MAC address for VF via netlink message to PF+VF# when possible Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1113474 When we set the MAC address of a network device as a part of setting up macvtap "passthrough" mode (where the domain has an emulated netdev connected to a host macvtap device that has exclusive use of the physical device, and sets the device MAC address to match its own, i.e. " "), we use ioctl(SIOCSIFHWADDR) giving it the name of that device. This is true even if it is an SRIOV Virtual Function (VF). But, when we are setting the MAC address / vlan ID of a VF in preparation for "hostdev network" passthrough (this is where we set the MAC address and vlan id of the VF after detaching the host net driver and before assigning the device to the domain with PCI passthrough, i.e. "", we do the setting via a netlink RTM_SETLINK message for that VF's Physical Function (PF), telling it the VF# we want to change. This sets an "administratively changed MAC" flag for that VF in the PF's driver, and from that point on (until the PF driver is reloaded, *not* merely the VF driver) that VF's MAC address can't be changed using ioctl(SIOCSIFHWADDR) - the only way to change it is via the PF with RTM_SETLINK. This means that if a VF is used for hostdev passthrough, it will have the admin flag set, and future attempts to use that VF for macvtap passthrough will fail. The solution to this problem is to check if the device being used for macvtap passthrough is actually a VF; if so, we use the netlink RTM_SETLINK message to the PF to set the VF's mac address instead of ioctl(SIOCSIFHWADDR) directly to the VF; if not, behavior does not change from previously. There are three pieces to making this work: 1) virNetDevMacVLan(Create|Delete)WithVPortProfile() now call virNetDev(Replace|Restore)NetConfig() rather than virNetDev(Replace|Restore)MacAddress() (simply passing -1 for VF# and vlanid). 2) virNetDev(Replace|Restore)NetConfig() check to see if the device is a VF. If so, they find the PF's name and VF#, allowing them to call virNetDev(Replace|Restore)VfConfig(). 3) To prevent mixups when detaching a macvtap passthrough device that had been attached while running an older version of libvirt, virNetDevRestoreVfConfig() is potentially given the preserved name of the VF, and if the proper statefile for a VF can't be found in the stateDir (${stateDir}/${pfname}_vf${vfid}), virNetDevRestoreMacAddress() is called instead (which will look in the file named ${stateDir}/${vfname}). This problem has existed in every version of libvirt that has both macvtap passthrough and interface type='hostdev'. Fortunately people seem to use one or the other though, so it hasn't caused any real world problem reports. (cherry picked from commit cb3fe38c74bd1fb4ef76c64c045cf48467a9d259) --- src/libvirt_private.syms | 2 -- src/util/virnetdev.c | 70 ++++++++++++++++++++++++++++++++----- src/util/virnetdev.h | 11 ------ src/util/virnetdevmacvlan.c | 4 +-- 4 files changed, 64 insertions(+), 23 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 933891fb80..8653727f1c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1685,9 +1685,7 @@ virNetDevGetVirtualFunctions; virNetDevGetVLanID; virNetDevIsVirtualFunction; virNetDevLinkDump; -virNetDevReplaceMacAddress; virNetDevReplaceNetConfig; -virNetDevRestoreMacAddress; virNetDevRestoreNetConfig; virNetDevRxFilterFree; virNetDevRxFilterModeTypeFromString; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2a0eb43b79..309fbb8982 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -351,7 +351,7 @@ int virNetDevGetMAC(const char *ifname, * Returns 0 on success, -1 on failure * */ -int +static int virNetDevReplaceMacAddress(const char *linkdev, const virMacAddr *macaddress, const char *stateDir) @@ -393,7 +393,7 @@ virNetDevReplaceMacAddress(const char *linkdev, * Returns 0 on success, -errno on failure. * */ -int +static int virNetDevRestoreMacAddress(const char *linkdev, const char *stateDir) { @@ -2143,7 +2143,8 @@ virNetDevReplaceVfConfig(const char *pflinkdev, int vf, } static int -virNetDevRestoreVfConfig(const char *pflinkdev, int vf, +virNetDevRestoreVfConfig(const char *pflinkdev, + int vf, const char *vflinkdev, const char *stateDir) { int rc = -1; @@ -2158,6 +2159,17 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf, 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; @@ -2211,11 +2223,31 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf, const virMacAddr *macaddress, int vlanid, const char *stateDir) { + int ret = -1; + char *pfdevname = NULL; + + if (vf == -1 && virNetDevIsVirtualFunction(linkdev)) { + /* 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) - return virNetDevReplaceMacAddress(linkdev, macaddress, stateDir); + ret = virNetDevReplaceMacAddress(linkdev, macaddress, stateDir); else - return virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid, - stateDir); + ret = virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid, + stateDir); + + cleanup: + VIR_FREE(pfdevname); + return ret; } /** @@ -2230,10 +2262,32 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf, 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)) { + /* 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) - return virNetDevRestoreMacAddress(linkdev, stateDir); + ret = virNetDevRestoreMacAddress(linkdev, stateDir); else - return virNetDevRestoreVfConfig(linkdev, vf, stateDir); + ret = virNetDevRestoreVfConfig(linkdev, vf, vfdevname, stateDir); + + cleanup: + VIR_FREE(pfdevname); + return ret; } #else /* defined(__linux__) && defined(HAVE_LIBNL) */ diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index de8b48014f..0c0f666fa9 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -113,17 +113,6 @@ 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; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index ec959a91c6..b5ee2f1f0f 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -842,7 +842,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, * emulate their switch in firmware. */ if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) { - if (virNetDevReplaceMacAddress(linkdev, macaddress, stateDir) < 0) + if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0) return -1; } @@ -977,7 +977,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, int vf = -1; if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) - ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir)); + ignore_value(virNetDevRestoreNetConfig(linkdev, vf, stateDir)); if (ifname) { if (virNetDevVPortProfileDisassociate(ifname,