Simplify PCI code #5426
Simplify PCI code #5426
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5426 +/- ##
==========================================
+ Coverage 82.68% 82.79% +0.11%
==========================================
Files 263 263
Lines 27478 27301 -177
==========================================
- Hits 22719 22603 -116
+ Misses 4759 4698 -61
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_refactoring
branch
from
cbe3edc to
f34198a
Compare
bchalios
added
the
Status: Awaiting review
bchalios
force-pushed
the
pci_refactoring
branch
3 times, most recently
from
ebc9a7a to
1fe2da9
Compare
There was a problem hiding this comment.
I think we should understand what is the trade-off between keeping with future-proof logic in the code or remove it altogether and add it back if in the future we add support for vfio.
Also, I dislike the use of asserts in library code, which makes it more coupled with the code itself. I think error handling should be done in the binary crate. But this is a stylistic preference, I wouldn't block the CR on this right now. This will however hinder any upstreaming efforts in the future, if any.
Sure, let's have the discussion. I'm pretty sure that there are things that we won't use whatsoever, e.g. ROM BAR, I/O type of BARs.
I completely agree! Ultimately, I'd like to move all the logic that handles devices, the bus, MSI-X and corresponding configuration under |
I'm not sure how much of that is used in practice with modern devices (probably you're right that ROM and IO are not used), but the current vfio code handles all types of BARs: https://github.com/Manciukic/firecracker/blob/poc/pcie/src/pci/src/vfio.rs#L548
gotcha! |
PciDevice trait was defining (optional) methods to allocate and deallocate BAR regions for devices. This logic is quite device specific and when invoked we typically know what device it refers to exactly. Currently, we only implement the allocate API for VirtIO devices (no deallocation whatsoever). Simplify things by dropping these APIs from the trait definition. Also, remove the 32bit allocator argument from allocate_bars(); we always only allocate a single 64-bit BAR for VirtIO devices. Signed-off-by: Babis Chalios <bchalios@amazon.es>
In our case, only VirtIO PCI devices use a BAR region. Moreover, this region has a fixed size and it's always a 64-bit region. This means we don't handle 32-bit or IO BARs and we don't support a ROM bar. Encode these assumptions to the logic that adds BAR regions and detects BAR reprogramming attempts from the guest side so that we simplify the code. Signed-off-by: Babis Chalios <bchalios@amazon.es>
Drop any ROM-related state we were holding in PciConfiguration and ensure we are always returning a side of zero for the ROM BAR. Signed-off-by: Babis Chalios <bchalios@amazon.es>
We only ever create type-0 (device) PCI configurations. The code that was handling Bridge types was effectively dead code. Drop it. Signed-off-by: Babis Chalios <bchalios@amazon.es>
Every time we were handling an MMIO write within a VirtIO device's BAR we were logging a debug-level message when the write was not leading to a device activation. This leads to way too verbose Firecracker logs up until a device was set up properly. These logs don't really provide any useful debugging information, so just drop it to keep logs readable. Signed-off-by: Babis Chalios <bchalios@amazon.es>
bchalios
force-pushed
the
pci_refactoring
branch
from
1fe2da9 to
e8b0ef4
Compare
This is the first in a series of PRs that are meant to simplify our PCI codebase
Changes
Mainly two changes:
PciDevicetraitReason
A lot of the PCI code we pulled from Cloud Hypervisor is built in the form of a library trying to be generic and support various functionalities of the PCI specification.a
In our case, we only support certain functionality. For example, we only have two types of PCI devices, VirtIO devices and the Root Port. Only VirtIO devices use BAR regions (64 bits). We don't use 32-bit, I/O or ROM BARs.
We'd like to move towards a PCI implementation which is tailored to our use-case instead of a generic library implementation.
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.