WIP: Refactor device managers, preparing for PCIe devices #5174

bchalios · 2025-04-29T12:47:51Z

Changes

Group all device managers in a single top-level device manager, which now becomes the owner of the MMIO and Port IO buses along with the resource allocator. Also, it gets rid of the our Bus implementation in favour of the vm-device one from Cloud Hypervisor.

Reason

PCIe devices also need to be registered to the MMIO and Port IO buses. So, it makes more sense to keep the buses outside of MMIO and PortIO device managers.

Regarding switching to using vm_devices::Bus, PCIe implementation is using it and it was quite simpler to switch our own implementation to the upstream type rather than the other way around.

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.

codecov · 2025-05-02T13:15:48Z

Codecov Report

Attention: Patch coverage is 84.33121% with 123 lines in your changes missing coverage. Please review.

Project coverage is 83.23%. Comparing base (c4e3bf8) to head (dd990d4).

Files with missing lines	Patch %	Lines
src/vmm/src/device_manager/mmio.rs	73.23%	38 Missing ⚠️
src/vmm/src/lib.rs	13.33%	26 Missing ⚠️
src/vmm/src/devices/legacy/rtc_pl031.rs	33.33%	12 Missing ⚠️
src/vm-device/src/bus.rs	91.66%	11 Missing ⚠️
src/vm-device/src/interrupt/mod.rs	0.00%	10 Missing ⚠️
src/vmm/src/device_manager/mod.rs	96.66%	7 Missing ⚠️
src/vmm/src/builder.rs	92.72%	4 Missing ⚠️
src/vmm/src/devices/pseudo/boot_timer.rs	0.00%	4 Missing ⚠️
src/vmm/src/devices/virtio/transport/mmio.rs	55.55%	4 Missing ⚠️
src/vmm/src/persist.rs	50.00%	3 Missing ⚠️
... and 2 more

Additional details and impacted files

@@               Coverage Diff                @@
##           feature/pcie    #5174      +/-   ##
================================================
+ Coverage         82.93%   83.23%   +0.29%     
================================================
  Files               251      253       +2     
  Lines             27073    27172      +99     
================================================
+ Hits              22454    22617     +163     
+ Misses             4619     4555      -64

Flag	Coverage Δ
5.10-c5n.metal	`83.54% <83.05%> (+0.16%)`	⬆️
5.10-m5n.metal	`83.54% <83.05%> (+0.16%)`	⬆️
5.10-m6a.metal	`82.77% <83.05%> (+0.17%)`	⬆️
5.10-m6g.metal	`79.75% <78.21%> (+0.53%)`	⬆️
5.10-m6i.metal	`83.53% <83.05%> (+0.15%)`	⬆️
5.10-m7a.metal-48xl	`82.75% <83.05%> (+0.17%)`	⬆️
5.10-m7g.metal	`79.75% <78.21%> (+0.53%)`	⬆️
5.10-m7i.metal-24xl	`83.50% <83.05%> (+0.16%)`	⬆️
5.10-m7i.metal-48xl	`83.50% <83.05%> (+0.16%)`	⬆️
5.10-m8g.metal-24xl	`79.75% <78.21%> (+0.53%)`	⬆️
5.10-m8g.metal-48xl	`79.75% <78.21%> (+0.53%)`	⬆️
6.1-c5n.metal	`83.59% <83.05%> (+0.16%)`	⬆️
6.1-m5n.metal	`83.58% <83.05%> (+0.16%)`	⬆️
6.1-m6a.metal	`82.82% <83.05%> (+0.17%)`	⬆️
6.1-m6g.metal	`79.75% <78.21%> (+0.53%)`	⬆️
6.1-m6i.metal	`83.57% <83.05%> (+0.16%)`	⬆️
6.1-m7a.metal-48xl	`82.80% <83.05%> (+0.18%)`	⬆️
6.1-m7g.metal	`79.75% <78.21%> (+0.53%)`	⬆️
6.1-m7i.metal-24xl	`83.60% <83.05%> (+0.16%)`	⬆️
6.1-m7i.metal-48xl	`83.60% <83.05%> (+0.15%)`	⬆️
6.1-m8g.metal-24xl	`79.75% <78.21%> (+0.53%)`	⬆️
6.1-m8g.metal-48xl	`79.75% <78.21%> (+0.53%)`	⬆️

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.

ShadowCurse · 2025-05-05T10:50:18Z

src/vmm/src/device_manager/mmio.rs

