diff mbox series

[1/4] util: fixing wrong assumption that PF has to have netdev assigned

Message ID 20181110125624.1168-2-radoslaw.biernacki@linaro.org
State New
Headers show
Series util: Fixing libvirt errors on cavium/thunder-nicvf | expand

Commit Message

Radoslaw Biernacki Nov. 10, 2018, 12:56 p.m. UTC 
libvirt wrongly assumes that VF netdev has to have the
netdev assigned to PF. There is no such requirement in SRIOV standard.
This patch change the virNetDevSwitchdevFeature() function to deal
with SRIOV devices which does not have netdev on PF. Also removes
one comment about PF netdev assumption.

One example of such devices is ThunderX VNIC.
By applying this change, VF device is used for virNetlinkCommand() as
it is the only netdev assigned to VNIC.

Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>

---
 src/util/virnetdev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.14.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Michal Prívozník Nov. 15, 2018, 11:23 a.m. UTC | #1 
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
> libvirt wrongly assumes that VF netdev has to have the

> netdev assigned to PF. There is no such requirement in SRIOV standard.

> This patch change the virNetDevSwitchdevFeature() function to deal

> with SRIOV devices which does not have netdev on PF. Also removes

> one comment about PF netdev assumption.

> 

> One example of such devices is ThunderX VNIC.

> By applying this change, VF device is used for virNetlinkCommand() as

> it is the only netdev assigned to VNIC.

> 

> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>

> ---

>  src/util/virnetdev.c | 10 +++++-----

>  1 file changed, 5 insertions(+), 5 deletions(-)

> 

> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c

> index 5867977df4..e55c538a29 100644

> --- a/src/util/virnetdev.c

> +++ b/src/util/virnetdev.c

> @@ -1359,9 +1359,6 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)

>      }

>  

>      if (!*pfname) {

> -        /* this shouldn't be possible. A VF can't exist unless its

> -         * PF device is bound to a network driver

> -         */

>          virReportError(VIR_ERR_INTERNAL_ERROR,

>                         _("The PF device for VF %s has no network device name"),

>                         ifname);

> @@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,

>      if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)

>          return ret;

>  

> -    if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)

> -        goto cleanup;

> +    if (is_vf == 1) {

> +        /* ignore error if PF does noto have netdev assigned

> +         * in that case pfname == NULL */

> +        ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));


Problem is that virNetDevGetPhysicalFunction() reports error on failure.
So either you need to take that out and put it into the other place that
calls the function (virNetDevReadNetConfig) or call virResetLastError().

> +    }

>  

>      pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :

>                                virNetDevGetPCIDevice(ifname);

> 


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Radoslaw Biernacki Nov. 17, 2018, 7:46 p.m. UTC | #2 
On Thu, 15 Nov 2018 at 12:23, Michal Privoznik <mprivozn@redhat.com> wrote:

> On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:

> > libvirt wrongly assumes that VF netdev has to have the

> > netdev assigned to PF. There is no such requirement in SRIOV standard.

> > This patch change the virNetDevSwitchdevFeature() function to deal

> > with SRIOV devices which does not have netdev on PF. Also removes

> > one comment about PF netdev assumption.

> >

> > One example of such devices is ThunderX VNIC.

> > By applying this change, VF device is used for virNetlinkCommand() as

> > it is the only netdev assigned to VNIC.

> >

> > Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>

> > ---

> >  src/util/virnetdev.c | 10 +++++-----

> >  1 file changed, 5 insertions(+), 5 deletions(-)

> >

> > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c

> > index 5867977df4..e55c538a29 100644

> > --- a/src/util/virnetdev.c

> > +++ b/src/util/virnetdev.c

> > @@ -1359,9 +1359,6 @@ virNetDevGetPhysicalFunction(const char *ifname,

> char **pfname)

> >      }

> >

> >      if (!*pfname) {

> > -        /* this shouldn't be possible. A VF can't exist unless its

> > -         * PF device is bound to a network driver

> > -         */

> >          virReportError(VIR_ERR_INTERNAL_ERROR,

> >                         _("The PF device for VF %s has no network device

> name"),

> >                         ifname);

> > @@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,

> >      if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)

> >          return ret;

> >

> > -    if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)

> > -        goto cleanup;

> > +    if (is_vf == 1) {

> > +        /* ignore error if PF does noto have netdev assigned

> > +         * in that case pfname == NULL */

> > +        ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));

>

> Problem is that virNetDevGetPhysicalFunction() reports error on failure.

