Merge ~fnordahl/ubuntu/+source/libvirt:bug/1892132-focal into ubuntu/+source/libvirt:ubuntu/focal-devel

[Frode Nordahl]

[Christian Ehrhardt  (paelzer)]

We've just completed the former libvirt SRU - well actually it still is in phasing but it can be considered complete. So it is the right time to do this.

* Changelog:
  - [x] changelog entry correct version and targeted codename
    There is
       libvirt | 6.0.0-0ubuntu8.12 | focal-proposed | source
    you'd need to rebase this

  - [+] changelog entries correct
  - [+] update-maintainer has been run

* New Delta:
  - [-] patches match what was proposed/committed upstream
  - [-] patches correctly included in debian/patches/series
  - [-] patches have correct DEP3 metadata

Both patches are adapted due to not using auto-free back then.
But both LGTM and are properly marked "Origin: backport," - thanks.

It isn't strictly required, but libvirt has that many patches that Ubuntu and started to structure them in directories.
You'll have (a subset of) directories in debian/patches for
- debian (custom patches for Debian)
- stable (pre release stable updates)
- revert (plain reverts of upstream)
- forwarded (a patch created for U/D and forwarded to upstream)
- ubuntu (custom patches for Ubuntu)
- ubuntu-aa (custom patches for Ubuntu in regard to apparmor)

Not all are sorted yet, but at least those we add we try to keep in line, therefore I'd ask to put them into debian/patches/ubuntu if that isn't too much to ask for.

In regard to metadata you have origin - thanks, that probably is the most important one.
There are many that can be added, but of the somewhat required fields you miss out on adding
Bug-Ubuntu: https://bugs.launchpad.net/bugs/<todo-bug>
Last-Update: 20xx-mm-dd

Also there is no pain, but some gain to keep the original patch description in the header.
In this case it explains a lot, no need to remove it here.

* Git/Maintenance
  - [+] testcases added or not needed for this
  - [-] commits are properly split (more important on -dev than on SRUs)

It is not strictly needed for SRUs, but still helps later maintenance (e.g. to cherry pick the changes but not the CL which always is different) if code-changes and changelog would be in individual commits. If you don't mind changing that would be appreciated.

* Build/Test:
  - not checked, is there a PPA with this somewhere already?

review: Needs Fixing

[Frode Nordahl (fnordahl)]

Thank you for the review, Christian!

The branch has been updated to address your insightful feedback.

The package is also building over here: https://launchpad.net/~fnordahl/+archive/ubuntu/lp-1892132

[Christian Ehrhardt  (paelzer)]

Hi Frode,
thanks for v2 - this now LGTM reciew-wise.
Currently the SRU queue for libvirt is free and for testing of this path it needs your (or other peoples) special HW anyway.

The SRU template int he bug LTGM as well and I think my usual testing would not provide much benefit for this change.

The PPA build LGTM as well - thanks for the link.
(you only built amd64, but for this change that should be ok)

There is one issue left.
I asked you to bump 6.0.0-0ubuntu8.12 that you had because there already was another 6.0.0-0ubuntu8.12 - but you made it 6.0.0-0ubuntu8.12.1 while it needs to be 6.0.0-0ubuntu8.13.

I can fix this on sponsoring or do you quickly want to update your branch?

review: Needs Fixing

[Frode Nordahl (fnordahl)]

Hi Christian, ta for re-review, and apologies for missing the request for .13 vs .12.1.

Have now updated the commit message and changelog file to point to 6.0.0-0ubuntu8.13

[Christian Ehrhardt  (paelzer)]

Thanks, sponsoring now ...

review: Approve

[Christian Ehrhardt  (paelzer)]

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading libvirt_6.0.0-0ubuntu8.13.dsc: done.
  Uploading libvirt_6.0.0-0ubuntu8.13.debian.tar.xz: done.
  Uploading libvirt_6.0.0-0ubuntu8.13_source.buildinfo: done.
  Uploading libvirt_6.0.0-0ubuntu8.13_source.changes: done.