+        match &self.serial {
+            Some(device) => {


This can be an assert, as the serial is registered right before this call. Otherwise maybe just remove this function and to the addition inline in the attach_legacy_devices_aarch64? (like we do for block devices)

ShadowCurse · 2025-05-05T10:56:28Z

src/vmm/src/device_manager/mmio.rs

-                            balloon.process_virtio_queues();
-                        }
+        let _: Result<(), MmioError> = self.for_each_virtio_device(|virtio_type, id, device| {
+            let mmio_transport_locked = device.inner.lock().expect("Poisoned locked");


s/locked/lock/

mmio_transport_locked now is essentially a reference to the actual MMIO transport (after being locked). So... semantics.

ShadowCurse · 2025-05-05T11:33:41Z

src/vmm/src/devices/virtio/transport/mmio.rs

-impl MmioTransport {
-    pub fn bus_read(&mut self, offset: u64, data: &mut [u8]) {
+impl vm_device::BusDevice for MmioTransport {
+    fn read(&mut self, base: u64, offset: u64, data: &mut [u8]) {


I see that the base arg is never used, so maybe it is worth removing it from a trait definition?
And same question about Option<Arc<Barrier>> return type in write function.

These come from the external crate. So I assume they are used somewhere even if we don't use them today in Firecracker.

ShadowCurse · 2025-05-05T11:48:19Z

src/vmm/src/builder.rs

+        let i8042 = Arc::new(Mutex::new(I8042Device::new(
+            reset_evt,
+            EventFd::new(libc::EFD_NONBLOCK).map_err(VmmError::EventFd)?,
+        )));


note: the I8042Device::new can be simplified by moving EventFd creation inside and asserting on it.

although, I didn't assert. I'm still returning an Error. This code is in the InstanceStart API call path.

Manciukic · 2025-05-06T10:43:22Z

src/vm-device/Cargo.toml

What are the gaps between the rust-vmm vm-devices create and this one? Is there any possibility of upstreaming these changes to the crate?

They have diverged significantly, but I think nobody is actually using the rust-vmm one, so in theory we should be able to upstream the CH one.

Manciukic · 2025-05-06T10:46:40Z

src/vmm/src/builder.rs

-    })));
+    let serial = Arc::new(Mutex::new(BusDevice::Serial(
+        SerialDevice::new(Some(std::io::stdin()), SerialOut::Stdout(std::io::stdout()))
+            .map_err(VmmError::EventFd)?,


nit: can this fail only due to EventFd?

Yup, it essentially fails only when we can't create the EventFd.

Manciukic · 2025-05-06T10:48:56Z

src/vmm/src/arch/aarch64/fdt.rs

-    device_info: &HashMap<(DeviceType, String), MMIODeviceInfo>,
+    virtio_devices: Vec<&MMIODeviceInfo>,
+    rtc: Option<&MMIODeviceInfo>,
+    serial: Option<&MMIODeviceInfo>,


can this be simplified by passing the DeviceManager as we do in ACPI?

I tried to do that, but that breaks hopelessly unit tests. It's gonna be a mess fixing it.

Manciukic · 2025-05-06T10:52:12Z

src/vmm/src/builder.rs

@@ -615,7 +615,11 @@ fn attach_legacy_devices_aarch64(
    if cmdline_contains_console {
        // Make stdout non-blocking.
        set_stdout_nonblocking();
-        let serial = setup_serial_device(event_manager)?;


I didn't follow why we're inlining the function here for arm but not for x86. Also, this could be moved to the earlier commit.

At this commit, the setup_serial_device still returns the old BusDevice thingy. The serial device on ARM is a MMIO device so it is not part of BusDevice any more, because it's an MMIO device, so it temporarily diverge. We revert to unifying them again when IO devices become vm_device::Bus as well.

Manciukic · 2025-05-06T10:57:31Z

src/vmm/src/device_manager/mmio.rs

-    #[cfg(target_arch = "x86_64")]
    pub(crate) dsdt_data: Vec<u8>,


any reason to leave this in in ARM?

The cfg just moved above the comment.

Manciukic · 2025-05-06T11:06:12Z

src/vmm/src/device_manager/mmio.rs

+        let _: Result<(), MmioError> = self.for_each_virtio_device(|virtio_type, id, device| {
+            let mmio_transport_locked = device.inner.lock().expect("Poisoned locked");
+            let mut virtio = mmio_transport_locked.locked_device();
+            match *virtio_type {


unrelated to the scope of this PR, but I'm wondering if the "kick" can become a trait function that we call on every device, rather than having this combination of downcast and device-specific logic.

yup, that's not such a bad idea.

Manciukic · 2025-05-06T11:18:57Z

src/vmm/src/device_manager/mmio.rs

-        self.bus
-            .insert(device.clone(), device_info.addr, device_info.len)?;


does the "register" function no longer actually register the device with the bus?

oh, good catch

Manciukic · 2025-05-06T11:20:47Z

src/vmm/src/device_manager/mod.rs

+        let boot_timer = Arc::new(Mutex::new(BootTimer::new(request_ts)));
+
+        self.mmio_devices
+            .register_mmio_boot_timer(&mut self.resource_allocator, boot_timer)?;


does this not get registered with the Bus?

Manciukic · 2025-05-06T11:23:15Z

src/vmm/src/device_manager/mod.rs

+        Ok(serial)
+    }
+
+    #[cfg_attr(target_arch = "aarch64", allow(unused))]


nit: why allow(unused)?

event_manager and vcpu_exit_evt are only used by the Port IO initialization code.

Bring in the vm-device crate from CloudHypervisor. We will be using it for adding PCIe support. Signed-off-by: Babis Chalios <[email protected]>

We use `SerialDevice` with Stdin as the input source. Encode this in the type so that we don't spill the generic all over the place. Signed-off-by: Babis Chalios <[email protected]> Co-authored-by: Egor Lazarchuk <[email protected]> Signed-off-by: Egor Lazarchuk <[email protected]> Signed-off-by: Babis Chalios <[email protected]>

Use the vm_device::Bus bus for all MMIO devices. This mainly to prepare for using it for PCIe devices. Also, sepate VirtIO devices from other MMIO devices inside the MMIODeviceManager struct. This makes iterating over VirtIO devices since we don't need to access two data structures as to get a reference to a VirtIO device any more. Signed-off-by: Babis Chalios <[email protected]>

We were always constructing RTCDevice using a set of metrics that were defined in the RTC module itself. Don't leak the metrics to other modules. Instead, create a new() function that always constructs it the correct way. Signed-off-by: Babis Chalios <[email protected]>

Use the vm_device::Bus bus for PortIO devices on x86. PCIe devices will use this as well. Signed-off-by: Babis Chalios <[email protected]>

PCIe spec mandates that software can access the configuration space of PCIe devices both via MMIO and Port IO accesses. As a result, PCIe devices will need to register to both buses (on x86). Change the organization of devices, so that MMIO and PIO device managers do not own the buses. Instead, introduce a DeviceManager object which holds the buses, the resource allocator and includes also all types of device managers (at the moment MMIO, PIO and ACPI). Signed-off-by: Babis Chalios <[email protected]>

We always create anew the keyboard interrupt event. Just create it inside `I8042Device::new()` and return an error if that fails. Signed-off-by: Babis Chalios <[email protected]>

To see if we the memory increase increases. Signed-off-by: Babis Chalios <[email protected]>

bchalios force-pushed the device_manager branch 4 times, most recently from 503fa5d to a7ec377 Compare April 30, 2025 15:41

bchalios force-pushed the feature/pcie branch from 87a7164 to 995a853 Compare May 2, 2025 08:56

bchalios force-pushed the device_manager branch 3 times, most recently from 903e15a to 564c7c0 Compare May 2, 2025 13:05

bchalios force-pushed the device_manager branch 3 times, most recently from f84229f to e6560c9 Compare May 2, 2025 23:08

ShadowCurse reviewed May 5, 2025

View reviewed changes

Manciukic reviewed May 6, 2025

View reviewed changes

bchalios force-pushed the feature/pcie branch from 995a853 to c4e3bf8 Compare May 7, 2025 09:28

bchalios force-pushed the device_manager branch 7 times, most recently from c6e5fcd to 0a85b66 Compare May 8, 2025 13:53

bchalios and others added 8 commits May 8, 2025 16:42

vm-device: add vm-device crate to repo

c26cf72

Bring in the vm-device crate from CloudHypervisor. We will be using it for adding PCIe support. Signed-off-by: Babis Chalios <[email protected]>

refactor: use vm_device::Bus for IO bus

2592035

Use the vm_device::Bus bus for PortIO devices on x86. PCIe devices will use this as well. Signed-off-by: Babis Chalios <[email protected]>

refactor: simplify creation of I8042 device

13da9c3

We always create anew the keyboard interrupt event. Just create it inside `I8042Device::new()` and return an error if that fails. Signed-off-by: Babis Chalios <[email protected]>

just testing: increase number of bytes we write to serial

dd990d4

To see if we the memory increase increases. Signed-off-by: Babis Chalios <[email protected]>

bchalios force-pushed the device_manager branch from e3be00c to dd990d4 Compare May 8, 2025 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Refactor device managers, preparing for PCIe devices #5174

WIP: Refactor device managers, preparing for PCIe devices #5174

bchalios commented Apr 29, 2025

codecov bot commented May 2, 2025 •

edited

Loading

ShadowCurse May 5, 2025

bchalios May 7, 2025

ShadowCurse May 5, 2025

bchalios May 7, 2025

ShadowCurse May 5, 2025

bchalios May 7, 2025

ShadowCurse May 5, 2025

bchalios May 7, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

Manciukic May 6, 2025

bchalios May 7, 2025

		#[cfg(target_arch = "x86_64")]
		pub(crate) dsdt_data: Vec<u8>,

		self.bus
		.insert(device.clone(), device_info.addr, device_info.len)?;

WIP: Refactor device managers, preparing for PCIe devices #5174

Are you sure you want to change the base?

WIP: Refactor device managers, preparing for PCIe devices #5174

Conversation

bchalios commented Apr 29, 2025

Changes

Reason

License Acceptance

PR Checklist

codecov bot commented May 2, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 2, 2025 •

edited

Loading