ext/pci: implement PCI path for device identification

doc/example.xml

@@ -22,7 +22,7 @@
             </features>
 
             <devices class="pci">
-                <device backend-domain="dom0" id="01_23.45">
+                <device backend-domain="dom0" id="01_23.4">
                   <option name="no-strict-reset">True</option>
                 </device>
             </devices>

doc/qubes-devices.rst

@@ -189,6 +189,75 @@ We must first create an assignment (`assign`) as required
 attached upon each VM startup. However, if a PCI device is currently in use
 by another VM, the startup of the second VM will fail.
 
+PCI devices addressing
+^^^^^^^^^^^^^^^^^^^^^^
+
+Qubes identifies PCI devices using a PCI path, instead of just
+segment/bus/device/function (aka SBDF). The latter is used by many tools (like
+`lspci`) but it's not guaranteed to remain stable across (seemingly unrelated)
+hardware or firmware changes. The PCI path is built by following PCIe bridges
+from the root port down to the target device. The path is constructed as
+follows:
+
+1. It starts with the root port address written as
+`[<SEGMENT>_]<BUS>_<DEVICE>.<FUNCTION>` (the segment part can be omitted if
+`0000`). All numbers written in hex, with `<SEGMENT>` (if present) padded to
+4 digits, `<BUS>` and `<DEVICE>` padded to 2 digits and `<FUNCTION>` as 1 digit.
+2. Subsequent bridges are added after a dash (`-`) in form of
+`<BUS-OFFSET>_<DEVICE>.<FUNCTION>`, where `<BUS-OFFSET>` is a bus number
+relative to the parent bridge's secondary bus number. In a simple case where
+the parent bridge has only one bus, the `<BUS-OFFSET>` will be `00`.
+3. Final device is added the same way as the bridges described in the second
+step.
+
+The path uses `<BUS-OFFSET>` instead of bridge's BUS number directly, to
+remain stable across adding/removing *other* bridges.
+
+For example, given the following PCI devices layout:
+
+.. code-block::
+
+    # lspci -t
+    -[0000:00]-+-00.0
+               +-00.2
+               +-01.0
+               +-02.0
+               +-02.2-[01]----00.0
+               +-02.4-[02]----00.0
+               +-03.0
+               +-03.1-[03-61]--
+               +-04.0
+               +-04.1-[62-c0]--
+               +-08.0
+               +-08.1-[c1]--+-00.0
+               |            +-00.1
+               |            +-00.2
+               |            +-00.3
+               |            +-00.4
+               |            +-00.5
+               |            \-00.6
+    ...
+
+
+The device 0000:c1:00.0 is behind bridge at 0000:00:08.1. In some cases there
+may be more bridges. There may be also more devices behind a single bridge - in
+the example above 0000:c1:00.0 is just a single multi-function device, but
+bridge 0000:00:03.1 can have multiple devices (covering buses `03` up to `61`).
+You can get bus ranges handled by a given bridge by looking at "secondary"
+and "subordinate" attributes on lspci (and similarly in sysfs):
+
+.. code-block::
+
+    00:08.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Phoenix Internal GPP Bridge to Bus [C:A] (prog-if 00 [Normal decode])
+        Subsystem: Device 0006:f111
+        Flags: bus master, fast devsel, latency 0, IRQ 106
+        Bus: primary=00, secondary=c1, subordinate=c1, sec-latency=0
+    ...
+
+The path for the 0000:c1:00.0 device is: `0000_00_08.1-00_00.0`, where the -00
+part means "the first bus behind this bridge". Since the segment is `0000`,
+this path can be written also as `00_08.1-00_00.0`.
+
 Microphone
 ----------
 

qubes/ext/pci.py

@@ -35,6 +35,7 @@
 import qubes.devices
 import qubes.ext
 from qubes.device_protocol import Port
+from qubes.utils import sbdf_to_path, path_to_sbdf, is_pci_path
 
 #: cache of PCI device classes
 pci_classes = None
@@ -80,18 +81,9 @@
 
 
 def pcidev_class(dev_xmldesc):
-    sysfs_path = dev_xmldesc.findtext("path")
-    assert sysfs_path
-    try:
-        with open(sysfs_path + "/class", encoding="ascii") as f_class:
-            class_id = f_class.read().strip()
-    except OSError:
-        return "unknown"
-
+    class_id = pcidev_interface(dev_xmldesc)
     if not qubes.ext.pci.pci_classes:
         qubes.ext.pci.pci_classes = load_pci_classes()
