src/qemu/qemu_domain_address.c | 11 ++--- src/util/virhostdev.c | 2 +- src/util/virnetdev.c | 50 ++++---------------- 3 files changed, 16 insertions(+), 47 deletions(-)
ThunderX is Cavium SoC. This platform contain SRIOV NIC. Unlike other commonly known network devices it does not have VF functionality duplicated in its PF. PF is purely management device (on HW level). This creates several problems with existing libvirt code as in many places libvirt assumes that each VF netdev has PF netdev assigned. This patch series trying to address issues which can be easily fixed. (mostly bug fixes found while working on full featured solution) First patch in series is most important as it allows to unblock the netdev detection and use <hostdev> on this platform. More details about those issues can be found at: https://bugs.linaro.org/show_bug.cgi?id=3778 https://bugs.launchpad.net/charm-nova-compute/+bug/1771662 Radoslaw Biernacki (4): util: fixing wrong assumption that PF has to have netdev assigned util: Code simplification util: Fix for NULL dereference util: Fixing invalid error checking from virPCIGetNetname() src/qemu/qemu_domain_address.c | 11 ++--- src/util/virhostdev.c | 2 +- src/util/virnetdev.c | 50 ++++---------------- 3 files changed, 16 insertions(+), 47 deletions(-) -- 2.14.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ThunderX is Cavium SoC. This platform contain SRIOV NIC. Unlike other commonly known network devices it does not have VF functionality duplicated in its PF. PF is purely management device (on HW level). This creates several problems with existing libvirt code as in many places libvirt assumes that each VF netdev has PF netdev assigned. This patch series trying to address issues which can be easily fixed. (mostly bug fixes found while working on full featured solution) First patch in series is most important as it allows to unblock the netdev detection and use <hostdev> on this platform.i <interface type="hostdev" still does not work as it requires bigger changes both on netdev driver and in libvirt itself. More details about those issues can be found at: https://bugs.linaro.org/show_bug.cgi?id=3778 https://bugs.launchpad.net/charm-nova-compute/+bug/1771662 v2: - error reporting taken out of virNetDevGetPhysicalFunction() and moved to calling function virNetDevGetVirtualFunctionInfo() - curly braces removed from single line - net-> model check removed as STREQ_NULLABLE() follows Radoslaw Biernacki (4): util: fixing wrong assumption that PF has to have netdev assigned util: Code simplification util: Fix for NULL dereference util: Fixing invalid error checking from virPCIGetNetname() src/qemu/qemu_domain_address.c | 13 ++-- src/util/virhostdev.c | 2 +- src/util/virnetdev.c | 62 +++++--------------- 3 files changed, 22 insertions(+), 55 deletions(-) -- 2.14.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Nov 17, 2018 at 1:17 PM Radoslaw Biernacki <radoslaw.biernacki@linaro.org> wrote: > > ThunderX is Cavium SoC. This platform contain SRIOV NIC. > Unlike other commonly known network devices it does not have VF functionality > duplicated in its PF. PF is purely management device (on HW level). > > This creates several problems with existing libvirt code as in many places > libvirt assumes that each VF netdev has PF netdev assigned. > > This patch series trying to address issues which can be easily fixed. > (mostly bug fixes found while working on full featured solution) > > First patch in series is most important as it allows to unblock the netdev > detection and use <hostdev> on this platform.i <interface type="hostdev" > still does not work as it requires bigger changes both on netdev driver > and in libvirt itself. > More details about those issues can be found at: > https://bugs.linaro.org/show_bug.cgi?id=3778 > https://bugs.launchpad.net/charm-nova-compute/+bug/1771662 > > v2: > - error reporting taken out of virNetDevGetPhysicalFunction() and moved > to calling function virNetDevGetVirtualFunctionInfo() > - curly braces removed from single line > - net-> model check removed as STREQ_NULLABLE() follows Thanks Radoslaw ! I know a v3 is in the works, but just an fyi that we ran this series through our OpenStack testing on a cluster of ThunderX nodes and found no issues. -dann -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Dec 12, 2018 at 12:23 PM dann frazier <dann.frazier@canonical.com> wrote: > > On Sat, Nov 17, 2018 at 1:17 PM Radoslaw Biernacki > <radoslaw.biernacki@linaro.org> wrote: > > > > ThunderX is Cavium SoC. This platform contain SRIOV NIC. > > Unlike other commonly known network devices it does not have VF functionality > > duplicated in its PF. PF is purely management device (on HW level). > > > > This creates several problems with existing libvirt code as in many places > > libvirt assumes that each VF netdev has PF netdev assigned. > > > > This patch series trying to address issues which can be easily fixed. > > (mostly bug fixes found while working on full featured solution) > > > > First patch in series is most important as it allows to unblock the netdev > > detection and use <hostdev> on this platform.i <interface type="hostdev" > > still does not work as it requires bigger changes both on netdev driver > > and in libvirt itself. > > More details about those issues can be found at: > > https://bugs.linaro.org/show_bug.cgi?id=3778 > > https://bugs.launchpad.net/charm-nova-compute/+bug/1771662 > > > > v2: > > - error reporting taken out of virNetDevGetPhysicalFunction() and moved > > to calling function virNetDevGetVirtualFunctionInfo() > > - curly braces removed from single line > > - net-> model check removed as STREQ_NULLABLE() follows > > Thanks Radoslaw ! I know a v3 is in the works, but just an fyi that we > ran this series through our OpenStack testing on a cluster of ThunderX > nodes and found no issues. hey Radoslaw, Thanks for all of your work on this so far on this. Do you expect to have time to prepare a v3, or would it help if I took a crack at it? -dann -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 (!*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);
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
Removing redundant sections of the code
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
---
src/util/virnetdev.c | 40 +++-----------------
1 file changed, 5 insertions(+), 35 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index f1c2ba8c17..9318de406c 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1441,34 +1441,16 @@ int
virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
int *vf)
{
- char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
- int ret = -1;
-
*pfname = NULL;
if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot get PF netdev name for VF %s"),
vfname);
- return ret;
+ return -1;
}
- if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
- goto cleanup;
-
- if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
- goto cleanup;
-
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
-
- cleanup:
- if (ret < 0)
- VIR_FREE(*pfname);
-
- VIR_FREE(vf_sysfs_path);
- VIR_FREE(pf_sysfs_path);
-
- return ret;
+ return virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);
}
#else /* !__linux__ */
@@ -1863,13 +1845,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/
- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}
if (pfDevName) {
@@ -2021,13 +1999,9 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/
- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}
/* if there is a PF, it's now in pfDevName, and linkdev is either
@@ -2226,13 +2200,9 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/
- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/17/18 8:51 PM, Radoslaw Biernacki wrote:
> Removing redundant sections of the code
>
> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/util/virnetdev.c | 40 +++-----------------
> 1 file changed, 5 insertions(+), 35 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index f1c2ba8c17..9318de406c 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1441,34 +1441,16 @@ int
> virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
> int *vf)
> {
> - char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
> - int ret = -1;
> -
> *pfname = NULL;
>
> if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Cannot get PF netdev name for VF %s"),
> vfname);
Sigh, this is overwriting an error reported by
virNetDevGetPhysicalFunction(). But it is pre-existing,
> - return ret;
> + return -1;
> }
>
> - if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
> - goto cleanup;
> -
> - if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
> - goto cleanup;
> -
> - ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
> -
> - cleanup:
> - if (ret < 0)
> - VIR_FREE(*pfname);
> -
> - VIR_FREE(vf_sysfs_path);
> - VIR_FREE(pf_sysfs_path);
> -
> - return ret;
> + return virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);
Just realized that previously if there was an error then *pfname was
NULL upon returning from the function. Now it might be allocated. How about:
ret = virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);
if (ret < 0)
VIR_FREE(*pfname);
return ret;
> }
>
> #else /* !__linux__ */
> @@ -1863,13 +1845,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
> * it to PF + VFname
> */
>
> - if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
> + if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
> goto cleanup;
> -
> pfDevName = pfDevOrig;
> -
> - if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
> - goto cleanup;
> }
>
> if (pfDevName) {
> @@ -2021,13 +1999,9 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
> * it to PF + VFname
> */
>
> - if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
> + if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
No, please keep the proper < 0 comparison.
> goto cleanup;
> -
> pfDevName = pfDevOrig;
> -
> - if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
> - goto cleanup;
> }
>
> /* if there is a PF, it's now in pfDevName, and linkdev is either
> @@ -2226,13 +2200,9 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
> * it to PF + VFname
> */
>
> - if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
> + if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
Here too.
> goto cleanup;
> -
> pfDevName = pfDevOrig;
> -
> - if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
> - goto cleanup;
> }
>
>
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
The device xml parser code does not set "model" while parsing
<interface type='hostdev'>
<source>
<address type='pci' domain='0x0002' bus='0x01' slot='0x00' function='0x2'/>
</source>
</interface>
virDomainDefPtr def->nets[i]->model can be NULL while latter compares strings with
STREQ instead of STREQ_NULLABLE.
Fixes: ac47e4a6225 (qemu: replace "def->nets[i]" with "net" and "def->sounds[i]" with "sound")
Fixes: c7fc151eec7 (qemu: assign virtio devices to PCIe slot when appropriate)
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
---
src/qemu/qemu_domain_address.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 73ed9cc68c..f9275ed810 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -232,10 +232,8 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def)
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (net->model &&
- STREQ(net->model, "spapr-vlan")) {
+ if (STREQ_NULLABLE(net->model, "spapr-vlan"))
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
- }
if (qemuDomainAssignSpaprVIOAddress(def, &net->info, VIO_ADDR_NET) < 0)
goto cleanup;
@@ -324,8 +322,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (net->model &&
- STREQ(net->model, "virtio") &&
+ if (STREQ_NULLABLE(net->model, "virtio") &&
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
net->info.type = type;
}
@@ -692,14 +689,14 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
* addresses for other hostdev devices.
*/
if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
- STREQ(net->model, "usb-net")) {
+ STREQ_NULLABLE(net->model, "usb-net")) {
return 0;
}
- if (STREQ(net->model, "virtio"))
+ if (STREQ_NULLABLE(net->model, "virtio"))
return virtioFlags;
- if (STREQ(net->model, "e1000e"))
+ if (STREQ_NULLABLE(net->model, "e1000e"))
return pcieFlags;
return pciFlags;
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
linkdev is In/Out function parameter as second order reference pointer
so requires first order dereference for checking NULLs which can be a
result from virPCIGetNetName()
Fixes: d6ee56d7237 (util: change virPCIGetNetName() to not return error if device has no net name)
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
---
src/util/virhostdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 1898f9eeb9..1d9345beda 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -317,7 +317,7 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
if (virPCIGetNetName(sysfs_path, 0, NULL, linkdev) < 0)
return -1;
- if (!linkdev) {
+ if (!(*linkdev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The device at %s has no network device name"),
sysfs_path);
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.