aarch64: Simplify GIC related structs definition

Combined the `GicDevice` struct in `arch` crate and the `Gic` struct in
`devices` crate.

After moving the KVM specific code for GIC in `arch`, a very thin wapper
layer `GicDevice` was left in `arch` crate. It is easy to combine it
with the `Gic` in `devices` crate.

Signed-off-by: Michael Zhao <michael.zhao@arm.com>

Cargo.lock

@@ -206,6 +206,7 @@ dependencies = [
  "bitflags",
  "byteorder",
  "epoll",
+ "hypervisor",
  "libc",
  "log",
  "versionize",

arch/src/aarch64/fdt.rs

@@ -15,6 +15,7 @@ use std::ffi::CStr;
 use std::fmt::Debug;
 use std::result;
 use std::str;
+use std::sync::{Arc, Mutex};
 
 use super::super::DeviceType;
 use super::super::GuestMemoryMmap;
@@ -90,7 +91,7 @@ pub fn create_fdt<T: DeviceInfoForFdt + Clone + Debug, S: ::std::hash::BuildHash
     vcpu_mpidr: Vec<u64>,
     vcpu_topology: Option<(u8, u8, u8)>,
     device_info: &HashMap<(DeviceType, String), T, S>,
-    gic_device: &dyn Vgic,
+    gic_device: &Arc<Mutex<dyn Vgic>>,
     initrd: &Option<InitramfsConfig>,
     pci_space_info: &[PciSpaceInfo],
     numa_nodes: &NumaNodes,
@@ -315,12 +316,12 @@ fn create_chosen_node(
     Ok(())
 }
 
-fn create_gic_node(fdt: &mut FdtWriter, gic_device: &dyn Vgic) -> FdtWriterResult<()> {
+fn create_gic_node(fdt: &mut FdtWriter, gic_device: &Arc<Mutex<dyn Vgic>>) -> FdtWriterResult<()> {
-    let gic_reg_prop = gic_device.device_properties();
+    let gic_reg_prop = gic_device.lock().unwrap().device_properties();
 
     let intc_node = fdt.begin_node("intc")?;
 
-    fdt.property_string("compatible", gic_device.fdt_compatibility())?;
+    fdt.property_string("compatible", gic_device.lock().unwrap().fdt_compatibility())?;
     fdt.property_null("interrupt-controller")?;
     // "interrupt-cells" field specifies the number of cells needed to encode an
     // interrupt source. The type shall be a <u32> and the value shall be 3 if no PPI affinity description
@@ -334,17 +335,17 @@ fn create_gic_node(fdt: &mut FdtWriter, gic_device: &dyn Vgic) -> FdtWriterResul
 
     let gic_intr_prop = [
         GIC_FDT_IRQ_TYPE_PPI,
-        gic_device.fdt_maint_irq(),
+        gic_device.lock().unwrap().fdt_maint_irq(),
         IRQ_TYPE_LEVEL_HI,
     ];
     fdt.property_array_u32("interrupts", &gic_intr_prop)?;
 
-    if gic_device.msi_compatible() {
+    if gic_device.lock().unwrap().msi_compatible() {
         let msic_node = fdt.begin_node("msic")?;
-        fdt.property_string("compatible", gic_device.msi_compatibility())?;
+        fdt.property_string("compatible", gic_device.lock().unwrap().msi_compatibility())?;
         fdt.property_null("msi-controller")?;
         fdt.property_u32("phandle", MSI_PHANDLE)?;
-        let msi_reg_prop = gic_device.msi_properties();
+        let msi_reg_prop = gic_device.lock().unwrap().msi_properties();
         fdt.property_array_u64("reg", &msi_reg_prop)?;
         fdt.end_node(msic_node)?;
     }

arch/src/aarch64/gic.rs

@@ -1,80 +0,0 @@
-// Copyright 2021 Arm Limited (or its affiliates). All rights reserved.
-
-use crate::layout;
-use anyhow::anyhow;
-use hypervisor::{arch::aarch64::gic::Vgic, CpuState};
-use std::result;
-use std::sync::Arc;
-use vm_memory::Address;
-use vm_migration::{Migratable, MigratableError, Pausable, Snapshot, Snapshottable, Transportable};
-
-/// Errors thrown while setting up the GIC.
-#[derive(Debug)]
-pub enum Error {
-    CreateGic(hypervisor::HypervisorVmError),
-}
-type Result<T> = result::Result<T, Error>;
-
-/// A wrapper around creating and using a hypervisor-agnostic vgic.
-pub struct GicDevice {
-    // The hypervisor abstracted GIC.
-    vgic: Box<dyn Vgic>,
-}
-
-impl GicDevice {
-    pub fn new(vm: &Arc<dyn hypervisor::Vm>, vcpu_count: u64) -> Result<GicDevice> {
-        let vgic = vm
-            .create_vgic(
-                vcpu_count,
-                layout::GIC_V3_DIST_START.raw_value(),
-                layout::GIC_V3_DIST_SIZE,
-                layout::GIC_V3_REDIST_SIZE,
-                layout::GIC_V3_ITS_SIZE,
-                layout::IRQ_NUM,
-            )
-            .unwrap();
-        Ok(GicDevice { vgic })
-    }
-
-    pub fn set_gicr_typers(&mut self, vcpu_states: &[CpuState]) {
-        self.vgic.set_gicr_typers(vcpu_states)
-    }
-
-    pub fn get_vgic(&self) -> &dyn Vgic {
-        &*self.vgic
-    }
-}
-
-pub const GIC_V3_ITS_SNAPSHOT_ID: &str = "gic-v3-its";
-impl Snapshottable for GicDevice {
-    fn id(&self) -> String {
-        GIC_V3_ITS_SNAPSHOT_ID.to_string()
-    }
-
-    fn snapshot(&mut self) -> std::result::Result<Snapshot, MigratableError> {
-        let state = self.vgic.state().unwrap();
-        Snapshot::new_from_state(&self.id(), &state)
-    }
-
-    fn restore(&mut self, snapshot: Snapshot) -> std::result::Result<(), MigratableError> {
-        self.vgic
-            .set_state(&snapshot.to_state(&self.id())?)
-            .map_err(|e| {
-                MigratableError::Restore(anyhow!("Could not restore GICv3ITS state {:?}", e))
-            })
-    }
-}
-
-impl Pausable for GicDevice {
-    fn pause(&mut self) -> std::result::Result<(), MigratableError> {
-        // Flush tables to guest RAM
-        self.vgic.save_data_tables().map_err(|e| {
-            MigratableError::Pause(anyhow!(
-                "Could not save GICv3ITS GIC pending tables {:?}",
-                e
-            ))
-        })
-    }
-}
-impl Transportable for GicDevice {}
-impl Migratable for GicDevice {}

arch/src/aarch64/mod.rs

@@ -4,8 +4,6 @@
 
 /// Module for the flattened device tree.
 pub mod fdt;
-/// Module for the global interrupt controller configuration.
-pub mod gic;
 /// Layout for this aarch64 system.
 pub mod layout;
 /// Logic for configuring aarch64 registers.
@@ -15,11 +13,12 @@ pub mod uefi;
 
 pub use self::fdt::DeviceInfoForFdt;
 use crate::{DeviceType, GuestMemoryMmap, NumaNodes, PciSpaceInfo, RegionType};
+use hypervisor::arch::aarch64::gic::Vgic;
 use log::{log_enabled, Level};
 use std::collections::HashMap;
 use std::convert::TryInto;
 use std::fmt::Debug;
-use std::sync::Arc;
+use std::sync::{Arc, Mutex};
 use vm_memory::{Address, GuestAddress, GuestMemory, GuestUsize};
 
 /// Errors thrown while configuring aarch64 system.
@@ -32,7 +31,7 @@ pub enum Error {
     WriteFdtToMemory(fdt::Error),
 
     /// Failed to create a GIC.
-    SetupGic(gic::Error),
+    SetupGic,
 
     /// Failed to compute the initramfs address.
     InitramfsAddress,
@@ -142,7 +141,7 @@ pub fn configure_system<T: DeviceInfoForFdt + Clone + Debug, S: ::std::hash::Bui
     initrd: &Option<super::InitramfsConfig>,
     pci_space_info: &[PciSpaceInfo],
     virtio_iommu_bdf: Option<u32>,
-    gic_device: &gic::GicDevice,
+    gic_device: &Arc<Mutex<dyn Vgic>>,
     numa_nodes: &NumaNodes,
     pmu_supported: bool,
 ) -> super::Result<()> {
@@ -152,7 +151,7 @@ pub fn configure_system<T: DeviceInfoForFdt + Clone + Debug, S: ::std::hash::Bui
         vcpu_mpidr,
         vcpu_topology,
         device_info,
-        gic_device.get_vgic(),
+        gic_device,
         initrd,
         pci_space_info,
         numa_nodes,

devices/Cargo.toml

@@ -11,6 +11,7 @@ arch = { path = "../arch" }
 bitflags = "1.3.2"
 byteorder = "1.4.3"
 epoll = "4.3.1"
+hypervisor = { path = "../hypervisor" }
 libc = "0.2.126"
 log = "0.4.17"
 versionize = "0.1.6"

devices/src/gic.rs

@@ -4,30 +4,34 @@
 
 use super::interrupt_controller::{Error, InterruptController};
 extern crate arch;
-use arch::aarch64::gic::GicDevice;
+use anyhow::anyhow;
+use arch::layout;
+use hypervisor::{arch::aarch64::gic::Vgic, CpuState};
 use std::result;
 use std::sync::{Arc, Mutex};
 use vm_device::interrupt::{
     InterruptIndex, InterruptManager, InterruptSourceConfig, InterruptSourceGroup,
     LegacyIrqSourceConfig, MsiIrqGroupConfig,
 };
+use vm_memory::Address;
+use vm_migration::{Migratable, MigratableError, Pausable, Snapshot, Snapshottable, Transportable};
 use vmm_sys_util::eventfd::EventFd;
 
 type Result<T> = result::Result<T, Error>;
 
-// Reserve 32 IRQs for legacy device.
+// Reserve 32 IRQs for legacy devices.
-pub const IRQ_LEGACY_BASE: usize = arch::layout::IRQ_BASE as usize;
+pub const IRQ_LEGACY_BASE: usize = layout::IRQ_BASE as usize;
 pub const IRQ_LEGACY_COUNT: usize = 32;
 
-// This Gic struct implements InterruptController to provide interrupt delivery service.
+// Gic (Generic Interupt Controller) struct provides all the functionality of a
-// The Gic source files in arch/ folder maintain the Aarch64 specific Gic device.
+// GIC device. It wraps a hypervisor-emulated GIC device (Vgic) provided by the
-// The 2 Gic instances could be merged together.
+// `hypervisor` crate.
-// Leave this refactoring to future. Two options may be considered:
+// Gic struct also implements InterruptController to provide interrupt delivery
-//   1. Move Gic*.rs from arch/ folder here.
+// service.
-//   2. Move this file and ioapic.rs to arch/, as they are architecture specific.
 pub struct Gic {
     interrupt_source_group: Arc<dyn InterruptSourceGroup>,
-    gic_device: Option<Arc<Mutex<GicDevice>>>,
+    // The hypervisor agnostic virtual GIC
+    vgic: Option<Arc<Mutex<dyn Vgic>>>,
 }
 
 impl Gic {
@@ -44,16 +48,32 @@ impl Gic {
 
         Ok(Gic {
             interrupt_source_group,
-            gic_device: None,
+            vgic: None,
         })
     }
 
-    pub fn set_gic_device(&mut self, gic_device: Arc<Mutex<GicDevice>>) {
+    pub fn create_vgic(
-        self.gic_device = Some(gic_device);
+        &mut self,
+        vm: &Arc<dyn hypervisor::Vm>,
+        vcpu_count: u64,
+    ) -> Result<Arc<Mutex<dyn Vgic>>> {
+        let vgic = vm
+            .create_vgic(
+                vcpu_count,
+                layout::GIC_V3_DIST_START.raw_value(),
+                layout::GIC_V3_DIST_SIZE,
+                layout::GIC_V3_REDIST_SIZE,
+                layout::GIC_V3_ITS_SIZE,
+                layout::IRQ_NUM,
+            )
+            .map_err(Error::CreateGic)?;
+        self.vgic = Some(vgic.clone());
+        Ok(vgic.clone())
     }
 
-    pub fn get_gic_device(&self) -> Option<&Arc<Mutex<GicDevice>>> {
+    pub fn set_gicr_typers(&mut self, vcpu_states: &[CpuState]) {
-        self.gic_device.as_ref()
+        let vgic = self.vgic.as_ref().unwrap().clone();
+        vgic.lock().unwrap().set_gicr_typers(vcpu_states);
     }
 }
 
@@ -97,3 +117,43 @@ impl InterruptController for Gic {
         self.interrupt_source_group.notifier(irq as InterruptIndex)
     }
 }
+
+pub const GIC_V3_ITS_SNAPSHOT_ID: &str = "gic-v3-its";
+impl Snapshottable for Gic {
+    fn id(&self) -> String {
+        GIC_V3_ITS_SNAPSHOT_ID.to_string()
+    }
+
+    fn snapshot(&mut self) -> std::result::Result<Snapshot, MigratableError> {
+        let vgic = self.vgic.as_ref().unwrap().clone();
+        let state = vgic.lock().unwrap().state().unwrap();
+        Snapshot::new_from_state(&self.id(), &state)
+    }
+
+    fn restore(&mut self, snapshot: Snapshot) -> std::result::Result<(), MigratableError> {
+        let vgic = self.vgic.as_ref().unwrap().clone();
+        vgic.lock()
+            .unwrap()
+            .set_state(&snapshot.to_state(&self.id())?)
+            .map_err(|e| {
+                MigratableError::Restore(anyhow!("Could not restore GICv3ITS state {:?}", e))
+            })?;
+        Ok(())
+    }
+}
+
+impl Pausable for Gic {
+    fn pause(&mut self) -> std::result::Result<(), MigratableError> {
+        // Flush tables to guest RAM
+        let vgic = self.vgic.as_ref().unwrap().clone();
+        vgic.lock().unwrap().save_data_tables().map_err(|e| {
+            MigratableError::Pause(anyhow!(
+                "Could not save GICv3ITS GIC pending tables {:?}",
+                e
+            ))
+        })?;
+        Ok(())
+    }
+}
+impl Transportable for Gic {}
+impl Migratable for Gic {}

devices/src/interrupt_controller.rs

@@ -24,6 +24,8 @@ pub enum Error {
     UpdateInterrupt(io::Error),
     /// Failed enabling the interrupt.
     EnableInterrupt(io::Error),
+    /// Failed creating GIC device.
+    CreateGic(hypervisor::HypervisorVmError),
 }
 
 type Result<T> = result::Result<T, Error>;

hypervisor/src/kvm/aarch64/gic/mod.rs

@@ -12,7 +12,6 @@ use icc_regs::{get_icc_regs, set_icc_regs};
 use redist_regs::{construct_gicr_typers, get_redist_regs, set_redist_regs};
 use serde::{Deserialize, Serialize};
 use std::any::Any;
-use std::boxed::Box;
 use std::convert::TryInto;
 use std::sync::Arc;
 
@@ -254,13 +253,13 @@ impl KvmGicV3Its {
         redist_size: u64,
         msi_size: u64,
         nr_irqs: u32,
-    ) -> Result<Box<dyn Vgic>> {
+    ) -> Result<KvmGicV3Its> {
         let vgic = Self::create_device(vm)?;
         let redists_size: u64 = redist_size * vcpu_count;
         let redists_addr: u64 = dist_addr - redists_size;
         let msi_addr: u64 = redists_addr - msi_size;
 
-        let mut gic_device = Box::new(KvmGicV3Its {
+        let mut gic_device = KvmGicV3Its {
             device: vgic,
             its_device: None,
             gicr_typers: vec![0; vcpu_count.try_into().unwrap()],
@@ -271,7 +270,7 @@ impl KvmGicV3Its {
             msi_addr,
             msi_size,
             vcpu_count,
-        });
+        };
 
         gic_device.init_device_attributes(vm, nr_irqs)?;
 

hypervisor/src/kvm/mod.rs

@@ -35,6 +35,8 @@ use std::os::unix::io::{AsRawFd, RawFd};
 use std::result;
 #[cfg(target_arch = "x86_64")]
 use std::sync::atomic::{AtomicBool, Ordering};
+#[cfg(target_arch = "aarch64")]
+use std::sync::Mutex;
 use std::sync::{Arc, RwLock};
 use vmm_sys_util::eventfd::EventFd;
 // x86_64 dependencies
@@ -266,8 +268,8 @@ impl vm::Vm for KvmVm {
         redist_size: u64,
         msi_size: u64,
         nr_irqs: u32,
-    ) -> vm::Result<Box<dyn Vgic>> {
+    ) -> vm::Result<Arc<Mutex<dyn Vgic>>> {
-        KvmGicV3Its::new(
+        let gic_device = KvmGicV3Its::new(
             self,
             vcpu_count,
             dist_addr,
@@ -276,7 +278,8 @@ impl vm::Vm for KvmVm {
             msi_size,
             nr_irqs,
         )
-        .map_err(|e| vm::HypervisorVmError::CreateVgic(anyhow!("Vgic error {:?}", e)))
+        .map_err(|e| vm::HypervisorVmError::CreateVgic(anyhow!("Vgic error {:?}", e)))?;
+        Ok(Arc::new(Mutex::new(gic_device)))
     }
     ///
     /// Registers an event to be signaled whenever a certain address is written to.

hypervisor/src/vm.rs

@@ -29,6 +29,8 @@ use kvm_ioctls::Cap;
 #[cfg(target_arch = "x86_64")]
 use std::fs::File;
 use std::sync::Arc;
+#[cfg(target_arch = "aarch64")]
+use std::sync::Mutex;
 use thiserror::Error;
 use vmm_sys_util::eventfd::EventFd;
 
@@ -285,7 +287,7 @@ pub trait Vm: Send + Sync {
         redist_size: u64,
         msi_size: u64,
         nr_irqs: u32,
-    ) -> Result<Box<dyn Vgic>>;
+    ) -> Result<Arc<Mutex<dyn Vgic>>>;
 
     /// Registers an event to be signaled whenever a certain address is written to.
     fn register_ioevent(

vmm/src/device_manager.rs

@@ -4326,10 +4326,6 @@ impl Pausable for DeviceManager {
         #[cfg(target_arch = "aarch64")]
         {
             self.get_interrupt_controller()
-                .unwrap()
-                .lock()
-                .unwrap()
-                .get_gic_device()
                 .unwrap()
                 .lock()
                 .unwrap()

vmm/src/vm.rs

@@ -37,8 +37,6 @@ use crate::{
     PciDeviceInfo, CPU_MANAGER_SNAPSHOT_ID, DEVICE_MANAGER_SNAPSHOT_ID, MEMORY_MANAGER_SNAPSHOT_ID,
 };
 use anyhow::anyhow;
-#[cfg(target_arch = "aarch64")]
-use arch::aarch64::gic::{GicDevice, GIC_V3_ITS_SNAPSHOT_ID};
 use arch::get_host_cpu_phys_bits;
 #[cfg(target_arch = "x86_64")]
 use arch::layout::{KVM_IDENTITY_MAP_START, KVM_TSS_START};
@@ -49,6 +47,8 @@ use arch::EntryPoint;
 use arch::PciSpaceInfo;
 use arch::{NumaNode, NumaNodes};
 #[cfg(target_arch = "aarch64")]
+use devices::gic::GIC_V3_ITS_SNAPSHOT_ID;
+#[cfg(target_arch = "aarch64")]
 use devices::interrupt_controller::{self, InterruptController};
 use devices::AcpiNotificationFlags;
 #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
@@ -1182,15 +1182,23 @@ impl Vm {
             .as_ref()
             .map(|(v, _)| *v);
 
-        let gic_device = GicDevice::new(
+        let vgic = self
-            &self.memory_manager.lock().as_ref().unwrap().vm,
+            .device_manager
-            self.cpu_manager.lock().unwrap().boot_vcpus() as u64,
+            .lock()
-        )
+            .unwrap()
-        .map_err(|e| {
+            .get_interrupt_controller()
-            Error::ConfigureSystem(arch::Error::PlatformSpecific(
+            .unwrap()
-                arch::aarch64::Error::SetupGic(e),
+            .lock()
-            ))
+            .unwrap()
-        })?;
+            .create_vgic(
+                &self.memory_manager.lock().as_ref().unwrap().vm,
+                self.cpu_manager.lock().unwrap().boot_vcpus() as u64,
+            )
+            .map_err(|_| {
+                Error::ConfigureSystem(arch::Error::PlatformSpecific(
+                    arch::aarch64::Error::SetupGic,
+                ))
+            })?;
 
         // PMU interrupt sticks to PPI, so need to be added by 16 to get real irq number.
         let pmu_supported = self
@@ -1213,23 +1221,12 @@ impl Vm {
             &initramfs_config,
             &pci_space_info,
             virtio_iommu_bdf.map(|bdf| bdf.into()),
-            &gic_device,
+            &vgic,
             &self.numa_nodes,
             pmu_supported,
         )
         .map_err(Error::ConfigureSystem)?;
 
-        // Update the GIC entity in device manager
-        let gic_device = Arc::new(Mutex::new(gic_device));
-        self.device_manager
-            .lock()
-            .unwrap()
-            .get_interrupt_controller()
-            .unwrap()
-            .lock()
-            .unwrap()
-            .set_gic_device(gic_device);
-
         // Activate gic device
         self.device_manager
             .lock()
@@ -2219,7 +2216,16 @@ impl Vm {
         vm_snapshot: &mut Snapshot,
     ) -> std::result::Result<(), MigratableError> {
         let saved_vcpu_states = self.cpu_manager.lock().unwrap().get_saved_states();
-        let gic_device = Arc::clone(
+        self.device_manager
+            .lock()
+            .unwrap()
+            .get_interrupt_controller()
+            .unwrap()
+            .lock()
+            .unwrap()
+            .set_gicr_typers(&saved_vcpu_states);
+
+        vm_snapshot.add_snapshot(
             self.device_manager
                 .lock()
                 .unwrap()
@@ -2227,17 +2233,9 @@ impl Vm {
                 .unwrap()
                 .lock()
                 .unwrap()
-                .get_gic_device()
+                .snapshot()?,
-                .unwrap(),
         );
 
-        gic_device
-            .lock()
-            .unwrap()
-            .set_gicr_typers(&saved_vcpu_states);
-
-        vm_snapshot.add_snapshot(gic_device.lock().unwrap().snapshot()?);
-
         Ok(())
     }
 
@@ -2254,7 +2252,14 @@ impl Vm {
         // Creating a GIC device here, as the GIC will not be created when
         // restoring the device manager. Note that currently only the bare GICv3
         // without ITS is supported.
-        let mut gic_device = GicDevice::new(&self.vm, vcpu_numbers.try_into().unwrap())
+        self.device_manager
+            .lock()
+            .unwrap()
+            .get_interrupt_controller()
+            .unwrap()
+            .lock()
+            .unwrap()
+            .create_vgic(&self.vm, vcpu_numbers.try_into().unwrap())
             .map_err(|e| MigratableError::Restore(anyhow!("Could not create GIC: {:#?}", e)))?;
 
         // PMU interrupt sticks to PPI, so need to be added by 16 to get real irq number.
@@ -2265,10 +2270,6 @@ impl Vm {
             .map_err(|e| MigratableError::Restore(anyhow!("Error init PMU: {:?}", e)))?;
 
         // Here we prepare the GICR_TYPER registers from the restored vCPU states.
-        gic_device.set_gicr_typers(&saved_vcpu_states);
-
-        let gic_device = Arc::new(Mutex::new(gic_device));
-        // Update the GIC entity in device manager
         self.device_manager
             .lock()
             .unwrap()
@@ -2276,11 +2277,15 @@ impl Vm {
             .unwrap()
             .lock()
             .unwrap()
-            .set_gic_device(gic_device.clone());
+            .set_gicr_typers(&saved_vcpu_states);
 
         // Restore GIC states.
         if let Some(gicv3_its_snapshot) = vm_snapshot.snapshots.get(GIC_V3_ITS_SNAPSHOT_ID) {
-            gic_device
+            self.device_manager
+                .lock()
+                .unwrap()
+                .get_interrupt_controller()
+                .unwrap()
                 .lock()
                 .unwrap()
                 .restore(*gicv3_its_snapshot.clone())?;