diff --git a/virtio-devices/src/transport/pci_common_config.rs b/virtio-devices/src/transport/pci_common_config.rs index 11e1d3ac7..379622bd9 100644 --- a/virtio-devices/src/transport/pci_common_config.rs +++ b/virtio-devices/src/transport/pci_common_config.rs @@ -14,7 +14,6 @@ use log::{debug, error, warn}; use serde::{Deserialize, Serialize}; use virtio_queue::{Queue, QueueT}; use vm_migration::{MigratableError, Pausable, Snapshot, Snapshottable}; -use vm_virtio::AccessPlatform; use super::pci_device::VIRTQ_MSI_NO_VECTOR; use crate::VirtioDevice; @@ -125,7 +124,7 @@ pub fn get_vring_size(t: VringType, queue_size: u16) -> u64 { /// le64 queue_avail; // 0x28 // read-write /// le64 queue_used; // 0x30 // read-write pub struct VirtioPciCommonConfig { - pub access_platform: Option>, + pub device: Arc>, pub driver_status: Arc, pub config_generation: u8, pub device_feature_select: u32, @@ -136,12 +135,9 @@ pub struct VirtioPciCommonConfig { } impl VirtioPciCommonConfig { - pub fn new( - state: VirtioPciCommonConfigState, - access_platform: Option>, - ) -> Self { + pub fn new(state: VirtioPciCommonConfigState, device: Arc>) -> Self { VirtioPciCommonConfig { - access_platform, + device, driver_status: Arc::new(AtomicU8::new(state.driver_status)), config_generation: state.config_generation, device_feature_select: state.device_feature_select, @@ -164,13 +160,7 @@ impl VirtioPciCommonConfig { } } - pub fn read( - &mut self, - offset: u64, - data: &mut [u8], - queues: &[Queue], - device: Arc>, - ) { + pub fn read(&mut self, offset: u64, data: &mut [u8], queues: &[Queue]) { assert!(data.len() <= 8); match data.len() { @@ -183,7 +173,7 @@ impl VirtioPciCommonConfig { LittleEndian::write_u16(data, v); } 4 => { - let v = self.read_common_config_dword(offset, device); + let v = self.read_common_config_dword(offset); LittleEndian::write_u32(data, v); } 8 => { @@ -194,26 +184,14 @@ impl VirtioPciCommonConfig { } } - #[allow(clippy::needless_pass_by_value)] - pub fn write( - &mut self, - offset: u64, - data: &[u8], - queues: &mut [Queue], - device: Arc>, - ) { + pub fn write(&mut self, offset: u64, data: &[u8], queues: &mut [Queue]) { assert!(data.len() <= 8); match data.len() { 1 => self.write_common_config_byte(offset, data[0]), 2 => self.write_common_config_word(offset, LittleEndian::read_u16(data), queues), 4 => { - self.write_common_config_dword( - offset, - LittleEndian::read_u32(data), - queues, - device, - ); + self.write_common_config_dword(offset, LittleEndian::read_u32(data), queues); } 8 => self.write_common_config_qword(offset, LittleEndian::read_u64(data), queues), _ => error!("invalid data length for virtio write: len {}", data.len()), @@ -285,8 +263,12 @@ impl VirtioPciCommonConfig { 0x1c => self.with_queue_mut(queues, |q| { let ready = value == 1; q.set_ready(ready); - // Translate address of descriptor table and vrings. - if ready && let Some(access_platform) = &self.access_platform { + let access_platform = if ready { + self.device.lock().unwrap().access_platform() + } else { + None + }; + if let Some(access_platform) = access_platform { let desc_table = match access_platform .translate_gva(q.desc_table(), get_vring_size(VringType::Desc, q.size())) { @@ -337,15 +319,12 @@ impl VirtioPciCommonConfig { } } - #[allow(clippy::needless_pass_by_value)] - fn read_common_config_dword(&self, offset: u64, device: Arc>) -> u32 { + fn read_common_config_dword(&self, offset: u64) -> u32 { debug!("read_common_config_dword: offset 0x{offset:x}"); match offset { 0x00 => self.device_feature_select, 0x04 => { - let locked_device = device.lock().unwrap(); - // Only 64 bits of features (2 pages) are defined for now, so limit - // device_feature_select to avoid shifting by 64 or more bits. + let locked_device = self.device.lock().unwrap(); if self.device_feature_select < 2 { (locked_device.features() >> (self.device_feature_select * 32)) as u32 } else { @@ -360,14 +339,7 @@ impl VirtioPciCommonConfig { } } - #[allow(clippy::needless_pass_by_value)] - fn write_common_config_dword( - &mut self, - offset: u64, - value: u32, - queues: &mut [Queue], - device: Arc>, - ) { + fn write_common_config_dword(&mut self, offset: u64, value: u32, queues: &mut [Queue]) { debug!("write_common_config_dword: offset 0x{offset:x}"); match offset { @@ -375,7 +347,7 @@ impl VirtioPciCommonConfig { 0x08 => self.driver_feature_select = value, 0x0c => { if self.driver_feature_select < 2 { - let mut locked_device = device.lock().unwrap(); + let mut locked_device = self.device.lock().unwrap(); locked_device .ack_features(u64::from(value) << (self.driver_feature_select * 32)); } @@ -472,8 +444,9 @@ mod unit_tests { #[test] fn write_base_regs() { + let dev: Arc> = Arc::new(Mutex::new(DummyDevice(0))); let mut regs = VirtioPciCommonConfig { - access_platform: None, + device: dev.clone(), driver_status: Arc::new(AtomicU8::new(0xaa)), config_generation: 0x55, device_feature_select: 0x0, @@ -483,72 +456,69 @@ mod unit_tests { msix_queues: Arc::new(Mutex::new(vec![0; 3])), }; - let dev = Arc::new(Mutex::new(DummyDevice(0))); let mut queues = Vec::new(); // Can set all bits of driver_status. - regs.write(0x14, &[0x55], &mut queues, dev.clone()); + regs.write(0x14, &[0x55], &mut queues); let mut read_back = vec![0x00]; - regs.read(0x14, &mut read_back, &queues, dev.clone()); + regs.read(0x14, &mut read_back, &queues); assert_eq!(read_back[0], 0x55); // The config generation register is read only. - regs.write(0x15, &[0xaa], &mut queues, dev.clone()); + regs.write(0x15, &[0xaa], &mut queues); let mut read_back = vec![0x00]; - regs.read(0x15, &mut read_back, &queues, dev.clone()); + regs.read(0x15, &mut read_back, &queues); assert_eq!(read_back[0], 0x55); // Device features is read-only and passed through from the device. - regs.write(0x04, &[0, 0, 0, 0], &mut queues, dev.clone()); + regs.write(0x04, &[0, 0, 0, 0], &mut queues); let mut read_back = vec![0, 0, 0, 0]; - regs.read(0x04, &mut read_back, &queues, dev.clone()); + regs.read(0x04, &mut read_back, &queues); assert_eq!(LittleEndian::read_u32(&read_back), DUMMY_FEATURES as u32); // Feature select registers are read/write. - regs.write(0x00, &[1, 2, 3, 4], &mut queues, dev.clone()); + regs.write(0x00, &[1, 2, 3, 4], &mut queues); let mut read_back = vec![0, 0, 0, 0]; - regs.read(0x00, &mut read_back, &queues, dev.clone()); + regs.read(0x00, &mut read_back, &queues); assert_eq!(LittleEndian::read_u32(&read_back), 0x0403_0201); - regs.write(0x08, &[1, 2, 3, 4], &mut queues, dev.clone()); + regs.write(0x08, &[1, 2, 3, 4], &mut queues); let mut read_back = vec![0, 0, 0, 0]; - regs.read(0x08, &mut read_back, &queues, dev.clone()); + regs.read(0x08, &mut read_back, &queues); assert_eq!(LittleEndian::read_u32(&read_back), 0x0403_0201); // 'queue_select' can be read and written. - regs.write(0x16, &[0xaa, 0x55], &mut queues, dev.clone()); + regs.write(0x16, &[0xaa, 0x55], &mut queues); let mut read_back = vec![0x00, 0x00]; - regs.read(0x16, &mut read_back, &queues, dev); + regs.read(0x16, &mut read_back, &queues); assert_eq!(read_back[0], 0xaa); assert_eq!(read_back[1], 0x55); } #[test] fn oob_queue_select_does_not_panic() { - // Regression test: reading/writing queue_msix_vector (offset 0x1a) - // with an out-of-bounds queue_select must not panic. + let dev: Arc> = Arc::new(Mutex::new(DummyDevice(0))); let mut regs = VirtioPciCommonConfig { - access_platform: None, + device: dev.clone(), driver_status: Arc::new(AtomicU8::new(0)), config_generation: 0, device_feature_select: 0, driver_feature_select: 0, queue_select: 0, msix_config: Arc::new(AtomicU16::new(0)), - msix_queues: Arc::new(Mutex::new(vec![0; 1])), // only 1 queue + msix_queues: Arc::new(Mutex::new(vec![0; 1])), }; - let dev = Arc::new(Mutex::new(DummyDevice(0))); let mut queues = vec![Queue::new(256).unwrap()]; // Set queue_select to an out-of-bounds value. - regs.write(0x16, &[0xFF, 0xFF], &mut queues, dev.clone()); + regs.write(0x16, &[0xFF, 0xFF], &mut queues); // Read queue_msix_vector — must not panic, should return VIRTQ_MSI_NO_VECTOR. let mut read_back = vec![0x00, 0x00]; - regs.read(0x1a, &mut read_back, &queues, dev.clone()); + regs.read(0x1a, &mut read_back, &queues); assert_eq!(LittleEndian::read_u16(&read_back), VIRTQ_MSI_NO_VECTOR); // Write queue_msix_vector — must not panic. - regs.write(0x1a, &[0xAB, 0xCD], &mut queues, dev); + regs.write(0x1a, &[0xAB, 0xCD], &mut queues); } } diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 54a29caa5..98abb0493 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -404,7 +404,7 @@ impl VirtioPciDevice { memory: GuestMemoryAtomic, device: Arc>, msix_num: u16, - access_platform: Option>, + access_platform: Option<&Arc>, interrupt_manager: &dyn InterruptManager, pci_device_bdf: u32, activate_evt: EventFd, @@ -422,7 +422,7 @@ impl VirtioPciDevice { } let num_queues = locked_device.queue_max_sizes().len(); - if let Some(access_platform) = &access_platform { + if let Some(access_platform) = access_platform { locked_device.set_access_platform(access_platform.clone()); } @@ -518,7 +518,7 @@ impl VirtioPciDevice { })?; let common_config = if let Some(common_config_state) = common_config_state { - VirtioPciCommonConfig::new(common_config_state, access_platform) + VirtioPciCommonConfig::new(common_config_state, device.clone()) } else { VirtioPciCommonConfig::new( VirtioPciCommonConfigState { @@ -530,7 +530,7 @@ impl VirtioPciDevice { msix_config: VIRTQ_MSI_NO_VECTOR, msix_queues: vec![VIRTQ_MSI_NO_VECTOR; num_queues], }, - access_platform, + device.clone(), ) }; @@ -1139,12 +1139,10 @@ impl PciDevice for VirtioPciDevice { fn read_bar(&mut self, _base: u64, offset: u64, data: &mut [u8]) { match offset { - o if o < COMMON_CONFIG_BAR_OFFSET + COMMON_CONFIG_SIZE => self.common_config.read( - o - COMMON_CONFIG_BAR_OFFSET, - data, - &self.queues, - self.device.clone(), - ), + o if o < COMMON_CONFIG_BAR_OFFSET + COMMON_CONFIG_SIZE => { + self.common_config + .read(o - COMMON_CONFIG_BAR_OFFSET, data, &self.queues); + } o if (ISR_CONFIG_BAR_OFFSET..ISR_CONFIG_BAR_OFFSET + ISR_CONFIG_SIZE).contains(&o) => { if let Some(v) = data.get_mut(0) { // Reading this register resets it to 0. @@ -1185,12 +1183,10 @@ impl PciDevice for VirtioPciDevice { fn write_bar(&mut self, _base: u64, offset: u64, data: &[u8]) -> Option> { let initial_ready = self.is_driver_ready(); match offset { - o if o < COMMON_CONFIG_BAR_OFFSET + COMMON_CONFIG_SIZE => self.common_config.write( - o - COMMON_CONFIG_BAR_OFFSET, - data, - &mut self.queues, - self.device.clone(), - ), + o if o < COMMON_CONFIG_BAR_OFFSET + COMMON_CONFIG_SIZE => { + self.common_config + .write(o - COMMON_CONFIG_BAR_OFFSET, data, &mut self.queues); + } o if (ISR_CONFIG_BAR_OFFSET..ISR_CONFIG_BAR_OFFSET + ISR_CONFIG_SIZE).contains(&o) => { if let Some(v) = data.first() { self.interrupt_status diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 9f65784ec..eec0da83c 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -4405,7 +4405,7 @@ impl DeviceManager { memory, virtio_device, msix_num, - access_platform, + access_platform.as_ref(), self.msi_interrupt_manager.as_ref(), pci_device_bdf.into(), self.activate_evt