From adc5bb79580d15e1266232f5d4a9348fbef83f96 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Fri, 17 Apr 2026 09:14:44 +0100 Subject: [PATCH] virtio-devices: Add a VirtioDevice reference to VirtioPciCommonConfig Replace the stored AccessPlatform reference with one to the VirtioDevice. By doing this not only does it allow the code to be simplified but also now makes it virtio spec compliant by only translating via the access platform if the feature is acknowledged. Signed-off-by: Rob Bradford --- .../src/transport/pci_common_config.rs | 104 +++++++----------- virtio-devices/src/transport/pci_device.rs | 28 ++--- vmm/src/device_manager.rs | 2 +- 3 files changed, 50 insertions(+), 84 deletions(-) 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