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, > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list