Re: [libvirt] [PATCH] util: set MAC address for VF via netlink message to PF + VF# when possible

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