Miscellaneous PCI fixes and more test coverage #5300
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/pcie #5300 +/- ##
================================================
- Coverage 80.20% 80.16% -0.05%
================================================
Files 265 265
Lines 30832 30863 +31
================================================
+ Hits 24730 24741 +11
- Misses 6102 6122 +20
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:
|
Manciukic
force-pushed
the
pcie/run-all-tests-on-pcie
branch
7 times, most recently
from
9f992ac to
fcaff05
Compare
10 tasks
Manciukic
force-pushed
the
pcie/run-all-tests-on-pcie
branch
3 times, most recently
from
104d1e1 to
f1dd2b4
Compare
The vmm was only checking the mmio device manager for finding the device to update. Use the generic device manager instead. Also update unit tests that expect a specific string. Signed-off-by: Riccardo Mancini <[email protected]>
The code managing the balloon logic is only looking at the mmio device manager. Make it use the generic device manager to find the device. Signed-off-by: Riccardo Mancini <[email protected]>
The device rename wasn't working on PCI devices because the code only checked the MMIO state. Fix the bug by looking for the device to rename in both the mmio and pci states. Signed-off-by: Riccardo Mancini <[email protected]>
Manciukic
force-pushed
the
pcie/run-all-tests-on-pcie
branch
8 times, most recently
from
56bb419 to
43484d5
Compare
Manciukic
added
the
Status: Awaiting review
ShadowCurse
reviewed
Jul 15, 2025
Manciukic
force-pushed
the
pcie/run-all-tests-on-pcie
branch
from
43484d5 to
e6e1683
Compare
Currently, we're limited to 24 GSI lines, which is too little for PCI devices. Keep the current ranges as "legacy GSI", and create a new range for "MSI GSI" that goes up to the kvm theoretical maximum of 4096 lines. Signed-off-by: Riccardo Mancini <[email protected]>
To have a more consistent naming, it's best to use GSI instead of IRQ, at least in places where it's meant just as an abstract index. Signed-off-by: Riccardo Mancini <[email protected]>
This patch makes 2 changes to make the test work on PCI: - simplify logic to find device address to be generic irrespective of ACPI/no-ACPI, PCI/no-PCI - move config offset from within the C program to the python test, as it's different between MMIO (0x100) and PCI (0x4000) Signed-off-by: Riccardo Mancini <[email protected]>
Tell systemd not to use "predictable names" for network devices (eg enp0s1), but keep the ethN set by the kernel. This is equivalent to passing net.ifnames=0 to the kernel command line. Signed-off-by: Riccardo Mancini <[email protected]>
pci=off is just an optimization to skip the probing, it shouldn't matter to the functionality of the tests. Dropping it to allow them to run with PCI. Signed-off-by: Riccardo Mancini <[email protected]>
All tests using uvm_plain or uvm_plain_any will start using PCI as well, allowing more coverage for the PCI code. This requires moving the PCI configuration to the VM factory from the spawn method. Signed-off-by: Riccardo Mancini <[email protected]>
This patch updates all the places in the code with a uvm_plain* fixture when that was equivalent to the previous behaviour. In particular: - microvm_factory.build(guest_kernel_linux_5_10, rootfs) => uvm_plain - microvm_factory.build(guest_kernel, rootfs) => uvm_plain_any Signed-off-by: Riccardo Mancini <[email protected]>
Simplify the test code by introducing two new fixtures that are used in a few places in the code. This will also allow these tests to run on PCI. Signed-off-by: Riccardo Mancini <[email protected]>
Run the test_run_concurrency with PCI enabled as well. Signed-off-by: Riccardo Mancini <[email protected]>
Refactor the code to use common fixtures and run all the tests with PCI enabled as well. Signed-off-by: Riccardo Mancini <[email protected]>
Run the initrd tests also with PCI enabled to verify everything is still working correctly. Signed-off-by: Riccardo Mancini <[email protected]>
Run test_memory_overhead performance test also with PCI enabled. Signed-off-by: Riccardo Mancini <[email protected]>
Run the restore latency tests also with PCI enabled to verify there is no change. Signed-off-by: Riccardo Mancini <[email protected]>
Manciukic
force-pushed
the
pcie/run-all-tests-on-pcie
branch
from
e6e1683 to
f4e07c7
Compare
Manciukic
changed the title
roypat
approved these changes
Jul 16, 2025
ShadowCurse
approved these changes
Jul 16, 2025
roypat
merged commit ba962ba
into
firecracker-microvm:feature/pcie
6 of 7 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Fix miscellaneous bugs introduced by PCI:
Also, add additional test coverage that would have caught the bugs above by moving the PCI configuration to the VM factory so that we can have uvm_plain* fixtures also run the tests on PCIe kernels.
Additionally, also run more tests on PCI: ideally all tests should be run on PCI, unless there's a specific reason to.
Reason
Not all tests were running on PCIe, hiding some issues.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.