PCI cleanup & MSI-X refactoring #5453
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
bchalios
force-pushed
the
pci_cleanup
branch
from
63788e4 to
1d5c3d0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5453 +/- ##
==========================================
+ Coverage 82.75% 82.79% +0.04%
==========================================
Files 263 263
Lines 27307 27226 -81
==========================================
- Hits 22597 22541 -56
+ Misses 4710 4685 -25
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:
|
bchalios
force-pushed
the
pci_cleanup
branch
2 times, most recently
from
72b76f8 to
3ff2084
Compare
bchalios
force-pushed
the
pci_cleanup
branch
4 times, most recently
from
06d8d72 to
51180ec
Compare
bchalios
added
the
Status: Awaiting review
bchalios
force-pushed
the
pci_cleanup
branch
from
51180ec to
30ebf77
Compare
We don't really use the programming interface field of the PCI header. It's always zero for the devices we support. Don't expose it as an argument for constructing PciConfiguration objects. Signed-off-by: Babis Chalios <[email protected]>
Drop PciBarConfiguration type which wasn't used anywhere any more. Signed-off-by: Babis Chalios <[email protected]>
The implementation of the PCI emulation components is heavily Firecracker opinionated. Move those inside a module of `vmm` crate and leave in the `pci` module only type definitions which can easily be reused across VMMs. We also take advantage of this refactoring to remove in various places the usage of `std::io::Error` as error types returned by emulation logic. Signed-off-by: Babis Chalios <[email protected]>
The code we inherited from Cloud Hypervisor was using a single `new` method for creating objects both for booted and snapshot resumed VMs. The `new` method had an optional `state` argument. When this argument was `Some(state)` the construction of the object happened through copying values from `state`. Split this functionality, for various types, in two distinct methods `new` & `from_state` which are only called for booted and snapshotted microVMs respectively. This is similar to the pattern we're using throughout Firecracker, reduces if/else branches, as well as the arguments of distinct methods. Signed-off-by: Babis Chalios <[email protected]>
Our implementation of MSI-X was taken from the equivalent Cloud Hypervisor one. Cloud Hypervisor supports non-KVM hypervisors, hence they introduced a few abstractions to cater for that. More specifically, they are using a trait (`InterruptSourceGroup`) to describe the underlying hypervisor-specific mechanism used to drive MSI-X interrupts from the device to the guest. Moreover, they use a couple of types to distinguish between MSI, MSI-X and legacy IRQ interrupts and how these can be configured. This is especially useful for MSI/MSI-X interrupts where the interrupt configuration is provided by the guest over PCI bus MMIO accesses. Finally, in order to allow for transparent error handling, `Result` types returned by related trait methods was returning `std::io::Error` types as the error type, which is not very intuitive. We don't need all of that. We clearly distinguish between legacy interrupts and MSI-X (we don't use at all MSI), so we can simply plug in all the definitions directly where we need them without any dynamic dispatch indirection. Moreover, various MSI-X components were stored all over the place creating a lot of unnecessary duplication. This commit refactors our implementation to get rid of the unnecessary bit of dynamic dispatch. We still keep the `VirtioInterrupt` trait which we use to abstract the actual interrupt type (IRQ or MSI-X) from VirtIO devices. Moreover, it removes the duplication of MSI-X related types across various types. We only duplicate the MSI-X configuration type (which is behind an Arc) in a few places to avoid needing to unnecessarily take a lock(). Moreover, we introduce a single interrupt related error type and get rid of the `std::io::Error` usage. Finally, it introduces a couple of metrics for interrupts that are useful information by themselves, but also help us maintain some unit testing assertions for which we were relying on a mock type that implemented `InterruptSourceGroup`. Signed-off-by: Babis Chalios <[email protected]>
We are only using the Bus-related definitions from there now. Move those in a module under `vmm` and drop the dependency to `vm-device`. Signed-off-by: Babis Chalios <[email protected]>
bchalios
force-pushed
the
pci_cleanup
branch
from
30ebf77 to
6de3060
Compare
Manciukic
approved these changes
Oct 3, 2025
ShadowCurse
approved these changes
Oct 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This is a follow up to #5426.
This time around, we mainly:
vmmcrate since it is quite opinionated and not necessarily re-usable by other hypervisors. Thepcicrate now only holdsenumdefinitions that are Rust representations of various PCI specification valuesvmmand get rid of thevm-devicevended crate.Reason
A lot of this implementation is hypervisor specific and it fits much better in the Firecracker's binary target rather than a generic PCI crate. This way we can remove a lot of dead code and reduce indirection (dynamic dispatch) as well as improve error handling.
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 checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
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.