From 4ba032f8967c660b8eafb3adaf9b44b95474c7c3 Mon Sep 17 00:00:00 2001 From: Ming Yang Date: Wed, 11 Aug 2021 14:52:26 +0800 Subject: [PATCH 8/8] virtio: fix dev_id initialization for virtio-pci and vfio device on aarch64 platform On aarch64 platform, dev_id information is necessary for virtio-pci device to send msix. This commit fixes the dev_id initialization in virtio_pci transport module, and set dev_id information according to the number of bus where the device is attached to. Signed-off-by: Ming Yang --- pci/src/config.rs | 2 + pci/src/msix.rs | 63 ++++++++++++++------ pci/src/root_port.rs | 10 +--- vfio/src/vfio_pci.rs | 34 +++++++---- virtio/src/virtio_pci.rs | 123 ++++++++++++++++++++++++--------------- 5 files changed, 150 insertions(+), 82 deletions(-) diff --git a/pci/src/config.rs b/pci/src/config.rs index d7bd348..873fe7a 100644 --- a/pci/src/config.rs +++ b/pci/src/config.rs @@ -221,6 +221,7 @@ pub enum RegionType { } /// Registered bar. +#[derive(Clone)] pub struct Bar { region_type: RegionType, address: u64, @@ -266,6 +267,7 @@ pub enum PcieDevType { } /// Configuration space of PCI/PCIe device. +#[derive(Clone)] pub struct PciConfig { /// Configuration space data. pub config: Vec, diff --git a/pci/src/msix.rs b/pci/src/msix.rs index 2831af4..5c04575 100644 --- a/pci/src/msix.rs +++ b/pci/src/msix.rs @@ -10,16 +10,19 @@ // NON-INFRINGEMENT, MERCHANTABILITY OR FIT FOR A PARTICULAR PURPOSE. // See the Mulan PSL v2 for more details. -use std::sync::{Arc, Mutex}; +use std::sync::atomic::{AtomicU16, Ordering}; +use std::sync::{Arc, Mutex, Weak}; use address_space::{GuestAddress, Region, RegionOps}; use hypervisor::{MsiVector, KVM_FDS}; use migration::{DeviceStateDesc, FieldDesc, MigrationHook, MigrationManager, StateTransfer}; use util::{byte_code::ByteCode, num_ops::round_up}; -use crate::config::{CapId, PciConfig, RegionType}; +use crate::config::{CapId, PciConfig, RegionType, SECONDARY_BUS_NUM}; use crate::errors::{Result, ResultExt}; -use crate::{le_read_u16, le_read_u32, le_read_u64, le_write_u16, le_write_u32, le_write_u64}; +use crate::{ + le_read_u16, le_read_u32, le_read_u64, le_write_u16, le_write_u32, le_write_u64, PciBus, +}; pub const MSIX_TABLE_ENTRY_SIZE: u16 = 16; pub const MSIX_TABLE_SIZE_MAX: u16 = 0x7ff; @@ -71,7 +74,7 @@ pub struct Msix { pub func_masked: bool, pub enabled: bool, pub msix_cap_offset: u16, - pub dev_id: u16, + pub dev_id: Arc, } impl Msix { @@ -90,7 +93,7 @@ impl Msix { func_masked: true, enabled: true, msix_cap_offset, - dev_id, + dev_id: Arc::new(AtomicU16::new(dev_id)), }; msix.mask_all_vectors(); msix @@ -148,7 +151,11 @@ impl Msix { le_write_u64(&mut self.pba, offset, old_val & pending_bit).unwrap(); } - fn register_memory_region(msix: Arc>, region: &Region, dev_id: u16) -> Result<()> { + fn register_memory_region( + msix: Arc>, + region: &Region, + dev_id: Arc, + ) -> Result<()> { let locked_msix = msix.lock().unwrap(); let table_size = locked_msix.table.len() as u64; let pba_size = locked_msix.pba.len() as u64; @@ -170,7 +177,7 @@ impl Msix { let is_masked: bool = locked_msix.is_vector_masked(vector); if was_masked && !is_masked { locked_msix.clear_pending_vector(vector); - locked_msix.notify(vector, dev_id); + locked_msix.notify(vector, dev_id.load(Ordering::Acquire)); } true @@ -264,7 +271,7 @@ impl StateTransfer for Msix { state.func_masked = self.func_masked; state.enabled = self.enabled; state.msix_cap_offset = self.msix_cap_offset; - state.dev_id = self.dev_id; + state.dev_id = self.dev_id.load(Ordering::Acquire); Ok(state.as_bytes().to_vec()) } @@ -280,7 +287,7 @@ impl StateTransfer for Msix { self.func_masked = msix_state.func_masked; self.enabled = msix_state.enabled; self.msix_cap_offset = msix_state.msix_cap_offset; - self.dev_id = msix_state.dev_id; + self.dev_id = Arc::new(AtomicU16::new(msix_state.dev_id)); Ok(()) } @@ -318,7 +325,7 @@ impl MigrationHook for Msix { msg_data: msg.data, masked: false, #[cfg(target_arch = "aarch64")] - dev_id: self.dev_id as u32, + dev_id: self.dev_id.load(Ordering::Acquire) as u32, }; if let Err(e) = locked_irq_table.add_msi_route(allocated_gsi, msi_vector) { bail!("Failed to add msi route to global irq routing table: {}", e); @@ -330,7 +337,7 @@ impl MigrationHook for Msix { if self.is_vector_pending(vector) { self.clear_pending_vector(vector); - send_msix(msg, self.dev_id); + send_msix(msg, self.dev_id.load(Ordering::Acquire)); } } } @@ -377,7 +384,12 @@ fn send_msix(msg: Message, dev_id: u16) { } /// MSI-X initialization. -pub fn init_msix(bar_id: usize, vector_nr: u32, config: &mut PciConfig, dev_id: u16) -> Result<()> { +pub fn init_msix( + bar_id: usize, + vector_nr: u32, + config: &mut PciConfig, + dev_id: Arc, +) -> Result<()> { if vector_nr > MSIX_TABLE_SIZE_MAX as u32 + 1 { bail!("Too many msix vectors."); } @@ -401,7 +413,7 @@ pub fn init_msix(bar_id: usize, vector_nr: u32, config: &mut PciConfig, dev_id: table_size, pba_size, msix_cap_offset as u16, - dev_id, + dev_id.load(Ordering::Acquire), ))); let bar_size = ((table_size + pba_size) as u64).next_power_of_two(); let region = Region::init_container_region(bar_size); @@ -415,6 +427,17 @@ pub fn init_msix(bar_id: usize, vector_nr: u32, config: &mut PciConfig, dev_id: Ok(()) } +pub fn update_dev_id(parent_bus: &Weak>, devfn: u8, dev_id: &Arc) { + let bus_num = parent_bus + .upgrade() + .unwrap() + .lock() + .unwrap() + .number(SECONDARY_BUS_NUM as usize); + let device_id = ((bus_num as u16) << 8) | (devfn as u16); + dev_id.store(device_id, Ordering::Release); +} + #[cfg(test)] mod tests { use super::*; @@ -425,9 +448,15 @@ mod tests { let mut pci_config = PciConfig::new(PCI_CONFIG_SPACE_SIZE, 2); // Too many vectors. - assert!(init_msix(0, MSIX_TABLE_SIZE_MAX as u32 + 2, &mut pci_config, 0).is_err()); - - init_msix(1, 2, &mut pci_config, 0).unwrap(); + assert!(init_msix( + 0, + MSIX_TABLE_SIZE_MAX as u32 + 2, + &mut pci_config, + Arc::new(AtomicU16::new(0)) + ) + .is_err()); + + init_msix(1, 2, &mut pci_config, Arc::new(AtomicU16::new(0))).unwrap(); let msix_cap_start = 64_u8; assert_eq!(pci_config.last_cap_end, 64 + MSIX_CAP_SIZE as u16); // Capabilities pointer @@ -492,7 +521,7 @@ mod tests { #[test] fn test_write_config() { let mut pci_config = PciConfig::new(PCI_CONFIG_SPACE_SIZE, 2); - init_msix(0, 2, &mut pci_config, 0).unwrap(); + init_msix(0, 2, &mut pci_config, Arc::new(AtomicU16::new(0))).unwrap(); let msix = pci_config.msix.as_ref().unwrap(); let mut locked_msix = msix.lock().unwrap(); locked_msix.enabled = false; diff --git a/pci/src/root_port.rs b/pci/src/root_port.rs index ba1b5f7..948e31d 100644 --- a/pci/src/root_port.rs +++ b/pci/src/root_port.rs @@ -10,6 +10,7 @@ // NON-INFRINGEMENT, MERCHANTABILITY OR FIT FOR A PARTICULAR PURPOSE. // See the Mulan PSL v2 for more details. +use std::sync::atomic::AtomicU16; use std::sync::{Arc, Mutex, Weak}; use address_space::Region; @@ -117,13 +118,8 @@ impl PciDevOps for RootPort { config_space[PREF_MEMORY_LIMIT as usize] = PREF_MEM_RANGE_64BIT; self.config .add_pcie_cap(self.devfn, self.port_num, PcieDevType::RootPort as u8)?; - #[cfg(target_arch = "aarch64")] - { - self.dev_id = self.set_dev_id(0, self.devfn); - init_msix(0, 1, &mut self.config, self.dev_id)?; - } - #[cfg(target_arch = "x86_64")] - init_msix(0, 1, &mut self.config, 0)?; + + init_msix(0, 1, &mut self.config, Arc::new(AtomicU16::new(0)))?; let parent_bus = self.parent_bus.upgrade().unwrap(); let mut locked_parent_bus = parent_bus.lock().unwrap(); diff --git a/vfio/src/vfio_pci.rs b/vfio/src/vfio_pci.rs index 5cc674a..640ff20 100644 --- a/vfio/src/vfio_pci.rs +++ b/vfio/src/vfio_pci.rs @@ -14,6 +14,7 @@ use std::collections::HashMap; use std::mem::size_of; use std::os::unix::io::{AsRawFd, RawFd}; use std::path::Path; +use std::sync::atomic::{AtomicU16, Ordering}; use std::sync::{Arc, Mutex, Weak}; use byteorder::{ByteOrder, LittleEndian}; @@ -36,9 +37,9 @@ use pci::config::{ }; use pci::errors::Result as PciResult; use pci::msix::{ - is_msix_enabled, Msix, MSIX_CAP_CONTROL, MSIX_CAP_ENABLE, MSIX_CAP_FUNC_MASK, MSIX_CAP_ID, - MSIX_CAP_SIZE, MSIX_CAP_TABLE, MSIX_TABLE_BIR, MSIX_TABLE_ENTRY_SIZE, MSIX_TABLE_OFFSET, - MSIX_TABLE_SIZE_MAX, + is_msix_enabled, update_dev_id, Msix, MSIX_CAP_CONTROL, MSIX_CAP_ENABLE, MSIX_CAP_FUNC_MASK, + MSIX_CAP_ID, MSIX_CAP_SIZE, MSIX_CAP_TABLE, MSIX_TABLE_BIR, MSIX_TABLE_ENTRY_SIZE, + MSIX_TABLE_OFFSET, MSIX_TABLE_SIZE_MAX, }; use pci::{ le_read_u16, le_read_u32, le_write_u16, le_write_u32, ranges_overlap, PciBus, PciDevOps, @@ -93,7 +94,7 @@ pub struct VfioPciDevice { // Maintains a list of GSI with irqfds that are registered to kvm. gsi_msi_routes: Arc>>, devfn: u8, - dev_id: u16, + dev_id: Arc, name: String, parent_bus: Weak>, } @@ -119,7 +120,7 @@ impl VfioPciDevice { vfio_bars: Arc::new(Mutex::new(Vec::with_capacity(PCI_NUM_BARS as usize))), gsi_msi_routes: Arc::new(Mutex::new(Vec::new())), devfn, - dev_id: 0, + dev_id: Arc::new(AtomicU16::new(0)), name, parent_bus, }) @@ -404,7 +405,7 @@ impl VfioPciDevice { table_size, table_size / 128, cap_offset as u16, - self.dev_id, + self.dev_id.load(Ordering::Acquire), ))); self.pci_config.msix = Some(msix.clone()); @@ -418,6 +419,9 @@ impl VfioPciDevice { let cloned_dev = self.vfio_device.clone(); let cloned_gsi_routes = self.gsi_msi_routes.clone(); + let parent_bus = self.parent_bus.clone(); + let dev_id = self.dev_id.clone(); + let devfn = self.devfn; let write = move |data: &[u8], _: GuestAddress, offset: u64| -> bool { let mut locked_msix = msix.lock().unwrap(); locked_msix.table[offset as usize..(offset as usize + data.len())] @@ -429,13 +433,15 @@ impl VfioPciDevice { } let entry = locked_msix.get_message(vector as u16); + + update_dev_id(&parent_bus, devfn, &dev_id); let msix_vector = MsiVector { msg_addr_lo: entry.address_lo, msg_addr_hi: entry.address_hi, msg_data: entry.data, masked: false, #[cfg(target_arch = "aarch64")] - dev_id: locked_msix.dev_id as u32, + dev_id: dev_id.load(Ordering::Acquire) as u32, }; let mut locked_gsi_routes = cloned_gsi_routes.lock().unwrap(); @@ -707,7 +713,7 @@ impl PciDevOps for VfioPciDevice { .lock() .unwrap() .number(SECONDARY_BUS_NUM as usize); - self.dev_id = self.set_dev_id(bus_num, self.devfn); + self.dev_id = Arc::new(AtomicU16::new(self.set_dev_id(bus_num, self.devfn))); } self.msix_info = Some(PciResultExt::chain_err(self.get_msix_info(), || { @@ -799,7 +805,8 @@ impl PciDevOps for VfioPciDevice { } if ranges_overlap(offset, end, COMMAND as usize, COMMAND as usize + 4) { - self.pci_config.write(offset, data, self.dev_id); + self.pci_config + .write(offset, data, self.dev_id.load(Ordering::Acquire)); if le_read_u32(&self.pci_config.config, offset).unwrap() & COMMAND_MEMORY_SPACE as u32 != 0 @@ -822,7 +829,8 @@ impl PciDevOps for VfioPciDevice { } } } else if ranges_overlap(offset, end, BAR_0 as usize, (BAR_5 as usize) + REG_SIZE) { - self.pci_config.write(offset, data, self.dev_id); + self.pci_config + .write(offset, data, self.dev_id.load(Ordering::Acquire)); if size == 4 && LittleEndian::read_u32(data) != 0xffff_ffff { let parent_bus = self.parent_bus.upgrade().unwrap(); @@ -838,7 +846,8 @@ impl PciDevOps for VfioPciDevice { } } else if ranges_overlap(offset, end, cap_offset, cap_offset + MSIX_CAP_SIZE as usize) { let was_enable = is_msix_enabled(cap_offset, &self.pci_config.config); - self.pci_config.write(offset, data, self.dev_id); + self.pci_config + .write(offset, data, self.dev_id.load(Ordering::Acquire)); let is_enable = is_msix_enabled(cap_offset, &self.pci_config.config); if !was_enable && is_enable { @@ -853,7 +862,8 @@ impl PciDevOps for VfioPciDevice { } } } else { - self.pci_config.write(offset, data, self.dev_id); + self.pci_config + .write(offset, data, self.dev_id.load(Ordering::Acquire)); } } diff --git a/virtio/src/virtio_pci.rs b/virtio/src/virtio_pci.rs index 03645dc..aaf009e 100644 --- a/virtio/src/virtio_pci.rs +++ b/virtio/src/virtio_pci.rs @@ -24,6 +24,7 @@ use pci::config::{ VENDOR_ID, }; use pci::errors::{ErrorKind, Result as PciResult, ResultExt}; +use pci::msix::update_dev_id; use pci::{config::PciConfig, init_msix, le_write_u16, ranges_overlap, PciBus, PciDevOps}; use util::byte_code::ByteCode; use vmm_sys_util::eventfd::EventFd; @@ -459,13 +460,14 @@ pub struct VirtioPciState { } /// Virtio-PCI device structure +#[derive(Clone)] pub struct VirtioPciDevice { /// Name of this device name: String, /// The entity of virtio device device: Arc>, /// Device id - dev_id: u16, + dev_id: Arc, /// Devfn devfn: u8, /// If this device is activated or not. @@ -500,7 +502,7 @@ impl VirtioPciDevice { VirtioPciDevice { name, device, - dev_id: 0_u16, + dev_id: Arc::new(AtomicU16::new(0)), devfn, device_activated: Arc::new(AtomicBool::new(false)), sys_mem, @@ -518,7 +520,7 @@ impl VirtioPciDevice { fn assign_interrupt_cb(&mut self) { let cloned_common_cfg = self.common_config.clone(); let cloned_msix = self.config.msix.clone(); - let dev_id = self.dev_id; + let dev_id = self.dev_id.clone(); let cb = Arc::new(Box::new( move |int_type: &VirtioInterruptType, queue: Option<&Queue>| { let vector = match int_type { @@ -533,7 +535,9 @@ impl VirtioPciDevice { }; if let Some(msix) = &cloned_msix { - msix.lock().unwrap().notify(vector, dev_id); + msix.lock() + .unwrap() + .notify(vector, dev_id.load(Ordering::Acquire)); } else { bail!("Failed to send interrupt, msix does not exist"); } @@ -618,14 +622,7 @@ impl VirtioPciDevice { true }; - let cloned_virtio_dev = self.device.clone(); - let cloned_common_cfg = self.common_config.clone(); - let cloned_virtio_queue = self.queues.clone(); - let cloned_activated_flag = self.device_activated.clone(); - let cloned_notify_evts = self.notify_eventfds.clone(); - let cloned_sys_mem = self.sys_mem.clone(); - let cloned_int_cb = self.interrupt_cb.clone(); - let cloned_msix = self.config.msix.as_ref().unwrap().clone(); + let cloned_pci_device = self.clone(); let common_write = move |data: &[u8], _addr: GuestAddress, offset: u64| -> bool { let value = match data.len() { 1 => data[0] as u32, @@ -639,13 +636,18 @@ impl VirtioPciDevice { return false; } }; - let old_dev_status = cloned_common_cfg.lock().unwrap().device_status; + let old_dev_status = cloned_pci_device + .common_config + .lock() + .unwrap() + .device_status; - if let Err(e) = cloned_common_cfg.lock().unwrap().write_common_config( - &cloned_virtio_dev, - offset, - value, - ) { + if let Err(e) = cloned_pci_device + .common_config + .lock() + .unwrap() + .write_common_config(&cloned_pci_device.device.clone(), offset, value) + { error!( "Failed to read common config of virtio-pci device, error is {}", e.display_chain(), @@ -653,21 +655,29 @@ impl VirtioPciDevice { return false; } - if !cloned_activated_flag.load(Ordering::Acquire) - && cloned_common_cfg.lock().unwrap().check_device_status( - CONFIG_STATUS_ACKNOWLEDGE - | CONFIG_STATUS_DRIVER - | CONFIG_STATUS_DRIVER_OK - | CONFIG_STATUS_FEATURES_OK, - CONFIG_STATUS_FAILED, - ) + if !cloned_pci_device.device_activated.load(Ordering::Acquire) + && cloned_pci_device + .common_config + .lock() + .unwrap() + .check_device_status( + CONFIG_STATUS_ACKNOWLEDGE + | CONFIG_STATUS_DRIVER + | CONFIG_STATUS_DRIVER_OK + | CONFIG_STATUS_FEATURES_OK, + CONFIG_STATUS_FAILED, + ) { - let queue_type = cloned_common_cfg.lock().unwrap().queue_type; - let queues_config = &cloned_common_cfg.lock().unwrap().queues_config; - let mut locked_queues = cloned_virtio_queue.lock().unwrap(); + let queue_type = cloned_pci_device.common_config.lock().unwrap().queue_type; + let queues_config = &cloned_pci_device + .common_config + .lock() + .unwrap() + .queues_config; + let mut locked_queues = cloned_pci_device.queues.lock().unwrap(); for q_config in queues_config.iter() { let queue = Queue::new(*q_config, queue_type).unwrap(); - if !queue.is_valid(&cloned_sys_mem) { + if !queue.is_valid(&cloned_pci_device.sys_mem) { error!("Failed to activate device: Invalid queue"); return false; } @@ -675,10 +685,10 @@ impl VirtioPciDevice { locked_queues.push(arc_queue.clone()); } - let queue_evts = cloned_notify_evts.clone().events; - if let Some(cb) = cloned_int_cb.clone() { - if let Err(e) = cloned_virtio_dev.lock().unwrap().activate( - cloned_sys_mem.clone(), + let queue_evts = cloned_pci_device.notify_eventfds.clone().events; + if let Some(cb) = cloned_pci_device.interrupt_cb.clone() { + if let Err(e) = cloned_pci_device.device.lock().unwrap().activate( + cloned_pci_device.sys_mem.clone(), cb, &locked_queues, queue_evts, @@ -689,20 +699,43 @@ impl VirtioPciDevice { error!("Failed to activate device: No interrupt callback"); return false; } - cloned_activated_flag.store(true, Ordering::Release); + cloned_pci_device + .device_activated + .store(true, Ordering::Release); + + update_dev_id( + &cloned_pci_device.parent_bus, + cloned_pci_device.devfn, + &cloned_pci_device.dev_id, + ); } - if old_dev_status != 0 && cloned_common_cfg.lock().unwrap().device_status == 0 { - let mut locked_queues = cloned_virtio_queue.lock().unwrap(); + if old_dev_status != 0 + && cloned_pci_device + .common_config + .lock() + .unwrap() + .device_status + == 0 + { + let mut locked_queues = cloned_pci_device.queues.lock().unwrap(); locked_queues.clear(); - cloned_activated_flag.store(false, Ordering::Release); + cloned_pci_device + .device_activated + .store(false, Ordering::Release); + let cloned_msix = cloned_pci_device.config.msix.as_ref().unwrap().clone(); cloned_msix.lock().unwrap().reset(); - if let Err(e) = cloned_virtio_dev.lock().unwrap().reset() { + if let Err(e) = cloned_pci_device.device.lock().unwrap().reset() { error!( "Failed to reset virtio device, error is {}", e.display_chain() ); } + update_dev_id( + &cloned_pci_device.parent_bus, + cloned_pci_device.devfn, + &cloned_pci_device.dev_id, + ); } true @@ -874,15 +907,12 @@ impl PciDevOps for VirtioPciDevice { self.modern_mem_region_map(notify_cap)?; let nvectors = self.device.lock().unwrap().queue_num() + 1; - #[cfg(target_arch = "aarch64")] - { - self.dev_id = self.set_dev_id(0, self.devfn); - } + init_msix( VIRTIO_PCI_MSIX_BAR_IDX as usize, nvectors as u32, &mut self.config, - self.dev_id, + self.dev_id.clone(), )?; self.assign_interrupt_cb(); @@ -950,7 +980,8 @@ impl PciDevOps for VirtioPciDevice { return; } - self.config.write(offset, data, self.dev_id); + self.config + .write(offset, data, self.dev_id.clone().load(Ordering::Acquire)); if ranges_overlap( offset, end, @@ -1388,7 +1419,7 @@ mod tests { VIRTIO_PCI_MSIX_BAR_IDX as usize, virtio_pci.device.lock().unwrap().queue_num() as u32 + 1, &mut virtio_pci.config, - virtio_pci.dev_id, + virtio_pci.dev_id.clone(), ) .unwrap(); // Prepare valid queue config -- 2.25.1