ext/pci: implement PCI path for device identification

PCI SBDF may change if _another_ device is added/removed from the
system. Use PCI device path to avoid this issue. The PCI device path is
more or less sysfs path, but with bus number behind bridges replaced
with relative bus number to bridge's secondary bus number. Each
component of the path is separated with '-', as that is allowed in
qrexec arguments.

For example device 0000:c1:00.0 behind bridge at 0000:00:08.1 will get
path 0000_00_08.1-00_00.0 (assuming bridge 0000:00:08.1 has secondary
bus at 0xc1). Resolving device path requires access to the system's
sysfs.

PCIDevice object will automatically resolve old device identifiers,
which partially covers migration path.

TODO:
- finalize migration path - adjust device assignments too
- consider still allowing attaching via SBDF, or otherwise expose function
  to translate SBDF to PCI path (an Admin API call?)
- fix salt listing/attaching devices to sys-net/sys-usb - depending on
  the above

Fixes QubesOS/qubes-issues#8681

Author: marmarek

qubes/ext/pci.py

@@ -44,6 +44,9 @@
 unsupported_devices_warned = set()
 
 
+SYSFS_PCI_DEVS_BASE = "/sys/bus/pci/devices"
+
+
 class UnsupportedDevice(Exception):
     pass
 
@@ -135,25 +138,153 @@ def _device_desc(hostdev_xml):
     )
 
 
+def sbdf_to_path(device_id: str):
+    """
+    Lookup full path for a given device
+
+    :param device_id: sbdf, for example 0000:02:03.0; accepts also libvirt format
+    like 0000_02_03_0
+    :return: converted identifier of None if device is not found
+    """
+    regex = re.compile(
+        r"\A(?:pci_)?((?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"
+    )
+
+    dev_match = regex.match(device_id)
+    if not dev_match:
+        raise ValueError("Invalid device identifier: {!r}".format(device_id))
+    if dev_match["segment"] is not None:
+        segment = dev_match["segment"]
+    else:
+        segment = "0000"
+    if int(dev_match["bus"], 16) == 0:
+        return (f"{segment}_" if segment != "0000" else "") + (
+            f"{dev_match['bus']}_"
+            f"{dev_match['device']}.{dev_match['function']}"
+        )
+    sbdf = (
+        f"{segment}:{dev_match['bus']}:"
+        f"{dev_match['device']}.{dev_match['function']}"
+    )
+    try:
+        sysfs_path = os.readlink(f"{SYSFS_PCI_DEVS_BASE}/{sbdf}")
+    except FileNotFoundError:
+        return None
+    # example: ../../../devices/pci0000:00/0000:00:1a.0/0000:02:00.0
+    path = sysfs_path.partition(f"/pci{segment}:")[2]
+    # drop also 00/ part, which may be also 40/, 80/ etc
+    path = path[3:]
+    bus_offset = 0
+    result_list = []
+    for path_part in path.split("/"):
+        bridge_match = regex.match(path_part)
+        if not bridge_match:
+            raise ValueError("Invalid bridge found: {!r}".format(path_part))
+        assert int(bridge_match["bus"], 16) >= bus_offset
+        bus_num = int(bridge_match["bus"], 16) - bus_offset
+        bridge_str = (
+            f"{bus_num:02x}_{bridge_match['device']}."
+            f"{bridge_match['function']}"
+        )
+        result_list.append(bridge_str)
+        try:
+            with open(
+                f"{SYSFS_PCI_DEVS_BASE}/{path_part}/secondary_bus_number"
+            ) as f:
+                # this one is in decimal
+                # this can raise ValueError, propagate it
+                bus_offset = int(f.read())
+        except FileNotFoundError:
+            # last device in chain, set to None so any further arithmetic
+            # will fail
+            bus_offset = None
+
+    if segment == "0000":
+        return "-".join(result_list)
+    return segment + "_" + "-".join(result_list)
+
+
+def path_to_sbdf(path: str):
+    """
+    Convert device path as done by *sbdf_to_path* back to SBDF
+    :param path:
+    :return:
+    """
+
+    regex = re.compile(
+        r"\A(?P<bus>[0-9a-f]+)[_:]"
+        r"(?P<device>[0-9a-f]+)[._](?P<function>[0-9a-f]+)\Z"
+    )
+
+    segment_re = re.compile(r"\A(?P<segment>[0-9a-f]{4})[_:](?P<rest>.*)\Z")
+
+    # default segment
+    segment = "0000"
+    bus_offset = 0
+    current_dev = ""
+    for path_part in path.split("-"):
+        # first part may include segment
+        if bus_offset == 0:
+            segment_match = segment_re.match(path_part)
+            if segment_match:
+                segment = segment_match["segment"]
+                path_part = segment_match["rest"]
+        part_match = regex.match(path_part)
+        if not part_match:
+            raise ValueError(
+                "Invalid PCI device path at {!r}".format(path_part)
+            )
+        bus_num = int(part_match["bus"], 16) + bus_offset
+        current_dev = (
+            f"{segment}:{bus_num:02x}:{part_match['device']}."
+            f"{part_match['function']}"
+        )
+        try:
+            with open(
+                f"{SYSFS_PCI_DEVS_BASE}/" f"{current_dev}/secondary_bus_number"
+            ) as f:
+                # this one is in decimal
+                # this can raise ValueError, propagate it
+                bus_offset = int(f.read())
+        except FileNotFoundError:
+            # last device in chain, set to None so any further arithmetic
+            # will fail
+            bus_offset = None
+
+    return current_dev
+
+
+def is_pci_path(device_id: str):
+    """Check if given device id is already a device path.
+
+    :param device_id: device id to check
+    :return:
+    """
+    path_re = re.compile(
+        r"\A([0-9a-f]{4}_)?00_[0-9a-f]{2}\.[0-9a-f]"
+        r"(-[0-9a-f]{2}_[0-9a-f]{2}\.[0-9a-f])*\Z"
+    )
+    return bool(path_re.match(device_id))
+
+
 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,
