Re: [libvirt] [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. "<interface type='direct'> <source mode='passthrough' .../>"), 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 before detaching the host net driver and assigning the device to the domain with PCI passthrough, i.e. "<interface type='hostdev'>", 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's driver is reloaded) that VF's MAC address can't be changed. 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 a 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); 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. --- (I'm still doing some testing on this, but as long as this approach doesn't cause any other regressions (so far it doesn't appear to), this is the final form. So any ACK given will be contingent on the functional tests passing.) src/util/virnetdev.c | 66 ++++++++++++++++++++++++++++++++++++++++----- src/util/virnetdevmacvlan.c | 4 +-- 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index a7903c3..a3f302f 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2243,7 +2243,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; @@ -2258,6 +2259,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; @@ -2311,11 +2323,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; } /** @@ -2330,10 +2362,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/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 5fd2097..ea628c0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -779,7 +779,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; } @@ -914,7 +914,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,