PCI: prepare for VFIO by ShadowCurse · Pull Request #5758 · firecracker-microvm/firecracker

ShadowCurse · 2026-03-13T12:49:44Z

Changes

Main change here is the detachment of BAR and MsixConfig handling from the PciConfiguration type. BARs and MsixConfig will be used as separate components by the VFIO, so to prevent duplication, move them away from PciConfiguration.

Reason

Preparation for VFIO

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

This functionality cannot be added in rust-vmm.

codecov · 2026-03-13T15:19:48Z

Codecov Report

❌ Patch coverage is 75.60976% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.91%. Comparing base (8b55c86) to head (f229ae5).

Files with missing lines	Patch %	Lines
src/vmm/src/pci/configuration.rs	80.00%	11 Missing ⚠️
src/vmm/src/pci/msix.rs	0.00%	10 Missing ⚠️
src/vmm/src/devices/virtio/transport/pci/device.rs	85.71%	8 Missing ⚠️
src/vmm/src/device_manager/pci_mngr.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5758      +/-   ##
==========================================
- Coverage   83.00%   82.91%   -0.09%     
==========================================
  Files         275      275              
  Lines       29383    29364      -19     
==========================================
- Hits        24388    24348      -40     
- Misses       4995     5016      +21

Flag	Coverage Δ
5.10-m5n.metal	`83.22% <75.60%> (-0.10%)`	⬇️
5.10-m6a.metal	`82.54% <75.60%> (-0.10%)`	⬇️
5.10-m6g.metal	`79.79% <75.60%> (-0.10%)`	⬇️
5.10-m6i.metal	`83.22% <75.60%> (-0.10%)`	⬇️
5.10-m7a.metal-48xl	`82.53% <75.60%> (-0.11%)`	⬇️
5.10-m7g.metal	`79.79% <75.60%> (-0.10%)`	⬇️
5.10-m7i.metal-24xl	`83.19% <75.60%> (-0.10%)`	⬇️
5.10-m7i.metal-48xl	`83.19% <75.60%> (-0.10%)`	⬇️
5.10-m8g.metal-24xl	`79.79% <75.60%> (-0.10%)`	⬇️
5.10-m8g.metal-48xl	`79.79% <75.60%> (-0.10%)`	⬇️
5.10-m8i.metal-48xl	`83.20% <75.60%> (-0.10%)`	⬇️
5.10-m8i.metal-96xl	`83.19% <75.60%> (-0.10%)`	⬇️
6.1-m5n.metal	`83.25% <75.60%> (-0.10%)`	⬇️
6.1-m6a.metal	`82.57% <75.60%> (?)`
6.1-m6g.metal	`79.79% <75.60%> (-0.10%)`	⬇️
6.1-m6i.metal	`83.25% <75.60%> (-0.09%)`	⬇️
6.1-m7a.metal-48xl	`82.56% <75.60%> (-0.10%)`	⬇️
6.1-m7g.metal	`79.79% <75.60%> (-0.11%)`	⬇️
6.1-m7i.metal-24xl	`83.26% <75.60%> (-0.09%)`	⬇️
6.1-m7i.metal-48xl	`83.26% <75.60%> (-0.10%)`	⬇️
6.1-m8g.metal-24xl	`79.78% <75.60%> (-0.11%)`	⬇️
6.1-m8g.metal-48xl	`79.79% <75.60%> (-0.10%)`	⬇️
6.1-m8i.metal-48xl	`83.26% <75.60%> (-0.10%)`	⬇️
6.1-m8i.metal-96xl	`83.26% <75.60%> (-0.10%)`	⬇️

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.

Manciukic · 2026-03-16T16:31:16Z

src/vmm/src/devices/virtio/transport/pci/device.rs