Successfully uploaded packages.

 * [new tag] upload/6.0.0-0ubuntu8.13 -> upload/6.0.0-0ubuntu8.13

diff --git a/debian/changelog b/debian/changelog
index 9aa9073..7ad6fee 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+libvirt (6.0.0-0ubuntu8.13) focal; urgency=medium
+
+  * Add support for switchdev NICs that link representor ports to parent PCI
+    device. (LP: #1892132)
+    - d/p/u/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
+    - d/p/u/lp-1892132-add-virNetDevGetPhysPortName.patch
+
+ -- Frode Nordahl <[e-mail]>  Fri, 16 Jul 2021 05:16:36 +0000
+
 libvirt (6.0.0-0ubuntu8.12) focal; urgency=medium
 
   * d/p/u/lp-1929202-*: fix pre-creation of images during migration
diff --git a/debian/patches/series b/debian/patches/series
index 814bac6..2b4f3a6 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -167,3 +167,5 @@ ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Move-monitor-call-out-of-t.patch
 ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Use-virHashNew-and-automat.patch
 ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Remove-ret-variable-and-cl.patch
 ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Fix-filling-of-capacity-wh.patch
+ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch
+ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
diff --git a/debian/patches/ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch b/debian/patches/ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
 new file mode 100644
index 0000000..2e468ed
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
@@ -0,0 +1,138 @@
+Description: To support kernels or dkms builds of mlx5_core driver with
+ https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=123f0f53dd64b67e34142485fe866a8a581f12f1
+Origin: backport, https://github.com/libvirt/libvirt/commit/5b1c525b1f3608156884aed0dc5e925306c1e260
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1892132
+Last-Update: 2021-08-13
+
+diff --git a/src/util/virpci.c b/src/util/virpci.c
+index 0b1222373e..a54be60da0 100644
+--- a/src/util/virpci.c
++++ b/src/util/virpci.c
+@@ -2424,9 +2424,9 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr,
+  * virPCIGetNetName:
+  * @device_link_sysfs_path: sysfs path to the PCI device
+  * @idx: used to choose which netdev when there are several
+- *       (ignored if physPortID is set)
++ *       (ignored if physPortID is set or physPortName is available)
+  * @physPortID: match this string in the netdev's phys_port_id
+- *       (or NULL to ignore and use idx instead)
++ *       (or NULL to ignore and use phys_port_name or idx instead)
+  * @netname: used to return the name of the netdev
+  *       (set to NULL (but returns success) if there is no netdev)
+  *
+@@ -2461,6 +2461,14 @@ virPCIGetNetName(const char *device_link_sysfs_path,
+     }
+ 
+     while (virDirRead(dir, &entry, pcidev_sysfs_net_path) > 0) {
++        /* save the first entry we find to use as a failsafe
++         * in case we don't match the phys_port_id. This is
++         * needed because some NIC drivers (e.g. i40e)
++         * implement phys_port_id for PFs, but not for VFs
++         */
++        if (!firstEntryName)
++            firstEntryName = g_strdup(entry->d_name);
++
+         /* if the caller sent a physPortID, compare it to the
+          * physportID of this netdev. If not, look for entry[idx].
+          */
+@@ -2471,19 +2479,35 @@ virPCIGetNetName(const char *device_link_sysfs_path,
+             /* if this one doesn't match, keep looking */
+             if (STRNEQ_NULLABLE(physPortID, thisPhysPortID)) {
+                 VIR_FREE(thisPhysPortID);
+-                /* save the first entry we find to use as a failsafe
+-                 * in case we don't match the phys_port_id. This is
+-                 * needed because some NIC drivers (e.g. i40e)
+-                 * implement phys_port_id for PFs, but not for VFs
+-                 */
+-                if (!firstEntryName)
+-                    firstEntryName = g_strdup(entry->d_name);
+-
+                 continue;
+             }
+         } else {
+-            if (i++ < idx)
+-                continue;
++            /* Most switch devices use phys_port_name instead of
++             * phys_port_id.
++             * NOTE: VFs' representors net devices can be linked to PF's PCI
++             * device, which mean that there'll be multiple net devices
++             * instances and to get a proper net device need to match on
++             * specific regex.
++             * To get PF netdev, for ex., used following regex:
++             * "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
++             * or to get exact VF's netdev next regex is used:
++             * "pf0vf1$"
++             */
++            g_autofree char *thisPhysPortName = NULL;
++            if (virNetDevGetPhysPortName(entry->d_name, &thisPhysPortName) < 0)
++                goto cleanup;
++
++            if (thisPhysPortName) {
++
++                /* if this one doesn't match, keep looking */
++                if (!virStringMatch(thisPhysPortName, VIR_PF_PHYS_PORT_NAME_REGEX))
++                    continue;
++
++            } else {
++
++                if (i++ < idx)
++                    continue;
++            }
+         }
+ 
+         *netname = g_strdup(entry->d_name);
+@@ -2493,24 +2517,21 @@ virPCIGetNetName(const char *device_link_sysfs_path,
+     }
+ 
+     if (ret < 0) {
+-        if (physPortID) {
+-            if (firstEntryName) {
+-                /* we didn't match the provided phys_port_id, but this
+-                 * is probably because phys_port_id isn't implemented
+-                 * for this NIC driver, so just return the first
+-                 * (probably only) netname we found.
+-                 */
+-                *netname = firstEntryName;
+-                firstEntryName = NULL;
+-                ret = 0;
+-            } else {
+-                virReportError(VIR_ERR_INTERNAL_ERROR,
+-                               _("Could not find network device with "
+-                                 "phys_port_id '%s' under PCI device at %s"),
+-                               physPortID, device_link_sysfs_path);
+-            }
++        if (firstEntryName) {
++            /* we didn't match the provided phys_port_id / find a
++             * phys_port_name matching VIR_PF_PHYS_PORT_NAME_REGEX / find
++             * as many net devices as the value of idx, but this is
++             * probably because phys_port_id / phys_port_name isn't
++             * implemented for this NIC driver, so just return the first
++             * (probably only) netname we found.
++             */
++            *netname = firstEntryName;
++            ret = 0;
+         } else {
+-            ret = 0; /* no netdev at the given index is *not* an error */
++            virReportError(VIR_ERR_INTERNAL_ERROR,
++                           _("Could not find any network device under PCI device at %s"),
++                           device_link_sysfs_path);
++            ret = -1;
+         }
+     }
+  cleanup:
+diff --git a/src/util/virpci.h b/src/util/virpci.h
+index f6796fc422..e47c766918 100644
+--- a/src/util/virpci.h
++++ b/src/util/virpci.h
+@@ -49,6 +49,11 @@ struct _virZPCIDeviceAddress {
+ 
+ #define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d"
+ 
++/* Represents format of PF's phys_port_name in switchdev mode:
++ * 'p%u' or 'p%us%u'. New line checked since value is readed from sysfs file.
++ */
++#define VIR_PF_PHYS_PORT_NAME_REGEX  "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
++
+ struct _virPCIDeviceAddress {
+     unsigned int domain;
+     unsigned int bus;
diff --git a/debian/patches/ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch b/debian/patches/ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch
 new file mode 100644
index 0000000..757067c
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch
@@ -0,0 +1,120 @@
+Description: To support kernels or dkms builds of mlx5_core driver with
+ https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=123f0f53dd64b67e34142485fe866a8a581f12f1
+Origin: backport, https://github.com/libvirt/libvirt/commit/97ebb982453bc23759c5f180799d6f2207b81c80
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1892132
+Last-Update: 2021-08-13
+
+diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
+index e2aad07c24..52c9343d63 100644
+--- a/src/util/virnetdev.c
++++ b/src/util/virnetdev.c
+@@ -1143,6 +1143,29 @@ virNetDevGetPCIDevice(const char *devName)
+ }
+ 
+ 
++/* A wrapper to get content of file from ifname SYSFS_NET_DIR
++ */
++static int
++virNetDevGetSysfsFileValue(const char *ifname,
++                           const char *fileName,
++                           char **sysfsFileData)
++{
++    g_autofree char *sysfsFile = NULL;
++
++    *sysfsFileData = NULL;
++
++    if (virNetDevSysfsFile(&sysfsFile, ifname, fileName) < 0)
++        return -1;
++
++    /* a failure to read just means the driver doesn't support
++     * <fileName>, so set success now and ignore the return from
++     * virFileReadAllQuiet().
++     */
++
++    ignore_value(virFileReadAllQuiet(sysfsFile, 1024, sysfsFileData));
++    return 0;
++}
++
+ /**
+  * virNetDevGetPhysPortID:
+  *
+@@ -1161,25 +1184,29 @@ int
+ virNetDevGetPhysPortID(const char *ifname,
+                        char **physPortID)
+ {
+-    int ret = -1;
+-    char *physPortIDFile = NULL;
+-
+-    *physPortID = NULL;
+-
+-    if (virNetDevSysfsFile(&physPortIDFile, ifname, "phys_port_id") < 0)
+-        goto cleanup;
+-
+-    /* a failure to read just means the driver doesn't support
+-     * phys_port_id, so set success now and ignore the return from
+-     * virFileReadAllQuiet().
+-     */
+-    ret = 0;
++    return virNetDevGetSysfsFileValue(ifname, "phys_port_id", physPortID);
++}
+ 
+-    ignore_value(virFileReadAllQuiet(physPortIDFile, 1024, physPortID));
+ 
+- cleanup:
+-    VIR_FREE(physPortIDFile);
+-    return ret;
++/**
++ * virNetDevGetPhysPortName:
++ *
++ * @ifname: name of a netdev
++ *
++ * @physPortName: pointer to char* that will receive @ifname's
++ *                phys_port_name from sysfs (null terminated
++ *                string). Could be NULL if @ifname's net driver doesn't
++ *                support phys_port_name (most netdev drivers
++ *                don't). Caller is responsible for freeing the string
++ *                when finished.
++ *
++ * Returns 0 on success or -1 on failure.
++ */
++int
++virNetDevGetPhysPortName(const char *ifname,
++                         char **physPortName)
++{
++    return virNetDevGetSysfsFileValue(ifname, "phys_port_name", physPortName);
+ }
+ 
+ 
+@@ -1461,6 +1488,17 @@ virNetDevGetPhysPortID(const char *ifname G_GNUC_UNUSED,
+     return 0;
+ }
+ 
++int
++virNetDevGetPhysPortName(const char *ifname G_GNUC_UNUSED,
++                       char **physPortName)
++{
++    /* this actually should never be called, and is just here to
++     * satisfy the linker.
++     */
++    *physPortName = NULL;
++    return 0;
++}
++
+ int
+ virNetDevGetVirtualFunctions(const char *pfname G_GNUC_UNUSED,
+                              char ***vfname G_GNUC_UNUSED,
+diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
+index 24b41498ed..26fe76cc2c 100644
+--- a/src/util/virnetdev.h
++++ b/src/util/virnetdev.h
+@@ -227,6 +227,10 @@ int virNetDevGetPhysPortID(const char *ifname,
+                            char **physPortID)
+     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+     G_GNUC_WARN_UNUSED_RESULT;
++int virNetDevGetPhysPortName(const char *ifname,
++                           char **physPortName)
++    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
++    G_GNUC_WARN_UNUSED_RESULT;
+ 
+ int virNetDevGetVirtualFunctions(const char *pfname,
+                                  char ***vfname,