From 957d3a7443e9583c87e7174f8693e78cd9693f66 Mon Sep 17 00:00:00 2001 From: Michael Zhao Date: Wed, 1 Jun 2022 13:24:12 +0800 Subject: [PATCH] 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 --- Cargo.lock | 1 + arch/src/aarch64/fdt.rs | 17 ++--- arch/src/aarch64/gic.rs | 80 ------------------------ arch/src/aarch64/mod.rs | 11 ++-- devices/Cargo.toml | 1 + devices/src/gic.rs | 90 ++++++++++++++++++++++----- devices/src/interrupt_controller.rs | 2 + hypervisor/src/kvm/aarch64/gic/mod.rs | 7 +-- hypervisor/src/kvm/mod.rs | 9 ++- hypervisor/src/vm.rs | 4 +- vmm/src/device_manager.rs | 4 -- vmm/src/vm.rs | 85 +++++++++++++------------ 12 files changed, 150 insertions(+), 161 deletions(-) delete mode 100644 arch/src/aarch64/gic.rs diff --git a/Cargo.lock b/Cargo.lock index ac9aa9b47..4bdbc133e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -206,6 +206,7 @@ dependencies = [ "bitflags", "byteorder", "epoll", + "hypervisor", "libc", "log", "versionize", diff --git a/arch/src/aarch64/fdt.rs b/arch/src/aarch64/fdt.rs index ff6738537..3832c5a4a 100644 --- a/arch/src/aarch64/fdt.rs +++ b/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, vcpu_topology: Option<(u8, u8, u8)>, device_info: &HashMap<(DeviceType, String), T, S>, - gic_device: &dyn Vgic, + gic_device: &Arc>, initrd: &Option, 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<()> { - let gic_reg_prop = gic_device.device_properties(); +fn create_gic_node(fdt: &mut FdtWriter, gic_device: &Arc>) -> FdtWriterResult<()> { + 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 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)?; } diff --git a/arch/src/aarch64/gic.rs b/arch/src/aarch64/gic.rs deleted file mode 100644 index 0cac35a09..000000000 --- a/arch/src/aarch64/gic.rs +++ /dev/null @@ -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 = result::Result; - -/// A wrapper around creating and using a hypervisor-agnostic vgic. -pub struct GicDevice { - // The hypervisor abstracted GIC. - vgic: Box, -} - -impl GicDevice { - pub fn new(vm: &Arc, vcpu_count: u64) -> Result { - 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 { - 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 {} diff --git a/arch/src/aarch64/mod.rs b/arch/src/aarch64/mod.rs index b582cc751..002e004af 100644 --- a/arch/src/aarch64/mod.rs +++ b/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, pci_space_info: &[PciSpaceInfo], virtio_iommu_bdf: Option, - gic_device: &gic::GicDevice, + gic_device: &Arc>, numa_nodes: &NumaNodes, pmu_supported: bool, ) -> super::Result<()> { @@ -152,7 +151,7 @@ pub fn configure_system = result::Result; -// Reserve 32 IRQs for legacy device. -pub const IRQ_LEGACY_BASE: usize = arch::layout::IRQ_BASE as usize; +// Reserve 32 IRQs for legacy devices. +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. -// The Gic source files in arch/ folder maintain the Aarch64 specific Gic device. -// The 2 Gic instances could be merged together. -// Leave this refactoring to future. Two options may be considered: -// 1. Move Gic*.rs from arch/ folder here. -// 2. Move this file and ioapic.rs to arch/, as they are architecture specific. +// Gic (Generic Interupt Controller) struct provides all the functionality of a +// GIC device. It wraps a hypervisor-emulated GIC device (Vgic) provided by the +// `hypervisor` crate. +// Gic struct also implements InterruptController to provide interrupt delivery +// service. pub struct Gic { interrupt_source_group: Arc, - gic_device: Option>>, + // The hypervisor agnostic virtual GIC + vgic: Option>>, } 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>) { - self.gic_device = Some(gic_device); + pub fn create_vgic( + &mut self, + vm: &Arc, + vcpu_count: u64, + ) -> Result>> { + 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>> { - self.gic_device.as_ref() + pub fn set_gicr_typers(&mut self, vcpu_states: &[CpuState]) { + 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 { + 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 {} diff --git a/devices/src/interrupt_controller.rs b/devices/src/interrupt_controller.rs index f33b22fa6..6cca81dfe 100644 --- a/devices/src/interrupt_controller.rs +++ b/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 = result::Result; diff --git a/hypervisor/src/kvm/aarch64/gic/mod.rs b/hypervisor/src/kvm/aarch64/gic/mod.rs index 33fff1599..0e56c7318 100644 --- a/hypervisor/src/kvm/aarch64/gic/mod.rs +++ b/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> { + ) -> Result { 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)?; diff --git a/hypervisor/src/kvm/mod.rs b/hypervisor/src/kvm/mod.rs index 77cf4b85f..eb609ee12 100644 --- a/hypervisor/src/kvm/mod.rs +++ b/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> { - KvmGicV3Its::new( + ) -> vm::Result>> { + 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. diff --git a/hypervisor/src/vm.rs b/hypervisor/src/vm.rs index d53cb1f0b..0c0457188 100644 --- a/hypervisor/src/vm.rs +++ b/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>; + ) -> Result>>; /// Registers an event to be signaled whenever a certain address is written to. fn register_ioevent( diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 9dcf1a738..0d811f456 100644 --- a/vmm/src/device_manager.rs +++ b/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() diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs index d457b415b..53ad71e3f 100644 --- a/vmm/src/vm.rs +++ b/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( - &self.memory_manager.lock().as_ref().unwrap().vm, - self.cpu_manager.lock().unwrap().boot_vcpus() as u64, - ) - .map_err(|e| { - Error::ConfigureSystem(arch::Error::PlatformSpecific( - arch::aarch64::Error::SetupGic(e), - )) - })?; + let vgic = self + .device_manager + .lock() + .unwrap() + .get_interrupt_controller() + .unwrap() + .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() - .unwrap(), + .snapshot()?, ); - 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())?;