[libvirt] [PATCH] netdev: assure that SRIOV PF is online before modifying VF params
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; -- 2.1.0
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? Michal
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?
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
On 05/15/2015 11:13 AM, Laine Stump 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
On 05/15/2015 05:35 AM, Michal Privoznik wrote: 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) :-)
participants (2)
-
Laine Stump -
Michal Privoznik