acpi: simplify the construction of VMGenID device

Decouple creation of VMGenID device with writing the generation ID in
guest memory. This way we can avoid keeping an `Option<VmGenId>` inside
the ACPI device manager. We can always create it and only write the
generation ID in guest memory once we are ready to activate the device
(when we have a functioning guest memory object).

Also, remove a few of the error types and fail in-place instead of
propagating errors all the way up.

Signed-off-by: Babis Chalios <bchalios@amazon.es>

Author: bchalios

src/vmm/src/builder.rs

@@ -29,7 +29,6 @@ use crate::device_manager::{
     AttachDeviceError, DeviceManager, DeviceManagerCreateError, DevicePersistError,
     DeviceRestoreArgs,
 };
-use crate::devices::acpi::vmgenid::VmGenIdError;
 use crate::devices::virtio::balloon::Balloon;
 use crate::devices::virtio::block::device::Block;
 use crate::devices::virtio::net::Net;
@@ -76,8 +75,6 @@ pub enum StartMicrovmError {
     /// Error creating legacy device: {0}
     #[cfg(target_arch = "x86_64")]
     CreateLegacyDevice(device_manager::legacy::LegacyDeviceError),
-    /// Error creating VMGenID device: {0}
-    CreateVMGenID(VmGenIdError),
     /// Error enabling PCIe support: {0}
     EnablePciDevices(#[from] PciManagerError),
     /// Error enabling pvtime on vcpu: {0}
@@ -258,7 +255,7 @@ pub fn build_microvm_for_boot(
         vm_resources.serial_out_path.as_ref(),
     )?;
 
-    device_manager.attach_vmgenid_device(vm.guest_memory(), &vm)?;
+    device_manager.attach_vmgenid_device(&vm)?;
 
     #[cfg(target_arch = "aarch64")]
     if vcpus[0].kvm_vcpu.supports_pvtime() {
@@ -943,10 +940,7 @@ pub(crate) mod tests {
 
     #[cfg(target_arch = "x86_64")]
     pub(crate) fn insert_vmgenid_device(vmm: &mut Vmm) {
-        vmm.device_manager
-            .attach_vmgenid_device(vmm.vm.guest_memory(), &vmm.vm)
-            .unwrap();
-        assert!(vmm.device_manager.acpi_devices.vmgenid.is_some());
+        vmm.device_manager.attach_vmgenid_device(&vmm.vm).unwrap();
     }
 
     pub(crate) fn insert_balloon_device(

src/vmm/src/device_manager/acpi.rs

@@ -2,84 +2,79 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use acpi_tables::{Aml, aml};
+use vm_memory::GuestMemoryError;
 
 use crate::Vm;
 use crate::devices::acpi::vmgenid::VmGenId;
+use crate::vstate::resources::ResourceAllocator;
 
-#[derive(Debug, Default)]
+#[derive(Debug, thiserror::Error, displaydoc::Display)]
+pub enum ACPIDeviceError {
+    /// Could not register GSI with KVM: {0}
+    RegisterIrq(#[from] kvm_ioctls::Error),
+    /// Could not write to guest memory: {0}
+    WriteGuestMemory(#[from] GuestMemoryError),
+}
+
+#[derive(Debug)]
 pub struct ACPIDeviceManager {
     /// VMGenID device
-    pub vmgenid: Option<VmGenId>,
+    pub vmgenid: VmGenId,
 }
 
 impl ACPIDeviceManager {
     /// Create a new ACPIDeviceManager object
-    pub fn new() -> Self {
-        Default::default()
-    }
-
-    /// Attach a new VMGenID device to the microVM
-    ///
-    /// This will register the device's interrupt with KVM
-    pub fn attach_vmgenid(&mut self, vmgenid: VmGenId, vm: &Vm) -> Result<(), kvm_ioctls::Error> {
-        vm.register_irq(&vmgenid.interrupt_evt, vmgenid.gsi)?;
-        self.vmgenid = Some(vmgenid);
-        Ok(())
+    pub fn new(resource_allocator: &mut ResourceAllocator) -> Self {
+        let vmgenid = VmGenId::new(resource_allocator);
+        ACPIDeviceManager { vmgenid }
     }
 
-    /// If it exists, notify guest VMGenID device that we have resumed from a snapshot.
-    pub fn notify_vmgenid(&mut self) -> Result<(), std::io::Error> {
-        if let Some(vmgenid) = &mut self.vmgenid {
-            vmgenid.notify_guest()?;
-        }
+    pub fn attach_vmgenid(&self, vm: &Vm) -> Result<(), ACPIDeviceError> {
+        vm.register_irq(&self.vmgenid.interrupt_evt, self.vmgenid.gsi)?;
+        self.vmgenid.activate(vm.guest_memory())?;
         Ok(())
     }
 }
 
 impl Aml for ACPIDeviceManager {
     fn append_aml_bytes(&self, v: &mut Vec<u8>) -> Result<(), aml::AmlError> {
-        // If we have a VMGenID device, create the AML for the device and GED interrupt handler
-        match self.vmgenid.as_ref() {
-            Some(vmgenid) => {
-                // AML for GED
-                aml::Device::new(
-                    "_SB_.GED_".try_into()?,
-                    vec![
-                        &aml::Name::new("_HID".try_into()?, &"ACPI0013")?,
-                        &aml::Name::new(
-                            "_CRS".try_into()?,
-                            &aml::ResourceTemplate::new(vec![&aml::Interrupt::new(
-                                true,
-                                true,
-                                false,
-                                false,
-                                vmgenid.gsi,
-                            )]),
-                        )?,
-                        &aml::Method::new(
-                            "_EVT".try_into()?,
-                            1,
-                            true,
-                            vec![&aml::If::new(
-                                // We know that the maximum IRQ number fits in a u8. We have up to
-                                // 32 IRQs in x86 and up to 128 in
-                                // ARM (look into
-                                // `vmm::crate::arch::layout::GSI_LEGACY_END`)
-                                #[allow(clippy::cast_possible_truncation)]
-                                &aml::Equal::new(&aml::Arg(0), &(vmgenid.gsi as u8)),
-                                vec![&aml::Notify::new(
-                                    &aml::Path::new("\\_SB_.VGEN")?,
-                                    &0x80usize,
-                                )],
-                            )],
-                        ),
-                    ],
-                )
-                .append_aml_bytes(v)?;
-                // AML for VMGenID itself.
-                vmgenid.append_aml_bytes(v)
-            }
-            None => Ok(()),
-        }
+        // AML for [`VmGenId`] device.
+        self.vmgenid.append_aml_bytes(v)?;
+
+        // Create the AML for the GED interrupt handler
+        aml::Device::new(
+            "_SB_.GED_".try_into()?,
+            vec![
+                &aml::Name::new("_HID".try_into()?, &"ACPI0013")?,
+                &aml::Name::new(
+                    "_CRS".try_into()?,
+                    &aml::ResourceTemplate::new(vec![&aml::Interrupt::new(
+                        true,
+                        true,
+                        false,
+                        false,
+                        self.vmgenid.gsi,
+                    )]),
+                )?,
+                &aml::Method::new(
+                    "_EVT".try_into()?,
+                    1,
+                    true,
+                    vec![&aml::If::new(
+                        // We know that the maximum IRQ number fits in a u8. We have up to
+                        // 32 IRQs in x86 and up to 128 in
+                        // ARM (look into
+                        // `vmm::crate::arch::layout::GSI_LEGACY_END`)
+                        #[allow(clippy::cast_possible_truncation)]
+                        &aml::Equal::new(&aml::Arg(0), &(self.vmgenid.gsi as u8)),
+                        vec![&aml::Notify::new(
+                            &aml::Path::new("\\_SB_.VGEN")?,
+                            &0x80usize,
+                        )],
+                    )],
+                ),
+            ],
+        )
+        .append_aml_bytes(v)
     }
 }

src/vmm/src/device_manager/mod.rs

@@ -18,12 +18,12 @@ use linux_loader::loader::Cmdline;
 use log::{error, info};
 use mmio::{MMIODeviceManager, MmioError};
 use pci_mngr::{PciDevices, PciDevicesConstructorArgs, PciManagerError};
-use persist::{ACPIDeviceManagerConstructorArgs, MMIODevManagerConstructorArgs};
+use persist::MMIODevManagerConstructorArgs;
 use serde::{Deserialize, Serialize};
 use utils::time::TimestampUs;
 use vmm_sys_util::eventfd::EventFd;
 
-use crate::devices::acpi::vmgenid::{VmGenId, VmGenIdError};
+use crate::device_manager::acpi::ACPIDeviceError;
 #[cfg(target_arch = "x86_64")]
 use crate::devices::legacy::I8042Device;
 #[cfg(target_arch = "aarch64")]
@@ -70,10 +70,8 @@ pub enum AttachDeviceError {
     MmioTransport(#[from] MmioError),
     /// Error inserting device in bus: {0}
     Bus(#[from] BusError),
-    /// Error creating VMGenID device: {0}
-    CreateVmGenID(#[from] VmGenIdError),
-    /// Error while registering VMGenID with KVM: {0}
-    AttachVmGenID(#[from] kvm_ioctls::Error),
+    /// Error while registering ACPI with KVM: {0}
+    AttachAcpiDevice(#[from] ACPIDeviceError),
     #[cfg(target_arch = "aarch64")]
     /// Cmdline error
     Cmdline,
@@ -176,7 +174,7 @@ impl DeviceManager {
             mmio_devices: MMIODeviceManager::new(),
             #[cfg(target_arch = "x86_64")]
             legacy_devices,
-            acpi_devices: ACPIDeviceManager::new(),
+            acpi_devices: ACPIDeviceManager::new(&mut vm.resource_allocator()),
             pci_devices: PciDevices::new(),
         })
     }
@@ -234,13 +232,8 @@ impl DeviceManager {
         Ok(())
     }
 
-    pub(crate) fn attach_vmgenid_device(
-        &mut self,
-        mem: &GuestMemoryMmap,
-        vm: &Vm,
-    ) -> Result<(), AttachDeviceError> {
-        let vmgenid = VmGenId::new(mem, &mut vm.resource_allocator())?;
-        self.acpi_devices.attach_vmgenid(vmgenid, vm)?;
+    pub(crate) fn attach_vmgenid_device(&mut self, vm: &Vm) -> Result<(), AttachDeviceError> {
+        self.acpi_devices.attach_vmgenid(vm)?;
         Ok(())
     }
 
@@ -394,7 +387,7 @@ pub enum DevicePersistError {
     /// Error restoring MMIO devices: {0}
     MmioRestore(#[from] persist::DevicePersistError),
     /// Error restoring ACPI devices: {0}
-    AcpiRestore(#[from] persist::ACPIDeviceManagerRestoreError),
+    AcpiRestore(#[from] ACPIDeviceError),
     /// Error restoring PCI devices: {0}
     PciRestore(#[from] PciManagerError),
     /// Error notifying VMGenID device: {0}
@@ -464,12 +457,8 @@ impl<'a> Persist<'a> for DeviceManager {
         let mmio_devices = MMIODeviceManager::restore(mmio_ctor_args, &state.mmio_state)?;
 
         // Restore ACPI devices
-        let acpi_ctor_args = ACPIDeviceManagerConstructorArgs {
-            mem: constructor_args.mem,
-            vm: constructor_args.vm,
-        };
-        let mut acpi_devices = ACPIDeviceManager::restore(acpi_ctor_args, &state.acpi_state)?;
-        acpi_devices.notify_vmgenid()?;
+        let mut acpi_devices = ACPIDeviceManager::restore(constructor_args.vm, &state.acpi_state)?;
+        acpi_devices.vmgenid.notify_guest()?;
 
         // Restore PCI devices
         let pci_ctor_args = PciDevicesConstructorArgs {
@@ -542,10 +531,12 @@ pub(crate) mod tests {
     use super::*;
     #[cfg(target_arch = "aarch64")]
     use crate::builder::tests::default_vmm;
+    use crate::vstate::resources::ResourceAllocator;
 
     pub(crate) fn default_device_manager() -> DeviceManager {
+        let mut resource_allocator = ResourceAllocator::new();
         let mmio_devices = MMIODeviceManager::new();
-        let acpi_devices = ACPIDeviceManager::new();
+        let acpi_devices = ACPIDeviceManager::new(&mut resource_allocator);
         let pci_devices = PciDevices::new();
 
         #[cfg(target_arch = "x86_64")]

src/vmm/src/device_manager/persist.rs

@@ -14,7 +14,8 @@ use super::acpi::ACPIDeviceManager;
 use super::mmio::*;
 #[cfg(target_arch = "aarch64")]
 use crate::arch::DeviceType;
-use crate::devices::acpi::vmgenid::{VMGenIDState, VMGenIdConstructorArgs, VmGenId, VmGenIdError};
+use crate::device_manager::acpi::ACPIDeviceError;
+use crate::devices::acpi::vmgenid::{VMGenIDState, VmGenId};
 #[cfg(target_arch = "aarch64")]
 use crate::devices::legacy::RTCDevice;
 use crate::devices::virtio::ActivateError;
@@ -156,50 +157,28 @@ impl fmt::Debug for MMIODevManagerConstructorArgs<'_> {
 
 #[derive(Default, Debug, Clone, Serialize, Deserialize)]
 pub struct ACPIDeviceManagerState {
-    vmgenid: Option<VMGenIDState>,
-}
-
-#[derive(Debug)]
-pub struct ACPIDeviceManagerConstructorArgs<'a> {
-    pub mem: &'a GuestMemoryMmap,
-    pub vm: &'a Vm,
-}
-
-#[derive(Debug, thiserror::Error, displaydoc::Display)]
-pub enum ACPIDeviceManagerRestoreError {
-    /// Could not register device: {0}
-    Interrupt(#[from] kvm_ioctls::Error),
-    /// Could not create VMGenID device: {0}
-    VMGenID(#[from] VmGenIdError),
+    vmgenid: VMGenIDState,
 }
 
 impl<'a> Persist<'a> for ACPIDeviceManager {
     type State = ACPIDeviceManagerState;
-    type ConstructorArgs = ACPIDeviceManagerConstructorArgs<'a>;
-    type Error = ACPIDeviceManagerRestoreError;
+    type ConstructorArgs = &'a Vm;
+    type Error = ACPIDeviceError;
 
     fn save(&self) -> Self::State {
         ACPIDeviceManagerState {
-            vmgenid: self.vmgenid.as_ref().map(|dev| dev.save()),
+            vmgenid: self.vmgenid.save(),
         }
     }
 
-    fn restore(
-        constructor_args: Self::ConstructorArgs,
-        state: &Self::State,
-    ) -> Result<Self, Self::Error> {
-        let mut dev_manager = ACPIDeviceManager::new();
-        if let Some(vmgenid_args) = &state.vmgenid {
-            let vmgenid = VmGenId::restore(
-                VMGenIdConstructorArgs {
-                    mem: constructor_args.mem,
-                    resource_allocator: &mut constructor_args.vm.resource_allocator(),
-                },
-                vmgenid_args,
-            )?;
-            dev_manager.attach_vmgenid(vmgenid, constructor_args.vm)?;
-        }
-        Ok(dev_manager)
+    fn restore(vm: Self::ConstructorArgs, state: &Self::State) -> Result<Self, Self::Error> {
+        let acpi_devices = ACPIDeviceManager {
+            // This can't fail
+            vmgenid: VmGenId::restore(vm.guest_memory(), &state.vmgenid).unwrap(),
+        };
+
+        acpi_devices.attach_vmgenid(vm)?;
+        Ok(acpi_devices)
     }
 }