util: set MAC address for VF via netlink message to PF+VF# when possible

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1113474

When we set the MAC address of a network device as a part of setting
up macvtap "passthrough" mode (where the domain has an emulated netdev
connected to a host macvtap device that has exclusive use of the
physical device, and sets the device MAC address to match its own,
i.e. "<interface type='direct'> <source mode='passthrough' .../>"), we
use ioctl(SIOCSIFHWADDR) giving it the name of that device. This is
true even if it is an SRIOV Virtual Function (VF).

But, when we are setting the MAC address / vlan ID of a VF in
preparation for "hostdev network" passthrough (this is where we set
the MAC address and vlan id of the VF after detaching the host net
driver and before assigning the device to the domain with PCI
passthrough, i.e. "<interface type='hostdev'>", we do the setting via
a netlink RTM_SETLINK message for that VF's Physical Function (PF),
telling it the VF# we want to change. This sets an "administratively
changed MAC" flag for that VF in the PF's driver, and from that point
on (until the PF driver is reloaded, *not* merely the VF driver) that
VF's MAC address can't be changed using ioctl(SIOCSIFHWADDR) - the
only way to change it is via the PF with RTM_SETLINK.

This means that if a VF is used for hostdev passthrough, it will have
the admin flag set, and future attempts to use that VF for macvtap
passthrough will fail.

The solution to this problem is to check if the device being used for
macvtap passthrough is actually a VF; if so, we use the netlink
RTM_SETLINK message to the PF to set the VF's mac address instead of
ioctl(SIOCSIFHWADDR) directly to the VF; if not, behavior does not
change from previously.

There are three pieces to making this work:

1) virNetDevMacVLan(Create|Delete)WithVPortProfile() now call
   virNetDev(Replace|Restore)NetConfig() rather than
   virNetDev(Replace|Restore)MacAddress() (simply passing -1 for VF#
   and vlanid).

2) virNetDev(Replace|Restore)NetConfig() check to see if the device is
   a VF. If so, they find the PF's name and VF#, allowing them to call
   virNetDev(Replace|Restore)VfConfig().

3) To prevent mixups when detaching a macvtap passthrough device that
   had been attached while running an older version of libvirt,
   virNetDevRestoreVfConfig() is potentially given the preserved name
   of the VF, and if the proper statefile for a VF can't be found in
   the stateDir (${stateDir}/${pfname}_vf${vfid}),
   virNetDevRestoreMacAddress() is called instead (which will look in
   the file named ${stateDir}/${vfname}).

This problem has existed in every version of libvirt that has both
macvtap passthrough and interface type='hostdev'. Fortunately people
seem to use one or the other though, so it hasn't caused any real
world problem reports.

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 48785d1ddfe94b67aa08c5812a4b14d76ae6c298..8c50ea2e78e88a173d35468c743af5a9bb8f9157 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1727,9 +1727,7 @@ virNetDevGetVirtualFunctions;
 virNetDevGetVLanID;
 virNetDevIsVirtualFunction;
 virNetDevLinkDump;
-virNetDevReplaceMacAddress;
 virNetDevReplaceNetConfig;
