From 67d4219489a3914acf0b9796d8f7bbc368cc835d Mon Sep 17 00:00:00 2001 From: Junichi Uekawa Date: Fri, 4 Feb 2022 15:50:06 +0900 Subject: [PATCH] system_allocator: use config object pattern for constructing In preparation to allow multiple low memory regions to be passed on, I wanted to make initialization simpler. Introduce `MemRegion` struct instead of tuple to help me understand it is a base and size. BUG=b:188011323 TEST=build Change-Id: Ie8b54354a25c478d5ad0a0185b7e07d28840dd87 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3439666 Reviewed-by: Daniel Verkamp Tested-by: kokoro Commit-Queue: Junichi Uekawa --- aarch64/src/lib.rs | 25 +++- devices/src/irqchip/kvm/x86_64.rs | 57 +++++--- devices/src/pci/ac97.rs | 24 +++- devices/src/pci/stub.rs | 24 +++- resources/src/lib.rs | 6 +- resources/src/system_allocator.rs | 214 +++++++++++++----------------- x86_64/src/lib.rs | 25 +++- 7 files changed, 203 insertions(+), 172 deletions(-) diff --git a/aarch64/src/lib.rs b/aarch64/src/lib.rs index 702662c238..a1a4dbde41 100644 --- a/aarch64/src/lib.rs +++ b/aarch64/src/lib.rs @@ -20,7 +20,7 @@ use hypervisor::{ }; use minijail::Minijail; use remain::sorted; -use resources::{MmioType, SystemAllocator}; +use resources::{MemRegion, MmioType, SystemAllocator, SystemAllocatorConfig}; use sync::Mutex; use thiserror::Error; use vm_control::BatteryType; @@ -561,12 +561,23 @@ impl AArch64 { guest_phys_end, high_mmio_base, ); }); - SystemAllocator::builder() - .add_low_mmio_addresses(AARCH64_MMIO_BASE, AARCH64_MMIO_SIZE) - .add_platform_mmio_addresses(plat_mmio_base, plat_mmio_size) - .add_high_mmio_addresses(high_mmio_base, high_mmio_size) - .create_allocator(AARCH64_IRQ_BASE) - .unwrap() + SystemAllocator::new(SystemAllocatorConfig { + io: None, + low_mmio: MemRegion { + base: AARCH64_MMIO_BASE, + size: AARCH64_MMIO_SIZE, + }, + high_mmio: MemRegion { + base: high_mmio_base, + size: high_mmio_size, + }, + platform_mmio: Some(MemRegion { + base: plat_mmio_base, + size: plat_mmio_size, + }), + first_irq: AARCH64_IRQ_BASE, + }) + .unwrap() } /// This adds any early platform devices for this architecture. diff --git a/devices/src/irqchip/kvm/x86_64.rs b/devices/src/irqchip/kvm/x86_64.rs index af0971da28..e3b13da5a2 100644 --- a/devices/src/irqchip/kvm/x86_64.rs +++ b/devices/src/irqchip/kvm/x86_64.rs @@ -720,14 +720,13 @@ mod tests { use super::*; use base::EventReadResult; - use hypervisor::kvm::Kvm; + use hypervisor::{ + kvm::Kvm, IoapicRedirectionTableEntry, PitRWMode, ProtectionType, TriggerMode, Vm, VmX86_64, + }; + use resources::{MemRegion, SystemAllocator, SystemAllocatorConfig}; use vm_memory::GuestMemory; - use hypervisor::{ - IoapicRedirectionTableEntry, PitRWMode, ProtectionType, TriggerMode, Vm, VmX86_64, - }; - - use super::super::super::tests::*; + use crate::irqchip::tests::*; use crate::IrqChip; /// Helper function for setting up a KvmKernelIrqChip @@ -905,12 +904,23 @@ mod tests { let mmio_bus = Bus::new(); let io_bus = Bus::new(); - let mut resources = SystemAllocator::builder() - .add_io_addresses(0xc000, 0x10000) - .add_low_mmio_addresses(0, 2048) - .add_high_mmio_addresses(2048, 4096) - .create_allocator(5) - .expect("failed to create SystemAllocator"); + let mut resources = SystemAllocator::new(SystemAllocatorConfig { + io: Some(MemRegion { + base: 0xc000, + size: 0x1_0000, + }), + low_mmio: MemRegion { + base: 0, + size: 2048, + }, + high_mmio: MemRegion { + base: 2048, + size: 4096, + }, + platform_mmio: None, + first_irq: 5, + }) + .expect("failed to create SystemAllocator"); // setup an event and a resample event for irq line 1 let evt = Event::new().expect("failed to create event"); @@ -1023,12 +1033,23 @@ mod tests { let mmio_bus = Bus::new(); let io_bus = Bus::new(); - let mut resources = SystemAllocator::builder() - .add_io_addresses(0xc000, 0x10000) - .add_low_mmio_addresses(0, 2048) - .add_high_mmio_addresses(2048, 4096) - .create_allocator(5) - .expect("failed to create SystemAllocator"); + let mut resources = SystemAllocator::new(SystemAllocatorConfig { + io: Some(MemRegion { + base: 0xc000, + size: 0x1_0000, + }), + low_mmio: MemRegion { + base: 0, + size: 2048, + }, + high_mmio: MemRegion { + base: 2048, + size: 4096, + }, + platform_mmio: None, + first_irq: 5, + }) + .expect("failed to create SystemAllocator"); // setup an event and a resample event for irq line 1 let evt = Event::new().expect("failed to create event"); diff --git a/devices/src/pci/ac97.rs b/devices/src/pci/ac97.rs index 91beec754b..c61d32d144 100644 --- a/devices/src/pci/ac97.rs +++ b/devices/src/pci/ac97.rs @@ -439,6 +439,7 @@ impl PciDevice for Ac97Dev { mod tests { use super::*; use audio_streams::shm_streams::MockShmStreamSource; + use resources::{MemRegion, SystemAllocatorConfig}; use vm_memory::GuestAddress; #[test] @@ -446,12 +447,23 @@ mod tests { let mem = GuestMemory::new(&[(GuestAddress(0u64), 4 * 1024 * 1024)]).unwrap(); let mut ac97_dev = Ac97Dev::new(mem, Ac97Backend::NULL, Box::new(MockShmStreamSource::new())); - let mut allocator = SystemAllocator::builder() - .add_io_addresses(0x1000_0000, 0x1000_0000) - .add_low_mmio_addresses(0x2000_0000, 0x1000_0000) - .add_high_mmio_addresses(0x3000_0000, 0x1000_0000) - .create_allocator(5) - .unwrap(); + let mut allocator = SystemAllocator::new(SystemAllocatorConfig { + io: Some(MemRegion { + base: 0x1000_0000, + size: 0x1000_0000, + }), + low_mmio: MemRegion { + base: 0x2000_0000, + size: 0x1000_0000, + }, + high_mmio: MemRegion { + base: 0x3000_0000, + size: 0x1000_0000, + }, + platform_mmio: None, + first_irq: 5, + }) + .unwrap(); assert!(ac97_dev.allocate_address(&mut allocator).is_ok()); assert!(ac97_dev.allocate_io_bars(&mut allocator).is_ok()); } diff --git a/devices/src/pci/stub.rs b/devices/src/pci/stub.rs index 07cb2cf442..97a92eef03 100644 --- a/devices/src/pci/stub.rs +++ b/devices/src/pci/stub.rs @@ -127,6 +127,7 @@ impl PciDevice for StubPciDevice { #[cfg(test)] mod test { use super::*; + use resources::{MemRegion, SystemAllocator, SystemAllocatorConfig}; const CONFIG: StubPciParameters = StubPciParameters { address: PciAddress { @@ -155,12 +156,23 @@ mod test { #[test] fn address_allocation() { - let mut allocator = SystemAllocator::builder() - .add_io_addresses(0x1000_0000, 0x1000_0000) - .add_low_mmio_addresses(0x2000_0000, 0x1000_0000) - .add_high_mmio_addresses(0x3000_0000, 0x1000_0000) - .create_allocator(5) - .unwrap(); + let mut allocator = SystemAllocator::new(SystemAllocatorConfig { + io: Some(MemRegion { + base: 0x1000_0000, + size: 0x1000_0000, + }), + low_mmio: MemRegion { + base: 0x2000_0000, + size: 0x1000_0000, + }, + high_mmio: MemRegion { + base: 0x3000_0000, + size: 0x1000_0000, + }, + platform_mmio: None, + first_irq: 5, + }) + .unwrap(); let mut device = StubPciDevice::new(&CONFIG); assert!(device.allocate_address(&mut allocator).is_ok()); diff --git a/resources/src/lib.rs b/resources/src/lib.rs index c8f7be11d5..34fef7bb4f 100644 --- a/resources/src/lib.rs +++ b/resources/src/lib.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; pub use crate::address_allocator::AddressAllocator; -pub use crate::system_allocator::{MmioType, SystemAllocator}; +pub use crate::system_allocator::{MemRegion, MmioType, SystemAllocator, SystemAllocatorConfig}; mod address_allocator; mod system_allocator; @@ -50,10 +50,6 @@ pub enum Error { ExistingAlloc(Alloc), #[error("Invalid Alloc: {0:?}")] InvalidAlloc(Alloc), - #[error("High MMIO address range not specified")] - MissingHighMMIOAddresses, - #[error("Low MMIO address range not specified")] - MissingLowMMIOAddresses, #[error("Platform MMIO address range not specified")] MissingPlatformMMIOAddresses, #[error("No IO address range specified")] diff --git a/resources/src/system_allocator.rs b/resources/src/system_allocator.rs index 5aec9c1e1c..9541f89b3a 100644 --- a/resources/src/system_allocator.rs +++ b/resources/src/system_allocator.rs @@ -6,37 +6,9 @@ use std::collections::BTreeMap; use base::pagesize; use crate::address_allocator::{AddressAllocator, AddressAllocatorSet}; -use crate::{Alloc, Error, Result}; +use crate::{Alloc, Result}; /// Manages allocating system resources such as address space and interrupt numbers. -/// -/// # Example - Use the `SystemAllocator` builder. -/// -/// ``` -/// # use resources::{Alloc, MmioType, SystemAllocator}; -/// if let Ok(mut a) = SystemAllocator::builder() -/// .add_io_addresses(0x1000, 0x10000) -/// .add_high_mmio_addresses(0x10000000, 0x10000000) -/// .add_low_mmio_addresses(0x30000000, 0x10000) -/// .create_allocator(5) { -/// assert_eq!(a.allocate_irq(), Some(5)); -/// assert_eq!(a.allocate_irq(), Some(6)); -/// assert_eq!( -/// a.mmio_allocator(MmioType::High) -/// .allocate( -/// 0x100, -/// Alloc::PciBar { bus: 0, dev: 0, func: 0, bar: 0 }, -/// "bar0".to_string() -/// ), -/// Ok(0x10000000) -/// ); -/// assert_eq!( -/// a.mmio_allocator(MmioType::High) -/// .get(&Alloc::PciBar { bus: 0, dev: 0, func: 0, bar: 0 }), -/// Some(&(0x10000000, 0x100, "bar0".to_string())) -/// ); -/// } -/// ``` /// MMIO address Type /// Low: address allocated from low_address_space @@ -46,6 +18,25 @@ pub enum MmioType { High, } +/// Region of memory. +pub struct MemRegion { + pub base: u64, + pub size: u64, +} + +pub struct SystemAllocatorConfig { + /// IO memory. Only for x86_64. + pub io: Option, + /// Low (<=4GB) MMIO region. + pub low_mmio: MemRegion, + /// High (>4GB) MMIO region. + pub high_mmio: MemRegion, + /// Platform MMIO space. Only for ARM. + pub platform_mmio: Option, + /// The first IRQ number to give out. + pub first_irq: u32, +} + #[derive(Debug)] pub struct SystemAllocator { io_address_space: Option, @@ -65,52 +56,48 @@ impl SystemAllocator { /// Can return `None` if `base` + `size` overflows a u64 or if alignment isn't a power /// of two. /// - /// * `io_base` - The starting address of IO memory. - /// * `io_size` - The size of IO memory. - /// * `high_base` - The starting address of high MMIO space. - /// * `high_size` - The size of high MMIO space. - /// * `low_base` - The starting address of low MMIO space. - /// * `low_size` - The size of low MMIO space. - /// * `platform_base` - The starting address of platform MMIO space. - /// * `platform_size` - The size of platform MMIO space. - /// * `first_irq` - The first irq number to give out. - fn new( - io_base: Option, - io_size: Option, - high_base: u64, - high_size: u64, - low_base: u64, - low_size: u64, - platform_base: Option, - platform_size: Option, - first_irq: u32, - ) -> Result { + pub fn new(config: SystemAllocatorConfig) -> Result { let page_size = pagesize() as u64; + Ok(SystemAllocator { - io_address_space: if let (Some(b), Some(s)) = (io_base, io_size) { - Some(AddressAllocator::new(b, s, Some(0x400), None)?) + io_address_space: if let Some(io) = config.io { + Some(AddressAllocator::new(io.base, io.size, Some(0x400), None)?) } else { None }, mmio_address_spaces: [ // MmioType::Low - AddressAllocator::new(low_base, low_size, Some(page_size), None)?, + AddressAllocator::new( + config.low_mmio.base, + config.low_mmio.size, + Some(page_size), + None, + )?, // MmioType::High - AddressAllocator::new(high_base, high_size, Some(page_size), None)?, + AddressAllocator::new( + config.high_mmio.base, + config.high_mmio.size, + Some(page_size), + None, + )?, ], pci_allocator: BTreeMap::new(), - mmio_platform_address_spaces: if let (Some(b), Some(s)) = (platform_base, platform_size) - { - Some(AddressAllocator::new(b, s, Some(page_size), None)?) + mmio_platform_address_spaces: if let Some(platform) = config.platform_mmio { + Some(AddressAllocator::new( + platform.base, + platform.size, + Some(page_size), + None, + )?) } else { None }, irq_allocator: AddressAllocator::new( - first_irq as u64, - 1024 - first_irq as u64, + config.first_irq as u64, + 1024 - config.first_irq as u64, Some(1), None, )?, @@ -118,11 +105,6 @@ impl SystemAllocator { }) } - /// Returns a `SystemAllocatorBuilder` that can create a new `SystemAllocator`. - pub fn builder() -> SystemAllocatorBuilder { - SystemAllocatorBuilder::new() - } - /// Reserves the next available system irq number. pub fn allocate_irq(&mut self) -> Option { let id = self.get_anon_alloc(); @@ -250,67 +232,53 @@ impl SystemAllocator { } } -/// Used to build a system address map for use in creating a `SystemAllocator`. -pub struct SystemAllocatorBuilder { - io_base: Option, - io_size: Option, - low_mmio_base: Option, - low_mmio_size: Option, - high_mmio_base: Option, - high_mmio_size: Option, - platform_mmio_base: Option, - platform_mmio_size: Option, -} +#[cfg(test)] +mod tests { + use super::*; -impl SystemAllocatorBuilder { - pub fn new() -> Self { - SystemAllocatorBuilder { - io_base: None, - io_size: None, - low_mmio_base: None, - low_mmio_size: None, - high_mmio_base: None, - high_mmio_size: None, - platform_mmio_base: None, - platform_mmio_size: None, - } - } + #[test] + fn example() { + let mut a = SystemAllocator::new(SystemAllocatorConfig { + io: Some(MemRegion { + base: 0x1000, + size: 0x1_0000, + }), + low_mmio: MemRegion { + base: 0x3000_0000, + size: 0x1_0000, + }, + high_mmio: MemRegion { + base: 0x1000_0000, + size: 0x1000_0000, + }, + platform_mmio: None, + first_irq: 5, + }) + .unwrap(); - pub fn add_io_addresses(mut self, base: u64, size: u64) -> Self { - self.io_base = Some(base); - self.io_size = Some(size); - self - } - - pub fn add_low_mmio_addresses(mut self, base: u64, size: u64) -> Self { - self.low_mmio_base = Some(base); - self.low_mmio_size = Some(size); - self - } - - pub fn add_high_mmio_addresses(mut self, base: u64, size: u64) -> Self { - self.high_mmio_base = Some(base); - self.high_mmio_size = Some(size); - self - } - - pub fn add_platform_mmio_addresses(mut self, base: u64, size: u64) -> Self { - self.platform_mmio_base = Some(base); - self.platform_mmio_size = Some(size); - self - } - - pub fn create_allocator(&self, first_irq: u32) -> Result { - SystemAllocator::new( - self.io_base, - self.io_size, - self.high_mmio_base.ok_or(Error::MissingHighMMIOAddresses)?, - self.high_mmio_size.ok_or(Error::MissingHighMMIOAddresses)?, - self.low_mmio_base.ok_or(Error::MissingLowMMIOAddresses)?, - self.low_mmio_size.ok_or(Error::MissingLowMMIOAddresses)?, - self.platform_mmio_base, - self.platform_mmio_size, - first_irq, - ) + assert_eq!(a.allocate_irq(), Some(5)); + assert_eq!(a.allocate_irq(), Some(6)); + assert_eq!( + a.mmio_allocator(MmioType::High).allocate( + 0x100, + Alloc::PciBar { + bus: 0, + dev: 0, + func: 0, + bar: 0 + }, + "bar0".to_string() + ), + Ok(0x10000000) + ); + assert_eq!( + a.mmio_allocator(MmioType::High).get(&Alloc::PciBar { + bus: 0, + dev: 0, + func: 0, + bar: 0 + }), + Some(&(0x10000000, 0x100, "bar0".to_string())) + ); } } diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs index e8e7053921..3b0a39fb22 100644 --- a/x86_64/src/lib.rs +++ b/x86_64/src/lib.rs @@ -63,7 +63,7 @@ use devices::{ use hypervisor::{HypervisorX86_64, ProtectionType, VcpuX86_64, Vm, VmX86_64}; use minijail::Minijail; use remain::sorted; -use resources::SystemAllocator; +use resources::{MemRegion, SystemAllocator, SystemAllocatorConfig}; use sync::Mutex; use thiserror::Error; use vm_control::{BatControl, BatteryType}; @@ -394,12 +394,23 @@ impl arch::LinuxArch for X8664arch { let guest_mem = vm.get_memory(); let high_mmio_start = Self::get_high_mmio_base(guest_mem); let high_mmio_size = Self::get_high_mmio_size(vm); - SystemAllocator::builder() - .add_io_addresses(0xc000, 0x1_0000) - .add_low_mmio_addresses(END_ADDR_BEFORE_32BITS, PCI_MMIO_SIZE) - .add_high_mmio_addresses(high_mmio_start, high_mmio_size) - .create_allocator(X86_64_IRQ_BASE) - .unwrap() + SystemAllocator::new(SystemAllocatorConfig { + io: Some(MemRegion { + base: 0xc000, + size: 0x1_0000, + }), + low_mmio: MemRegion { + base: END_ADDR_BEFORE_32BITS, + size: PCI_MMIO_SIZE, + }, + high_mmio: MemRegion { + base: high_mmio_start, + size: high_mmio_size, + }, + platform_mmio: None, + first_irq: X86_64_IRQ_BASE, + }) + .unwrap() } fn build_vm(