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