-virNetDevRestoreMacAddress;
 virNetDevRestoreNetConfig;
 virNetDevRxFilterFree;
 virNetDevRxFilterModeTypeFromString;

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 21fa9d02e91dd24592da7f834f2d4aa4427ecfa1..b7ea52452253fa16095f6614900a6de8a6259898 100644 (file)
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -360,7 +360,7 @@ int virNetDevGetMAC(const char *ifname,
  * Returns 0 on success, -1 on failure
  *
  */
-int
+static int
 virNetDevReplaceMacAddress(const char *linkdev,
                            const virMacAddr *macaddress,
                            const char *stateDir)
@@ -402,7 +402,7 @@ virNetDevReplaceMacAddress(const char *linkdev,
  * Returns 0 on success, -errno on failure.
  *
  */
-int
+static int
 virNetDevRestoreMacAddress(const char *linkdev,
                            const char *stateDir)
 {
@@ -2286,7 +2286,8 @@ virNetDevReplaceVfConfig(const char *pflinkdev, int vf,
 }
 
 static int
-virNetDevRestoreVfConfig(const char *pflinkdev, int vf,
+virNetDevRestoreVfConfig(const char *pflinkdev,
+                         int vf, const char *vflinkdev,
                          const char *stateDir)
 {
     int rc = -1;
@@ -2301,6 +2302,17 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf,
                     stateDir, pflinkdev, vf) < 0)
         return rc;
 
+    if (vflinkdev && !virFileExists(path)) {
+        /* this VF's config may have been stored with
+         * virNetDevReplaceMacAddress while running an older version
+         * of libvirt. If so, the ${pf}_vf${id} file won't exist. In
+         * that case, try to restore using the older method with the
+         * VF's name directly.
+         */
+        rc = virNetDevRestoreMacAddress(vflinkdev, stateDir);
+        goto cleanup;
+    }
+
     if (virFileReadAll(path, 128, &fileData) < 0)
         goto cleanup;
 
@@ -2354,11 +2366,31 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf,
                           const virMacAddr *macaddress, int vlanid,
                           const char *stateDir)
 {
+    int ret = -1;
+    char *pfdevname = NULL;
+
+    if (vf == -1 && virNetDevIsVirtualFunction(linkdev)) {
+        /* If this really *is* a VF and the caller just didn't know
+         * it, we should set the MAC address via PF+vf# instead of
+         * setting directly via VF, because the latter will be
+         * rejected any time after the former has been done.
+         */
+        if (virNetDevGetPhysicalFunction(linkdev, &pfdevname) < 0)
+            goto cleanup;
+        if (virNetDevGetVirtualFunctionIndex(pfdevname, linkdev, &vf) < 0)
+            goto cleanup;
+        linkdev = pfdevname;
+    }
+
     if (vf == -1)
-        return virNetDevReplaceMacAddress(linkdev, macaddress, stateDir);
+        ret = virNetDevReplaceMacAddress(linkdev, macaddress, stateDir);
     else
-        return virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid,
-                                        stateDir);
+        ret = virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid,
+                                       stateDir);
+
+ cleanup:
+    VIR_FREE(pfdevname);
+    return ret;
 }
 
 /**
@@ -2373,10 +2405,32 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf,
 int
 virNetDevRestoreNetConfig(const char *linkdev, int vf, const char *stateDir)
 {
+    int ret = -1;
+    char *pfdevname = NULL;
+    const char *vfdevname = NULL;
+
+    if (vf == -1 && virNetDevIsVirtualFunction(linkdev)) {
+        /* If this really *is* a VF and the caller just didn't know
+         * it, we should set the MAC address via PF+vf# instead of
+         * setting directly via VF, because the latter will be
+         * rejected any time after the former has been done.
+         */
+        if (virNetDevGetPhysicalFunction(linkdev, &pfdevname) < 0)
+            goto cleanup;
+        if (virNetDevGetVirtualFunctionIndex(pfdevname, linkdev, &vf) < 0)
+            goto cleanup;
+        vfdevname = linkdev;
+        linkdev = pfdevname;
+    }
+
     if (vf == -1)
-        return virNetDevRestoreMacAddress(linkdev, stateDir);
+        ret = virNetDevRestoreMacAddress(linkdev, stateDir);
     else
-        return virNetDevRestoreVfConfig(linkdev, vf, stateDir);
+        ret = virNetDevRestoreVfConfig(linkdev, vf, vfdevname, stateDir);
+
+ cleanup:
+    VIR_FREE(pfdevname);
+    return ret;
 }
 
 #else /* defined(__linux__) && defined(HAVE_LIBNL) */

diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 72acda5ce47725eb0f6e0caeea14725edaa0d595..190b70e9547f07b717f2b9d4377eefc27e087245 100644 (file)
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -114,17 +114,6 @@ int virNetDevGetMAC(const char *ifname,
                     virMacAddrPtr macaddr)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 
-int virNetDevReplaceMacAddress(const char *linkdev,
-                               const virMacAddr *macaddress,
-                               const char *stateDir)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
-    ATTRIBUTE_RETURN_CHECK;
-
-int virNetDevRestoreMacAddress(const char *linkdev,
-                               const char *stateDir)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
-
-
 int virNetDevSetMTU(const char *ifname,
                     int mtu)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 57b03d08b5473e9d14da62e065bc89c8a3aee05e..213b8eb637e511b6c42560f87bbd00573f424192 100644 (file)
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -779,7 +779,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
      * emulate their switch in firmware.
      */
     if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
-        if (virNetDevReplaceMacAddress(linkdev, macaddress, stateDir) < 0)
+        if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0)
             return -1;
     }
 
@@ -914,7 +914,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
     int vf = -1;
 
     if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU)
-        ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir));
+        ignore_value(virNetDevRestoreNetConfig(linkdev, vf, stateDir));
 
     if (ifname) {
         if (virNetDevVPortProfileDisassociate(ifname,