-    if class_id.startswith("0x"):
-        class_id = class_id[2:]
     try:
         # ignore prog-if
         return qubes.ext.pci.pci_classes[class_id[0:4]]
@@ -147,22 +139,20 @@
 class PCIDevice(qubes.device_protocol.DeviceInfo):
     # pylint: disable=too-few-public-methods
     regex = re.compile(
-        r"\A(?P<bus>[0-9a-f]+)_(?P<device>[0-9a-f]+)\."
-        r"(?P<function>[0-9a-f]+)\Z"
+        r"\A((?P<segment>[0-9a-f]{4})[_:])?(?P<bus>[0-9a-f]{2})[_:]"
+        r"(?P<device>[0-9a-f]{2})\.(?P<function>[0-9a-f])\Z"
     )
     _libvirt_regex = re.compile(
-        r"\Apci_0000_(?P<bus>[0-9a-f]+)_(?P<device>[0-9a-f]+)_"
-        r"(?P<function>[0-9a-f]+)\Z"
+        r"\Apci_(?P<segment>[0-9a-f]{4})_(?P<bus>[0-9a-f]{2})_"
+        r"(?P<device>[0-9a-f]{2})_(?P<function>[0-9a-f])\Z"
     )
 
     def __init__(self, port: Port, libvirt_name=None):
         if libvirt_name:
             dev_match = self._libvirt_regex.match(libvirt_name)
             if not dev_match:
                 raise UnsupportedDevice(libvirt_name)
-            port_id = "{bus}_{device}.{function}".format(
-                **dev_match.groupdict()
-            )
+            port_id = sbdf_to_path(libvirt_name)
             port = Port(
                 backend_domain=port.backend_domain,
                 port_id=port_id,
@@ -171,14 +161,24 @@
 
         super().__init__(port)
 
-        dev_match = self.regex.match(port.port_id)
+        if is_pci_path(port.port_id):
+            sbdf = path_to_sbdf(port.port_id)
+        else:
+            sbdf = port.port_id
+        dev_match = self.regex.match(sbdf)
         if not dev_match:
             raise ValueError(
-                "Invalid device identifier: {!r}".format(port.port_id)
+                "Invalid device identifier: {!r} (sbdf: {!r})".format(
+                    port.port_id, sbdf
+                )
             )
 
+        self.data["sbdf"] = sbdf
+
         for group in self.regex.groupindex:
             setattr(self, group, dev_match.group(group))
+        if getattr(self, "segment") is None:
+            self.segment = "0000"
 
         # lazy loading
         self._description: Optional[str] = None
@@ -258,7 +258,7 @@
     def libvirt_name(self):
         # pylint: disable=no-member
         # noinspection PyUnresolvedReferences
-        return f"pci_0000_{self.bus}_{self.device}_{self.function}"
+        return f"pci_{self.segment}_{self.bus}_{self.device}_{self.function}"
 
     @property
     def description(self):
@@ -389,31 +389,31 @@
             if hostdev.get("type") != "pci":
                 continue
             address = hostdev.find("source/address")
+            segment = address.get("domain")[2:]
             bus = address.get("bus")[2:]
             device = address.get("slot")[2:]
             function = address.get("function")[2:]
 
-            port_id = "{bus}_{device}.{function}".format(
+            libvirt_name = "pci_{segment}_{bus}_{device}_{function}".format(
+                segment=segment,
                 bus=bus,
                 device=device,
                 function=function,
             )
             yield PCIDevice(
                 Port(
                     backend_domain=vm.app.domains[0],
-                    port_id=port_id,
+                    port_id=None,
                     devclass="pci",
-                )
+                ),
+                libvirt_name=libvirt_name,
             ), {}
 
     @qubes.ext.handler("device-pre-attach:pci")
     def on_device_pre_attached_pci(self, vm, event, device, options):
         # pylint: disable=unused-argument
-        if not os.path.exists(
-            "/sys/bus/pci/devices/0000:{}".format(
-                device.port_id.replace("_", ":")
-            )
-        ):
+        sbdf = path_to_sbdf(device.port_id)
+        if sbdf is None or not os.path.exists(f"/sys/bus/pci/devices/{sbdf}"):
             raise qubes.exc.QubesException(
                 "Invalid PCI device: {}".format(device.port_id)
             )
@@ -467,7 +467,7 @@
         ) as p:
             result = p.communicate()[0].decode()
         m = re.search(
-            r"^(\d+.\d+)\s+0000:{}$".format(device.port_id.replace("_", ":")),
+            r"^(\d+.\d+)\s+{}$".format(device.data["sbdf"]),
             result,
             flags=re.MULTILINE,
         )

