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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list