From ccdb65553b9ea39ff87a75f9489ebd9afbe4cc3f Mon Sep 17 00:00:00 2001 From: Riccardo Mancini Date: Fri, 21 Nov 2025 15:14:00 +0000 Subject: [PATCH] chore(pci): gracefully handle errors in the PCI restore code All errors were previously unwrapped, differently from MMIO where they were handled. Refactor the code to handle the errors in the same way as MMIO. Signed-off-by: Riccardo Mancini --- src/vmm/src/builder.rs | 4 +- src/vmm/src/device_manager/mod.rs | 57 ++++++- src/vmm/src/device_manager/pci_mngr.rs | 222 +++++++++++-------------- src/vmm/src/device_manager/persist.rs | 59 +------ 4 files changed, 157 insertions(+), 185 deletions(-) diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index 0fcac5e8681..9712e9c4491 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -26,7 +26,7 @@ use crate::cpu_config::templates::{GetCpuTemplate, GetCpuTemplateError, GuestCon use crate::device_manager; use crate::device_manager::pci_mngr::PciManagerError; use crate::device_manager::{ - AttachDeviceError, DeviceManager, DeviceManagerCreateError, DevicePersistError, + AttachDeviceError, DeviceManager, DeviceManagerCreateError, DeviceManagerPersistError, DeviceRestoreArgs, }; use crate::devices::acpi::vmgenid::VmGenIdError; @@ -423,7 +423,7 @@ pub enum BuildMicrovmFromSnapshotError { /// Failed to apply VMM secccomp filter: {0} SeccompFiltersInternal(#[from] crate::seccomp::InstallationError), /// Failed to restore devices: {0} - RestoreDevices(#[from] DevicePersistError), + RestoreDevices(#[from] DeviceManagerPersistError), } /// Builds and starts a microVM based on the provided MicrovmState. diff --git a/src/vmm/src/device_manager/mod.rs b/src/vmm/src/device_manager/mod.rs index ad30da5b5db..5dc6e7c979a 100644 --- a/src/vmm/src/device_manager/mod.rs +++ b/src/vmm/src/device_manager/mod.rs @@ -31,11 +31,20 @@ use crate::devices::legacy::RTCDevice; use crate::devices::legacy::serial::SerialOut; use crate::devices::legacy::{IER_RDA_BIT, IER_RDA_OFFSET, SerialDevice}; use crate::devices::pseudo::BootTimer; +use crate::devices::virtio::ActivateError; +use crate::devices::virtio::balloon::BalloonError; +use crate::devices::virtio::block::BlockError; use crate::devices::virtio::device::VirtioDevice; +use crate::devices::virtio::mem::persist::VirtioMemPersistError; +use crate::devices::virtio::net::persist::NetPersistError; +use crate::devices::virtio::pmem::persist::PmemPersistError; +use crate::devices::virtio::rng::persist::EntropyPersistError; use crate::devices::virtio::transport::mmio::{IrqTrigger, MmioTransport}; +use crate::devices::virtio::vsock::{VsockError, VsockUnixBackendError}; use crate::resources::VmResources; use crate::snapshot::Persist; use crate::utils::open_file_write_nonblock; +use crate::vmm_config::mmds::MmdsConfigError; use crate::vstate::bus::BusError; use crate::vstate::memory::GuestMemoryMmap; use crate::{EmulateSerialInitError, EventManager, Vm}; @@ -389,14 +398,50 @@ pub struct DevicesState { pub pci_state: pci_mngr::PciDevicesState, } +/// Errors for (de)serialization of the devices. #[derive(Debug, thiserror::Error, displaydoc::Display)] pub enum DevicePersistError { + /// Balloon: {0} + Balloon(#[from] BalloonError), + /// Block: {0} + Block(#[from] BlockError), + /// MMIO Device manager: {0} + MmioDeviceManager(#[from] mmio::MmioError), + /// Mmio transport + MmioTransport, + /// PCI Device manager: {0} + PciDeviceManager(#[from] PciManagerError), + /// Bus error: {0} + Bus(#[from] BusError), + #[cfg(target_arch = "aarch64")] + /// Legacy: {0} + Legacy(#[from] std::io::Error), + /// Net: {0} + Net(#[from] NetPersistError), + /// Vsock: {0} + Vsock(#[from] VsockError), + /// VsockUnixBackend: {0} + VsockUnixBackend(#[from] VsockUnixBackendError), + /// MmdsConfig: {0} + MmdsConfig(#[from] MmdsConfigError), + /// Entropy: {0} + Entropy(#[from] EntropyPersistError), + /// Pmem: {0} + Pmem(#[from] PmemPersistError), + /// virtio-mem: {0} + VirtioMem(#[from] VirtioMemPersistError), + /// Could not activate device: {0} + DeviceActivation(#[from] ActivateError), +} + +#[derive(Debug, thiserror::Error, displaydoc::Display)] +pub enum DeviceManagerPersistError { /// Error restoring MMIO devices: {0} - MmioRestore(#[from] persist::DevicePersistError), + MmioRestore(DevicePersistError), /// Error restoring ACPI devices: {0} AcpiRestore(#[from] persist::ACPIDeviceManagerRestoreError), /// Error restoring PCI devices: {0} - PciRestore(#[from] PciManagerError), + PciRestore(DevicePersistError), /// Error notifying VMGenID device: {0} VmGenidUpdate(#[from] std::io::Error), /// Error resetting serial console: {0} @@ -430,7 +475,7 @@ impl std::fmt::Debug for DeviceRestoreArgs<'_> { impl<'a> Persist<'a> for DeviceManager { type State = DevicesState; type ConstructorArgs = DeviceRestoreArgs<'a>; - type Error = DevicePersistError; + type Error = DeviceManagerPersistError; fn save(&self) -> Self::State { DevicesState { @@ -461,7 +506,8 @@ impl<'a> Persist<'a> for DeviceManager { vm_resources: constructor_args.vm_resources, instance_id: constructor_args.instance_id, }; - let mmio_devices = MMIODeviceManager::restore(mmio_ctor_args, &state.mmio_state)?; + let mmio_devices = MMIODeviceManager::restore(mmio_ctor_args, &state.mmio_state) + .map_err(DeviceManagerPersistError::MmioRestore)?; // Restore ACPI devices let acpi_ctor_args = ACPIDeviceManagerConstructorArgs { @@ -479,7 +525,8 @@ impl<'a> Persist<'a> for DeviceManager { instance_id: constructor_args.instance_id, event_manager: constructor_args.event_manager, }; - let pci_devices = PciDevices::restore(pci_ctor_args, &state.pci_state)?; + let pci_devices = PciDevices::restore(pci_ctor_args, &state.pci_state) + .map_err(DeviceManagerPersistError::PciRestore)?; let device_manager = DeviceManager { mmio_devices, diff --git a/src/vmm/src/device_manager/pci_mngr.rs b/src/vmm/src/device_manager/pci_mngr.rs index a2270097686..ec5e477a1f1 100644 --- a/src/vmm/src/device_manager/pci_mngr.rs +++ b/src/vmm/src/device_manager/pci_mngr.rs @@ -11,6 +11,7 @@ use log::{debug, error, warn}; use serde::{Deserialize, Serialize}; use super::persist::MmdsState; +use crate::device_manager::DevicePersistError; use crate::devices::pci::PciSegment; use crate::devices::virtio::balloon::Balloon; use crate::devices::virtio::balloon::persist::{BalloonConstructorArgs, BalloonState}; @@ -37,7 +38,6 @@ use crate::pci::bus::PciRootError; use crate::resources::VmResources; use crate::snapshot::Persist; use crate::vmm_config::memory_hotplug::MemoryHotplugConfig; -use crate::vmm_config::mmds::MmdsConfigError; use crate::vstate::bus::BusError; use crate::vstate::interrupts::InterruptError; use crate::vstate::memory::GuestMemoryMmap; @@ -65,8 +65,6 @@ pub enum PciManagerError { VirtioPciDevice(#[from] VirtioPciDeviceError), /// KVM error: {0} Kvm(#[from] vmm_sys_util::errno::Error), - /// MMDS error: {0} - Mmds(#[from] MmdsConfigError), } impl PciDevices { @@ -271,7 +269,7 @@ impl<'a> Debug for PciDevicesConstructorArgs<'a> { impl<'a> Persist<'a> for PciDevices { type State = PciDevicesState; type ConstructorArgs = PciDevicesConstructorArgs<'a>; - type Error = PciManagerError; + type Error = DevicePersistError; fn save(&self) -> Self::State { let mut state = PciDevicesState::default(); @@ -440,61 +438,52 @@ impl<'a> Persist<'a> for PciDevices { pci_devices.attach_pci_segment(constructor_args.vm)?; if let Some(balloon_state) = &state.balloon_device { - let device = Arc::new(Mutex::new( - Balloon::restore( - BalloonConstructorArgs { mem: mem.clone() }, - &balloon_state.device_state, - ) - .unwrap(), - )); + let device = Arc::new(Mutex::new(Balloon::restore( + BalloonConstructorArgs { mem: mem.clone() }, + &balloon_state.device_state, + )?)); constructor_args .vm_resources .balloon .set_device(device.clone()); - pci_devices - .restore_pci_device( - constructor_args.vm, - device, - &balloon_state.device_id, - &balloon_state.transport_state, - constructor_args.event_manager, - ) - .unwrap() + pci_devices.restore_pci_device( + constructor_args.vm, + device, + &balloon_state.device_id, + &balloon_state.transport_state, + constructor_args.event_manager, + )? } for block_state in &state.block_devices { - let device = Arc::new(Mutex::new( - Block::restore( - BlockConstructorArgs { mem: mem.clone() }, - &block_state.device_state, - ) - .unwrap(), - )); + let device = Arc::new(Mutex::new(Block::restore( + BlockConstructorArgs { mem: mem.clone() }, + &block_state.device_state, + )?)); constructor_args .vm_resources .block .add_virtio_device(device.clone()); - pci_devices - .restore_pci_device( - constructor_args.vm, - device, - &block_state.device_id, - &block_state.transport_state, - constructor_args.event_manager, - ) - .unwrap() + pci_devices.restore_pci_device( + constructor_args.vm, + device, + &block_state.device_id, + &block_state.transport_state, + constructor_args.event_manager, + )? } // Initialize MMDS if MMDS state is included. if let Some(mmds) = &state.mmds { - constructor_args - .vm_resources - .set_mmds_basic_config(mmds.version, mmds.imds_compat, constructor_args.instance_id) - .unwrap(); + constructor_args.vm_resources.set_mmds_basic_config( + mmds.version, + mmds.imds_compat, + constructor_args.instance_id, + )?; } else if state .net_devices .iter() @@ -507,125 +496,108 @@ impl<'a> Persist<'a> for PciDevices { } for net_state in &state.net_devices { - let device = Arc::new(Mutex::new( - Net::restore( - NetConstructorArgs { - mem: mem.clone(), - mmds: constructor_args - .vm_resources - .mmds - .as_ref() - // Clone the Arc reference. - .cloned(), - }, - &net_state.device_state, - ) - .unwrap(), - )); + let device = Arc::new(Mutex::new(Net::restore( + NetConstructorArgs { + mem: mem.clone(), + mmds: constructor_args + .vm_resources + .mmds + .as_ref() + // Clone the Arc reference. + .cloned(), + }, + &net_state.device_state, + )?)); constructor_args .vm_resources .net_builder .add_device(device.clone()); - pci_devices - .restore_pci_device( - constructor_args.vm, - device, - &net_state.device_id, - &net_state.transport_state, - constructor_args.event_manager, - ) - .unwrap() + pci_devices.restore_pci_device( + constructor_args.vm, + device, + &net_state.device_id, + &net_state.transport_state, + constructor_args.event_manager, + )? } if let Some(vsock_state) = &state.vsock_device { let ctor_args = VsockUdsConstructorArgs { cid: vsock_state.device_state.frontend.cid, }; - let backend = - VsockUnixBackend::restore(ctor_args, &vsock_state.device_state.backend).unwrap(); - let device = Arc::new(Mutex::new( - Vsock::restore( - VsockConstructorArgs { - mem: mem.clone(), - backend, - }, - &vsock_state.device_state.frontend, - ) - .unwrap(), - )); + let backend = VsockUnixBackend::restore(ctor_args, &vsock_state.device_state.backend)?; + let device = Arc::new(Mutex::new(Vsock::restore( + VsockConstructorArgs { + mem: mem.clone(), + backend, + }, + &vsock_state.device_state.frontend, + )?)); constructor_args .vm_resources .vsock .set_device(device.clone()); - pci_devices - .restore_pci_device( - constructor_args.vm, - device, - &vsock_state.device_id, - &vsock_state.transport_state, - constructor_args.event_manager, - ) - .unwrap() + pci_devices.restore_pci_device( + constructor_args.vm, + device, + &vsock_state.device_id, + &vsock_state.transport_state, + constructor_args.event_manager, + )? } if let Some(entropy_state) = &state.entropy_device { let ctor_args = EntropyConstructorArgs { mem: mem.clone() }; - let device = Arc::new(Mutex::new( - Entropy::restore(ctor_args, &entropy_state.device_state).unwrap(), - )); + let device = Arc::new(Mutex::new(Entropy::restore( + ctor_args, + &entropy_state.device_state, + )?)); constructor_args .vm_resources .entropy .set_device(device.clone()); - pci_devices - .restore_pci_device( - constructor_args.vm, - device, - &entropy_state.device_id, - &entropy_state.transport_state, - constructor_args.event_manager, - ) - .unwrap() + pci_devices.restore_pci_device( + constructor_args.vm, + device, + &entropy_state.device_id, + &entropy_state.transport_state, + constructor_args.event_manager, + )? } for pmem_state in &state.pmem_devices { - let device = Arc::new(Mutex::new( - Pmem::restore( - PmemConstructorArgs { - mem, - vm: constructor_args.vm.as_ref(), - }, - &pmem_state.device_state, - ) - .unwrap(), - )); + let device = Arc::new(Mutex::new(Pmem::restore( + PmemConstructorArgs { + mem, + vm: constructor_args.vm.as_ref(), + }, + &pmem_state.device_state, + )?)); constructor_args .vm_resources .pmem .add_device(device.clone()); - pci_devices - .restore_pci_device( - constructor_args.vm, - device, - &pmem_state.device_id, - &pmem_state.transport_state, - constructor_args.event_manager, - ) - .unwrap() + pci_devices.restore_pci_device( + constructor_args.vm, + device, + &pmem_state.device_id, + &pmem_state.transport_state, + constructor_args.event_manager, + )? } if let Some(memory_device) = &state.memory_device { let ctor_args = VirtioMemConstructorArgs::new(Arc::clone(constructor_args.vm)); - let device = VirtioMem::restore(ctor_args, &memory_device.device_state).unwrap(); + let device = VirtioMem::restore(ctor_args, &memory_device.device_state)?; constructor_args.vm_resources.memory_hotplug = Some(MemoryHotplugConfig { total_size_mib: device.total_size_mib(), @@ -634,15 +606,13 @@ impl<'a> Persist<'a> for PciDevices { }); let arcd_device = Arc::new(Mutex::new(device)); - pci_devices - .restore_pci_device( - constructor_args.vm, - arcd_device, - &memory_device.device_id, - &memory_device.transport_state, - constructor_args.event_manager, - ) - .unwrap() + pci_devices.restore_pci_device( + constructor_args.vm, + arcd_device, + &memory_device.device_id, + &memory_device.transport_state, + constructor_args.event_manager, + )? } Ok(pci_devices) diff --git a/src/vmm/src/device_manager/persist.rs b/src/vmm/src/device_manager/persist.rs index 4a90c9f219f..01b3c499267 100644 --- a/src/vmm/src/device_manager/persist.rs +++ b/src/vmm/src/device_manager/persist.rs @@ -14,82 +14,37 @@ use super::acpi::ACPIDeviceManager; use super::mmio::*; #[cfg(target_arch = "aarch64")] use crate::arch::DeviceType; +use crate::device_manager::DevicePersistError; use crate::devices::acpi::vmgenid::{VMGenIDState, VMGenIdConstructorArgs, VmGenId, VmGenIdError}; #[cfg(target_arch = "aarch64")] use crate::devices::legacy::RTCDevice; -use crate::devices::virtio::ActivateError; +use crate::devices::virtio::balloon::Balloon; use crate::devices::virtio::balloon::persist::{BalloonConstructorArgs, BalloonState}; -use crate::devices::virtio::balloon::{Balloon, BalloonError}; -use crate::devices::virtio::block::BlockError; use crate::devices::virtio::block::device::Block; use crate::devices::virtio::block::persist::{BlockConstructorArgs, BlockState}; use crate::devices::virtio::device::VirtioDevice; use crate::devices::virtio::generated::virtio_ids; use crate::devices::virtio::mem::VirtioMem; -use crate::devices::virtio::mem::persist::{ - VirtioMemConstructorArgs, VirtioMemPersistError, VirtioMemState, -}; +use crate::devices::virtio::mem::persist::{VirtioMemConstructorArgs, VirtioMemState}; use crate::devices::virtio::net::Net; -use crate::devices::virtio::net::persist::{ - NetConstructorArgs, NetPersistError as NetError, NetState, -}; +use crate::devices::virtio::net::persist::{NetConstructorArgs, NetState}; use crate::devices::virtio::persist::{MmioTransportConstructorArgs, MmioTransportState}; use crate::devices::virtio::pmem::device::Pmem; -use crate::devices::virtio::pmem::persist::{ - PmemConstructorArgs, PmemPersistError as PmemError, PmemState, -}; +use crate::devices::virtio::pmem::persist::{PmemConstructorArgs, PmemState}; use crate::devices::virtio::rng::Entropy; -use crate::devices::virtio::rng::persist::{ - EntropyConstructorArgs, EntropyPersistError as EntropyError, EntropyState, -}; +use crate::devices::virtio::rng::persist::{EntropyConstructorArgs, EntropyState}; use crate::devices::virtio::transport::mmio::{IrqTrigger, MmioTransport}; use crate::devices::virtio::vsock::persist::{ VsockConstructorArgs, VsockState, VsockUdsConstructorArgs, }; -use crate::devices::virtio::vsock::{Vsock, VsockError, VsockUnixBackend, VsockUnixBackendError}; +use crate::devices::virtio::vsock::{Vsock, VsockUnixBackend}; use crate::mmds::data_store::MmdsVersion; use crate::resources::VmResources; use crate::snapshot::Persist; use crate::vmm_config::memory_hotplug::MemoryHotplugConfig; -use crate::vmm_config::mmds::MmdsConfigError; -use crate::vstate::bus::BusError; use crate::vstate::memory::GuestMemoryMmap; use crate::{EventManager, Vm}; -/// Errors for (de)serialization of the MMIO device manager. -#[derive(Debug, thiserror::Error, displaydoc::Display)] -pub enum DevicePersistError { - /// Balloon: {0} - Balloon(#[from] BalloonError), - /// Block: {0} - Block(#[from] BlockError), - /// Device manager: {0} - DeviceManager(#[from] super::mmio::MmioError), - /// Mmio transport - MmioTransport, - /// Bus error: {0} - Bus(#[from] BusError), - #[cfg(target_arch = "aarch64")] - /// Legacy: {0} - Legacy(#[from] std::io::Error), - /// Net: {0} - Net(#[from] NetError), - /// Vsock: {0} - Vsock(#[from] VsockError), - /// VsockUnixBackend: {0} - VsockUnixBackend(#[from] VsockUnixBackendError), - /// MmdsConfig: {0} - MmdsConfig(#[from] MmdsConfigError), - /// Entropy: {0} - Entropy(#[from] EntropyError), - /// Pmem: {0} - Pmem(#[from] PmemError), - /// virtio-mem: {0} - VirtioMem(#[from] VirtioMemPersistError), - /// Could not activate device: {0} - DeviceActivation(#[from] ActivateError), -} - /// Holds the state of a MMIO VirtIO device #[derive(Debug, Clone, Serialize, Deserialize)] pub struct VirtioDeviceState {