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 74.58493% with 199 lines in your changes missing coverage. Please review.

Project coverage is 82.88%. Comparing base (995a853) to head (e6560c9).

Files with missing lines	Patch %	Lines
src/vmm/src/device_manager/mmio.rs	56.55%	53 Missing ⚠️
src/vm-device/src/bus.rs	62.87%	49 Missing ⚠️
src/vmm/src/device_manager/mod.rs	88.20%	27 Missing ⚠️
src/vmm/src/lib.rs	13.33%	26 Missing ⚠️
src/vmm/src/devices/legacy/rtc_pl031.rs	16.66%	15 Missing ⚠️
src/vm-device/src/interrupt/mod.rs	0.00%	10 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%   82.88%   -0.05%     
================================================
  Files               251      253       +2     
  Lines             27069    27169     +100     
================================================
+ Hits              22450    22520      +70     
- Misses             4619     4649      +30

Flag	Coverage Δ
5.10-c5n.metal	`83.38% <76.85%> (+<0.01%)`	⬆️
5.10-m5n.metal	`83.38% <76.85%> (+<0.01%)`	⬆️
5.10-m6a.metal	`82.59% <76.85%> (+<0.01%)`	⬆️
5.10-m6g.metal	`79.19% <61.87%> (-0.03%)`	⬇️
5.10-m6i.metal	`83.36% <76.85%> (+<0.01%)`	⬆️
5.10-m7a.metal-48xl	`82.58% <76.85%> (+<0.01%)`	⬆️
5.10-m7g.metal	`79.19% <61.87%> (-0.04%)`	⬇️
5.10-m7i.metal-24xl	`83.33% <76.85%> (ø)`
5.10-m7i.metal-48xl	`83.34% <76.85%> (+0.01%)`	⬆️
5.10-m8g.metal-24xl	`79.19% <61.87%> (-0.03%)`	⬇️
5.10-m8g.metal-48xl	`79.19% <61.87%> (-0.03%)`	⬇️
6.1-c5n.metal	`83.42% <76.85%> (+<0.01%)`	⬆️
6.1-m5n.metal	`83.42% <76.85%> (+<0.01%)`	⬆️
6.1-m6a.metal	`82.64% <76.85%> (+<0.01%)`	⬆️
6.1-m6g.metal	`79.18% <61.87%> (-0.04%)`	⬇️
6.1-m6i.metal	`83.41% <76.85%> (-0.01%)`	⬇️
6.1-m7a.metal-48xl	`82.63% <76.85%> (+<0.01%)`	⬆️
6.1-m7g.metal	`79.18% <61.87%> (-0.04%)`	⬇️
6.1-m7i.metal-24xl	`83.43% <76.85%> (-0.01%)`	⬇️
6.1-m7i.metal-48xl	`83.43% <76.85%> (+<0.01%)`	⬆️
6.1-m8g.metal-24xl	`79.19% <61.87%> (-0.03%)`	⬇️
6.1-m8g.metal-48xl	`79.19% <61.87%> (-0.03%)`	⬇️

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.

Bring in the vm-device crate from CloudHypervisor. We will be using it for adding PCIe support. Signed-off-by: Babis Chalios <bchalios@amazon.es>

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 <bchalios@amazon.es> Co-authored-by: Egor Lazarchuk <yegorlz@amazon.co.uk> Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk> Signed-off-by: Babis Chalios <bchalios@amazon.es>

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 <bchalios@amazon.es>

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 <bchalios@amazon.es>

Use the vm_device::Bus bus for PortIO devices on x86. PCIe devices will use this as well. Signed-off-by: Babis Chalios <bchalios@amazon.es>

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 <bchalios@amazon.es>

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/

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.

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.

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?

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?

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?

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.

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?

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.

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?

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)?

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 2 times, most recently from ffb9ed0 to f84229f Compare May 2, 2025 16:36

bchalios and others added 6 commits May 2, 2025 21:15

vm-device: add vm-device crate to repo

c59f079

Bring in the vm-device crate from CloudHypervisor. We will be using it for adding PCIe support. Signed-off-by: Babis Chalios <bchalios@amazon.es>

refactor: use vm_device::Bus for IO bus

97ca55f

Use the vm_device::Bus bus for PortIO devices on x86. PCIe devices will use this as well. Signed-off-by: Babis Chalios <bchalios@amazon.es>

bchalios force-pushed the device_manager branch 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

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

ShadowCurse May 5, 2025

ShadowCurse May 5, 2025

ShadowCurse May 5, 2025

Manciukic May 6, 2025

Manciukic May 6, 2025

Manciukic May 6, 2025

Manciukic May 6, 2025

Manciukic May 6, 2025

Manciukic May 6, 2025

Manciukic May 6, 2025

Manciukic May 6, 2025

Manciukic May 6, 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

codecov bot commented May 2, 2025 •

edited

Loading