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

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

Commit Message

Radoslaw Biernacki Nov. 17, 2018, 7:51 p.m.
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.
The error report was moved outside from virNetDevGetPhysicalFunction()
and the error message changed slightly to reflect other potential
root causes of error.

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 | 22 ++++++++++----------
 1 file changed, 11 insertions(+), 11 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. 20, 2018, 10:17 a.m. | #1
On 11/17/18 8:51 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.

> The error report was moved outside from virNetDevGetPhysicalFunction()

> and the error message changed slightly to reflect other potential

> root causes of error.

> 

> 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 | 22 ++++++++++----------

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

> 

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

> index 5867977df4..f1c2ba8c17 100644

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

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

> @@ -1358,15 +1358,8 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)

>          goto cleanup;

>      }

>  

> -    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);


If you remove this error reporting you have to make sure that all the
callers do report it (if needed).

Worse, this function has a non-Linux stub which sets errno = ENOSYS and
reports an error. Therefore I think the right solution is to keep this
error in and ..

> +    if (!*pfname)

>          goto cleanup;

> -    }

>  

>      ret = 0;

>   cleanup:

> @@ -1453,8 +1446,12 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,

>  

>      *pfname = NULL;

>  

> -    if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)

> +    if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {

> +        virReportError(VIR_ERR_INTERNAL_ERROR,

> +                       _("Cannot get PF netdev name for VF %s"),

> +                       vfname);

>          return ret;

> +    }

>  

>      if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)

>          goto cleanup;

> @@ -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));


.. just call this function like this:

  if (virNetDevGetPhysicalFunction(ifname, &pfname) < 0) {
    /* Ignore error if PF does not have a netdev assigned
     * in which case pfname == NULL. */
    virResetLastError();
  }


Sorry for not realizing this in v1.

Michal

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

Patch

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5867977df4..f1c2ba8c17 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1358,15 +1358,8 @@  virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
         goto cleanup;
     }
 
-    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);
+    if (!*pfname)
         goto cleanup;
-    }
 
     ret = 0;
  cleanup:
@@ -1453,8 +1446,12 @@  virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
 
     *pfname = NULL;
 
-    if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
+    if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Cannot get PF netdev name for VF %s"),
+                       vfname);
         return ret;
+    }
 
     if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
         goto cleanup;
@@ -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);