Skip to content

Enhance cfg routing in pci express hierarchy to support 1/2/4 bytes #3560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
shenw0000 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Enhance cfg routing in pci express hierarchy to support 1/2/4 bytes #3560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions vm/devices/cxl/src/test/cxl_test_device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}

Expand Down
83 changes: 83 additions & 0 deletions vm/devices/pci/pci_core/src/cfg_space_emu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Comment on lines +1034 to +1035
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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
}
Expand Down
44 changes: 37 additions & 7 deletions vm/devices/pci/pcie/src/port.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -680,6 +685,8 @@ impl PcieDownstreamPort {
res => return res,
}
}

data.copy_from_slice(&dword_value.to_le_bytes()[..data.len()]);
Comment on lines 670 to +689
} else if *bus != *bus_range.start() {
tracelimit::warn_ratelimited!(
"invalid access: bus number to access not within port's bus number range"
Expand All @@ -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
Expand All @@ -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,
Comment on lines +724 to 732
value,
write_dword,
);

if let Some(result) = result {
Expand Down Expand Up @@ -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::*;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
));

Expand Down
80 changes: 15 additions & 65 deletions vm/devices/pci/pcie/src/root.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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
}

Expand All @@ -498,51 +479,18 @@ 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);
}
DecodedEcamAccess::Unroutable => {
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,));
}
}

Expand Down Expand Up @@ -641,21 +589,21 @@ 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(
&mut self,
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)
}
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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));
}

Expand Down
Loading
Loading