From 61e603b025083ece8ebf12dc3664c0a5f613f400 Mon Sep 17 00:00:00 2001

Message-Id: <61e603b025083ece8ebf12dc3664c0a5f613f400@dist-git>

From: Laine Stump <laine@laine.org>

Date: Thu, 13 Apr 2017 14:29:37 -0400

Subject: [PATCH] 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).

Resolves: https://bugzilla.redhat.com/1442040 (RHEL 7.3.z)

Resolves: https://bugzilla.redhat.com/1415609 (RHEL 7.4)

(cherry picked from commit 86556e167a634599ac9975bd5dad98e3ef9f7017)

---

 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 0223877ff..a510ebc2e 100644

--- a/src/util/virnetdev.c

+++ b/src/util/virnetdev.c

@@ -1054,6 +1054,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:

  *

@@ -2001,6 +2027,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 */

@@ -2096,6 +2123,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, "

@@ -2104,27 +2133,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

@@ -2168,6 +2231,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,

  cleanup:

     VIR_FREE(pfDevOrig);

     VIR_FREE(vfDevOrig);

+    virPCIDeviceFree(vfPCIDevice);

     return ret;

 }

-- 

2.12.2