> So either you need to take that out and put it into the other place that

> calls the function (virNetDevReadNetConfig) or call virResetLastError().

>


Moved error reporting out of  virNetDevGetPhysicalFunction().
Fortunately with 2/4 patch, only virNetDevGetVirtualFunctionInfo() calls it.
Fixed in v2.


> > +    }

> >

> >      pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :

> >                                virNetDevGetPCIDevice(ifname);

> >

>

> Michal

>
<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, 15 Nov 2018 at 12:23, Michal Privoznik &lt;<a href="mailto:mprivozn@redhat.com" target="_blank">mprivozn@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:<br>
&gt; libvirt wrongly assumes that VF netdev has to have the<br>
&gt; netdev assigned to PF. There is no such requirement in SRIOV standard.<br>
&gt; This patch change the virNetDevSwitchdevFeature() function to deal<br>
&gt; with SRIOV devices which does not have netdev on PF. Also removes<br>
&gt; one comment about PF netdev assumption.<br>
&gt; <br>
&gt; One example of such devices is ThunderX VNIC.<br>
&gt; By applying this change, VF device is used for virNetlinkCommand() as<br>
&gt; it is the only netdev assigned to VNIC.<br>
&gt; <br>
&gt; Signed-off-by: Radoslaw Biernacki &lt;<a href="mailto:radoslaw.biernacki@linaro.org" target="_blank">radoslaw.biernacki@linaro.org</a>&gt;<br>
&gt; ---<br>
&gt;  src/util/virnetdev.c | 10 +++++-----<br>
&gt;  1 file changed, 5 insertions(+), 5 deletions(-)<br>
&gt; <br>
&gt; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c<br>
&gt; index 5867977df4..e55c538a29 100644<br>
&gt; --- a/src/util/virnetdev.c<br>
&gt; +++ b/src/util/virnetdev.c<br>
&gt; @@ -1359,9 +1359,6 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)<br>
&gt;      }<br>
&gt;  <br>
&gt;      if (!*pfname) {<br>
&gt; -        /* this shouldn&#39;t be possible. A VF can&#39;t exist unless its<br>
&gt; -         * PF device is bound to a network driver<br>
&gt; -         */<br>
&gt;          virReportError(VIR_ERR_INTERNAL_ERROR,<br>
&gt;                         _(&quot;The PF device for VF %s has no network device name&quot;),<br>
&gt;                         ifname);<br>
&gt; @@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,<br>
&gt;      if ((is_vf = virNetDevIsVirtualFunction(ifname)) &lt; 0)<br>
&gt;          return ret;<br>
&gt;  <br>
&gt; -    if (is_vf == 1 &amp;&amp; virNetDevGetPhysicalFunction(ifname, &amp;pfname) &lt; 0)<br>
&gt; -        goto cleanup;<br>
&gt; +    if (is_vf == 1) {<br>
&gt; +        /* ignore error if PF does noto have netdev assigned<br>
&gt; +         * in that case pfname == NULL */<br>
&gt; +        ignore_value(virNetDevGetPhysicalFunction(ifname, &amp;pfname));<br>
<br>
Problem is that virNetDevGetPhysicalFunction() reports error on failure.<br>
So either you need to take that out and put it into the other place that<br>
calls the function (virNetDevReadNetConfig) or call virResetLastError().<br></blockquote><div><br></div><div>Moved error reporting out of  virNetDevGetPhysicalFunction().</div><div>Fortunately with 2/4 patch, only virNetDevGetVirtualFunctionInfo() calls it.</div><div>Fixed in v2.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
&gt; +    }<br>
&gt;  <br>
&gt;      pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :<br>
&gt;                                virNetDevGetPCIDevice(ifname);<br>
&gt; <br>
<br>
Michal<br>
</blockquote></div></div></div></div></div>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox series

Patch

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5867977df4..e55c538a29 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1359,9 +1359,6 @@  virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
     }
 
     if (!*pfname) {
-        /* this shouldn't be possible. A VF can't exist unless its
-         * PF device is bound to a network driver
-         */
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("The PF device for VF %s has no network device name"),
                        ifname);
@@ -3182,8 +3179,11 @@  virNetDevSwitchdevFeature(const char *ifname,
     if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
         return ret;
 
-    if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
-        goto cleanup;
+    if (is_vf == 1) {
+        /* ignore error if PF does noto have netdev assigned
+         * in that case pfname == NULL */
+        ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));
+    }
 
     pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
                               virNetDevGetPCIDevice(ifname);