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

[Michal Privoznik]

On 15.05.2015 17:13, 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?

I think the better is if libvirt sets the PF online. So my ACK to the
patch still holds.

Michal