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 <[email protected]>

Author: Manciukic

src/vmm/src/builder.rs

@@ -26,8 +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,
-    DeviceRestoreArgs,
+    AttachDeviceError, DeviceManager, DeviceManagerCreateError, DeviceManagerPersistError, DeviceRestoreArgs
 };
 use crate::devices::acpi::vmgenid::VmGenIdError;
 use crate::devices::virtio::balloon::Balloon;
@@ -423,7 +422,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.

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,51 @@ 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 +476,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 +507,7 @@ 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,7 @@ 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,

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();
@@ -444,8 +442,7 @@ impl<'a> Persist<'a> for PciDevices {
                 Balloon::restore(
                     BalloonConstructorArgs { mem: mem.clone() },
                     &balloon_state.device_state,
-                )
-                .unwrap(),
+                )?,
             ));
 
             constructor_args
@@ -460,17 +457,15 @@ impl<'a> Persist<'a> for PciDevices {
                     &balloon_state.device_id,
                     &balloon_state.transport_state,
                     constructor_args.event_manager,
-                )
-                .unwrap()
+                )?
         }
 
         for block_state in &state.block_devices {
             let device = Arc::new(Mutex::new(
                 Block::restore(
                     BlockConstructorArgs { mem: mem.clone() },
                     &block_state.device_state,
-                )
-                .unwrap(),
+                )?,
             ));
 
             constructor_args
@@ -485,16 +480,14 @@ impl<'a> Persist<'a> for PciDevices {
                     &block_state.device_id,
                     &block_state.transport_state,
                     constructor_args.event_manager,
-                )
-                .unwrap()
+                )?
         }
 
         // 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();
+                .set_mmds_basic_config(mmds.version, mmds.imds_compat, constructor_args.instance_id)?;
         } else if state
             .net_devices
             .iter()
@@ -519,8 +512,7 @@ impl<'a> Persist<'a> for PciDevices {
                             .cloned(),
                     },
                     &net_state.device_state,
-                )
-                .unwrap(),
+                )?,
             ));
 
             constructor_args
@@ -535,25 +527,23 @@ impl<'a> Persist<'a> for PciDevices {
                     &net_state.device_id,
                     &net_state.transport_state,
                     constructor_args.event_manager,
-                )
-                .unwrap()
+                )?
         }
 
         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();
+                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,
-                )
-                .unwrap(),
+                )?,
             ));
 
             constructor_args
@@ -568,15 +558,14 @@ impl<'a> Persist<'a> for PciDevices {
                     &vsock_state.device_id,
                     &vsock_state.transport_state,
                     constructor_args.event_manager,
-                )
-                .unwrap()
+                )?
         }
 
         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(),
+                Entropy::restore(ctor_args, &entropy_state.device_state)?,
             ));
 
             constructor_args
@@ -591,8 +580,7 @@ impl<'a> Persist<'a> for PciDevices {
                     &entropy_state.device_id,
                     &entropy_state.transport_state,
                     constructor_args.event_manager,
-                )
-                .unwrap()
+                )?
         }
 
         for pmem_state in &state.pmem_devices {
@@ -603,8 +591,7 @@ impl<'a> Persist<'a> for PciDevices {
                         vm: constructor_args.vm.as_ref(),
                     },
                     &pmem_state.device_state,
-                )
-                .unwrap(),
+                )?,
             ));
 
             constructor_args
@@ -619,13 +606,12 @@ impl<'a> Persist<'a> for PciDevices {
                     &pmem_state.device_id,
                     &pmem_state.transport_state,
                     constructor_args.event_manager,
-                )
-                .unwrap()
+                )?
         }
 
         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(),
@@ -641,8 +627,7 @@ impl<'a> Persist<'a> for PciDevices {
                     &memory_device.device_id,
                     &memory_device.transport_state,
                     constructor_args.event_manager,
-                )
-                .unwrap()
+                )?
         }
 
         Ok(pci_devices)

src/vmm/src/device_manager/persist.rs

@@ -14,82 +14,45 @@ 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::persist::{BalloonConstructorArgs, BalloonState};
-use crate::devices::virtio::balloon::{Balloon, BalloonError};
-use crate::devices::virtio::block::BlockError;
+use crate::devices::virtio::balloon::Balloon;
 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,
+    VirtioMemConstructorArgs, VirtioMemState,
 };
 use crate::devices::virtio::net::Net;
 use crate::devices::virtio::net::persist::{
-    NetConstructorArgs, NetPersistError as NetError, NetState,
+    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,
+    PmemConstructorArgs, PmemState,
 };
 use crate::devices::virtio::rng::Entropy;
 use crate::devices::virtio::rng::persist::{
-    EntropyConstructorArgs, EntropyPersistError as EntropyError, EntropyState,
+    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<T> {