@@ -162,14 +293,24 @@ def __init__(self, port: Port, libvirt_name=None):
 
         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.sbdf = sbdf
+
         for group in self.regex.groupindex:
             setattr(self, group, dev_match.group(group))
+        if self.segment is None:
+            self.segment = "0000"
 
         # lazy loading
         self._description: Optional[str] = None
@@ -249,7 +390,7 @@ def parent_device(self) -> Optional[qubes.device_protocol.DeviceInfo]:
     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):
@@ -380,31 +521,31 @@ def on_device_list_attached(self, vm, event, **kwargs):
             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)
             )
@@ -458,7 +599,7 @@ def on_device_pre_detached_pci(self, vm, event, port):
         ) 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.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.ext.pci import sbdf_to_path, path_to_sbdf
+
+orig_open = open
 
 
 class TestVM(object):
@@ -107,17 +112,55 @@ 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/bus/pci/devices"
+)
+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/bus/pci/devices"
+
+
+@mock.patch("qubes.ext.pci.SYSFS_PCI_DEVS_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()

qubes/tests/vm/qubesvm.py

@@ -1527,6 +1527,7 @@ def test_600_libvirt_xml_hvm_pcidev(self):
             <hostdev type="pci" managed="yes">
                 <source>
                     <address
+                        domain="0x0000"
                         bus="0x00"
                         slot="0x00"
                         function="0x0" />
@@ -1637,6 +1638,7 @@ def test_600_libvirt_xml_hvm_pcidev_s0ix(self):
                 <source
                     powerManagementFiltering="no">
                     <address
+                        domain="0x0000"
                         bus="0x00"
                         slot="0x00"
                         function="0x0" />

rpm_spec/core-dom0.spec.in

@@ -568,6 +568,7 @@ done
 /usr/share/qubes/templates/libvirt/devices/pci.xml
 /usr/share/qubes/templates/libvirt/devices/net.xml
 /usr/share/qubes/tests-data/dispvm-open-thunderbird-attachment
+/usr/share/qubes/tests-data/sysfs
 /usr/lib/tmpfiles.d/qubes.conf
 %attr(0755,root,root) /usr/lib/qubes/create-snapshot
 %attr(0755,root,root) /usr/lib/qubes/destroy-snapshot

templates/libvirt/devices/pci.xml

@@ -12,6 +12,7 @@
 {% endif %}
     >
         <address
+            domain="0x{{ device.segment }}"
             bus="0x{{ device.bus }}"
             slot="0x{{ device.device }}"
             function="0x{{ device.function }}" />

tests-data/sysfs/sys/bus/pci/devices/0000:00:18.4

@@ -0,0 +1 @@
+../../../devices/pci0000:00/0000:00:18.4

tests-data/sysfs/sys/bus/pci/devices/0000:c0:03.5

@@ -0,0 +1 @@
+../../../devices/pci0000:c0/0000:c0:03.5

tests-data/sysfs/sys/bus/pci/devices/0000:c5:00.0

@@ -0,0 +1 @@
+../../../devices/pci0000:c0/0000:c0:03.5/0000:c5:00.0

tests-data/sysfs/sys/bus/pci/devices/0000:c6:00.0

@@ -0,0 +1 @@
+../../../devices/pci0000:c0/0000:c0:03.5/0000:c5:00.0/0000:c6:00.0

tests-data/sysfs/sys/devices/pci0000:c0/0000:c0:03.5/0000:c5:00.0/secondary_bus_number

@@ -0,0 +1 @@
+198