Skip to content

Conversation

@bchalios
Copy link
Contributor

@bchalios bchalios commented Aug 8, 2025

Reason

According to the VirtIO spec section 2.1.2, when we enter a failed state we need to to set DEVICE_NEEDS_RESET status field and if DRIVER_OK is set we need to must send a device configuration change interrupt to the driver.

In MMIO code we were trying to follow this logic, but we were trying to get the interrupt object from the device, which fails (and panics) because interrupts are only set in the device upon successful activation. In the PCI transport, instead, we were not doing this at all.

Changes

For VirtIO devices, the transport layer always holds a reference to the interrupt object of the device. Use this reference in cases that we need to send an interrupt from the transport emulation logic. Moreover, add the missing logic in PCI transport to send a configuration update interrupt to the driver when we enter a failed state due to activation errors.

Also, there are places in VirtIO-MMIO code that takes and releases the virtio device lock multiple times within the same code branch. Change the logic to only do this once.

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 checkbuild --all to verify that the PR passes
    build checks on all supported architectures.
  • 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.

According to the VirtIO spec section 2.1.2, when we enter a failed state
we need to to set `DEVICE_NEEDS_RESET` status field and if `DRIVER_OK`
is set we need to must send a device configuration change interrupt to
the driver.

In MMIO code we were trying to follow this logic, but we were trying to
get the interrupt object from the device, which fails (and panics)
because interrupts are only set in the device upon successful
activation. In the PCI transport, instead, we were not doing this at
all.

The transport layers hold a reference to the interrupts at all times.
So, add the missing logic to the PCI transport and use the transport
layer reference in both transports to avoid panics.

Signed-off-by: Babis Chalios <[email protected]>
MMIO transport layer holds a Mutex to the VirtIO device. Within the
logic that handles the handshake between the driver and the device,
there were cases where we would take and release the lock multiple times
within the same code branch.

Change this to only take the lock once.

Signed-off-by: Babis Chalios <[email protected]>
@bchalios bchalios added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Aug 8, 2025
@roypat roypat enabled auto-merge (rebase) August 8, 2025 09:51
@codecov
Copy link

codecov bot commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.64%. Comparing base (c9b0e64) to head (56830e5).
⚠️ Report is 2 commits behind head on feature/pcie.

Files with missing lines Patch % Lines
src/vmm/src/devices/virtio/transport/pci/device.rs 0.00% 8 Missing ⚠️
src/vmm/src/devices/virtio/transport/mmio.rs 93.75% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           feature/pcie    #5351      +/-   ##
================================================
- Coverage         80.64%   80.64%   -0.01%     
================================================
  Files               265      265              
  Lines             30675    30674       -1     
================================================
- Hits              24738    24737       -1     
  Misses             5937     5937              
Flag Coverage Δ
5.10-c5n.metal 80.43% <62.50%> (+<0.01%) ⬆️
5.10-m5n.metal 80.43% <62.50%> (-0.01%) ⬇️
5.10-m6a.metal 79.63% <62.50%> (-0.01%) ⬇️
5.10-m6g.metal 76.94% <62.50%> (-0.01%) ⬇️
5.10-m6i.metal 80.41% <62.50%> (-0.01%) ⬇️
5.10-m7a.metal-48xl 79.62% <62.50%> (-0.01%) ⬇️
5.10-m7g.metal 76.94% <62.50%> (-0.01%) ⬇️
5.10-m7i.metal-24xl 80.39% <62.50%> (-0.01%) ⬇️
5.10-m7i.metal-48xl 80.39% <62.50%> (-0.01%) ⬇️
5.10-m8g.metal-24xl 76.94% <62.50%> (-0.01%) ⬇️
5.10-m8g.metal-48xl 76.94% <62.50%> (-0.01%) ⬇️
6.1-c5n.metal 80.47% <62.50%> (-0.01%) ⬇️
6.1-m5n.metal 80.47% <62.50%> (-0.01%) ⬇️
6.1-m6a.metal 79.68% <62.50%> (-0.01%) ⬇️
6.1-m6g.metal 76.94% <62.50%> (-0.01%) ⬇️
6.1-m6i.metal 80.46% <62.50%> (-0.01%) ⬇️
6.1-m7a.metal-48xl 79.67% <62.50%> (-0.01%) ⬇️
6.1-m7g.metal 76.94% <62.50%> (-0.01%) ⬇️
6.1-m7i.metal-24xl 80.47% <62.50%> (-0.02%) ⬇️
6.1-m7i.metal-48xl 80.48% <62.50%> (-0.01%) ⬇️
6.1-m8g.metal-24xl 76.94% <62.50%> (-0.01%) ⬇️
6.1-m8g.metal-48xl 76.94% <62.50%> (-0.01%) ⬇️

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.

@roypat roypat merged commit 2e0459c into firecracker-microvm:feature/pcie Aug 8, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Awaiting review Indicates that a pull request is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants