refactor: Split VmError enum into arch specific versions

Introduce `enum ArchVmError` which contains architecture specific
variants for VM operations (mostly related to snapshot
creation/restore). Do this by adapting exiting architecture specific
enums that were used for snapshot restoration (x86_64)/GIC creation
(aarch64).

While we're at it, remove unused/duplicated enum variants, and on x86
use the SetIrqChip enum variant for failures inside `setup_irq_chip`
instead of producing some generic `VmError`.

Admittedly, there's a lot more cleanup that can be done here, but for
now this is enough to attain my goals (separating architecture specific
code).

No functional change intended (apart from error messages changing).

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

src/vmm/src/builder.rs

@@ -71,7 +71,7 @@ use crate::vmm_config::machine_config::{MachineConfig, MachineConfigError};
 use crate::vstate::kvm::Kvm;
 use crate::vstate::memory::{GuestAddress, GuestMemory, GuestMemoryMmap};
 use crate::vstate::vcpu::{Vcpu, VcpuConfig, VcpuError};
-use crate::vstate::vm::Vm;
+use crate::vstate::vm::{Vm, VmError};
 use crate::{device_manager, EventManager, Vmm, VmmError};
 
 /// Errors associated with starting the instance.
@@ -436,7 +436,7 @@ pub enum BuildMicrovmFromSnapshotError {
     /// Could not set TSC scaling within the snapshot: {0}
     SetTsc(#[from] crate::vstate::vcpu::SetTscError),
     /// Failed to restore microVM state: {0}
-    RestoreState(#[from] crate::vstate::vm::RestoreStateError),
+    RestoreState(#[from] crate::vstate::vm::ArchVmError),
     /// Failed to update microVM configuration: {0}
     VmUpdateConfig(#[from] MachineConfigError),
     /// Failed to restore MMIO device: {0}
@@ -675,6 +675,7 @@ where
 #[cfg(target_arch = "x86_64")]
 pub fn setup_interrupt_controller(vm: &mut Vm) -> Result<(), StartMicrovmError> {
     vm.setup_irqchip()
+        .map_err(VmError::Arch)
         .map_err(VmmError::Vm)
         .map_err(StartMicrovmError::Internal)
 }
@@ -683,6 +684,7 @@ pub fn setup_interrupt_controller(vm: &mut Vm) -> Result<(), StartMicrovmError>
 #[cfg(target_arch = "aarch64")]
 pub fn setup_interrupt_controller(vm: &mut Vm, vcpu_count: u8) -> Result<(), StartMicrovmError> {
     vm.setup_irqchip(vcpu_count)
+        .map_err(VmError::Arch)
         .map_err(VmmError::Vm)
         .map_err(StartMicrovmError::Internal)
 }

src/vmm/src/persist.rs

@@ -129,7 +129,7 @@ pub enum MicrovmStateError {
     /// Cannot save Vcpu state: {0}
     SaveVcpuState(vstate::vcpu::VcpuError),
     /// Cannot save Vm state: {0}
-    SaveVmState(vstate::vm::VmError),
+    SaveVmState(vstate::vm::ArchVmError),
     /// Cannot signal Vcpu: {0}
     SignalVcpu(VcpuSendEventError),
     /// Vcpu is in unexpected state.

src/vmm/src/vstate/vm/aarch64.rs

@@ -4,24 +4,25 @@
 use serde::{Deserialize, Serialize};
 
 use crate::arch::aarch64::gic::GicState;
-use crate::vstate::vm::VmError;
 use crate::Vm;
 
 /// Error type for [`Vm::restore_state`]
-#[derive(Debug, thiserror::Error, displaydoc::Display)]
-pub enum RestoreStateError {
-    /// {0}
-    GicError(crate::arch::aarch64::gic::GicError),
-    /// {0}
-    VmError(VmError),
+#[derive(Debug, PartialEq, Eq, thiserror::Error, displaydoc::Display)]
+pub enum ArchVmError {
+    /// Error creating the global interrupt controller: {0}
+    VmCreateGIC(crate::arch::aarch64::gic::GicError),
+    /// Failed to save the VM's GIC state: {0}
+    SaveGic(crate::arch::aarch64::gic::GicError),
+    /// Failed to restore the VM's GIC state: {0}
+    RestoreGic(crate::arch::aarch64::gic::GicError),
 }
 
 impl Vm {
     /// Creates the GIC (Global Interrupt Controller).
-    pub fn setup_irqchip(&mut self, vcpu_count: u8) -> Result<(), VmError> {
+    pub fn setup_irqchip(&mut self, vcpu_count: u8) -> Result<(), ArchVmError> {
         self.irqchip_handle = Some(
             crate::arch::aarch64::gic::create_gic(&self.fd, vcpu_count.into(), None)
-                .map_err(VmError::VmCreateGIC)?,
+                .map_err(ArchVmError::VmCreateGIC)?,
         );
         Ok(())
     }
@@ -32,12 +33,12 @@ impl Vm {
     }
 
     /// Saves and returns the Kvm Vm state.
-    pub fn save_state(&self, mpidrs: &[u64]) -> Result<crate::vstate::vm::VmState, VmError> {
-        Ok(crate::vstate::vm::VmState {
+    pub fn save_state(&self, mpidrs: &[u64]) -> Result<VmState, ArchVmError> {
+        Ok(VmState {
             gic: self
                 .get_irqchip()
                 .save_device(mpidrs)
-                .map_err(VmError::SaveGic)?,
+                .map_err(ArchVmError::SaveGic)?,
         })
     }
 
@@ -46,14 +47,10 @@ impl Vm {
     /// # Errors
     ///
     /// When [`crate::arch::aarch64::gic::GICDevice::restore_device`] errors.
-    pub fn restore_state(
-        &mut self,
-        mpidrs: &[u64],
-        state: &crate::vstate::vm::VmState,
-    ) -> Result<(), RestoreStateError> {
+    pub fn restore_state(&mut self, mpidrs: &[u64], state: &VmState) -> Result<(), ArchVmError> {
         self.get_irqchip()
             .restore_device(mpidrs, &state.gic)
-            .map_err(RestoreStateError::GicError)?;
+            .map_err(ArchVmError::RestoreGic)?;
         Ok(())
     }
 }

src/vmm/src/vstate/vm/mod.rs

@@ -22,7 +22,7 @@ mod arch;
 #[path = "aarch64.rs"]
 mod arch;
 
-pub use arch::{RestoreStateError, VmState};
+pub use arch::{ArchVmError, VmState};
 
 /// Errors associated with the wrappers over KVM ioctls.
 /// Needs `rustfmt::skip` to make multiline comments work
@@ -31,37 +31,12 @@ pub use arch::{RestoreStateError, VmState};
 pub enum VmError {
     /// Cannot set the memory regions: {0}
     SetUserMemoryRegion(kvm_ioctls::Error),
-    #[cfg(target_arch = "aarch64")]
-    /// Error creating the global interrupt controller: {0}
-    VmCreateGIC(crate::arch::aarch64::gic::GicError),
     /// Cannot open the VM file descriptor: {0}
     VmFd(kvm_ioctls::Error),
-    #[cfg(target_arch = "x86_64")]
-    /// Failed to get KVM vm pit state: {0}
-    VmGetPit2(kvm_ioctls::Error),
-    #[cfg(target_arch = "x86_64")]
-    /// Failed to get KVM vm clock: {0}
-    VmGetClock(kvm_ioctls::Error),
-    #[cfg(target_arch = "x86_64")]
-    /// Failed to get KVM vm irqchip: {0}
-    VmGetIrqChip(kvm_ioctls::Error),
-    #[cfg(target_arch = "x86_64")]
-    /// Failed to set KVM vm pit state: {0}
-    VmSetPit2(kvm_ioctls::Error),
-    #[cfg(target_arch = "x86_64")]
-    /// Failed to set KVM vm clock: {0}
-    VmSetClock(kvm_ioctls::Error),
-    #[cfg(target_arch = "x86_64")]
-    /// Failed to set KVM vm irqchip: {0}
-    VmSetIrqChip(kvm_ioctls::Error),
     /// Cannot configure the microvm: {0}
     VmSetup(kvm_ioctls::Error),
-    #[cfg(target_arch = "aarch64")]
-    /// Failed to save the VM's GIC state: {0}
-    SaveGic(crate::arch::aarch64::gic::GicError),
-    #[cfg(target_arch = "aarch64")]
-    /// Failed to restore the VM's GIC state: {0}
-    RestoreGic(crate::arch::aarch64::gic::GicError),
+    /// {0}
+    Arch(#[from] ArchVmError),
 }
 
 /// A wrapper around creating and using a VM.

src/vmm/src/vstate/vm/x86_64.rs

@@ -9,14 +9,13 @@ use kvm_bindings::{
 };
 use serde::{Deserialize, Serialize};
 
-use crate::vstate::vm::VmError;
 use crate::Vm;
 
 /// Error type for [`Vm::restore_state`]
 #[allow(missing_docs)]
 #[cfg(target_arch = "x86_64")]
-#[derive(Debug, thiserror::Error, displaydoc::Display, PartialEq, Eq)]
-pub enum RestoreStateError {
+#[derive(Debug, PartialEq, Eq, thiserror::Error, displaydoc::Display)]
+pub enum ArchVmError {
     /// Set PIT2 error: {0}
     SetPit2(kvm_ioctls::Error),
     /// Set clock error: {0}
@@ -27,8 +26,18 @@ pub enum RestoreStateError {
     SetIrqChipPicSlave(kvm_ioctls::Error),
     /// Set IrqChipIoAPIC error: {0}
     SetIrqChipIoAPIC(kvm_ioctls::Error),
-    /// VM error: {0}
-    VmError(VmError),
+    /// Failed to get KVM vm pit state: {0}
+    VmGetPit2(kvm_ioctls::Error),
+    /// Failed to get KVM vm clock: {0}
+    VmGetClock(kvm_ioctls::Error),
+    /// Failed to get KVM vm irqchip: {0}
+    VmGetIrqChip(kvm_ioctls::Error),
+    /// Failed to set KVM vm pit state: {0}
+    VmSetPit2(kvm_ioctls::Error),
+    /// Failed to set KVM vm clock: {0}
+    VmSetClock(kvm_ioctls::Error),
+    /// Failed to set KVM vm irqchip: {0}
+    VmSetIrqChip(kvm_ioctls::Error),
 }
 
 impl Vm {
@@ -42,42 +51,46 @@ impl Vm {
     /// - [`kvm_ioctls::VmFd::set_irqchip`] errors.
     /// - [`kvm_ioctls::VmFd::set_irqchip`] errors.
     /// - [`kvm_ioctls::VmFd::set_irqchip`] errors.
-    pub fn restore_state(&mut self, state: &VmState) -> Result<(), RestoreStateError> {
+    pub fn restore_state(&mut self, state: &VmState) -> Result<(), ArchVmError> {
         self.fd
             .set_pit2(&state.pitstate)
-            .map_err(RestoreStateError::SetPit2)?;
+            .map_err(ArchVmError::SetPit2)?;
         self.fd
             .set_clock(&state.clock)
-            .map_err(RestoreStateError::SetClock)?;
+            .map_err(ArchVmError::SetClock)?;
         self.fd
             .set_irqchip(&state.pic_master)
-            .map_err(RestoreStateError::SetIrqChipPicMaster)?;
+            .map_err(ArchVmError::SetIrqChipPicMaster)?;
         self.fd
             .set_irqchip(&state.pic_slave)
-            .map_err(RestoreStateError::SetIrqChipPicSlave)?;
+            .map_err(ArchVmError::SetIrqChipPicSlave)?;
         self.fd
             .set_irqchip(&state.ioapic)
-            .map_err(RestoreStateError::SetIrqChipIoAPIC)?;
+            .map_err(ArchVmError::SetIrqChipIoAPIC)?;
         Ok(())
     }
 
     /// Creates the irq chip and an in-kernel device model for the PIT.
-    pub fn setup_irqchip(&self) -> Result<(), VmError> {
-        self.fd.create_irq_chip().map_err(VmError::VmSetup)?;
+    pub fn setup_irqchip(&self) -> Result<(), ArchVmError> {
+        self.fd
+            .create_irq_chip()
+            .map_err(ArchVmError::VmSetIrqChip)?;
         // We need to enable the emulation of a dummy speaker port stub so that writing to port 0x61
         // (i.e. KVM_SPEAKER_BASE_ADDRESS) does not trigger an exit to user space.
         let pit_config = kvm_pit_config {
             flags: KVM_PIT_SPEAKER_DUMMY,
             ..Default::default()
         };
-        self.fd.create_pit2(pit_config).map_err(VmError::VmSetup)
+        self.fd
+            .create_pit2(pit_config)
+            .map_err(ArchVmError::VmSetIrqChip)
     }
 
     /// Saves and returns the Kvm Vm state.
-    pub fn save_state(&self) -> Result<VmState, VmError> {
-        let pitstate = self.fd.get_pit2().map_err(VmError::VmGetPit2)?;
+    pub fn save_state(&self) -> Result<VmState, ArchVmError> {
+        let pitstate = self.fd.get_pit2().map_err(ArchVmError::VmGetPit2)?;
 
-        let mut clock = self.fd.get_clock().map_err(VmError::VmGetClock)?;
+        let mut clock = self.fd.get_clock().map_err(ArchVmError::VmGetClock)?;
         // This bit is not accepted in SET_CLOCK, clear it.
         clock.flags &= !KVM_CLOCK_TSC_STABLE;
 
@@ -87,23 +100,23 @@ impl Vm {
         };
         self.fd
             .get_irqchip(&mut pic_master)
-            .map_err(VmError::VmGetIrqChip)?;
+            .map_err(ArchVmError::VmGetIrqChip)?;
 
         let mut pic_slave = kvm_irqchip {
             chip_id: KVM_IRQCHIP_PIC_SLAVE,
             ..Default::default()
         };
         self.fd
             .get_irqchip(&mut pic_slave)
-            .map_err(VmError::VmGetIrqChip)?;
+            .map_err(ArchVmError::VmGetIrqChip)?;
 
         let mut ioapic = kvm_irqchip {
             chip_id: KVM_IRQCHIP_IOAPIC,
             ..Default::default()
         };
         self.fd
             .get_irqchip(&mut ioapic)
-            .map_err(VmError::VmGetIrqChip)?;
+            .map_err(ArchVmError::VmGetIrqChip)?;
 
         Ok(VmState {
             pitstate,