qubes/tests/devices_pci.py

@@ -17,11 +17,16 @@
 #
 # You should have received a copy of the GNU Lesser General Public
 # License along with this library; if not, see <https://www.gnu.org/licenses/>.
+import os.path
+import unittest
 from unittest import mock
 
 import qubes.tests
 import qubes.ext.pci
 from qubes.device_protocol import DeviceInterface
+from qubes.utils import sbdf_to_path, path_to_sbdf
+
+orig_open = open
 
 
 class TestVM(object):
@@ -107,17 +112,53 @@ def mock_file_open(filename: str, *_args, **_kwargs):
 \t09  CANBUS
 \t80  Serial bus controller
 """
-    elif filename.startswith("/sys/devices/pci"):
-        content = "0x0c0330"
     else:
-        raise OSError()
+        return orig_open(filename, *_args, **_kwargs)
 
     file_object = mock.mock_open(read_data=content).return_value
     file_object.__iter__.return_value = content
     return file_object
 
 
-class TC_00_Block(qubes.tests.QubesTestCase):
+# prefer location in git checkout
+tests_sysfs_path = os.path.dirname(__file__) + "/../../tests-data/sysfs/sys"
+if not os.path.exists(tests_sysfs_path):
+    # but if not there, look for package installed one
+    tests_sysfs_path = "/usr/share/qubes/tests-data/sysfs/sys"
+
+
+@mock.patch("qubes.utils.SYSFS_BASE", tests_sysfs_path)
+class TC_00_helpers(qubes.tests.QubesTestCase):
+    def test_000_sbdf_to_path1(self):
+        path = sbdf_to_path("0000:c6:00.0")
+        self.assertEqual(path, "c0_03.5-00_00.0-00_00.0")
+
+    def test_001_sbdf_to_path2(self):
+        path = sbdf_to_path("0000:00:18.4")
+        self.assertEqual(path, "00_18.4")
+
+    def test_002_sbdf_to_path_libvirt(self):
+        path = sbdf_to_path("pci_0000_00_18_4")
+        self.assertEqual(path, "00_18.4")
+
+    def test_003_sbdf_to_path_default_segment1(self):
+        path = sbdf_to_path("00:18.4")
+        self.assertEqual(path, "00_18.4")
+
+    def test_004_sbdf_to_path_default_segment2(self):
+        path = sbdf_to_path("0000:00:18.4")
+        self.assertEqual(path, "00_18.4")
+
+    def test_010_path_to_sbdf1(self):
+        path = path_to_sbdf("0000_c0_03.5-00_00.0-00_00.0")
+        self.assertEqual(path, "0000:c6:00.0")
+
+    def test_011_path_to_sbdf2(self):
+        path = path_to_sbdf("0000_00_18.4")
+        self.assertEqual(path, "0000:00:18.4")
+
+
+class TC_10_PCI(qubes.tests.QubesTestCase):
     def setUp(self):
         super().setUp()
         self.ext = qubes.ext.pci.PCIDeviceExtension()
@@ -143,7 +184,7 @@ def test_000_unsupported_device(self):
                     mock.Mock(
                         **{
                             "XMLDesc.return_value": PCI_XML.format(
-                                *["1000"] * 3
+                                *["10000"] * 3
                             ),
                             "listCaps.return_value": ["pci"],
                         }

qubes/tests/vm/init.py

@@ -84,7 +84,7 @@ def setUp(self):
             </features>
 
             <devices class="pci">
-                <device backend-domain="domain1" id="00_11.22">
+                <device backend-domain="domain1" id="00_11.2">
                   <option name="no-strict-reset">True</option>
                 </device>
             </devices>
@@ -125,7 +125,7 @@ def test_000_load(self):
 
         self.assertTrue(
             list(vm.devices["pci"].get_assigned_devices())[0].matches(
-                qubes.ext.pci.PCIDevice(Port(vm, "00_11.22", "pci"))
+                qubes.ext.pci.PCIDevice(Port(vm, "00_11.2", "pci"))
             )
         )