refactor(acpi): Propagate VMClock/VMGenID error

Now VMClock is the last device that gets GSI allocated.  Because VMClock
panics when GSI allocation fails, test_attach_too_many_devices now
fails.  Define error types and propagate errors so that the API client
receives the error message.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>

Author: zulinx86

src/vmm/src/device_manager/acpi.rs

@@ -3,21 +3,20 @@
 
 #[cfg(target_arch = "x86_64")]
 use acpi_tables::{Aml, aml};
-use vm_memory::GuestMemoryError;
 
 use crate::Vm;
-use crate::devices::acpi::vmclock::VmClock;
-use crate::devices::acpi::vmgenid::VmGenId;
+use crate::devices::acpi::vmclock::{VmClock, VmClockError};
+use crate::devices::acpi::vmgenid::{VmGenId, VmGenIdError};
 use crate::vstate::memory::GuestMemoryMmap;
 
 #[derive(Debug, thiserror::Error, displaydoc::Display)]
 pub enum ACPIDeviceError {
-    /// Could not register GSI with KVM: {0}
+    /// VMGenID: {0}
+    VmGenId(#[from] VmGenIdError),
+    /// VMClock: {0}
+    VmClock(#[from] VmClockError),
+    /// Could not register IRQ with KVM: {0}
     RegisterIrq(#[from] kvm_ioctls::Error),
-    /// Could not write to guest memory: {0}
-    WriteGuestMemory(#[from] GuestMemoryError),
-    /// Could not notify guest: {0}
-    NotifyGuest(#[from] std::io::Error),
 }
 
 // Although both VMGenID and VMClock devices are always present, they should be instantiated when
@@ -39,12 +38,14 @@ impl ACPIDeviceManager {
         }
     }
 
-    pub fn attach_vmgenid(&mut self, vm: &Vm) {
-        self.vmgenid = Some(VmGenId::new(&mut vm.resource_allocator()));
+    pub fn attach_vmgenid(&mut self, vm: &Vm) -> Result<(), ACPIDeviceError> {
+        self.vmgenid = Some(VmGenId::new(&mut vm.resource_allocator())?);
+        Ok(())
     }
 
-    pub fn attach_vmclock(&mut self, vm: &Vm) {
-        self.vmclock = Some(VmClock::new(&mut vm.resource_allocator()));
+    pub fn attach_vmclock(&mut self, vm: &Vm) -> Result<(), ACPIDeviceError> {
+        self.vmclock = Some(VmClock::new(&mut vm.resource_allocator())?);
+        Ok(())
     }
 
     pub fn vmgenid(&self) -> &VmGenId {
@@ -72,11 +73,12 @@ impl ACPIDeviceManager {
         Ok(())
     }
 
-    pub fn post_restore_vmclock(&mut self, mem: &GuestMemoryMmap) {
+    pub fn post_restore_vmclock(&mut self, mem: &GuestMemoryMmap) -> Result<(), ACPIDeviceError> {
         self.vmclock
             .as_mut()
             .expect("Missing VMClock device")
-            .post_restore(mem);
+            .post_restore(mem)?;
+        Ok(())
     }
 }
 

src/vmm/src/device_manager/mod.rs

@@ -233,13 +233,13 @@ impl DeviceManager {
     }
 
     pub(crate) fn attach_vmgenid_device(&mut self, vm: &Vm) -> Result<(), AttachDeviceError> {
-        self.acpi_devices.attach_vmgenid(vm);
+        self.acpi_devices.attach_vmgenid(vm)?;
         self.acpi_devices.activate_vmgenid(vm)?;
         Ok(())
     }
 
     pub(crate) fn attach_vmclock_device(&mut self, vm: &Vm) -> Result<(), AttachDeviceError> {
-        self.acpi_devices.attach_vmclock(vm);
+        self.acpi_devices.attach_vmclock(vm)?;
         self.acpi_devices.activate_vmclock(vm)?;
         Ok(())
     }
@@ -560,8 +560,8 @@ pub(crate) mod tests {
         let mut resource_allocator = ResourceAllocator::new();
         let mmio_devices = MMIODeviceManager::new();
         let acpi_devices = ACPIDeviceManager::new(
-            VmGenId::new(&mut resource_allocator),
-            VmClock::new(&mut resource_allocator),
+            VmGenId::new(&mut resource_allocator).unwrap(),
+            VmClock::new(&mut resource_allocator).unwrap(),
         );
         let pci_devices = PciDevices::new();
 

src/vmm/src/device_manager/persist.rs

@@ -194,7 +194,7 @@ impl<'a> Persist<'a> for ACPIDeviceManager {
         acpi_devices.post_restore_vmgenid()?;
 
         acpi_devices.activate_vmclock(vm)?;
-        acpi_devices.post_restore_vmclock(vm.guest_memory());
+        acpi_devices.post_restore_vmclock(vm.guest_memory())?;
 
         Ok(acpi_devices)
     }

src/vmm/src/devices/acpi/vmclock.rs

@@ -43,6 +43,20 @@ macro_rules! write_vmclock_field {
     };
 }
 
+#[derive(Debug, thiserror::Error, displaydoc::Display)]
+pub enum VmClockError {
+    /// Could not create EventFd: {0}
+    CreateEventFd(std::io::Error),
+    /// Could not allocate GSI: {0}
+    AllocateGsi(vm_allocator::Error),
+    /// Could not allocate guest memory: {0}
+    AllocateMemory(vm_allocator::Error),
+    /// Could not write VMClock data to guest memory: {0}
+    WriteGuestMemory(#[from] GuestMemoryError),
+    /// Could not notify guest: {0}
+    NotifyGuest(std::io::Error),
+}
+
 /// VMclock device
 ///
 /// This device emulates the VMclock device which allows passing information to the guest related
@@ -62,22 +76,21 @@ pub struct VmClock {
 
 impl VmClock {
     /// Create a new [`VmClock`] device for a newly booted VM
-    pub fn new(resource_allocator: &mut ResourceAllocator) -> VmClock {
+    pub fn new(resource_allocator: &mut ResourceAllocator) -> Result<VmClock, VmClockError> {
         let addr = resource_allocator
             .allocate_system_memory(
                 VMCLOCK_SIZE as u64,
                 VMCLOCK_SIZE as u64,
                 AllocPolicy::LastMatch,
             )
-            .expect("vmclock: could not allocate guest memory for device");
+            .map_err(VmClockError::AllocateMemory)?;
 
         let gsi = resource_allocator
             .allocate_gsi_legacy(1)
-            .expect("vmclock: Could not allocate GSI for VMClock: {err}")[0];
+            .map_err(VmClockError::AllocateGsi)?[0];
 
         let interrupt_evt = EventFdTrigger::new(
-            EventFd::new(libc::EFD_NONBLOCK)
-                .expect("vmclock: Could not create EventFd for VMClock device: {err}"),
+            EventFd::new(libc::EFD_NONBLOCK).map_err(VmClockError::CreateEventFd)?,
         );
 
         let mut inner = vmclock_abi {
@@ -90,22 +103,22 @@ impl VmClock {
             ..Default::default()
         };
 
-        VmClock {
+        Ok(VmClock {
             guest_address: GuestAddress(addr),
             interrupt_evt,
             gsi,
             inner,
-        }
+        })
     }
 
     /// Activate [`VmClock`] device
-    pub fn activate(&self, mem: &GuestMemoryMmap) -> Result<(), GuestMemoryError> {
+    pub fn activate(&self, mem: &GuestMemoryMmap) -> Result<(), VmClockError> {
         mem.write_slice(self.inner.as_slice(), self.guest_address)?;
         Ok(())
     }
 
     /// Bump the VM generation counter and notify guest after snapshot restore
-    pub fn post_restore(&mut self, mem: &GuestMemoryMmap) {
+    pub fn post_restore(&mut self, mem: &GuestMemoryMmap) -> Result<(), VmClockError> {
         write_vmclock_field!(self, mem, seq_count, self.inner.seq_count | 1);
 
         // This fence ensures guest sees all previous writes. It is matched to a
@@ -133,8 +146,9 @@ impl VmClock {
         write_vmclock_field!(self, mem, seq_count, self.inner.seq_count.wrapping_add(1));
         self.interrupt_evt
             .trigger()
-            .expect("vmclock: could not send guest notification: {err}");
+            .map_err(VmClockError::NotifyGuest)?;
         debug!("vmclock: notifying guest about VMClock updates");
+        Ok(())
     }
 }
 
@@ -154,7 +168,7 @@ pub struct VmClockState {
 impl<'a> Persist<'a> for VmClock {
     type State = VmClockState;
     type ConstructorArgs = ();
-    type Error = Infallible;
+    type Error = VmClockError;
 
     fn save(&self) -> Self::State {
         VmClockState {
@@ -166,8 +180,7 @@ impl<'a> Persist<'a> for VmClock {
 
     fn restore(vm: Self::ConstructorArgs, state: &Self::State) -> Result<Self, Self::Error> {
         let interrupt_evt = EventFdTrigger::new(
-            EventFd::new(libc::EFD_NONBLOCK)
-                .expect("vmclock: Could not create EventFd for VMClock device: {err}"),
+            EventFd::new(libc::EFD_NONBLOCK).map_err(VmClockError::CreateEventFd)?,
         );
         Ok(VmClock {
             guest_address: GuestAddress(state.guest_address),
@@ -231,7 +244,7 @@ mod tests {
 
     fn default_vmclock() -> VmClock {
         let mut resource_allocator = ResourceAllocator::new();
-        VmClock::new(&mut resource_allocator)
+        VmClock::new(&mut resource_allocator).unwrap()
     }
 
     #[test]

src/vmm/src/devices/acpi/vmgenid.rs

@@ -20,6 +20,20 @@ use crate::vstate::resources::ResourceAllocator;
 /// Bytes of memory we allocate for VMGenID device
 pub const VMGENID_MEM_SIZE: u64 = 16;
 
+#[derive(Debug, thiserror::Error, displaydoc::Display)]
+pub enum VmGenIdError {
+    /// Could not create EventFd: {0}
+    CreateEventFd(std::io::Error),
+    /// Could not allocate GSI: {0}
+    AllocateGsi(vm_allocator::Error),
+    /// Could not allocate guest memory: {0}
+    AllocateMemory(vm_allocator::Error),
+    /// Could not write generation ID to guest memory: {0}
+    WriteGuestMemory(#[from] GuestMemoryError),
+    /// Could not notify guest: {0}
+    NotifyGuest(std::io::Error),
+}
+
 /// Virtual Machine Generation ID device
 ///
 /// VMGenID is an emulated device which exposes to the guest a 128-bit cryptographically random
@@ -43,38 +57,37 @@ pub struct VmGenId {
 impl VmGenId {
     /// Create a new Vm Generation Id device using an address in the guest for writing the
     /// generation ID and a GSI for sending device notifications.
-    pub fn from_parts(guest_address: GuestAddress, gsi: u32) -> Self {
+    pub fn from_parts(guest_address: GuestAddress, gsi: u32) -> Result<Self, VmGenIdError> {
         debug!(
             "vmgenid: building VMGenID device. Address: {:#010x}. IRQ: {}",
             guest_address.0, gsi
         );
         let interrupt_evt = EventFdTrigger::new(
-            EventFd::new(libc::EFD_NONBLOCK)
-                .expect("vmgenid: Could not create EventFd for VMGenID device"),
+            EventFd::new(libc::EFD_NONBLOCK).map_err(VmGenIdError::CreateEventFd)?,
         );
         let gen_id = Self::make_genid();
 
-        Self {
+        Ok(Self {
             gen_id,
             interrupt_evt,
             guest_address,
             gsi,
-        }
+        })
     }
 
     /// Create a new VMGenID device
     ///
     /// Allocate memory and a GSI for sending notifications and build the device
-    pub fn new(resource_allocator: &mut ResourceAllocator) -> Self {
+    pub fn new(resource_allocator: &mut ResourceAllocator) -> Result<Self, VmGenIdError> {
         let gsi = resource_allocator
             .allocate_gsi_legacy(1)
-            .expect("vmgenid: Could not allocate GSI for VMGenID");
+            .map_err(VmGenIdError::AllocateGsi)?[0];
         // The generation ID needs to live in an 8-byte aligned buffer
         let addr = resource_allocator
             .allocate_system_memory(VMGENID_MEM_SIZE, 8, vm_allocator::AllocPolicy::LastMatch)
-            .expect("vmgenid: Could not allocate guest RAM for VMGenID");
+            .map_err(VmGenIdError::AllocateMemory)?;
 
-        Self::from_parts(GuestAddress(addr), gsi[0])
+        Self::from_parts(GuestAddress(addr), gsi)
     }
 
     // Create a 16-bytes random number
@@ -88,22 +101,22 @@ impl VmGenId {
     ///
     /// This will only have effect if we have updated the generation ID in guest memory, i.e. when
     /// re-creating the device after snapshot resumption.
-    pub fn post_restore(&self) -> Result<(), std::io::Error> {
+    pub fn post_restore(&self) -> Result<(), VmGenIdError> {
         self.interrupt_evt
             .trigger()
-            .inspect_err(|err| error!("vmgenid: could not send guest notification: {err}"))?;
+            .map_err(VmGenIdError::NotifyGuest)?;
         debug!("vmgenid: notifying guest about new generation ID");
         Ok(())
     }
 
     /// Attach the [`VmGenId`] device
-    pub fn activate(&self, mem: &GuestMemoryMmap) -> Result<(), GuestMemoryError> {
+    pub fn activate(&self, mem: &GuestMemoryMmap) -> Result<(), VmGenIdError> {
         debug!(
             "vmgenid: writing new generation ID to guest: {:#034x}",
             self.gen_id
         );
         mem.write_slice(&self.gen_id.to_le_bytes(), self.guest_address)
-            .inspect_err(|err| error!("vmgenid: could not write generation ID to guest: {err}"))?;
+            .map_err(VmGenIdError::WriteGuestMemory)?;
 
         Ok(())
     }
@@ -122,7 +135,7 @@ pub struct VMGenIDState {
 impl<'a> Persist<'a> for VmGenId {
     type State = VMGenIDState;
     type ConstructorArgs = ();
-    type Error = Infallible;
+    type Error = VmGenIdError;
 
     fn save(&self) -> Self::State {
         VMGenIDState {
@@ -132,7 +145,7 @@ impl<'a> Persist<'a> for VmGenId {
     }
 
     fn restore(_: Self::ConstructorArgs, state: &Self::State) -> Result<Self, Self::Error> {
-        Ok(Self::from_parts(GuestAddress(state.addr), state.gsi))
+        Self::from_parts(GuestAddress(state.addr), state.gsi)
     }
 }
 

tests/integration_tests/functional/test_max_devices.py

@@ -74,12 +74,9 @@ def test_attach_too_many_devices(uvm_plain):
     # Attempting to start a microVM with more than
     # `MAX_DEVICES_ATTACHED` devices should fail.
     error_str = (
-        ("Could not find an available device slot on the PCI bus.")
+        "Could not find an available device slot on the PCI bus."
         if test_microvm.pci_enabled
-        else (
-            "Failed to allocate requested resource: The requested resource"
-            " is not available."
-        )
+        else "Could not allocate GSI: The requested resource is not available."
     )
     with pytest.raises(RuntimeError, match=error_str):
         test_microvm.start()