From 89218b6d1eec2e652dc06e3c5dca35af00dd2fad Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Thu, 14 Apr 2022 18:13:57 +0200 Subject: [PATCH] pci: Replace BAR tuple with PciBarConfiguration In order to make the code more consistent and easier to read, we remove the former tuple that was used to describe a BAR, replacing it with the existing structure PciBarConfiguration. Signed-off-by: Sebastien Boeuf --- pci/src/bus.rs | 12 +++--- pci/src/configuration.rs | 20 ++++++++-- pci/src/device.rs | 4 +- pci/src/vfio.rs | 18 ++++----- pci/src/vfio_user.rs | 5 +-- virtio-devices/src/transport/pci_device.rs | 46 ++++++++++------------ vmm/src/device_manager.rs | 12 +++--- 7 files changed, 62 insertions(+), 55 deletions(-) diff --git a/pci/src/bus.rs b/pci/src/bus.rs index 1a302b5fc..ff033ddab 100644 --- a/pci/src/bus.rs +++ b/pci/src/bus.rs @@ -6,13 +6,13 @@ use crate::configuration::{ PciBarRegionType, PciBridgeSubclass, PciClassCode, PciConfiguration, PciHeaderType, }; use crate::device::{DeviceRelocation, Error as PciDeviceError, PciDevice}; +use crate::PciBarConfiguration; use byteorder::{ByteOrder, LittleEndian}; use std::any::Any; use std::collections::HashMap; use std::ops::DerefMut; use std::sync::{Arc, Barrier, Mutex}; use vm_device::{Bus, BusDevice}; -use vm_memory::{Address, GuestAddress, GuestUsize}; const VENDOR_ID_INTEL: u16 = 0x8086; const DEVICE_ID_INTEL_VIRT_PCIE_HOST: u16 = 0x0d57; @@ -122,21 +122,21 @@ impl PciBus { dev: Arc>, #[cfg(target_arch = "x86_64")] io_bus: &Bus, mmio_bus: &Bus, - bars: Vec<(GuestAddress, GuestUsize, PciBarRegionType)>, + bars: Vec, ) -> Result<()> { - for (address, size, type_) in bars { - match type_ { + for bar in bars { + match bar.region_type() { PciBarRegionType::IoRegion => { #[cfg(target_arch = "x86_64")] io_bus - .insert(dev.clone(), address.raw_value(), size) + .insert(dev.clone(), bar.addr(), bar.size()) .map_err(PciRootError::PioInsert)?; #[cfg(target_arch = "aarch64")] error!("I/O region is not supported"); } PciBarRegionType::Memory32BitRegion | PciBarRegionType::Memory64BitRegion => { mmio_bus - .insert(dev.clone(), address.raw_value(), size) + .insert(dev.clone(), bar.addr(), bar.size()) .map_err(PciRootError::MmioInsert)?; } } diff --git a/pci/src/configuration.rs b/pci/src/configuration.rs index d51b14684..27072972b 100644 --- a/pci/src/configuration.rs +++ b/pci/src/configuration.rs @@ -961,15 +961,27 @@ impl PciBarConfiguration { self } - pub fn get_size(&self) -> u64 { - self.size - } - #[must_use] pub fn set_region_type(mut self, region_type: PciBarRegionType) -> Self { self.region_type = region_type; self } + + pub fn idx(&self) -> usize { + self.idx + } + + pub fn addr(&self) -> u64 { + self.addr + } + + pub fn size(&self) -> u64 { + self.size + } + + pub fn region_type(&self) -> PciBarRegionType { + self.region_type + } } #[cfg(test)] diff --git a/pci/src/device.rs b/pci/src/device.rs index 05a99dbe9..27f054605 100644 --- a/pci/src/device.rs +++ b/pci/src/device.rs @@ -3,13 +3,13 @@ // found in the LICENSE-BSD-3-Clause file. use crate::configuration::{self, PciBarRegionType}; +use crate::PciBarConfiguration; use std::any::Any; use std::fmt::{self, Display}; use std::sync::{Arc, Barrier, Mutex}; use std::{self, io, result}; use vm_allocator::{AddressAllocator, SystemAllocator}; use vm_device::{BusDevice, Resource}; -use vm_memory::{GuestAddress, GuestUsize}; #[derive(Debug)] pub enum Error { @@ -60,7 +60,7 @@ pub trait PciDevice: BusDevice { _allocator: &Arc>, _mmio_allocator: &mut AddressAllocator, _resources: Option>, - ) -> Result> { + ) -> Result> { Ok(Vec::new()) } diff --git a/pci/src/vfio.rs b/pci/src/vfio.rs index 4e21a3361..764f53cb5 100644 --- a/pci/src/vfio.rs +++ b/pci/src/vfio.rs @@ -364,8 +364,8 @@ impl VfioCommon { mmio_allocator: &mut AddressAllocator, vfio_wrapper: &dyn Vfio, resources: Option>, - ) -> Result, PciDeviceError> { - let mut ranges = Vec::new(); + ) -> Result, PciDeviceError> { + let mut bars = Vec::new(); let mut bar_id = VFIO_PCI_BAR0_REGION_INDEX as u32; // Going through all regular regions to compute the BAR size. @@ -378,7 +378,7 @@ impl VfioCommon { let restored_bar_addr = if let Some(resources) = &resources { match resources - .get(ranges.len()) + .get(bars.len()) .ok_or(PciDeviceError::MissingResource)? { Resource::MmioAddressRange { base, .. } => Some(GuestAddress(*base)), @@ -499,7 +499,7 @@ impl VfioCommon { } // We can now build our BAR configuration block. - let config = PciBarConfiguration::default() + let bar = PciBarConfiguration::default() .set_index(bar_id as usize) .set_address(bar_addr.raw_value()) .set_size(region_size) @@ -507,15 +507,15 @@ impl VfioCommon { if bar_id == VFIO_PCI_ROM_REGION_INDEX { self.configuration - .add_pci_rom_bar(&config, flags & 0x1) + .add_pci_rom_bar(&bar, flags & 0x1) .map_err(|e| PciDeviceError::IoRegistrationFailed(bar_addr.raw_value(), e))?; } else { self.configuration - .add_pci_bar(&config) + .add_pci_bar(&bar) .map_err(|e| PciDeviceError::IoRegistrationFailed(bar_addr.raw_value(), e))?; } - ranges.push((bar_addr, region_size, region_type)); + bars.push(bar); self.mmio_regions.push(MmioRegion { start: bar_addr, length: region_size, @@ -533,7 +533,7 @@ impl VfioCommon { } } - Ok(ranges) + Ok(bars) } pub(crate) fn free_bars( @@ -1339,7 +1339,7 @@ impl PciDevice for VfioPciDevice { allocator: &Arc>, mmio_allocator: &mut AddressAllocator, resources: Option>, - ) -> Result, PciDeviceError> { + ) -> Result, PciDeviceError> { self.common .allocate_bars(allocator, mmio_allocator, &self.vfio_wrapper, resources) } diff --git a/pci/src/vfio_user.rs b/pci/src/vfio_user.rs index df30596d7..0b5058935 100644 --- a/pci/src/vfio_user.rs +++ b/pci/src/vfio_user.rs @@ -4,7 +4,7 @@ // use crate::vfio::{Interrupt, Vfio, VfioCommon, VfioError}; -use crate::{BarReprogrammingParams, PciBarRegionType, VfioPciError}; +use crate::{BarReprogrammingParams, PciBarConfiguration, VfioPciError}; use crate::{ PciBdf, PciClassCode, PciConfiguration, PciDevice, PciDeviceError, PciHeaderType, PciSubclass, }; @@ -25,7 +25,6 @@ use vm_device::{BusDevice, Resource}; use vm_memory::bitmap::AtomicBitmap; use vm_memory::{ Address, GuestAddress, GuestAddressSpace, GuestMemory, GuestMemoryRegion, GuestRegionMmap, - GuestUsize, }; use vmm_sys_util::eventfd::EventFd; @@ -396,7 +395,7 @@ impl PciDevice for VfioUserPciDevice { allocator: &Arc>, mmio_allocator: &mut AddressAllocator, resources: Option>, - ) -> Result, PciDeviceError> { + ) -> Result, PciDeviceError> { self.common .allocate_bars(allocator, mmio_allocator, &self.vfio_wrapper, resources) } diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 92c2b4080..4112a6113 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -36,7 +36,7 @@ use vm_device::interrupt::{ InterruptIndex, InterruptManager, InterruptSourceGroup, MsiIrqGroupConfig, }; use vm_device::{BusDevice, Resource}; -use vm_memory::{Address, ByteValued, GuestAddress, GuestMemoryAtomic, GuestUsize, Le32}; +use vm_memory::{Address, ByteValued, GuestAddress, GuestMemoryAtomic, Le32}; use vm_migration::{ Migratable, MigratableError, Pausable, Snapshot, Snapshottable, Transportable, VersionMapped, }; @@ -333,7 +333,7 @@ pub struct VirtioPciDevice { cap_pci_cfg_info: VirtioPciCfgCapInfo, // Details of bar regions to free - bar_regions: Vec<(GuestAddress, GuestUsize, PciBarRegionType)>, + bar_regions: Vec, // EventFd to signal on to request activation activate_evt: EventFd, @@ -842,9 +842,8 @@ impl PciDevice for VirtioPciDevice { allocator: &Arc>, mmio_allocator: &mut AddressAllocator, resources: Option>, - ) -> std::result::Result, PciDeviceError> - { - let mut ranges = Vec::new(); + ) -> std::result::Result, PciDeviceError> { + let mut bars = Vec::new(); let device_clone = self.device.clone(); let device = device_clone.lock().unwrap(); let settings_bar_addr = if let Some(resources) = &resources { @@ -870,7 +869,6 @@ impl PciDevice for VirtioPciDevice { Some(CAPABILITY_BAR_SIZE), ) .ok_or(PciDeviceError::IoAllocationFailed(CAPABILITY_BAR_SIZE))?; - ranges.push((addr, CAPABILITY_BAR_SIZE, region_type)); (addr, region_type) } else { let region_type = PciBarRegionType::Memory32BitRegion; @@ -883,38 +881,34 @@ impl PciDevice for VirtioPciDevice { Some(CAPABILITY_BAR_SIZE), ) .ok_or(PciDeviceError::IoAllocationFailed(CAPABILITY_BAR_SIZE))?; - ranges.push((addr, CAPABILITY_BAR_SIZE, region_type)); (addr, region_type) }; - self.bar_regions - .push((virtio_pci_bar_addr, CAPABILITY_BAR_SIZE, region_type)); - let config = PciBarConfiguration::default() + let bar = PciBarConfiguration::default() .set_index(VIRTIO_COMMON_BAR_INDEX) .set_address(virtio_pci_bar_addr.raw_value()) .set_size(CAPABILITY_BAR_SIZE) .set_region_type(region_type); - self.configuration.add_pci_bar(&config).map_err(|e| { + self.configuration.add_pci_bar(&bar).map_err(|e| { PciDeviceError::IoRegistrationFailed(virtio_pci_bar_addr.raw_value(), e) })?; + bars.push(bar); + // Once the BARs are allocated, the capabilities can be added to the PCI configuration. self.add_pci_capabilities(VIRTIO_COMMON_BAR_INDEX as u8)?; // Allocate a dedicated BAR if there are some shared memory regions. if let Some(shm_list) = device.get_shm_regions() { - let config = PciBarConfiguration::default() + let bar = PciBarConfiguration::default() .set_index(VIRTIO_SHM_BAR_INDEX) .set_address(shm_list.addr.raw_value()) .set_size(shm_list.len); self.configuration - .add_pci_bar(&config) + .add_pci_bar(&bar) .map_err(|e| PciDeviceError::IoRegistrationFailed(shm_list.addr.raw_value(), e))?; - let region_type = PciBarRegionType::Memory64BitRegion; - ranges.push((shm_list.addr, shm_list.len, region_type)); - self.bar_regions - .push((shm_list.addr, shm_list.len, region_type)); + bars.push(bar); for (idx, shm) in shm_list.region_list.iter().enumerate() { let shm_cap = VirtioPciCap64::new( @@ -930,7 +924,9 @@ impl PciDevice for VirtioPciDevice { } } - Ok(ranges) + self.bar_regions = bars.clone(); + + Ok(bars) } fn free_bars( @@ -938,13 +934,13 @@ impl PciDevice for VirtioPciDevice { allocator: &mut SystemAllocator, mmio_allocator: &mut AddressAllocator, ) -> std::result::Result<(), PciDeviceError> { - for (addr, length, type_) in self.bar_regions.drain(..) { - match type_ { + for bar in self.bar_regions.drain(..) { + match bar.region_type() { PciBarRegionType::Memory32BitRegion => { - allocator.free_mmio_hole_addresses(addr, length); + allocator.free_mmio_hole_addresses(GuestAddress(bar.addr()), bar.size()); } PciBarRegionType::Memory64BitRegion => { - mmio_allocator.free(addr, length); + mmio_allocator.free(GuestAddress(bar.addr()), bar.size()); } _ => error!("Unexpected PCI bar type"), } @@ -955,9 +951,9 @@ impl PciDevice for VirtioPciDevice { fn move_bar(&mut self, old_base: u64, new_base: u64) -> result::Result<(), std::io::Error> { // We only update our idea of the bar in order to support free_bars() above. // The majority of the reallocation is done inside DeviceManager. - for (addr, _, _) in self.bar_regions.iter_mut() { - if (*addr).0 == old_base { - *addr = GuestAddress(new_base); + for bar in self.bar_regions.iter_mut() { + if bar.addr() == old_base { + *bar = bar.set_address(new_base); } } diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 7281b01ef..b372b0181 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -3285,16 +3285,16 @@ impl DeviceManager { .map_err(DeviceManagerError::AddPciDevice)?; let mut new_resources = Vec::new(); - for pci_bar in bars.iter() { - match pci_bar.2 { + for bar in bars { + match bar.region_type() { PciBarRegionType::IoRegion => new_resources.push(Resource::PioAddressRange { - base: pci_bar.0.raw_value() as u16, - size: pci_bar.1 as u16, + base: bar.addr() as u16, + size: bar.size() as u16, }), PciBarRegionType::Memory32BitRegion | PciBarRegionType::Memory64BitRegion => { new_resources.push(Resource::MmioAddressRange { - base: pci_bar.0.raw_value(), - size: pci_bar.1 as u64, + base: bar.addr(), + size: bar.size() as u64, }) } }