Re: [libvirt] [PATCH] netdev: assure that SRIOV PF is online before modifying VF params

On 05/15/2015 05:35 AM, Michal Privoznik wrote: > On 14.05.2015 21:38, Laine Stump wrote: >> The kernel won't complain if you set the mac address and vlan tag for >> an SRIOV VF via its PF, and it will even let you assign the PF to a >> guest using PCI device assignment or macvtap passthrough. But if the >> PF isn't online, the device won't be usable in the guest. This patch >> makes sure that it is turned on. >> >> Since multiple guests/VFs could use the same PF, there is no point in >> ever setting the PF *off*line. >> >> This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=893738 >> >> Originally filed against RHEL6, but present in every version of >> libvirt until today. >> --- >> src/util/virnetdev.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c >> index e14b401..7022dfa 100644 >> --- a/src/util/virnetdev.c >> +++ b/src/util/virnetdev.c >> @@ -2258,6 +2258,17 @@ virNetDevReplaceVfConfig(const char *pflinkdev, int vf, >> char macstr[VIR_MAC_STRING_BUFLEN]; >> char *fileData = NULL; >> int ifindex = -1; >> + bool pfIsOnline; >> + >> + /* Assure that PF is online prior to twiddling with the VF. It >> + * *should* be, but if the PF isn't online the changes made to the >> + * VF via the PF won't take effect, yet there will be no error >> + * reported. >> + */ >> + if (virNetDevGetOnline(pflinkdev, &pfIsOnline) < 0) >> + return ret; >> + if (!pfIsOnline && virNetDevSetOnline(pflinkdev, true) < 0) >> + return ret; >> >> if (virNetDevGetVfConfig(pflinkdev, vf, &oldmac, &oldvlanid) < 0) >> goto cleanup; >> > ACK. Should we set the device back to its previous state if something > goes wrong later in the function? I don't think so. The PF needs to be on in order for any VF to work properly, and we can't assume that nobody else has started trying to use it since we set it online. (this is similar to what we do with the ip_forward setting in the network driver). I guess an alternative would be to *never* set the PF online in libvirt, but instead to log an error an fail; still better than silently failing to pass traffic. Any opinions on which is the better approach?