From 02e27c06f9a4cdbf44a44d8403abe2154b6cc640 Mon Sep 17 00:00:00 2001 From: shenw0000 Date: Fri, 22 May 2026 15:53:25 +0000 Subject: [PATCH] Enhance cfg routing in pci express hierarchy to support 1/2/4 bytes --- vm/devices/cxl/src/test/cxl_test_device.rs | 9 ++- vm/devices/pci/pci_core/src/cfg_space_emu.rs | 83 ++++++++++++++++++++ vm/devices/pci/pcie/src/port.rs | 44 +++++++++-- vm/devices/pci/pcie/src/root.rs | 80 ++++--------------- vm/devices/pci/pcie/src/switch.rs | 73 ++++++++++++----- 5 files changed, 196 insertions(+), 93 deletions(-) diff --git a/vm/devices/cxl/src/test/cxl_test_device.rs b/vm/devices/cxl/src/test/cxl_test_device.rs index 671c134421..2767f38f04 100644 --- a/vm/devices/cxl/src/test/cxl_test_device.rs +++ b/vm/devices/cxl/src/test/cxl_test_device.rs @@ -458,11 +458,16 @@ impl MmioIntercept for CxlTestDevice { impl PciConfigSpace for CxlTestDevice { fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult { - self.cfg_space.read_u32(offset, value) + let mut bytes = [0u8; 4]; + let result = self.cfg_space.read(offset, &mut bytes); + if let IoResult::Ok = result { + *value = u32::from_le_bytes(bytes); + } + result } fn pci_cfg_write(&mut self, offset: u16, value: u32) -> IoResult { - self.cfg_space.write_u32(offset, value) + self.cfg_space.write(offset, &value.to_le_bytes()) } } diff --git a/vm/devices/pci/pci_core/src/cfg_space_emu.rs b/vm/devices/pci/pci_core/src/cfg_space_emu.rs index e48b327a31..6e3faf5e77 100644 --- a/vm/devices/pci/pci_core/src/cfg_space_emu.rs +++ b/vm/devices/pci/pci_core/src/cfg_space_emu.rs @@ -1001,6 +1001,41 @@ impl ConfigSpaceType0Emulator { self.state = ConfigSpaceType0EmulatorState::new(); } + /// Read from config space using a naturally aligned 1/2/4-byte buffer. + pub fn read(&self, offset: u16, data: &mut [u8]) -> IoResult { + if let Err(err) = validate_cfg_buffer_access(offset, data.len()) { + return IoResult::Err(err); + } + + let dword_offset = offset & !3; + let byte_shift = (offset & 3) * 8; + let mut dword_value = 0u32; + match self.read_u32(dword_offset, &mut dword_value) { + IoResult::Ok => {} + res => return res, + } + + let shifted = dword_value >> byte_shift; + data.copy_from_slice(&shifted.to_le_bytes()[..data.len()]); + + IoResult::Ok + } + + /// Write to config space using a naturally aligned 1/2/4-byte buffer. + pub fn write(&mut self, offset: u16, data: &[u8]) -> IoResult { + if let Err(err) = validate_cfg_buffer_access(offset, data.len()) { + return IoResult::Err(err); + } + + let dword_offset = offset & !3; + let byte_shift = (offset & 3) * 8; + let mut bytes = [0u8; 4]; + bytes[..data.len()].copy_from_slice(data); + let write_dword = u32::from_le_bytes(bytes) << byte_shift; + + self.write_u32(dword_offset, write_dword) + } + /// Read from the config space. `offset` must be 32-bit aligned. pub fn read_u32(&self, offset: u16, value: &mut u32) -> IoResult { use cfg_space::HeaderType00; @@ -1310,6 +1345,41 @@ impl ConfigSpaceType1Emulator { } } + /// Read from config space using a naturally aligned 1/2/4-byte buffer. + pub fn read(&self, offset: u16, data: &mut [u8]) -> IoResult { + if let Err(err) = validate_cfg_buffer_access(offset, data.len()) { + return IoResult::Err(err); + } + + let dword_offset = offset & !3; + let byte_shift = (offset & 3) * 8; + let mut dword_value = 0u32; + match self.read_u32(dword_offset, &mut dword_value) { + IoResult::Ok => {} + res => return res, + } + + let shifted = dword_value >> byte_shift; + data.copy_from_slice(&shifted.to_le_bytes()[..data.len()]); + + IoResult::Ok + } + + /// Write to config space using a naturally aligned 1/2/4-byte buffer. + pub fn write(&mut self, offset: u16, data: &[u8]) -> IoResult { + if let Err(err) = validate_cfg_buffer_access(offset, data.len()) { + return IoResult::Err(err); + } + + let dword_offset = offset & !3; + let byte_shift = (offset & 3) * 8; + let mut bytes = [0u8; 4]; + bytes[..data.len()].copy_from_slice(data); + let write_dword = u32::from_le_bytes(bytes) << byte_shift; + + self.write_u32(dword_offset, write_dword) + } + /// Read from the config space. `offset` must be 32-bit aligned. pub fn read_u32(&self, offset: u16, value: &mut u32) -> IoResult { use cfg_space::HeaderType01; @@ -1476,6 +1546,19 @@ fn from_low_high(low: u16, high: u16) -> u32 { (u32::from(high) << 16) | u32::from(low) } +fn validate_cfg_buffer_access(offset: u16, len: usize) -> Result<(), IoError> { + match len { + 1 | 2 | 4 => {} + _ => return Err(IoError::InvalidAccessSize), + } + + if !offset.is_multiple_of(len as u16) { + return Err(IoError::UnalignedAccess); + } + + Ok(()) +} + fn to_low_high(value: u32) -> (u16, u16) { (value as u16, (value >> 16) as u16) } diff --git a/vm/devices/pci/pcie/src/port.rs b/vm/devices/pci/pcie/src/port.rs index 9389a5932c..15dbf24a99 100644 --- a/vm/devices/pci/pcie/src/port.rs +++ b/vm/devices/pci/pcie/src/port.rs @@ -653,8 +653,12 @@ impl PcieDownstreamPort { bus: &u8, function: &u8, cfg_offset: u16, - value: &mut u32, + data: &mut [u8], ) -> IoResult { + if let Err(err) = validate_cfg_forward_access(cfg_offset, data.len()) { + return IoResult::Err(err); + } + let bus_range = self.cfg_space.assigned_bus_range(); // If the bus range is 0..=0, this indicates invalid/uninitialized bus configuration @@ -666,12 +670,13 @@ impl PcieDownstreamPort { if bus_range.contains(bus) { if let Some((_, device)) = &mut self.link { let secondary_bus = *bus_range.start(); + let mut dword_value = !0u32; let result = device.pci_cfg_read_with_routing( secondary_bus, *bus, *function, cfg_offset, - value, + &mut dword_value, ); if let Some(result) = result { @@ -680,6 +685,8 @@ impl PcieDownstreamPort { res => return res, } } + + data.copy_from_slice(&dword_value.to_le_bytes()[..data.len()]); } else if *bus != *bus_range.start() { tracelimit::warn_ratelimited!( "invalid access: bus number to access not within port's bus number range" @@ -697,8 +704,12 @@ impl PcieDownstreamPort { bus: &u8, function: &u8, cfg_offset: u16, - value: u32, + data: &[u8], ) -> IoResult { + if let Err(err) = validate_cfg_forward_access(cfg_offset, data.len()) { + return IoResult::Err(err); + } + let bus_range = self.cfg_space.assigned_bus_range(); // If the bus range is 0..=0, this indicates invalid/uninitialized bus configuration @@ -710,12 +721,16 @@ impl PcieDownstreamPort { if bus_range.contains(bus) { if let Some((_, device)) = &mut self.link { let secondary_bus = *bus_range.start(); + let mut bytes = [0u8; 4]; + bytes[..data.len()].copy_from_slice(data); + let write_dword = u32::from_le_bytes(bytes); + let result = device.pci_cfg_write_with_routing( secondary_bus, *bus, *function, cfg_offset, - value, + write_dword, ); if let Some(result) = result { @@ -825,6 +840,19 @@ impl PcieDownstreamPort { } } +fn validate_cfg_forward_access(cfg_offset: u16, len: usize) -> Result<(), IoError> { + match len { + 1 | 2 | 4 => {} + _ => return Err(IoError::InvalidAccessSize), + } + + if !cfg_offset.is_multiple_of(len as u16) { + return Err(IoError::UnalignedAccess); + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -1078,7 +1106,7 @@ mod tests { }), )); - let mut value = 0; + let mut value = [0u8; 4]; // All accesses on the secondary bus go through // pci_cfg_read_with_routing — the linked device is responsible // for dispatching function 0 to its own config space. @@ -1139,12 +1167,14 @@ mod tests { // All accesses on the secondary bus go through // pci_cfg_write_with_routing — the linked device is responsible // for dispatching function 0 to its own config space. + let write0 = 0xAAAA_0000u32.to_le_bytes(); assert!(matches!( - port.forward_cfg_write_with_routing(&1, &0, 0x10, 0xAAAA_0000), + port.forward_cfg_write_with_routing(&1, &0, 0x10, &write0), IoResult::Ok )); + let write1 = 0xBBBB_0000u32.to_le_bytes(); assert!(matches!( - port.forward_cfg_write_with_routing(&1, &2, 0x14, 0xBBBB_0000), + port.forward_cfg_write_with_routing(&1, &2, 0x14, &write1), IoResult::Ok )); diff --git a/vm/devices/pci/pcie/src/root.rs b/vm/devices/pci/pcie/src/root.rs index adb7a660cb..5ed0362006 100644 --- a/vm/devices/pci/pcie/src/root.rs +++ b/vm/devices/pci/pcie/src/root.rs @@ -35,7 +35,6 @@ use pci_core::spec::hwid::Subclass; use std::collections::HashMap; use std::sync::Arc; use vmcore::device_state::ChangeDeviceState; -use zerocopy::IntoBytes; /// A generic PCI Express root complex emulator. #[derive(InspectMut)] @@ -452,40 +451,22 @@ impl MmioIntercept for GenericPcieRootComplex { return IoResult::Err(err); } - // N.B. Emulators internally only support 4-byte aligned accesses to - // 4-byte registers, but the guest can use 1-, 2-, or 4 byte memory - // instructions to access ECAM. This function reads the 4-byte aligned - // value then shifts it around as needed before copying the data into - // the intercept completion bytes. - - let dword_aligned_addr = addr & !3; - let mut dword_value = !0; - match self.decode_ecam_access(dword_aligned_addr) { + match self.decode_ecam_access(addr) { DecodedEcamAccess::UnexpectedIntercept => { tracing::error!("unexpected intercept at address 0x{:16x}", addr); } DecodedEcamAccess::Unroutable => { tracelimit::warn_ratelimited!("unroutable config space access"); + data.fill(0xFF); } DecodedEcamAccess::InternalBus(port, cfg_offset) => { - check_result!(port.port.cfg_space.read_u32(cfg_offset, &mut dword_value)); + check_result!(port.port.cfg_space.read(cfg_offset, data)); } DecodedEcamAccess::DownstreamPort(port, bus_number, function, cfg_offset) => { - check_result!(port.forward_cfg_read( - &bus_number, - &function, - cfg_offset & !3, - &mut dword_value, - )); + check_result!(port.forward_cfg_read(&bus_number, &function, cfg_offset, data,)); } } - let byte_offset_within_dword = (addr & 3) as usize; - data.copy_from_slice( - &dword_value.as_bytes() - [byte_offset_within_dword..byte_offset_within_dword + data.len()], - ); - IoResult::Ok } @@ -498,35 +479,7 @@ impl MmioIntercept for GenericPcieRootComplex { return IoResult::Err(err); } - // N.B. Emulators internally only support 4-byte aligned accesses to - // 4-byte registers, but the guest can use 1-, 2-, or 4-byte memory - // instructions to access ECAM. If the guest is using a 1- or 2-byte - // instruction, this function reads the 4-byte aligned configuration - // register, masks in the new bytes being written by the guest, and - // uses the resulting value for write emulation. - - let dword_aligned_addr = addr & !3; - let write_dword = match data.len() { - 4 => { - let mut temp: u32 = 0; - temp.as_mut_bytes().copy_from_slice(data); - temp - } - _ => { - let mut temp_bytes: [u8; 4] = [0, 0, 0, 0]; - check_result!(self.mmio_read(dword_aligned_addr, &mut temp_bytes)); - - let byte_offset_within_dword = (addr & 3) as usize; - temp_bytes[byte_offset_within_dword..byte_offset_within_dword + data.len()] - .copy_from_slice(data); - - let mut temp: u32 = 0; - temp.as_mut_bytes().copy_from_slice(&temp_bytes); - temp - } - }; - - match self.decode_ecam_access(dword_aligned_addr) { + match self.decode_ecam_access(addr) { DecodedEcamAccess::UnexpectedIntercept => { tracing::error!("unexpected intercept at address 0x{:16x}", addr); } @@ -534,15 +487,10 @@ impl MmioIntercept for GenericPcieRootComplex { tracelimit::warn_ratelimited!("unroutable config space access"); } DecodedEcamAccess::InternalBus(port, cfg_offset) => { - check_result!(port.port.cfg_space.write_u32(cfg_offset, write_dword)); + check_result!(port.port.cfg_space.write(cfg_offset, data)); } DecodedEcamAccess::DownstreamPort(port, bus_number, function, cfg_offset) => { - check_result!(port.forward_cfg_write( - &bus_number, - &function, - cfg_offset, - write_dword, - )); + check_result!(port.forward_cfg_write(&bus_number, &function, cfg_offset, data,)); } } @@ -641,10 +589,10 @@ impl RootPort { bus: &u8, function: &u8, cfg_offset: u16, - value: &mut u32, + data: &mut [u8], ) -> IoResult { self.port - .forward_cfg_read_with_routing(bus, function, cfg_offset, value) + .forward_cfg_read_with_routing(bus, function, cfg_offset, data) } fn forward_cfg_write( @@ -652,10 +600,10 @@ impl RootPort { bus: &u8, function: &u8, cfg_offset: u16, - value: u32, + data: &[u8], ) -> IoResult { self.port - .forward_cfg_write_with_routing(bus, function, cfg_offset, value) + .forward_cfg_write_with_routing(bus, function, cfg_offset, data) } } @@ -817,6 +765,7 @@ mod tests { use cxl_spec::CxlComponentRegisterType; use cxl_spec::component_registers::test_helper::TestCxlComponentRegisterBlock; use pal_async::async_test; + use zerocopy::IntoBytes; fn instantiate_root_complex( start_bus: u8, @@ -1277,15 +1226,16 @@ mod tests { assert_eq!(bus_range, 0..=0); // Test that forwarding returns Ok but doesn't crash when bus range is invalid - let mut value = 0u32; + let mut value = [0u8; 4]; let result = root_port .port .forward_cfg_read_with_routing(&1, &0, 0x0, &mut value); assert!(matches!(result, IoResult::Ok)); + let write_value = 0u32.to_le_bytes(); let result = root_port .port - .forward_cfg_write_with_routing(&1, &0, 0x0, value); + .forward_cfg_write_with_routing(&1, &0, 0x0, &write_value); assert!(matches!(result, IoResult::Ok)); } diff --git a/vm/devices/pci/pcie/src/switch.rs b/vm/devices/pci/pcie/src/switch.rs index 3d69bddea7..c9ef3d84a9 100644 --- a/vm/devices/pci/pcie/src/switch.rs +++ b/vm/devices/pci/pcie/src/switch.rs @@ -313,7 +313,12 @@ impl GenericPcieSwitch { value: &mut u32, ) -> Option { if let Some((_, downstream_port)) = self.downstream_ports.get_mut(&function) { - Some(downstream_port.port.cfg_space.read_u32(cfg_offset, value)) + let mut bytes = [0u8; 4]; + let result = downstream_port.port.cfg_space.read(cfg_offset, &mut bytes); + if let IoResult::Ok = result { + *value = u32::from_le_bytes(bytes); + } + Some(result) } else { // No downstream switch port found for this device function None @@ -328,7 +333,12 @@ impl GenericPcieSwitch { value: u32, ) -> Option { if let Some((_, downstream_port)) = self.downstream_ports.get_mut(&function) { - Some(downstream_port.port.cfg_space.write_u32(cfg_offset, value)) + Some( + downstream_port + .port + .cfg_space + .write(cfg_offset, &value.to_le_bytes()), + ) } else { // No downstream switch port found for this device function None @@ -352,11 +362,17 @@ impl GenericPcieSwitch { } if downstream_bus_range.contains(&bus) { - return Some( - downstream_port - .port - .forward_cfg_read_with_routing(&bus, &function, cfg_offset, value), + let mut dword_bytes = [0u8; 4]; + let result = downstream_port.port.forward_cfg_read_with_routing( + &bus, + &function, + cfg_offset, + &mut dword_bytes, ); + if let IoResult::Ok = result { + *value = u32::from_le_bytes(dword_bytes); + } + return Some(result); } } @@ -381,11 +397,13 @@ impl GenericPcieSwitch { } if downstream_bus_range.contains(&bus) { - return Some( - downstream_port - .port - .forward_cfg_write_with_routing(&bus, &function, cfg_offset, value), - ); + let dword_bytes = value.to_le_bytes(); + return Some(downstream_port.port.forward_cfg_write_with_routing( + &bus, + &function, + cfg_offset, + &dword_bytes, + )); } } @@ -445,13 +463,20 @@ impl PciConfigSpace for GenericPcieSwitch { /// Reads the switch's own upstream-port config space (Type 0 view). fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult { // Forward to the upstream port's configuration space (the switch presents as the upstream port) - self.upstream_port.cfg_space.read_u32(offset, value) + let mut bytes = [0u8; 4]; + let result = self.upstream_port.cfg_space.read(offset, &mut bytes); + if let IoResult::Ok = result { + *value = u32::from_le_bytes(bytes); + } + result } /// Writes the switch's own upstream-port config space (Type 0 view). fn pci_cfg_write(&mut self, offset: u16, value: u32) -> IoResult { // Forward to the upstream port's configuration space (the switch presents as the upstream port) - self.upstream_port.cfg_space.write_u32(offset, value) + self.upstream_port + .cfg_space + .write(offset, &value.to_le_bytes()) } fn pci_cfg_read_with_routing( @@ -472,7 +497,12 @@ impl PciConfigSpace for GenericPcieSwitch { // targeting the switch's own upstream port config space. if target_bus == secondary_bus { if function == 0 { - return self.upstream_port.cfg_space.read_u32(offset, value); + let mut bytes = [0u8; 4]; + let result = self.upstream_port.cfg_space.read(offset, &mut bytes); + if let IoResult::Ok = result { + *value = u32::from_le_bytes(bytes); + } + return result; } } @@ -499,7 +529,10 @@ impl PciConfigSpace for GenericPcieSwitch { // targeting the switch's own upstream port config space. if target_bus == secondary_bus { if function == 0 { - return self.upstream_port.cfg_space.write_u32(offset, value); + return self + .upstream_port + .cfg_space + .write(offset, &value.to_le_bytes()); } } @@ -1523,23 +1556,25 @@ mod tests { // Type 0 config read to bus 1 (secondary), function 0 — this should // read the switch's upstream port config space and return its // vendor/device ID. - let mut value = 0u32; + let mut value = [0u8; 4]; let result = port.forward_cfg_read_with_routing(&1, &0, 0x0, &mut value); assert!(matches!(result, IoResult::Ok)); let expected = (UPSTREAM_SWITCH_PORT_DEVICE_ID as u32) << 16 | (VENDOR_ID as u32); assert_eq!( - value, expected, + u32::from_le_bytes(value), + expected, "Type 0 access to bus 1 function 0 must read the switch's upstream port" ); // Non-zero function on the same bus should return no device (switch // upstream port is single-function). - let mut value2 = 0u32; + let mut value2 = [0u8; 4]; let result2 = port.forward_cfg_read_with_routing(&1, &1, 0x0, &mut value2); assert!(matches!(result2, IoResult::Ok)); assert_eq!( - value2, !0, + u32::from_le_bytes(value2), + !0, "Non-zero function should return all-1s (no device)" ); }