Re: [libvirt] [PATCH 19/19] util: try *really* hard to set the MAC address of an SRIOV VF

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 6cf0463..861d725 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1053,6 +1053,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: * @@ -1942,6 +1968,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 */ @@ -2037,6 +2064,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, " @@ -2045,27 +2074,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 @@ -2109,6 +2172,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf, cleanup: VIR_FREE(pfDevOrig); VIR_FREE(vfDevOrig); + VIR_FREE(vfPCIDevice);