[libvirt] [PATCH] 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 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
On 04/10/2015 01:47 PM, Laine Stump wrote:
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 ^^ Looks like a close ")" is needed...
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(-)
Perhaps in order to avoid someone externally calling virNetDevRestoreMacAddress and virNetDevReplaceMacAddress, they should be come local/static to virnetdev.c thus forcing other module callers to use virNetDevRestoreVfConfig or virNetDevReplaceNetConfig Not a requirement, but it potentially avoids future issues... In any case, ACK as long as your testing went well as your explanation seems perfectly reasonable and succinct John
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,
On 04/15/2015 02:47 PM, John Ferlan wrote:
On 04/10/2015 01:47 PM, Laine Stump wrote:
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 ^^ Looks like a close ")" is needed...
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(-)
Perhaps in order to avoid someone externally calling virNetDevRestoreMacAddress and virNetDevReplaceMacAddress, they should be come local/static to virnetdev.c thus forcing other module callers to use virNetDevRestoreVfConfig or virNetDevReplaceNetConfig
Yeah, I thought about that, but vacillated about whether to make it part of the same patch or a separate patch. Then as usually I forgot about it. I think I'll just make them static as part of this patch.
Not a requirement, but it potentially avoids future issues...
In any case, ACK as long as your testing went well as your explanation seems perfectly reasonable and succinct
Yep, everything worked out okay. I was unable to make macvtap bridge/private mode work on the VF interfaces after this patch, but then I went back to older libvirt and found that those modes don't work on VFs *before* the patch either, and I don't think it's a regression, I think it just never worked due to limitations about promiscuous mode on VFs. In the end I think it's only practical to do those modes on the PF, with the VF's useful for macvtap passthrough and direct device assignment. I'll push this after making the abovementioned functions static. Thanks for the review!
participants (2)
-
John Ferlan -
Laine Stump