Skip to content

Conversation

bchalios
Copy link
Contributor

@bchalios bchalios commented Jul 29, 2025

Changes

Various fixes in the PCI code:

  • Relax a redundant assertion in MSI code. The condition of the assertion is anyways checked by the code. When the condition is true, we print an error message.
  • Get rid of a HashMap that was tracking objects index by integers starting from 0. Use a Vec instead.
  • Fix the size of the HashMap used to track interrupt routes to the maximum possible interrupt number (considering IRQs and MSIs) we can have.
  • Validate the queue index selected by guest VirtIO code when using the PCI transport.

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.

@bchalios bchalios added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jul 29, 2025
@codecov
Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.35%. Comparing base (d02902c) to head (d93aecf).
⚠️ Report is 6 commits behind head on feature/pcie.

Files with missing lines Patch % Lines
src/pci/src/msix.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           feature/pcie    #5334      +/-   ##
================================================
+ Coverage         80.23%   80.35%   +0.12%     
================================================
  Files               265      265              
  Lines             30817    30821       +4     
================================================
+ Hits              24725    24767      +42     
+ Misses             6092     6054      -38     
Flag Coverage Δ
5.10-c5n.metal 80.11% <88.46%> (+0.13%) ⬆️
5.10-m5n.metal 80.11% <88.46%> (+0.13%) ⬆️
5.10-m6a.metal 79.31% <88.46%> (+0.13%) ⬆️
5.10-m6g.metal 76.63% <88.46%> (+0.20%) ⬆️
5.10-m6i.metal 80.11% <88.46%> (+0.13%) ⬆️
5.10-m7a.metal-48xl 79.30% <88.46%> (+0.14%) ⬆️
5.10-m7g.metal 76.63% <88.46%> (+0.20%) ⬆️
5.10-m7i.metal-24xl 80.08% <88.46%> (+0.13%) ⬆️
5.10-m7i.metal-48xl 80.08% <88.46%> (+0.13%) ⬆️
5.10-m8g.metal-24xl 76.63% <88.46%> (+0.20%) ⬆️
5.10-m8g.metal-48xl 76.63% <88.46%> (+0.20%) ⬆️
6.1-c5n.metal 80.15% <88.46%> (+0.13%) ⬆️
6.1-m5n.metal 80.15% <88.46%> (+0.14%) ⬆️
6.1-m6a.metal 79.36% <88.46%> (+0.14%) ⬆️
6.1-m6g.metal 76.63% <88.46%> (+0.20%) ⬆️
6.1-m6i.metal 80.15% <88.46%> (+0.13%) ⬆️
6.1-m7a.metal-48xl 79.35% <88.46%> (+0.14%) ⬆️
6.1-m7g.metal 76.63% <88.46%> (+0.20%) ⬆️
6.1-m7i.metal-24xl 80.16% <88.46%> (+0.13%) ⬆️
6.1-m7i.metal-48xl 80.16% <88.46%> (+0.13%) ⬆️
6.1-m8g.metal-24xl 76.63% <88.46%> (+0.20%) ⬆️
6.1-m8g.metal-48xl 76.63% <88.46%> (+0.20%) ⬆️

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 previously approved these changes Jul 29, 2025
roypat
roypat previously approved these changes Jul 29, 2025
bchalios added 6 commits July 29, 2025 17:45
Probably missed during a rebase of `feature/pcie` on top of `main`
branch.

Signed-off-by: Babis Chalios <[email protected]>
It is true that writes/reads of an MSI-X table are either 32 or 64 bits
long. However, we do check for this invariant in the `match` expression
just after the assertion. If the invariant is not held (the guest tried
to read/write with an invalid length) we just print an error and
continue. This branch of the `match` block is never reached due to the
assertion itself.

To simplify things, just remove the assertion and let the `match` block
logic handle invalid memory accesses. This should also help us better
fuzz the bus accesses.

Do add a check that the data access is up to 8 bytes long. These are
all MMIO or Port IO accesses and they can't be bigger than 8 bytes. So
this assertion should never fail in production (unless there's a KVM
bug or we try to run Firecracker in some architecture that allows more
than 64bit memory accesses).

Signed-off-by: Babis Chalios <[email protected]>
We were using a Hashmap to store the GSIs that were used by the vectors
of an MSI-X group. These vectors were always indexed starting by 0, so
we can just use a simple Vec.

Signed-off-by: Babis Chalios <[email protected]>
We are using a HashMap to track the interrupt routes we use in the
system. The index to the HashMap is the GSI of the interrupt route.
We know the maximum number of GSIs we have available so pre-allocate the
space for the HashMap to avoid reallocations at runtime.

Signed-off-by: Babis Chalios <[email protected]>
Fix a bug in the common VirtIO configuration for PCI transport where we
would use `queue_select` to read the queue's MSI vector without
validating it matches a valid queue. This could lead in panics when
accessing the `msix_queue` array. The spec states that in such cases we
should return `NO_VECTOR` (0xffff), so do that.

Signed-off-by: Babis Chalios <[email protected]>
We were shifting by the wrong number of bits for 2-byte accesses.

Signed-off-by: Babis Chalios <[email protected]>
@bchalios bchalios merged commit 5421d0c into firecracker-microvm:feature/pcie Jul 29, 2025
6 of 7 checks passed
@bchalios bchalios deleted the msix_assertions branch July 30, 2025 09:37
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