12:47 p.m.
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 before detaching the host net
driver and 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's driver is reloaded) that VF's MAC address can't be changed.
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 a 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); 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.
---
(I'm still doing some testing on this, but as long as this approach
doesn't cause any other regressions (so far it doesn't appear to),
this is the final form. So any ACK given will be contingent on the
functional tests passing.)
src/util/virnetdev.c | 66 ++++++++++++++++++++++++++++++++++++++++-----
src/util/virnetdevmacvlan.c | 4 +--
2 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index a7903c3..a3f302f 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -2243,7 +2243,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;
@@ -2258,6 +2259,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;
@@ -2311,11 +2323,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;
}
/**
@@ -2330,10 +2362,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/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 5fd2097..ea628c0 100644
--- 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,
--
2.1.0