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.

On the Aarch64, we need to recreate the expected dtb files because
default microVMs always create entries for the VMGenID device.

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

Author: bchalios

src/vmm/src/arch/aarch64/fdt.rs

@@ -275,17 +275,15 @@ fn create_chosen_node(
     Ok(())
 }
 
-fn create_vmgenid_node(fdt: &mut FdtWriter, vmgenid: &Option<VmGenId>) -> Result<(), FdtError> {
-    if let Some(vmgenid_info) = vmgenid {
-        let vmgenid = fdt.begin_node("vmgenid")?;
-        fdt.property_string("compatible", "microsoft,vmgenid")?;
-        fdt.property_array_u64("reg", &[vmgenid_info.guest_address.0, VMGENID_MEM_SIZE])?;
-        fdt.property_array_u32(
-            "interrupts",
-            &[GIC_FDT_IRQ_TYPE_SPI, vmgenid_info.gsi, IRQ_TYPE_EDGE_RISING],
-        )?;
-        fdt.end_node(vmgenid)?;
-    }
+fn create_vmgenid_node(fdt: &mut FdtWriter, vmgenid: &VmGenId) -> Result<(), FdtError> {
+    let vmgenid_node = fdt.begin_node("vmgenid")?;
+    fdt.property_string("compatible", "microsoft,vmgenid")?;
+    fdt.property_array_u64("reg", &[vmgenid.guest_address.0, VMGENID_MEM_SIZE])?;
+    fdt.property_array_u32(
+        "interrupts",
+        &[GIC_FDT_IRQ_TYPE_SPI, vmgenid.gsi, IRQ_TYPE_EDGE_RISING],
+    )?;
+    fdt.end_node(vmgenid_node)?;
     Ok(())
 }
 
@@ -586,29 +584,6 @@ mod tests {
         .unwrap();
     }
 
-    #[test]
-    fn test_create_fdt_with_vmgenid() {
-        let mem = arch_mem(layout::FDT_MAX_SIZE + 0x1000);
-        let mut device_manager = default_device_manager();
-        let kvm = Kvm::new(vec![]).unwrap();
-        let vm = Vm::new(&kvm).unwrap();
-        let gic = create_gic(vm.fd(), 1, None).unwrap();
-        let mut cmdline = kernel_cmdline::Cmdline::new(4096).unwrap();
-        cmdline.insert("console", "/dev/tty0").unwrap();
-
-        device_manager.attach_vmgenid_device(&mem, &vm).unwrap();
-
-        create_fdt(
-            &mem,
-            vec![0],
-            CString::new("console=tty0").unwrap(),
-            &device_manager,
-            &gic,
-            &None,
-        )
-        .unwrap();
-    }
-
     #[test]
     fn test_create_fdt() {
         let mem = arch_mem(layout::FDT_MAX_SIZE + 0x1000);

src/vmm/src/arch/aarch64/output_GICv3.dtb

@@ -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/arch/aarch64/output_initrd_GICv3.dtb

@@ -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/builder.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/acpi.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)
     }
 }