[libvirt] [PATCH 00/19] Fix saving/setting/restoring SR-IOV VF MAC address

[Michal Privoznik]

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