refactor: move vcpu creation into struct Vm

This abstracts away the weird irqchip setup dance that builder.rs has
been doing (where depending on architecture, irqchip needs to be created
before or after vcpu creation. Removes a whole bunch of code that
builder.rs that didn't really have too much business being there

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, VmError};
+use crate::vstate::vm::Vm;
 use crate::{device_manager, EventManager, Vmm, VmmError};
 
 /// Errors associated with starting the instance.
@@ -175,10 +175,6 @@ fn create_vmm_and_vcpus(
         .map_err(VmmError::Vm)
         .map_err(StartMicrovmError::Internal)?;
 
-    let vcpus_exit_evt = EventFd::new(libc::EFD_NONBLOCK)
-        .map_err(VmmError::EventFd)
-        .map_err(Internal)?;
-
     let resource_allocator = ResourceAllocator::new()?;
 
     // Instantiate the MMIO device manager.
@@ -187,13 +183,13 @@ fn create_vmm_and_vcpus(
     // Instantiate ACPI device manager.
     let acpi_device_manager = ACPIDeviceManager::new();
 
-    // For x86_64 we need to create the interrupt controller before calling `KVM_CREATE_VCPUS`
-    // while on aarch64 we need to do it the other way around.
-    #[cfg(target_arch = "x86_64")]
-    let (vcpus, pio_device_manager) = {
-        setup_interrupt_controller(&mut vm)?;
-        let vcpus = create_vcpus(&vm, vcpu_count, &vcpus_exit_evt).map_err(Internal)?;
+    let (vcpus, vcpus_exit_evt) = vm
+        .create_vcpus(vcpu_count)
+        .map_err(VmmError::Vm)
+        .map_err(Internal)?;
 
+    #[cfg(target_arch = "x86_64")]
+    let pio_device_manager = {
         // Make stdout non blocking.
         set_stdout_nonblocking();
 
@@ -208,25 +204,10 @@ fn create_vmm_and_vcpus(
             .map_err(Internal)?;
 
         // create pio dev manager with legacy devices
-        let pio_device_manager = {
-            // TODO Remove these unwraps.
-            let mut pio_dev_mgr = PortIODeviceManager::new(serial_device, reset_evt).unwrap();
-            pio_dev_mgr.register_devices(vm.fd()).unwrap();
-            pio_dev_mgr
-        };
-
-        (vcpus, pio_device_manager)
-    };
-
-    // On aarch64, the vCPUs need to be created (i.e call KVM_CREATE_VCPU) before setting up the
-    // IRQ chip because the `KVM_CREATE_VCPU` ioctl will return error if the IRQCHIP
-    // was already initialized.
-    // Search for `kvm_arch_vcpu_create` in arch/arm/kvm/arm.c.
-    #[cfg(target_arch = "aarch64")]
-    let vcpus = {
-        let vcpus = create_vcpus(&vm, vcpu_count, &vcpus_exit_evt).map_err(Internal)?;
-        setup_interrupt_controller(&mut vm, vcpu_count)?;
-        vcpus
+        // TODO Remove these unwraps.
+        let mut pio_dev_mgr = PortIODeviceManager::new(serial_device, reset_evt).unwrap();
+        pio_dev_mgr.register_devices(vm.fd()).unwrap();
+        pio_dev_mgr
     };
 
     let vmm = Vmm {
@@ -671,24 +652,6 @@ where
     })
 }
 
-/// Sets up the irqchip for a x86_64 microVM.
-#[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)
-}
-
-/// Sets up the irqchip for a aarch64 microVM.
-#[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)
-}
-
 /// Sets up the serial device.
 pub fn setup_serial_device(
     event_manager: &mut EventManager,
@@ -746,16 +709,6 @@ fn attach_legacy_devices_aarch64(
         .map_err(VmmError::RegisterMMIODevice)
 }
 
-fn create_vcpus(vm: &Vm, vcpu_count: u8, exit_evt: &EventFd) -> Result<Vec<Vcpu>, VmmError> {
-    let mut vcpus = Vec::with_capacity(vcpu_count as usize);
-    for cpu_idx in 0..vcpu_count {
-        let exit_evt = exit_evt.try_clone().map_err(VmmError::EventFd)?;
-        let vcpu = Vcpu::new(cpu_idx, vm, exit_evt).map_err(VmmError::VcpuCreate)?;
-        vcpus.push(vcpu);
-    }
-    Ok(vcpus)
-}
-
 /// Configures the system for booting Linux.
 #[cfg_attr(target_arch = "aarch64", allow(unused))]
 pub fn configure_system_for_boot(
@@ -1127,11 +1080,6 @@ pub(crate) mod tests {
     pub(crate) fn default_vmm() -> Vmm {
         let guest_memory = arch_mem(128 << 20);
 
-        let vcpus_exit_evt = EventFd::new(libc::EFD_NONBLOCK)
-            .map_err(VmmError::EventFd)
-            .map_err(StartMicrovmError::Internal)
-            .unwrap();
-
         let kvm = Kvm::new(vec![]).unwrap();
         let mut vm = Vm::new(&kvm).unwrap();
         vm.memory_init(&guest_memory).unwrap();
@@ -1153,15 +1101,7 @@ pub(crate) mod tests {
         )
         .unwrap();
 
-        #[cfg(target_arch = "x86_64")]
-        setup_interrupt_controller(&mut vm).unwrap();
-
-        #[cfg(target_arch = "aarch64")]
-        {
-            let exit_evt = EventFd::new(libc::EFD_NONBLOCK).unwrap();
-            let _vcpu = Vcpu::new(1, &vm, exit_evt).unwrap();
-            setup_interrupt_controller(&mut vm, 1).unwrap();
-        }
+        let (_, vcpus_exit_evt) = vm.create_vcpus(1).unwrap();
 
         Vmm {
             events_observer: Some(std::io::stdin()),
@@ -1380,24 +1320,6 @@ pub(crate) mod tests {
         );
     }
 
-    #[test]
-    fn test_create_vcpus() {
-        let vcpu_count = 2;
-        let guest_memory = arch_mem(128 << 20);
-
-        let kvm = Kvm::new(vec![]).expect("Cannot create Kvm");
-        #[allow(unused_mut)]
-        let mut vm = Vm::new(&kvm).unwrap();
-        vm.memory_init(&guest_memory).unwrap();
-        let evfd = EventFd::new(libc::EFD_NONBLOCK).unwrap();
-
-        #[cfg(target_arch = "x86_64")]
-        setup_interrupt_controller(&mut vm).unwrap();
-
-        let vcpu_vec = create_vcpus(&vm, vcpu_count, &evfd).unwrap();
-        assert_eq!(vcpu_vec.len(), vcpu_count as usize);
-    }
-
     #[test]
     fn test_attach_net_devices() {
         let mut event_manager = EventManager::new().expect("Unable to create EventManager");

src/vmm/src/device_manager/legacy.rs

@@ -248,8 +248,8 @@ mod tests {
 
     #[test]
     fn test_register_legacy_devices() {
-        let (_, mut vm, _) = setup_vm_with_memory(0x1000);
-        crate::builder::setup_interrupt_controller(&mut vm).unwrap();
+        let (_, vm, _) = setup_vm_with_memory(0x1000);
+        vm.setup_irqchip().unwrap();
         let mut ldm = PortIODeviceManager::new(
             Arc::new(Mutex::new(BusDevice::Serial(SerialDevice {
                 serial: Serial::with_events(

src/vmm/src/device_manager/mmio.rs

@@ -549,7 +549,7 @@ mod tests {
     use crate::test_utils::multi_region_mem;
     use crate::vstate::kvm::Kvm;
     use crate::vstate::memory::{GuestAddress, GuestMemoryMmap};
-    use crate::{builder, Vm};
+    use crate::Vm;
 
     const QUEUE_SIZES: &[u16] = &[64];
 
@@ -673,9 +673,9 @@ mod tests {
         let mut cmdline = kernel_cmdline::Cmdline::new(4096).unwrap();
         let dummy = Arc::new(Mutex::new(DummyDevice::new()));
         #[cfg(target_arch = "x86_64")]
-        builder::setup_interrupt_controller(&mut vm).unwrap();
+        vm.setup_irqchip().unwrap();
         #[cfg(target_arch = "aarch64")]
-        builder::setup_interrupt_controller(&mut vm, 1).unwrap();
+        vm.setup_irqchip(1).unwrap();
 
         device_manager
             .register_virtio_test_device(
@@ -702,9 +702,9 @@ mod tests {
 
         let mut cmdline = kernel_cmdline::Cmdline::new(4096).unwrap();
         #[cfg(target_arch = "x86_64")]
-        builder::setup_interrupt_controller(&mut vm).unwrap();
+        vm.setup_irqchip().unwrap();
         #[cfg(target_arch = "aarch64")]
-        builder::setup_interrupt_controller(&mut vm, 1).unwrap();
+        vm.setup_irqchip(1).unwrap();
 
         for _i in crate::arch::IRQ_BASE..=crate::arch::IRQ_MAX {
             device_manager
@@ -756,9 +756,9 @@ mod tests {
         let mem_clone = guest_mem.clone();
 
         #[cfg(target_arch = "x86_64")]
-        builder::setup_interrupt_controller(&mut vm).unwrap();
+        vm.setup_irqchip().unwrap();
         #[cfg(target_arch = "aarch64")]
-        builder::setup_interrupt_controller(&mut vm, 1).unwrap();
+        vm.setup_irqchip(1).unwrap();
 
         let mut device_manager = MMIODeviceManager::new();
         let mut resource_allocator = ResourceAllocator::new().unwrap();

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

@@ -37,6 +37,18 @@ impl ArchVm {
         })
     }
 
+    pub(super) fn arch_pre_create_vcpus(&mut self, _: u8) -> Result<(), ArchVmError> {
+        Ok(())
+    }
+
+    pub(super) fn arch_post_create_vcpus(&mut self, nr_vcpus: u8) -> Result<(), ArchVmError> {
+        // On aarch64, the vCPUs need to be created (i.e call KVM_CREATE_VCPU) before setting up the
+        // IRQ chip because the `KVM_CREATE_VCPU` ioctl will return error if the IRQCHIP
+        // was already initialized.
+        // Search for `kvm_arch_vcpu_create` in arch/arm/kvm/arm.c.
+        self.setup_irqchip(nr_vcpus)
+    }
+
     /// Creates the GIC (Global Interrupt Controller).
     pub fn setup_irqchip(&mut self, vcpu_count: u8) -> Result<(), ArchVmError> {
         self.irqchip_handle = Some(

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

@@ -7,6 +7,7 @@
 
 use kvm_bindings::{kvm_userspace_memory_region, KVM_MEM_LOG_DIRTY_PAGES};
 use kvm_ioctls::VmFd;
+use vmm_sys_util::eventfd::EventFd;
 
 #[cfg(target_arch = "x86_64")]
 use crate::utils::u64_to_usize;
@@ -21,10 +22,13 @@ mod arch;
 
 pub use arch::{ArchVm as Vm, ArchVmError, VmState};
 
+use crate::vstate::vcpu::VcpuError;
+use crate::Vcpu;
+
 /// Errors associated with the wrappers over KVM ioctls.
 /// Needs `rustfmt::skip` to make multiline comments work
 #[rustfmt::skip]
-#[derive(Debug, PartialEq, Eq, thiserror::Error, displaydoc::Display)]
+#[derive(Debug, thiserror::Error, displaydoc::Display)]
 pub enum VmError {
     /// Cannot set the memory regions: {0}
     SetUserMemoryRegion(kvm_ioctls::Error),
@@ -34,10 +38,34 @@ pub enum VmError {
     VmSetup(kvm_ioctls::Error),
     /// {0}
     Arch(#[from] ArchVmError),
+    /// Error during eventfd operations: {0}
+    EventFd(std::io::Error),
+    /// Failed to create vcpu: {0}
+    CreateVcpu(VcpuError),
 }
 
 /// Contains Vm functions that are usable across CPU architectures
 impl Vm {
+    /// Creates the specified number of [`Vcpu`]s.
+    ///
+    /// The returned [`EventFd`] is written to whenever any of the vcpus exit.
+    pub fn create_vcpus(&mut self, vcpu_count: u8) -> Result<(Vec<Vcpu>, EventFd), VmError> {
+        self.arch_pre_create_vcpus(vcpu_count)?;
+
+        let exit_evt = EventFd::new(libc::EFD_NONBLOCK).map_err(VmError::EventFd)?;
+
+        let mut vcpus = Vec::with_capacity(vcpu_count as usize);
+        for cpu_idx in 0..vcpu_count {
+            let exit_evt = exit_evt.try_clone().map_err(VmError::EventFd)?;
+            let vcpu = Vcpu::new(cpu_idx, self, exit_evt).map_err(VmError::CreateVcpu)?;
+            vcpus.push(vcpu);
+        }
+
+        self.arch_post_create_vcpus(vcpu_count)?;
+
+        Ok((vcpus, exit_evt))
+    }
+
     /// Initializes the guest memory.
     pub fn memory_init(&self, guest_mem: &GuestMemoryMmap) -> Result<(), VmError> {
         self.set_kvm_memory_regions(guest_mem)?;
@@ -88,7 +116,7 @@ impl Vm {
 #[cfg(test)]
 pub(crate) mod tests {
     use super::*;
-    use crate::test_utils::single_region_mem;
+    use crate::test_utils::{arch_mem, single_region_mem};
     use crate::vstate::kvm::Kvm;
     use crate::vstate::memory::GuestMemoryMmap;
 
@@ -139,4 +167,19 @@ pub(crate) mod tests {
             "Cannot set the memory regions: Invalid argument (os error 22)"
         );
     }
+
+    #[test]
+    fn test_create_vcpus() {
+        let vcpu_count = 2;
+        let guest_memory = arch_mem(128 << 20);
+
+        let kvm = Kvm::new(vec![]).expect("Cannot create Kvm");
+        #[allow(unused_mut)]
+        let mut vm = Vm::new(&kvm).unwrap();
+        vm.memory_init(&guest_memory).unwrap();
+
+        let (vcpu_vec, _) = vm.create_vcpus(vcpu_count).unwrap();
+
+        assert_eq!(vcpu_vec.len(), vcpu_count as usize);
+    }
 }

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

@@ -59,6 +59,15 @@ impl ArchVm {
         Ok(ArchVm { fd, msrs_to_save })
     }
 
+    pub(super) fn arch_pre_create_vcpus(&mut self, _: u8) -> Result<(), ArchVmError> {
+        // For x86_64 we need to create the interrupt controller before calling `KVM_CREATE_VCPUS`
+        self.setup_irqchip()
+    }
+
+    pub(super) fn arch_post_create_vcpus(&mut self, _: u8) -> Result<(), ArchVmError> {
+        Ok(())
+    }
+
     /// Restores the KVM VM state.
     ///
     /// # Errors