1:32 p.m.
These two patches are the v2 of a single patch that I sent last
week:
https://www.redhat.com/archives/libvir-list/2015-August/msg00889.html
At the time I sent that patch, I had no hardware to test it on, but
was operating on a misunderstood assumption - someone in the know had
told me that libvirt didn't need to save/restore the MAC address of
VFs for 802.1Qbh macvtap passthrough, and I extrapolated from there
that we didn't need to *set* the MAC address of the VF either. This
makes sense, since that is what happens for 802.1Qbh *device*
passthrough, but based on my own tests the 802.1Qbh "associate" fails
unless libvirt sets the VF's MAC address prior to associate.
So this patch takes a different approach from V1 - instead of
completely skipping the save/set/restore of MAC address for VFs that
are using 802.1Qbh, it still does it, but uses the "old" method of
ioctl(SIOCGIFHWADDR) and ioctl(SIOCSIFHWADDR).
I *did* have the proper hardware to test this time, and it works (both
for a Cisco VMFEX card using 802.1Qbh and for an Intel 82576 card with
straight macvtap passthrough).
Laine Stump (2):
util: make virNetDev(Replace|Restore)MacAddress public functions
util: don't use netlink to save/set mac for
macvtap+passthrough+802.1Qbh
src/libvirt_private.syms | 2 +
src/util/virnetdev.c | 172 ++++++++++++++++++++++----------------------
src/util/virnetdev.h | 10 +++
src/util/virnetdevmacvlan.c | 26 +++++--
4 files changed, 121 insertions(+), 89 deletions(-)
--
2.1.0
1:32 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: make virNetDev(Replace|Restore)MacAddress public functions
These functions were made static as a part of commit cbfe38c since
they were no longer called from outside virnetdev.c. We once again
need to call them from another file, so this patch makes them once
again public.
---
src/libvirt_private.syms | 2 +
src/util/virnetdev.c | 172 ++++++++++++++++++++++++-----------------------
src/util/virnetdev.h | 10 +++
3 files changed, 99 insertions(+), 85 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d57bf5b..67112ab 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1768,7 +1768,9 @@ virNetDevGetVirtualFunctions;
virNetDevGetVLanID;
virNetDevIsVirtualFunction;
virNetDevLinkDump;
+virNetDevReplaceMacAddress;
virNetDevReplaceNetConfig;
+virNetDevRestoreMacAddress;
virNetDevRestoreNetConfig;
virNetDevRxFilterFree;
virNetDevRxFilterModeTypeFromString;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5fcf805..579ff3b 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -360,6 +360,92 @@ int virNetDevGetMAC(const char *ifname,
#endif
+/**
+ * virNetDevReplaceMacAddress:
+ * @macaddress: new MAC address for interface
+ * @linkdev: name of interface
+ * @stateDir: directory to store old MAC address
+ *
+ * Returns 0 on success, -1 on failure
+ *
+ */
+int
+virNetDevReplaceMacAddress(const char *linkdev,
+ const virMacAddr *macaddress,
+ const char *stateDir)
+{
+ virMacAddr oldmac;
+ char *path = NULL;
+ char macstr[VIR_MAC_STRING_BUFLEN];
+ int ret = -1;
+
+ if (virNetDevGetMAC(linkdev, &oldmac) < 0)
+ return -1;
+
+ if (virAsprintf(&path, "%s/%s",
+ stateDir,
+ linkdev) < 0)
+ return -1;
+ virMacAddrFormat(&oldmac, macstr);
+ if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
+ virReportSystemError(errno, _("Unable to preserve mac for %s"),
+ linkdev);
+ goto cleanup;
+ }
+
+ if (virNetDevSetMAC(linkdev, macaddress) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(path);
+ return ret;
+}
+
+/**
+ * virNetDevRestoreMacAddress:
+ * @linkdev: name of interface
+ * @stateDir: directory containing old MAC address
+ *
+ * Returns 0 on success, -errno on failure.
+ *
+ */
+int
+virNetDevRestoreMacAddress(const char *linkdev,
+ const char *stateDir)
+{
+ int rc = -1;
+ char *oldmacname = NULL;
+ char *macstr = NULL;
+ char *path = NULL;
+ virMacAddr oldmac;
+
+ if (virAsprintf(&path, "%s/%s",
+ stateDir,
+ linkdev) < 0)
+ return -1;
+
+ if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0)
+ goto cleanup;
+
+ if (virMacAddrParse(macstr, &oldmac) != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot parse MAC address from '%s'"),
+ oldmacname);
+ goto cleanup;
+ }
+
+ /*reset mac and remove file-ignore results*/
+ rc = virNetDevSetMAC(linkdev, &oldmac);
+ ignore_value(unlink(path));
+
+ cleanup:
+ VIR_FREE(macstr);
+ VIR_FREE(path);
+ return rc;
+}
+
+
#if defined(SIOCGIFMTU) && defined(HAVE_STRUCT_IFREQ)
/**
* virNetDevGetMTU:
@@ -1838,94 +1924,10 @@ virNetDevSysfsFile(char **pf_sysfs_device_link ATTRIBUTE_UNUSED,
return -1;
}
+
#endif /* !__linux__ */
#if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)
-/**
- * virNetDevReplaceMacAddress:
- * @macaddress: new MAC address for interface
- * @linkdev: name of interface
- * @stateDir: directory to store old MAC address
- *
- * Returns 0 on success, -1 on failure
- *
- */
-static int
-virNetDevReplaceMacAddress(const char *linkdev,
- const virMacAddr *macaddress,
- const char *stateDir)
-{
- virMacAddr oldmac;
- char *path = NULL;
- char macstr[VIR_MAC_STRING_BUFLEN];
- int ret = -1;
-
- if (virNetDevGetMAC(linkdev, &oldmac) < 0)
- return -1;
-
- if (virAsprintf(&path, "%s/%s",
- stateDir,
- linkdev) < 0)
- return -1;
- virMacAddrFormat(&oldmac, macstr);
- if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
- virReportSystemError(errno, _("Unable to preserve mac for %s"),
- linkdev);
- goto cleanup;
- }
-
- if (virNetDevSetMAC(linkdev, macaddress) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- VIR_FREE(path);
- return ret;
-}
-
-/**
- * virNetDevRestoreMacAddress:
- * @linkdev: name of interface
- * @stateDir: directory containing old MAC address
- *
- * Returns 0 on success, -errno on failure.
- *
- */
-static int
-virNetDevRestoreMacAddress(const char *linkdev,
- const char *stateDir)
-{
- int rc = -1;
- char *oldmacname = NULL;
- char *macstr = NULL;
- char *path = NULL;
- virMacAddr oldmac;
-
- if (virAsprintf(&path, "%s/%s",
- stateDir,
- linkdev) < 0)
- return -1;
-
- if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0)
- goto cleanup;
-
- if (virMacAddrParse(macstr, &oldmac) != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot parse MAC address from '%s'"),
- oldmacname);
- goto cleanup;
- }
-
- /*reset mac and remove file-ignore results*/
- rc = virNetDevSetMAC(linkdev, &oldmac);
- ignore_value(unlink(path));
-
- cleanup:
- VIR_FREE(macstr);
- VIR_FREE(path);
- return rc;
-}
-
static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
[IFLA_VF_MAC] = { .type = NLA_UNSPEC,
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index fff881c..a27504b 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -114,6 +114,16 @@ 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;
--
2.1.0
1:32 p.m.
New subject: [libvirt] [PATCHv2 2/2] util: don't use netlink to save/set mac for macvtap+passthrough+802.1Qbh
Before libvirt sets the MAC address of the physdev (the physical
ethernet device) linked to a macvtap passthrough device, it always
saves the previous MAC address to restore when the guest is finished
(following a "leave nothing behind" policy). For a long time it
accomplished the save/restore with a combination of
ioctl(SIOCGIFHWADDR) and ioctl(SIOCSIFHWADDR), but in commit cbfe38c
(first in libvirt 1.2.15) this was changed to use netlink RTM_GETLINK
and RTM_SETLINK commands sent to the Physical Function (PF) of any
device that was detected to be a Virtual Function (VF).
We later found out that this caused problems with any devices using
the Cisco enic driver (e.g. vmfex cards) because the enic driver
hasn't implemented the function that is called to gather the
information in the IFLA_VFINFO_LIST attribute of RTM_GETLINK
(ndo_get_vf_config() for those keeping score), so we would never get
back a useful response.
In an ideal world, all drivers would implement all functions, but it
turns out that in this case we can work around this omission without
any bad side effects - since all macvtap passthrough <interface>
definitions pointing to a physdev that uses the enic driver *must*
have a <virtualport type='802.1Qbh'>, and since no other type of
ethernet devices use 802.1Qbh, libvirt can change its behavior in this
case to use the old-style. ioctl(SIOC[GS]IFHWADDR). That's what this
patch does.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1257004
---
src/util/virnetdevmacvlan.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 3bc3d73..89985b8 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -778,9 +778,22 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
* This is especially important when using SRIOV capable cards that
* emulate their switch in firmware.
*/
+
if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
- if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0)
- return -1;
+ if (virtPortProfile &&
+ virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBH) {
+ /* The Cisco enic driver (the only card that uses
+ * 802.1Qbh) doesn't support IFLA_VFINFO_LIST, which is
+ * required for virNetDevReplaceNetConfig(), so we must
+ * use this function (which uses ioctl(SIOCGIFHWADDR)
+ * instead or virNetDevReplaceNetConfig()
+ */
+ if (virNetDevReplaceMacAddress(linkdev, macaddress, stateDir) < 0)
+ return -1;
+ } else {
+ if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0)
+ return -1;
+ }
}
if (tgifname) {
@@ -913,8 +926,13 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
int ret = 0;
int vf = -1;
- if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU)
- ignore_value(virNetDevRestoreNetConfig(linkdev, vf, stateDir));
+ if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
+ if (virtPortProfile &&
+ virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBH)
+ ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir));
+ else
+ ignore_value(virNetDevRestoreNetConfig(linkdev, vf, stateDir));
+ }
if (ifname) {
if (virNetDevVPortProfileDisassociate(ifname,
--
2.1.0
8:11 a.m.
On 01.09.2015 20:32, Laine Stump wrote:
These two patches are the v2 of a single patch that I sent last
week:
https://www.redhat.com/archives/libvir-list/2015-August/msg00889.html
At the time I sent that patch, I had no hardware to test it on, but
was operating on a misunderstood assumption - someone in the know had
told me that libvirt didn't need to save/restore the MAC address of
VFs for 802.1Qbh macvtap passthrough, and I extrapolated from there
that we didn't need to *set* the MAC address of the VF either. This
makes sense, since that is what happens for 802.1Qbh *device*
passthrough, but based on my own tests the 802.1Qbh "associate" fails
unless libvirt sets the VF's MAC address prior to associate.
So this patch takes a different approach from V1 - instead of
completely skipping the save/set/restore of MAC address for VFs that
are using 802.1Qbh, it still does it, but uses the "old" method of
ioctl(SIOCGIFHWADDR) and ioctl(SIOCSIFHWADDR).
I *did* have the proper hardware to test this time, and it works (both
for a Cisco VMFEX card using 802.1Qbh and for an Intel 82576 card with
straight macvtap passthrough).
Laine Stump (2):
util: make virNetDev(Replace|Restore)MacAddress public functions
util: don't use netlink to save/set mac for
macvtap+passthrough+802.1Qbh
src/libvirt_private.syms | 2 +
src/util/virnetdev.c | 172 ++++++++++++++++++++++----------------------
src/util/virnetdev.h | 10 +++
src/util/virnetdevmacvlan.c | 26 +++++--
4 files changed, 121 insertions(+), 89 deletions(-)
ACK series
Michal
3402
days inactive
3404
days old
3 comments
2 participants
participants (2)
-
Laine Stump -
Michal Privoznik