fix(acpi): Instantiate ACPI devices when attached

Since ACPI devices (i.e. VMGenID and VMClock) are always present, we
avoided wrapping them with Option [1].  However, the refactoring changed
the order of GSI allocation (i.e. ACPI devices come before virtio
devices), which caused performance regression for some reasons.
Although we need to dive deep into why it affected performance, we
should keep the existing order until we find out the root cause.

[1]: a52d58d
Signed-off-by: Takahiro Itazuri <itazur@amazon.com>

Author: zulinx86

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

@@ -97,8 +97,8 @@ pub fn create_fdt(
     create_clock_node(&mut fdt_writer)?;
     create_psci_node(&mut fdt_writer)?;
     create_devices_node(&mut fdt_writer, device_manager)?;
-    create_vmgenid_node(&mut fdt_writer, &device_manager.acpi_devices.vmgenid)?;
-    create_vmclock_node(&mut fdt_writer, &device_manager.acpi_devices.vmclock)?;
+    create_vmgenid_node(&mut fdt_writer, device_manager.acpi_devices.vmgenid())?;
+    create_vmclock_node(&mut fdt_writer, device_manager.acpi_devices.vmclock())?;
     create_pci_nodes(&mut fdt_writer, &device_manager.pci_devices)?;
 
     // End Header node.

src/vmm/src/device_manager/acpi.rs

@@ -8,7 +8,7 @@ use vm_memory::GuestMemoryError;
 use crate::Vm;
 use crate::devices::acpi::vmclock::VmClock;
 use crate::devices::acpi::vmgenid::VmGenId;
-use crate::vstate::resources::ResourceAllocator;
+use crate::vstate::memory::GuestMemoryMmap;
 
 #[derive(Debug, thiserror::Error, displaydoc::Display)]
 pub enum ACPIDeviceError {
@@ -23,40 +23,70 @@ pub enum ACPIDeviceError {
 #[derive(Debug)]
 pub struct ACPIDeviceManager {
     /// VMGenID device
-    pub vmgenid: VmGenId,
+    pub vmgenid: Option<VmGenId>,
     /// VMclock device
-    pub vmclock: VmClock,
+    pub vmclock: Option<VmClock>,
 }
 
 impl ACPIDeviceManager {
     /// Create a new ACPIDeviceManager object
-    pub fn new(resource_allocator: &mut ResourceAllocator) -> Self {
+    pub fn new() -> Self {
+        // Although both VMGenID and VMClock devices are always present, they should be instantiated
+        // when they are attached to preserve the existing ordering of GSI allocation.
         ACPIDeviceManager {
-            vmgenid: VmGenId::new(resource_allocator),
-            vmclock: VmClock::new(resource_allocator),
+            vmgenid: None,
+            vmclock: None,
         }
     }
 
-    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())?;
+    pub fn attach_vmgenid(&mut self, vm: &Vm) {
+        self.vmgenid = Some(VmGenId::new(&mut vm.resource_allocator()));
+    }
+
+    pub fn attach_vmclock(&mut self, vm: &Vm) {
+        self.vmclock = Some(VmClock::new(&mut vm.resource_allocator()));
+    }
+
+    pub fn vmgenid(&self) -> &VmGenId {
+        self.vmgenid.as_ref().expect("Missing VMGenID device")
+    }
+
+    pub fn vmclock(&self) -> &VmClock {
+        self.vmclock.as_ref().expect("Missing VMClock device")
+    }
+
+    pub fn activate_vmgenid(&self, vm: &Vm) -> Result<(), ACPIDeviceError> {
+        vm.register_irq(&self.vmgenid().interrupt_evt, self.vmgenid().gsi)?;
+        self.vmgenid().activate(vm.guest_memory())?;
+        Ok(())
+    }
+
+    pub fn activate_vmclock(&self, vm: &Vm) -> Result<(), ACPIDeviceError> {
+        vm.register_irq(&self.vmclock().interrupt_evt, self.vmclock().gsi)?;
+        self.vmclock().activate(vm.guest_memory())?;
         Ok(())
     }
 
-    pub fn attach_vmclock(&self, vm: &Vm) -> Result<(), ACPIDeviceError> {
-        vm.register_irq(&self.vmclock.interrupt_evt, self.vmclock.gsi)?;
-        self.vmclock.activate(vm.guest_memory())?;
+    pub fn do_post_restore_vmgenid(&self) -> Result<(), ACPIDeviceError> {
+        self.vmgenid().do_post_restore()?;
         Ok(())
     }
+
+    pub fn do_post_restore_vmclock(&mut self, mem: &GuestMemoryMmap) {
+        self.vmclock
+            .as_mut()
+            .expect("Missing VMClock device")
+            .do_post_restore(mem);
+    }
 }
 
 #[cfg(target_arch = "x86_64")]
 impl Aml for ACPIDeviceManager {
     fn append_aml_bytes(&self, v: &mut Vec<u8>) -> Result<(), aml::AmlError> {
         // AML for [`VmGenId`] device.
-        self.vmgenid.append_aml_bytes(v)?;
+        self.vmgenid().append_aml_bytes(v)?;
         // AML for [`VmClock`] device.
-        self.vmclock.append_aml_bytes(v)?;
+        self.vmclock().append_aml_bytes(v)?;
 
         // Create the AML for the GED interrupt handler
         aml::Device::new(
@@ -66,8 +96,8 @@ impl Aml for ACPIDeviceManager {
                 &aml::Name::new(
                     "_CRS".try_into()?,
                     &aml::ResourceTemplate::new(vec![
-                        &aml::Interrupt::new(true, true, false, false, self.vmgenid.gsi),
-                        &aml::Interrupt::new(true, true, false, false, self.vmclock.gsi),
+                        &aml::Interrupt::new(true, true, false, false, self.vmgenid().gsi),
+                        &aml::Interrupt::new(true, true, false, false, self.vmclock().gsi),
                     ]),
                 )?,
                 // We know that the maximum IRQ number fits in a u8. We have up to
@@ -81,15 +111,15 @@ impl Aml for ACPIDeviceManager {
                     vec![
                         &aml::If::new(
                             #[allow(clippy::cast_possible_truncation)]
-                            &aml::Equal::new(&aml::Arg(0), &(self.vmgenid.gsi as u8)),
+                            &aml::Equal::new(&aml::Arg(0), &(self.vmgenid().gsi as u8)),
                             vec![&aml::Notify::new(
                                 &aml::Path::new("\\_SB_.VGEN")?,
                                 &0x80usize,
                             )],
                         ),
                         &aml::If::new(
                             #[allow(clippy::cast_possible_truncation)]
-                            &aml::Equal::new(&aml::Arg(0), &(self.vmclock.gsi as u8)),
+                            &aml::Equal::new(&aml::Arg(0), &(self.vmclock().gsi as u8)),
                             vec![&aml::Notify::new(
                                 &aml::Path::new("\\_SB_.VCLK")?,
                                 &0x80usize,

src/vmm/src/device_manager/mod.rs

@@ -174,7 +174,7 @@ impl DeviceManager {
             mmio_devices: MMIODeviceManager::new(),
             #[cfg(target_arch = "x86_64")]
             legacy_devices,
-            acpi_devices: ACPIDeviceManager::new(&mut vm.resource_allocator()),
+            acpi_devices: ACPIDeviceManager::new(),
             pci_devices: PciDevices::new(),
         })
     }
@@ -233,12 +233,14 @@ 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(())
     }
 
@@ -550,12 +552,17 @@ pub(crate) mod tests {
     use super::*;
     #[cfg(target_arch = "aarch64")]
     use crate::builder::tests::default_vmm;
+    use crate::devices::acpi::vmclock::VmClock;
+    use crate::devices::acpi::vmgenid::VmGenId;
     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(&mut resource_allocator);
+        let acpi_devices = ACPIDeviceManager {
+            vmgenid: Some(VmGenId::new(&mut resource_allocator)),
+            vmclock: Some(VmClock::new(&mut resource_allocator)),
+        };
         let pci_devices = PciDevices::new();
 
         #[cfg(target_arch = "x86_64")]

src/vmm/src/device_manager/persist.rs

@@ -177,24 +177,24 @@ impl<'a> Persist<'a> for ACPIDeviceManager {
 
     fn save(&self) -> Self::State {
         ACPIDeviceManagerState {
-            vmgenid: self.vmgenid.save(),
-            vmclock: self.vmclock.save(),
+            vmgenid: self.vmgenid().save(),
+            vmclock: self.vmclock().save(),
         }
     }
 
     fn restore(vm: Self::ConstructorArgs, state: &Self::State) -> Result<Self, Self::Error> {
         let mut acpi_devices = ACPIDeviceManager {
             // Safe to unwrap() here, this will never return an error.
-            vmgenid: VmGenId::restore((), &state.vmgenid).unwrap(),
+            vmgenid: Some(VmGenId::restore((), &state.vmgenid).unwrap()),
             // Safe to unwrap() here, this will never return an error.
-            vmclock: VmClock::restore((), &state.vmclock).unwrap(),
+            vmclock: Some(VmClock::restore((), &state.vmclock).unwrap()),
         };
 
-        acpi_devices.attach_vmgenid(vm)?;
-        acpi_devices.vmgenid.do_post_restore()?;
+        acpi_devices.activate_vmgenid(vm)?;
+        acpi_devices.do_post_restore_vmgenid()?;
 
-        acpi_devices.attach_vmclock(vm)?;
-        acpi_devices.vmclock.do_post_restore(vm.guest_memory());
+        acpi_devices.activate_vmclock(vm)?;
+        acpi_devices.do_post_restore_vmclock(vm.guest_memory());
 
         Ok(acpi_devices)
     }