On 03/10/2017 09:34 PM, Laine Stump wrote:
There are multiple bugs filed against both libvirt and the kernel related to incorrect restoration of MAC addresses for SR-IOV VFs, both when used via VFIO device assignment and when used for macvtap passthrough mode https://bugzilla.redhat.com/1415609 - libvirt https://bugzilla.redhat.com/1341248 - kernel (igb) https://bugzilla.redhat.com/1415609 - kernel (ixgbe) https://bugzilla.redhat.com/1302166 - kernel (mlx, fixed) Beyond that, there are other problems with incorrect setting and restoration of VF MAC addresses that don't have their own bug reports (although they may be partially described in the comments of the BZes mentioned above). This patch series attempts (and I hope succeeds!) to fix all of these problems, which can be summarized in these three points: * The VF's own MAC address was not being set prior to using it for macvtap passthrough mode. Instead, due to serious misconception on my part, the "admin MAC" for that VF was set in the PF. The problem is that the admin MAC isn't put into use on the VF immediately; rather it is unused until the next time the VF driver is initialized (on either the host or in a guest) so setting it had no practical effect, meaning that the macvtap device's MAC didn't match the MAC of the VF device it was attached to - this meant that traffic wouldn't pass unless the device was in promiscuous mode (and apparently it was back when the code was changed to behave this way?). * The VF's own MAC address was never restored after it had been used (for both VFIO device assignment and for macvtap passthrough mode). Only the "admin MAC" address (which, again, won't take effect until/unless the VF driver is reloaded/reinitialized) was restored. * If the original admin MAC was 00:00:00:00:00:00, libvirt would save that, then attempt to restore it when the guest was finished with the device, but for some/many SRIOV-capable net drivers, an all 0 MAC is not allowed to be set (even though that is its initial value). This would result in the MAC not being changed (of course, since this is the admin MAC, that didn't actually matter, but the combination of the error message and the VF's own MAC still being set to the value used by the guest, users mistakenly believed this was the source of networking problems (e.g. when the guest moved to another host, but the old host still had an interface showing its MAC address). There are lots of details in the patches. 01 - 07 just clean up and slightly modify some existing utility functions. 08 - 11 define some functions to save/restore netdev MAC/vlan settings, and 12-14 change the existing setup/teardown code for macvtap and hostdev to use the new functions, then 15 and 17-19 modify their behavior further, while 16 just removes functions that are no longer used. In the end, the VF's own MAC *and* admin MAC are saved for all SRIOV VFs when we begin using a VF, and the VF's MAC is restored when finished. Since the admin MAC doesn't actually have any immediate practical effect, we don't concern ourselves with assuring it is restored in all cases (in particular, when use use the VF for VFIO assignment, we only restore the VF's MAC, but not the admin MAC during teardown, and when using the VF for macvtap passthrough we avoid restoring the admin MAC because that would unnecessarily turn on the "administratively set" flag in the PF driver (described in the commit log for one of these patches). I might decide to fix the former later, but for now it just unnecessarily complicates the code for no real gain). Laine Stump (19): util: permit querying a VF MAC address or VLAN tag by itself util: remove unused args from virNetDevSetVfConfig() util: use cleanup label consistently in virHostdevNetConfigReplace() util: eliminate useless local variable util: make virMacAddrParse more versatile util: change virPCIGetNetName() to not return error if device has no net name util: make virPCIGetDeviceAddressFromSysfsLink() public util: new function virPCIDeviceRebind() util: new internal function to permit silent failure of virNetDevSetMAC() util: new function virNetDevPFGetVF() 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 src/libvirt_private.syms | 10 +- src/util/virhostdev.c | 171 ++++++-- src/util/virmacaddr.c | 2 +- src/util/virnetdev.c | 937 +++++++++++++++++++++++++++++++------------- src/util/virnetdev.h | 25 ++ src/util/virnetdevmacvlan.c | 45 ++- src/util/virpci.c | 65 ++- src/util/virpci.h | 4 + 8 files changed, 933 insertions(+), 326 deletions(-)
Okay, I've gone through the patches and they look okay. ACK to all except for 11/19 where I'd like to see the file having some format (e.g. JSON). Now let me grab a six pack and clean my head :-).
Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