-        } else {
-            self.configuration
-                .write_config_register(reg_idx, offset, data);
+        if BAR0_REG as usize <= reg_idx && reg_idx < (BAR0_REG + NUM_BAR_REGS) as usize {


nit: would a match or a if/else if/else be more clear in this case?

if self.cap_pci_cfg_info.offset would be changed to be a register and not a byte offset, then this can be a match. But I did not want to touch unrelated code here. In general, I think we should agree on if we use registers or byte_offsets in the PCI code. I can do a follow up PR with this (or push it into #5752).

Can we define 3 variables (or use functions/macros) and do if in_bar_range {} else if in_cap_pci_cfg_range {} else {}?

done in the last commit

Manciukic · 2026-03-16T16:49:47Z

src/vmm/src/pci/configuration.rs

+        if data.len() == 4 {
+            if bar.about_to_be_read {
+                data.copy_from_slice(bar.size.as_bytes());
+                bar.about_to_be_read = false;


I'm not sure this is exactly equivalent as the current code, where we actually have 2 fields, the current programmed addr and the register value. When the guest writes all 1s, only those beyond the size mask are persisted and the reprogramming detection is not kicked. When the guest writes something else, then we store the actual value and detect if the BAR was moved. How does reprogram detection works here?

The logic here is to always ignore writes (unless it is a size reading write). This makes the BAR relocation be ignored. If the driver will try to relocate it, it will write to the BAR (and this write will be ignored) and then it will read the addr back to validate the write. Since we ignore it, it will read the old addr and assume that the relocation failed. At least Linux kernel does this: https://elixir.bootlin.com/linux/v6.19.8/source/drivers/pci/setup-res.c#L107.

Basically I tried to minimize the logic here to only handle the sizing write since this is all we can currently support. The BAR relocation (or even resizing) can be implemented later (and it will be a bit different for virtio and vfio devices anyway)

src/vmm/src/pci/configuration.rs

ilstam · 2026-04-02T09:49:05Z

src/vmm/src/devices/virtio/transport/pci/device.rs

        self.configuration.detect_bar_reprogramming(reg_idx, data)
    }

-    fn move_bar(&mut self, old_base: u64, new_base: u64) -> Result<(), DeviceRelocationError> {


We did not get a dead code warning?

And can this be merged with "pci: virtio: refactor: remove BAR handling from PciConfiguration"?

I removed this commit in general as move_bar is actually a trait function

ilstam · 2026-04-02T10:40:58Z

src/vmm/src/pci/configuration.rs

+///   Bits 31:4: Base Address (read/write, writable bits depend on size)
+#[derive(Debug, Default, Clone, Copy, Serialize, Deserialize)]
+pub struct Bar {
+    /// Encoded address value of the register.


nit: This comment could clarify what 'encoded' means in this case (lower bit carrying information)

ilstam · 2026-04-02T10:42:09Z

src/vmm/src/pci/configuration.rs

+    /// Encoded size value of the register.
+    pub encoded_size: u32,
+    /// Indicator if the register was prepared to be read as the `size` instead of `addr`
+    pub about_to_be_read: bool,


Would size_read be more descriptive?

For me size_read sounds like: "we did read of the size and here it is".

ilstam · 2026-04-02T11:14:06Z

src/vmm/src/pci/configuration.rs

+        if let Ok(value) = u32::read_from_bytes(data)
+            && value == 0xffff_ffff
+        {
+            assert!(offset == 0);


Should we assert here()? Can the guest trigger this?

these read/write functions supposed to only be called by internal code. And that internal code will need to ensure that guest provided values make sense.

ilstam · 2026-04-02T11:15:31Z

src/vmm/src/pci/configuration.rs

+            assert!(offset == 0);
+            self.bars[bar_idx as usize].about_to_be_read = true;
+        } else {
+            // No BAR relocation/resizing support. Ignore write.


Can we add a comment saying that the PCIe spec doesn't offer any way to reject this, but in practice Linux checks if it succeeded?

ilstam · 2026-04-02T11:19:59Z

src/vmm/src/pci/configuration.rs

+                data.copy_from_slice(bar.encoded_addr.as_bytes());
+            }
+        } else {
+            assert!(offset < 4);


Again, do these asserts here mean that a "wrong" guest read will trigger a crash?

same as previous comment

ilstam · 2026-04-02T11:21:49Z

src/vmm/src/pci/configuration.rs

    registers: [u32; NUM_CONFIGURATION_REGISTERS],
    writable_bits: [u32; NUM_CONFIGURATION_REGISTERS], // writable bits for each register.
-    bars: [PciBar; NUM_BAR_REGS],
+    bars: [PciBar; NUM_BAR_REGS as usize],


Can we avoid changing this type to minimize this diff? Otherwise do it in a separate patch, that patch is already quite long.

split into multiple commits

ilstam · 2026-04-02T11:25:03Z

src/vmm/src/devices/virtio/transport/pci/device.rs

+            VIRTIO_BAR_INDEX,
            virtio_pci_bar_addr,
            CAPABILITY_BAR_SIZE,
+            false,


nit: Perhaps an enum type Prefetchable / Non-prefetchable would be more descriptive and avoid the comment above?

I would use enum if there were more than 2 options, but here it does not seem like it would change much.

ilstam · 2026-04-02T13:13:06Z

src/vmm/src/pci/configuration.rs

+    /// BARs
+    pub bars: [Bar; 6],
+}
+impl Bars {


I would like this patch to get smaller so that it's easier to review and argue about what it's doing. Can we have a patch that adds the new implementation along with the tests (and tell clippy to ignore dead code temporarily if needed)? And then in future commits we swap to using it?

did the split

ilstam · 2026-04-02T13:21:01Z

src/vmm/src/devices/virtio/transport/pci/device.rs

-        } else {
-            self.configuration
-                .write_config_register(reg_idx, offset, data);
+        if BAR0_REG as usize <= reg_idx && reg_idx < (BAR0_REG + NUM_BAR_REGS) as usize {


So this overlaps with the code in PciConfigMmio::config_space_write()? The commit title only talks about PciConfiguration. I would prefer it to be more explicit about this overlap.

ilstam · 2026-04-02T13:24:24Z

src/vmm/src/devices/virtio/transport/pci/device.rs

-        } else {
-            self.configuration
-                .write_config_register(reg_idx, offset, data);
+        if BAR0_REG as usize <= reg_idx && reg_idx < (BAR0_REG + NUM_BAR_REGS) as usize {


Can we define 3 variables (or use functions/macros) and do if in_bar_range {} else if in_cap_pci_cfg_range {} else {}?

ilstam · 2026-04-02T13:37:50Z

src/vmm/src/pci/bus.rs

@@ -550,10 +541,10 @@ mod tests {

        fn detect_bar_reprogramming(


Why don't we remove this?

this is not really about VFIO preparation. I can remove this in another PR if you want.

ilstam · 2026-04-02T13:50:05Z

src/vmm/src/devices/virtio/transport/pci/device.rs

        self.configuration.detect_bar_reprogramming(reg_idx, data)
    }

-    fn move_bar(&mut self, old_base: u64, new_base: u64) -> Result<(), DeviceRelocationError> {


And can this be merged with "pci: virtio: refactor: remove BAR handling from PciConfiguration"?

ilstam · 2026-04-02T14:04:34Z

src/vmm/src/devices/virtio/transport/pci/device.rs

            cap_pci_cfg_info: VirtioPciCfgCapInfo::default(),
            bars: Bars::default(),
+            msix_config,
+            msix_config_offset: 0,


I think from this name it's ambiguous whether this is a capability offset or a BAR offset for the MSI-x table.

renamed to msix_config_cap_offset

ilstam · 2026-04-02T15:37:14Z

src/vmm/src/devices/virtio/transport/pci/device.rs

-            let base = reg_idx as usize * 4;
-            if base + offset as usize >= self.cap_pci_cfg_info.offset as usize
-                && base + offset as usize + data.len()
+            // Handle access to the header of the MsixCapability. The rest of the


As discussed offline I find this a bit weird so I'll wait to hear for more opinions on this.

For some reason we had 2 constants that we used interchangeably to specify the BAR for virtio device. Use the more reasonably named one and delete the old one. Also delete VIRTIO_SHM_BAR_INDEX since it is never used. No functional changes. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Add a new Bars type that handles basic interactions with PCI BAR registers. This type is separate from PciConfiguration and can be reused for future VFIO work. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Change BAR0_REG and NUM_BAR_REGS u8. Update PciConfiguration code and tests to use the new types. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Make VirtioPciDevice use the new Bars type instead of relying on PciConfiguration for BAR handling. This separates BAR management from PCI configuration space, making PciConfiguration unaware of BARs. The VirtioPciDevice now directly handles BAR reads/writes and uses Bars for address tracking instead of a bare bar_address field. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Previous commit created a separate `Bars` type to handle BARs region and made VirtioPciDevice type to use it instead of PciConfiguration to handle BARs region. This made BARs handling in PciConfiguration redundant, so this commit removes it. This also removes `detect_bar_reprogramming` support from PciConfiguration. But Firecracker does not support BAR relocation anyway, this does not affect functionality. The removal of `detect_bar_reprogramming` also required to remove the unit test for it in the pci/bus.rs. No functional change. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Following previous commits notion, move MsixConfig handling out of PciConfiguration into VirtioPciDevice. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

This constant is only used as u16, so change it's type from u8 to u16 to avoid unnecessary conversions. Also rename with `_IDX` to have similarity with `reg_idx` we use in PCI code. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Move the comparisons into boolean vars to make dispatch code a bit more readable. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

ShadowCurse self-assigned this Mar 13, 2026

ShadowCurse added the Type: Enhancement Indicates new feature requests label Mar 13, 2026

ShadowCurse force-pushed the pci_prepare_for_vfio branch 4 times, most recently from 07cd55a to 8e74111 Compare March 13, 2026 15:13

ShadowCurse marked this pull request as ready for review March 13, 2026 15:41

ShadowCurse added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Mar 13, 2026

Manciukic reviewed Mar 16, 2026

View reviewed changes

ShadowCurse force-pushed the pci_prepare_for_vfio branch 5 times, most recently from 8b1b015 to 8050f34 Compare March 26, 2026 09:41

ShadowCurse force-pushed the pci_prepare_for_vfio branch 2 times, most recently from dda34d8 to a615239 Compare March 31, 2026 11:08

ilstam reviewed Apr 2, 2026

View reviewed changes

ShadowCurse force-pushed the pci_prepare_for_vfio branch 2 times, most recently from 4e30a8d to 473e3ff Compare April 2, 2026 13:42

ilstam reviewed Apr 2, 2026

View reviewed changes

ShadowCurse force-pushed the pci_prepare_for_vfio branch 4 times, most recently from 512fc75 to 1754d4a Compare April 2, 2026 16:45

ShadowCurse added 3 commits April 2, 2026 17:45

pci: add Bars type and associated unit tests

bf0c4db

Add a new Bars type that handles basic interactions with PCI BAR registers. This type is separate from PciConfiguration and can be reused for future VFIO work. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

pci: change BAR0_REG and NUM_BAR_REGS types

6c7bd6d

Change BAR0_REG and NUM_BAR_REGS u8. Update PciConfiguration code and tests to use the new types. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

ShadowCurse added 4 commits April 2, 2026 17:45

pci: virtio: refactor: move MsixConfig interaction to VirtioPciDevice

8483a93

Following previous commits notion, move MsixConfig handling out of PciConfiguration into VirtioPciDevice. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

pci: refactor: make BAR0_REG_IDX u16

7c53d11

This constant is only used as u16, so change it's type from u8 to u16 to avoid unnecessary conversions. Also rename with `_IDX` to have similarity with `reg_idx` we use in PCI code. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

ShadowCurse force-pushed the pci_prepare_for_vfio branch from 1754d4a to 7c53d11 Compare April 2, 2026 16:45

pci: refactor: make read/write_config_register a bit easier to read

f229ae5

Move the comparisons into boolean vars to make dispatch code a bit more readable. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

		@@ -550,10 +541,10 @@ mod tests {

		fn detect_bar_reprogramming(

Conversation

ShadowCurse commented Mar 13, 2026

Changes

Reason

License Acceptance

PR Checklist

Uh oh!

codecov bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShadowCurse Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 13, 2026 •

edited

Loading

ShadowCurse Apr 2, 2026 •

edited

Loading