Skip to content

DRAFT: Add support for virtio-pci transport #5240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 18 commits into
base: feature/pcie
Choose a base branch
from

Conversation

bchalios
Copy link
Contributor

@bchalios bchalios commented Jun 2, 2025

Changes

...

Reason

...

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

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 24.30939% with 1507 lines in your changes missing coverage. Please review.

Project coverage is 75.84%. Comparing base (ca4f1e7) to head (9d18f11).

Files with missing lines Patch % Lines
src/vmm/src/devices/virtio/transport/pci/device.rs 0.00% 741 Missing ⚠️
src/vmm/src/device_manager/pci_mngr.rs 8.51% 387 Missing ⚠️
src/vmm/src/vstate/vm.rs 15.12% 174 Missing ⚠️
.../src/devices/virtio/transport/pci/common_config.rs 40.32% 111 Missing ⚠️
src/vmm/src/builder.rs 73.88% 35 Missing ⚠️
src/vmm/src/device_manager/mod.rs 90.59% 11 Missing ⚠️
src/vmm/src/devices/virtio/balloon/persist.rs 8.33% 11 Missing ⚠️
src/vmm/src/devices/virtio/queue.rs 8.33% 11 Missing ⚠️
src/pci/src/msix.rs 0.00% 10 Missing ⚠️
src/vmm/src/devices/virtio/net/device.rs 36.36% 7 Missing ⚠️
... and 6 more

