ext/pci: implement PCI path for device identification #667
Conversation
This goes further in the direction of d2d2ad8 "pci: get device class out of libvirt xml".
PCI device function is just one hex digit.
The issue was about device with segment part exceeding 16 bits, which technically is not spec compliant. Adjust the test to be about a device that was problematic initially. QubesOS/qubes-issues#6932
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #667 +/- ##
==========================================
+ Coverage 70.14% 70.31% +0.16%
==========================================
Files 59 59
Lines 12726 12819 +93
==========================================
+ Hits 8927 9014 +87
- Misses 3799 3805 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025041403-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025031804-4.3&flavor=update
Failed tests16 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/132953#dependencies 10 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:16 performance degradations
Remaining performance tests:56 tests
|
|
Has this been tested on real hardware? I like the idea but I think it would be best to make sure that it actually works.
I read this as:
What is the something? |
See issue description, commit message, or simply this PR description. But yes, this needs to be also added to a documentation.
Yes. |
DemiMarie
left a comment
DemiMarie
left a comment
There was a problem hiding this comment.
Have you checked that the PCI path stays stable when other devices behind the same bridge or other bridges are added or removed?
| segment_match = segment_re.match(path_part) | ||
| if segment_match: | ||
| segment = segment_match["segment"] | ||
| path_part = segment_match["rest"] |
There was a problem hiding this comment.
No need for a regex match here. Just use [5:].
There was a problem hiding this comment.
That would be very easy to miss if the regex changes in the future.
qubes/utils.py
Outdated
| segment = dev_match["segment"] | ||
| else: | ||
| segment = "0000" | ||
| if int(dev_match["bus"], 16) == 0: |
There was a problem hiding this comment.
| if int(dev_match["bus"], 16) == 0: | |
| if dev_match["bus"] == "00": |
qubes/utils.py
Outdated
| # example: ../../../devices/pci0000:00/0000:00:1a.0/0000:02:00.0 | ||
| path = sysfs_path.partition(f"/pci{segment}:")[2] |
There was a problem hiding this comment.
I suggest following each .. component to make sure that one does reach /sys/devices/pci{segment}.
| bus_num = int(bridge_match["bus"], 16) - bus_offset | ||
| bridge_str = ( | ||
| f"{bus_num:02x}_{bridge_match['device']}." |
There was a problem hiding this comment.
| bus_num = int(bridge_match["bus"], 16) - bus_offset | |
| bridge_str = ( | |
| f"{bus_num:02x}_{bridge_match['device']}." | |
| bridge_str = ( | |
| f"{bridge_match["bus"]}_{bridge_match['device']}." |
No need to go from string to int and back.
There was a problem hiding this comment.
Nope, you missed the key part: - bus offset
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. As a side effect, this adds also support for multi-segment systems, PCI segment is no longer hardcoded to 0000. PCIDevice object will automatically resolve old device identifiers, which partially covers migration path. The other part is converting BDF to device path when loading device assignments. Tests include subset of sysfs from a real system that has multiple bridges. TODO: - 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
3 participants
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:
to translate SBDF to PCI path (an Admin API call?)
the above
Fixes QubesOS/qubes-issues#8681