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

[Laine Stump]

On 05/15/2015 11:13 AM, Laine Stump wrote:
> 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?

Dan Berrange just pointed out on IRC that setting the PF online would
cause it to do IPv6 autoconfig on the host (that's the default when
there is no config for the interface), which might not be what the admin
expected or wanted, so it's a better idea to fail in the case that the
PF is offline, and tell them they need to turn it on in the host network
config.

I just redid the patch to fail/log an error:

  https://www.redhat.com/archives/libvir-list/2015-May/msg00541.html

Thanks for the quick review though (and the question that made me
re-think) :-)