❌ Your project check has failed because the head coverage (75.84%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@               Coverage Diff                @@
##           feature/pcie    #5240      +/-   ##
================================================
- Coverage         79.23%   75.84%   -3.40%     
================================================
  Files               262      265       +3     
  Lines             29347    30945    +1598     
================================================
+ Hits              23254    23469     +215     
- Misses             6093     7476    +1383     
Flag Coverage Δ
5.10-c5n.metal 75.23% <22.08%> (-3.82%) ⬇️
5.10-m5n.metal 75.23% <22.08%> (-3.82%) ⬇️
5.10-m6a.metal 74.27% <22.08%> (-3.91%) ⬇️
5.10-m6g.metal 71.49% <22.77%> (-3.70%) ⬇️
5.10-m6i.metal 75.22% <22.08%> (-3.82%) ⬇️
5.10-m7a.metal-48xl 74.25% <22.08%> (-3.91%) ⬇️
5.10-m7g.metal 71.49% <22.77%> (-3.70%) ⬇️
5.10-m7i.metal-24xl 75.20% <22.08%> (-3.82%) ⬇️
5.10-m7i.metal-48xl 75.19% <22.08%> (-3.82%) ⬇️
5.10-m8g.metal-24xl 71.48% <22.77%> (-3.70%) ⬇️
5.10-m8g.metal-48xl 71.48% <22.77%> (-3.70%) ⬇️
6.1-c5n.metal 75.27% <22.08%> (-3.82%) ⬇️
6.1-m5n.metal 75.27% <22.08%> (-3.82%) ⬇️
6.1-m6a.metal 74.31% <22.08%> (-3.91%) ⬇️
6.1-m6g.metal 71.49% <22.77%> (-3.70%) ⬇️
6.1-m6i.metal 75.27% <22.08%> (-3.82%) ⬇️
6.1-m7a.metal-48xl 74.30% <22.08%> (-3.90%) ⬇️
6.1-m7g.metal 71.49% <22.77%> (-3.70%) ⬇️
6.1-m7i.metal-24xl 75.28% <22.08%> (-3.83%) ⬇️
6.1-m7i.metal-48xl 75.28% <22.08%> (-3.83%) ⬇️
6.1-m8g.metal-24xl 71.48% <22.77%> (-3.70%) ⬇️
6.1-m8g.metal-48xl 71.48% <22.77%> (-3.70%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bchalios bchalios force-pushed the virtio-pci branch 7 times, most recently from a80faa9 to 15ed3eb Compare June 4, 2025 13:49
@bchalios bchalios force-pushed the virtio-pci branch 5 times, most recently from fa1530a to 61c6be1 Compare June 11, 2025 11:09
bchalios added 16 commits June 11, 2025 13:25
We need the new KvmIrqRouting FamStruct wrapper from kvm-bindings, which
though forces us to update vmm-sys-util to 0.14.0 and also bump all
downstream dependencies of vmm-sys-util to use that version.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Define thiserror::Error and displaydoc::Display for various error types
in the vended PCI crate. This way we can embed them in our error types
downstream. Also export a few types and struct fields that were private
and we will be needing them.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Instead of returning an `EventFd` type, which will actually force us to
clone the file descriptor in the Firecracker side.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
We'd like to be able to store Vm within an atomic reference so we can
pass it around and share it with other components. The main issue with
doing this change is that we need Vm to be `mut` during initialization
and the builder.rs code was creating Vmm with Vm embedded in it.

To solve this, we break down the initialization of the Vmm object. We
first create its individual parts (Vm, Kvm and DeviceManager), perform
any necessary initialization logic on Vm and once this done add it
within an Arc.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Add logic to track the device interrupts used by the microVM. This is
not strictly needed right now, but we will need it when adding support
for MSI-X interrupts. MSI-X interrupts are configured at runtime and we
need to interact with KVM to set the interruput routes. To do it, we
need to keep track all of the interrupts the VM is using.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Enable Vm to vend and manage MSI/MSI-X interrupts. This adds the logic
to create a set of MSI vectors and then handle their lifetime.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Vm object is now maintaining information about the interrupts (both
traditional IRQs and MSI-X vectors) that are being used by microVM
devices. Extend snapshot format to save this information during
snapshotting.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Add a VirtIO PCI transport implementation. Nothing uses it at the
moment. This requires a few changes in our vended pci and vm-device
crates.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Merge the device-related errors that DeviceManager might return. This
way, we can avoid adding yet another error type for PCI devices and
reduce some the variants of StartMicrovmError.

Suggested-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Signed-off-by: Babis Chalios <bchalios@amazon.es>
Commit d8c2714 (refactor: use VirtioInterrupt in VirtIO devices) which
refactored devices to use new VirtioInterrupt type introduced a bug
with the index used to trigger a queue interrupt. Instead of using the
actual queue index, we were using the index of the used descriptor.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Apparently, PCI needs Queue::size to be initialized to the maximum
possible size supported by the device, otherwise initialization fails.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Remove the flags in FADT that were declaring we do not support MSI and
PCI ASPM.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Create VirtIO devices using the PCI transport layer when user launched
microVM with --enable-pci.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
We are now calling KVM_CHECK_EXTENSION for checking the
KVM_CAP_MSI_DEVID capability. We are also calling KVM_SET_GSI_ROUTING to
set the interrupts routes and KVM_IRQFD to set/unset interrupt lines.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Add some unit tests to PciSegment. We now test that the
next_device_bdf() method and the initialization logic work as expected.
We also check that the configuration space of the PCI segment is
correctly registered with the MMIO and, on x86, PIO bus.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
vm-allocator now allows us to (De)serialize IdAllocator and
AddressAllocator types. Add ResourceAllocator in DeviceManager snapshot
state and restore it when loading a snapshot. Like this we can avoid
doing the ExactMatch allocations during snapshot resumes for reserving
the exact same MMIO ranges.

Moreover, change DeviceManager and PciDevices to provide save/restore
functionality via the Persist trait. Like that we can avoid first
creating the objects and then restoring their state, overwriting their
fields.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
bchalios added 2 commits June 11, 2025 13:25
VirtIO MMIO restore logic activates the device the moment we restore the
device state, if the device was activated when snapshotted. Move the
activation responsibility to the logic the restores the MMIO transport.
The reason for this change is that that's how it will be done for the
PCI transport. Unifying this will allow us reusing the same types for
restoring the non-transport state of devices.

Note that we needed to change the way Net devices are saved/restored.
RxBuffer type of Net devices holds RX descriptors that we have parsed
from the Queue ahead of time. The way we restored this info was
manipulating the queue to re-parse the RX descriptors during the restore
phase. However, we need the device to be activated to do so, which now
isn't. So, instead of storing this info inside the snapshot make sure we
have flushed everything before taking the snapshot.

Also, simplify a bit the types that we use for serializing/deserializing
the state of a device.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Support serializing the device-specific and transport state of a VirtIO
device that uses the PCI transport.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant