6:56 a.m.
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
6:56 a.m.
New subject: [libvirt] [PATCH 1/4] util: fixing wrong assumption that PF has to have netdev assigned
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(a)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
5:23 a.m.
New subject: [libvirt] [PATCH 1/4] util: fixing wrong assumption that PF has to have netdev assigned
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(a)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
1:46 p.m.
New subject: [libvirt] [PATCH 1/4] util: fixing wrong assumption that PF has to have netdev assigned
On Thu, 15 Nov 2018 at 12:23, Michal Privoznik <mprivozn(a)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(a)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
6:56 a.m.
New subject: [libvirt] [PATCH 2/4] util: Code simplification
Removing redundant sections of the code
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki(a)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
5:23 a.m.
New subject: [libvirt] [PATCH 2/4] util: Code simplification
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
Removing redundant sections of the code
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki(a)linaro.org>
---
src/util/virnetdev.c | 40 +++-----------------
1 file changed, 5 insertions(+), 35 deletions(-)
ACK
Michal
6:56 a.m.
New subject: [libvirt] [PATCH 3/4] util: Fix for NULL dereference
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(a)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
5:23 a.m.
New subject: [libvirt] [PATCH 3/4] util: Fix for NULL dereference
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(a)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
1:41 p.m.
New subject: [libvirt] [PATCH 3/4] util: Fix for NULL dereference
On Thu, 15 Nov 2018 at 12:23, Michal Privoznik <mprivozn(a)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(a)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
6:56 a.m.
New subject: [libvirt] [PATCH 4/4] util: Fixing invalid error checking from virPCIGetNetname()
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(a)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
5:23 a.m.
New subject: [libvirt] [PATCH 4/4] util: Fixing invalid error checking from virPCIGetNetname()
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(a)linaro.org>
---
src/util/virhostdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK
Michal
1:51 p.m.
New subject: [libvirt] [PATCH v2 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.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
1:51 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: fixing wrong assumption that PF has to have netdev assigned
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(a)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
4:17 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: fixing wrong assumption that PF has to have netdev assigned
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(a)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
1:51 p.m.
New subject: [libvirt] [PATCH v2 2/4] util: Code simplification
Removing redundant sections of the code
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki(a)linaro.org>
Reviewed-by: Michal Privoznik <mprivozn(a)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
4:17 a.m.
New subject: [libvirt] [PATCH v2 2/4] util: Code simplification
On 11/17/18 8:51 PM, Radoslaw Biernacki wrote:
Removing redundant sections of the code
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki(a)linaro.org>
Reviewed-by: Michal Privoznik <mprivozn(a)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
1:51 p.m.
New subject: [libvirt] [PATCH v2 3/4] util: Fix for NULL dereference
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(a)linaro.org>
Reviewed-by: Michal Privoznik <mprivozn(a)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
1:51 p.m.
New subject: [libvirt] [PATCH v2 4/4] util: Fixing invalid error checking from virPCIGetNetname()
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(a)linaro.org>
Reviewed-by: Michal Privoznik <mprivozn(a)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
1:23 p.m.
New subject: [libvirt] [PATCH v2 0/4] util: Fixing libvirt errors on cavium/thunder-nicvf
On Sat, Nov 17, 2018 at 1:17 PM Radoslaw Biernacki
<radoslaw.biernacki(a)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
11:36 a.m.
New subject: [libvirt] [PATCH v2 0/4] util: Fixing libvirt errors on cavium/thunder-nicvf
On Wed, Dec 12, 2018 at 12:23 PM dann frazier
<dann.frazier(a)canonical.com> wrote:
On Sat, Nov 17, 2018 at 1:17 PM Radoslaw Biernacki
<radoslaw.biernacki(a)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
2143
days inactive
2203
days old
19 comments
3 participants
participants (3)
-
dann frazier -
Michal Privoznik -
Radoslaw Biernacki