[libvirt] [PATCH 0/4] util: Fixing libvirt errors on cavium/thunder-nicvf
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
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)); + } pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) : virNetDevGetPCIDevice(ifname); -- 2.14.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
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
Removing redundant sections of the code Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org> --- 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 e55c538a29..117ec41869 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1445,30 +1445,12 @@ 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) - return ret; - - 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 -1; - 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
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
Removing redundant sections of the code
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org> --- src/util/virnetdev.c | 40 +++----------------- 1 file changed, 5 insertions(+), 35 deletions(-)
ACK Michal
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> --- src/qemu/qemu_domain_address.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 27c9bfb946..15d25481d8 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -232,8 +232,7 @@ 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; } @@ -325,7 +324,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, virDomainNetDefPtr net = def->nets[i]; if (net->model && - STREQ(net->model, "virtio") && + STREQ_NULLABLE(net->model, "virtio") && net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { net->info.type = type; } @@ -634,14 +633,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
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
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> --- src/qemu/qemu_domain_address.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 27c9bfb946..15d25481d8 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -232,8 +232,7 @@ 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; }
We don't require curly braces around single line bodies. Actually the opposite, our coding style says there should be none. This exception to the rule was discussed many times but without any result. Anyway, 'make syntax-check' would have caught this.
@@ -325,7 +324,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, virDomainNetDefPtr net = def->nets[i];
if (net->model && - STREQ(net->model, "virtio") && + STREQ_NULLABLE(net->model, "virtio") &&
Looks like you've forgotten to remove net->model check ;-)
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { net->info.type = type; } @@ -634,14 +633,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;
The rest looks okay. Michal
On Thu, 15 Nov 2018 at 12:23, Michal Privoznik <mprivozn@redhat.com> wrote:
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
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> --- src/qemu/qemu_domain_address.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 27c9bfb946..15d25481d8 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -232,8 +232,7 @@ 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; }
We don't require curly braces around single line bodies. Actually the opposite, our coding style says there should be none. This exception to the rule was discussed many times but without any result. Anyway, 'make syntax-check' would have caught this.
Sorry Michal, overlooked that. Fixed in v2.
@@ -325,7 +324,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr
def,
virDomainNetDefPtr net = def->nets[i];
if (net->model && - STREQ(net->model, "virtio") && + STREQ_NULLABLE(net->model, "virtio") &&
Looks like you've forgotten to remove net->model check ;-)
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { net->info.type = type; } @@ -634,14 +633,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;
The rest looks okay.
Michal
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> --- 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
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
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> --- src/util/virhostdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Michal
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
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
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
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
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
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
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
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
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
participants (3)
-
dann frazier -
Michal Privoznik -
Radoslaw Biernacki