From 07c029ec21c352b79fa3b542a4544253568af971 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jan 2024 18:10:57 +0100 Subject: [PATCH 1/7] Add a fixme for using ACPI --- pcid/src/main.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pcid/src/main.rs b/pcid/src/main.rs index df1799ffb0..83d7ef2ce7 100644 --- a/pcid/src/main.rs +++ b/pcid/src/main.rs @@ -581,8 +581,11 @@ fn main(args: Args) { let pci = state.preferred_cfg_access(); - info!("PCI BS/DV/FN VEND:DEVI CL.SC.IN.RV"); + info!("PCI SG-BS:DV.F VEND:DEVI CL.SC.IN.RV"); + // FIXME Use full ACPI for enumerating the host bridges. MCFG only describes the first + // host bridge, while multi-processor systems likely have a host bridge for each CPU. + // See also https://www.kernel.org/doc/html/latest/PCI/acpi-info.html let mut bus_nums = vec![0]; let mut bus_i = 0; while bus_i < bus_nums.len() { From ae7e2dac9f7eed4ec86535ed8afc7396131b9c2f Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jan 2024 18:41:28 +0100 Subject: [PATCH 2/7] Only parse a single MCFG ACPI table There should only be one and Linux only parses the first one too. --- Cargo.lock | 1 - pcid/Cargo.toml | 1 - pcid/src/pcie/mod.rs | 46 ++++++++++++++++++++------------------------ 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1ec51a7bb2..a75774a6d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -968,7 +968,6 @@ dependencies = [ "redox_syscall 0.4.1", "serde", "serde_json", - "smallvec 1.11.0", "structopt", "thiserror", "toml 0.5.11", diff --git a/pcid/Cargo.toml b/pcid/Cargo.toml index 1927fa84f0..6c276cc52b 100644 --- a/pcid/Cargo.toml +++ b/pcid/Cargo.toml @@ -24,7 +24,6 @@ redox-log = "0.1" redox_syscall = "0.4" serde = { version = "1", features = ["derive"] } serde_json = "1" -smallvec = "1" structopt = { version = "0.3", default-features = false, features = [ "paw" ] } thiserror = "1" toml = "0.5" diff --git a/pcid/src/pcie/mod.rs b/pcid/src/pcie/mod.rs index 623471a230..46072ef883 100644 --- a/pcid/src/pcie/mod.rs +++ b/pcid/src/pcie/mod.rs @@ -4,8 +4,6 @@ use std::{fmt, fs, io, mem, ptr, slice}; use syscall::PAGE_SIZE; -use smallvec::{smallvec, SmallVec}; - use crate::pci::{CfgAccess, Pci, PciAddress, PciIter}; pub const MCFG_NAME: [u8; 4] = *b"MCFG"; @@ -73,29 +71,21 @@ impl fmt::Debug for Mcfg { } pub struct Mcfgs { - tables: SmallVec<[Vec; 2]>, + bytes: Vec, } impl Mcfgs { - pub fn tables<'a>(&'a self) -> impl Iterator + 'a { - self.tables.iter().filter_map(|bytes| { - let mcfg = plain::from_bytes::(bytes).ok()?; - if mcfg.length as usize > bytes.len() { + pub fn table<'a>(&'a self) -> Option<&'a Mcfg> { + let mcfg = plain::from_bytes::(&self.bytes).ok()?; + if mcfg.length as usize > self.bytes.len() { return None; } Some(mcfg) - }) - } - pub fn allocs<'a>(&'a self) -> impl Iterator + 'a { - self.tables() - .map(|table| table.base_addr_structs().iter()) - .flatten() } pub fn fetch() -> io::Result { let table_dir = fs::read_dir("acpi:tables")?; - let mut tables = smallvec![]; for table_direntry in table_dir { let table_path = table_direntry?.path(); @@ -107,24 +97,30 @@ impl Mcfgs { // is fine. let table_filename = table_path.file_name().unwrap().as_encoded_bytes(); if table_filename.get(0..4) == Some(&MCFG_NAME) { - tables.push(fs::read(table_path)?); + // There should only be a single MCFG table and Linux ignores + // all MCFG tables beyond the first too. + return Ok(Self { + bytes: fs::read(table_path)?, + }); } } - Ok(Self { tables }) + Err(io::Error::new( + io::ErrorKind::NotFound, + "couldn't find mcfg table", + )) } - pub fn table_and_alloc_at_bus(&self, bus: u8) -> Option<(&Mcfg, &PcieAlloc)> { - self.tables().find_map(|table| { - Some(( - table, + + pub fn at_bus(&self, bus: u8) -> Option<&PcieAlloc> { + if let Some(table) = (&self).table() { + Some( table.base_addr_structs().iter().find(|addr_struct| { (addr_struct.start_bus..=addr_struct.end_bus).contains(&bus) })?, - )) - }) + ) + } else { + None } - pub fn at_bus(&self, bus: u8) -> Option<&PcieAlloc> { - self.table_and_alloc_at_bus(bus).map(|(_, alloc)| alloc) } } @@ -133,7 +129,7 @@ impl fmt::Debug for Mcfgs { struct Tables<'a>(&'a Mcfgs); impl<'a> fmt::Debug for Tables<'a> { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_list().entries(self.0.tables()).finish() + f.debug_list().entries(self.0.table()).finish() } } From 8d87b701ba9360caaaa3d2e3909b4024ea264027 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jan 2024 19:10:35 +0100 Subject: [PATCH 3/7] Remove Mcfgs type This wrapper was only useful back when we accepted multiple MCFG tables. --- pcid/src/pcie/mod.rs | 136 ++++++++++++++++++++----------------------- 1 file changed, 64 insertions(+), 72 deletions(-) diff --git a/pcid/src/pcie/mod.rs b/pcid/src/pcie/mod.rs index 46072ef883..272f904534 100644 --- a/pcid/src/pcie/mod.rs +++ b/pcid/src/pcie/mod.rs @@ -41,7 +41,65 @@ pub struct PcieAlloc { unsafe impl plain::Plain for PcieAlloc {} impl Mcfg { - pub fn base_addr_structs(&self) -> &[PcieAlloc] { + fn fetch() -> io::Result<&'static Mcfg> { + let table_dir = fs::read_dir("acpi:tables")?; + + for table_direntry in table_dir { + let table_path = table_direntry?.path(); + + // Every directory entry has to have a filename unless + // the filesystem (or in this case acpid) misbehaves. + // If it misbehaves we have worse problems than pcid + // crashing. `as_encoded_bytes()` returns some superset + // of ASCII, so directly comparing it with an ASCII name + // is fine. + let table_filename = table_path.file_name().unwrap().as_encoded_bytes(); + if table_filename.get(0..4) == Some(&MCFG_NAME) { + let bytes = fs::read(table_path)?.into_boxed_slice(); + if Mcfg::parse(&*bytes).is_none() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "couldn't find mcfg table", + )); + } + + // Leaking memory is fine as this function is only called once + // and we need this for the entire lifetime of pcid anyway. + let bytes = Box::leak(bytes); + + // This unwrap is fine as we checked that it will return Some above. + let mcfg = Mcfg::parse(bytes).unwrap(); + + // There should only be a single MCFG table and Linux ignores + // all MCFG tables beyond the first too, so doing an early + // return here is fine. + return Ok(mcfg); + } + } + + Err(io::Error::new( + io::ErrorKind::NotFound, + "couldn't find mcfg table", + )) + } + + fn parse<'a>(bytes: &'a [u8]) -> Option<&'a Mcfg> { + let mcfg = plain::from_bytes::(bytes).ok()?; + if mcfg.length as usize > bytes.len() { + return None; + } + Some(mcfg) + } + + fn at_bus(&self, bus: u8) -> Option<&PcieAlloc> { + Some( + self.base_addr_structs() + .iter() + .find(|addr_struct| (addr_struct.start_bus..=addr_struct.end_bus).contains(&bus))?, + ) + } + + fn base_addr_structs(&self) -> &[PcieAlloc] { let total_length = self.length as usize; let len = total_length - mem::size_of::(); // safe because the length cannot be changed arbitrarily @@ -53,6 +111,7 @@ impl Mcfg { } } } + impl fmt::Debug for Mcfg { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_struct("Mcfg") @@ -70,76 +129,9 @@ impl fmt::Debug for Mcfg { } } -pub struct Mcfgs { - bytes: Vec, -} - -impl Mcfgs { - pub fn table<'a>(&'a self) -> Option<&'a Mcfg> { - let mcfg = plain::from_bytes::(&self.bytes).ok()?; - if mcfg.length as usize > self.bytes.len() { - return None; - } - Some(mcfg) - } - - pub fn fetch() -> io::Result { - let table_dir = fs::read_dir("acpi:tables")?; - - for table_direntry in table_dir { - let table_path = table_direntry?.path(); - - // Every directory entry has to have a filename unless - // the filesystem (or in this case acpid) misbehaves. - // If it misbehaves we have worse problems than pcid - // crashing. `as_encoded_bytes()` returns some superset - // of ASCII, so directly comparing it with an ASCII name - // is fine. - let table_filename = table_path.file_name().unwrap().as_encoded_bytes(); - if table_filename.get(0..4) == Some(&MCFG_NAME) { - // There should only be a single MCFG table and Linux ignores - // all MCFG tables beyond the first too. - return Ok(Self { - bytes: fs::read(table_path)?, - }); - } - } - - Err(io::Error::new( - io::ErrorKind::NotFound, - "couldn't find mcfg table", - )) - } - - pub fn at_bus(&self, bus: u8) -> Option<&PcieAlloc> { - if let Some(table) = (&self).table() { - Some( - table.base_addr_structs().iter().find(|addr_struct| { - (addr_struct.start_bus..=addr_struct.end_bus).contains(&bus) - })?, - ) - } else { - None - } - } -} - -impl fmt::Debug for Mcfgs { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - struct Tables<'a>(&'a Mcfgs); - impl<'a> fmt::Debug for Tables<'a> { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_list().entries(self.0.table()).finish() - } - } - - f.debug_tuple("Mcfgs").field(&Tables(self)).finish() - } -} - pub struct Pcie { lock: Mutex<()>, - mcfgs: Mcfgs, + mcfg: &'static Mcfg, maps: Mutex>, fallback: Arc, } @@ -148,11 +140,11 @@ unsafe impl Sync for Pcie {} impl Pcie { pub fn new(fallback: Arc) -> io::Result { - let mcfgs = Mcfgs::fetch()?; + let mcfg = Mcfg::fetch()?; Ok(Self { lock: Mutex::new(()), - mcfgs, + mcfg, maps: Mutex::new(BTreeMap::new()), fallback, }) @@ -181,7 +173,7 @@ impl Pcie { offset: u16, f: F, ) -> T { - let (base_address_phys, starting_bus) = match self.mcfgs.at_bus(address.bus()) { + let (base_address_phys, starting_bus) = match self.mcfg.at_bus(address.bus()) { Some(t) => (t.base_addr, t.start_bus), None => return f(None), }; From cb9edcb7d6f5437183a7f653d098c7b1843462db Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jan 2024 19:11:05 +0100 Subject: [PATCH 4/7] Reduce log verbosity of Capability::parse_vendor by combining some logs --- pcid/src/pci/cap.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pcid/src/pci/cap.rs b/pcid/src/pci/cap.rs index 02b3301450..99fc3e09b2 100644 --- a/pcid/src/pci/cap.rs +++ b/pcid/src/pci/cap.rs @@ -182,10 +182,9 @@ impl Capability { }) } unsafe fn parse_vendor(reader: &R, offset: u8) -> Self { - log::info!("Vendor specific offset: {}", offset); - log::info!("Vendor specific next: {}", reader.read_u8((offset+1).into())); + let next = reader.read_u8(u16::from(offset+1)); let length = reader.read_u8(u16::from(offset+2)); - log::info!("Vendor specific cap len: {}", length); + log::info!("Vendor specific offset: {offset:#02x} next: {next:#02x} cap len: {length:#02x}"); let data = if length > 0 { let mut raw_data = reader.read_range(offset.into(), length.into()); raw_data.drain(3..).collect() From 23d963361a2b9da9fe282ee1122d0e7476d0ac88 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jan 2024 19:14:22 +0100 Subject: [PATCH 5/7] Fix a couple of warnings --- pcid/src/pcie/mod.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/pcid/src/pcie/mod.rs b/pcid/src/pcie/mod.rs index 272f904534..7a21b0e9a9 100644 --- a/pcid/src/pcie/mod.rs +++ b/pcid/src/pcie/mod.rs @@ -4,7 +4,7 @@ use std::{fmt, fs, io, mem, ptr, slice}; use syscall::PAGE_SIZE; -use crate::pci::{CfgAccess, Pci, PciAddress, PciIter}; +use crate::pci::{CfgAccess, Pci, PciAddress}; pub const MCFG_NAME: [u8; 4] = *b"MCFG"; @@ -164,9 +164,6 @@ impl Pcie { | ((address.function() as usize) << 12) | (offset as usize) } - fn addr_offset_in_dwords(starting_bus: u8, address: PciAddress, offset: u16) -> usize { - Self::addr_offset_in_bytes(starting_bus, address, offset) / mem::size_of::() - } unsafe fn with_pointer) -> T>( &self, address: PciAddress, @@ -199,9 +196,6 @@ impl Pcie { (offset as usize / mem::size_of::()) as isize, ))) } - pub fn buses<'pcie>(&'pcie self) -> PciIter<'pcie> { - PciIter::new(self) - } } impl CfgAccess for Pcie { From 529b4935ee8abdf1c729dfe1f9b1ad848fc42ac5 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jan 2024 19:50:11 +0100 Subject: [PATCH 6/7] Physmap all buses upfront This avoids a costly Mutex lock and BTreeMap lookup for each config space access. --- pcid/src/pcie/mod.rs | 93 ++++++++++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 38 deletions(-) diff --git a/pcid/src/pcie/mod.rs b/pcid/src/pcie/mod.rs index 7a21b0e9a9..b8f4da6c0a 100644 --- a/pcid/src/pcie/mod.rs +++ b/pcid/src/pcie/mod.rs @@ -1,9 +1,6 @@ -use std::collections::BTreeMap; use std::sync::{Arc, Mutex}; use std::{fmt, fs, io, mem, ptr, slice}; -use syscall::PAGE_SIZE; - use crate::pci::{CfgAccess, Pci, PciAddress}; pub const MCFG_NAME: [u8; 4] = *b"MCFG"; @@ -131,8 +128,7 @@ impl fmt::Debug for Mcfg { pub struct Pcie { lock: Mutex<()>, - mcfg: &'static Mcfg, - maps: Mutex>, + bus_maps: Vec>, fallback: Arc, } unsafe impl Send for Pcie {} @@ -142,25 +138,52 @@ impl Pcie { pub fn new(fallback: Arc) -> io::Result { let mcfg = Mcfg::fetch()?; + let alloc_maps = (0..=255) + .map(|bus| { + if let Some(alloc) = mcfg.at_bus(bus) { + Some(unsafe { Self::physmap_pcie_bus(alloc, bus) }) + } else { + None + } + }) + .collect(); + Ok(Self { lock: Mutex::new(()), - mcfg, - maps: Mutex::new(BTreeMap::new()), + bus_maps: alloc_maps, fallback, }) } - fn addr_offset_in_bytes(starting_bus: u8, address: PciAddress, offset: u16) -> usize { + + unsafe fn physmap_pcie_bus(alloc: &PcieAlloc, bus: u8) -> (*mut u32, usize) { + let base_phys = alloc.base_addr as usize + (((bus - alloc.start_bus) as usize) << 20); + let map_size = 1 << 20; + let ptr = common::physmap( + base_phys, + map_size, + common::Prot { + read: true, + write: true, + }, + common::MemoryType::Uncacheable, + ) + .unwrap_or_else(|error| { + panic!( + "failed to physmap pcie configuration space for segment {} bus {} @ {:p}: {:?}", + { alloc.seg_group_num }, + bus, + base_phys as *const u32, + error, + ) + }) as *mut u32; + (ptr, map_size) + } + + fn bus_addr_offset_in_bytes(address: PciAddress, offset: u16) -> usize { assert_eq!(offset & 0xFFFC, offset, "pcie offset not dword-aligned"); assert_eq!(offset & 0x0FFF, offset, "pcie offset larger than 4095"); - assert_eq!( - address.segment(), - 0, - "multiple segments not yet implemented" - ); - - (((address.bus() - starting_bus) as usize) << 20) - | ((address.device() as usize) << 15) + ((address.device() as usize) << 15) | ((address.function() as usize) << 12) | (offset as usize) } @@ -170,28 +193,20 @@ impl Pcie { offset: u16, f: F, ) -> T { - let (base_address_phys, starting_bus) = match self.mcfg.at_bus(address.bus()) { - Some(t) => (t.base_addr, t.start_bus), + assert_eq!( + address.segment(), + 0, + "multiple segments not yet implemented" + ); + + let bus_addr = match self.bus_maps[address.bus() as usize] { + Some(bus_addr) => bus_addr, None => return f(None), }; - let mut maps_lock = self.maps.lock().unwrap(); - let virt_pointer = maps_lock.entry(address).or_insert_with(|| { - common::physmap( - base_address_phys as usize + Self::addr_offset_in_bytes(starting_bus, address, 0), - PAGE_SIZE, - common::Prot { - read: true, - write: true, - }, - common::MemoryType::Uncacheable, - ) - .unwrap_or_else(|error| { - panic!( - "failed to physmap pcie configuration space for {}: {:?}", - address, error - ) - }) as *mut u32 - }); + let virt_pointer = unsafe { + // FIXME use byte_add once stable + (bus_addr.0 as *mut u8).add(Self::bus_addr_offset_in_bytes(address, 0)) as *mut u32 + }; f(Some(&mut *virt_pointer.offset( (offset as usize / mem::size_of::()) as isize, ))) @@ -222,8 +237,10 @@ impl CfgAccess for Pcie { impl Drop for Pcie { fn drop(&mut self) { - for address in self.maps.lock().unwrap().values().copied() { - let _ = unsafe { syscall::funmap(address as usize, PAGE_SIZE) }; + for &map in &self.bus_maps { + if let Some((ptr, size)) = map { + let _ = unsafe { syscall::funmap(ptr as usize, size) }; + } } } } From cf7fbacdaf6b89059f8e9421ef019e07cc5746d4 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jan 2024 19:53:39 +0100 Subject: [PATCH 7/7] Stop leaking Mcfg again We don't need it after Pcie::new anymore --- pcid/src/pcie/mod.rs | 55 ++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 32 deletions(-) diff --git a/pcid/src/pcie/mod.rs b/pcid/src/pcie/mod.rs index b8f4da6c0a..a5f41014ad 100644 --- a/pcid/src/pcie/mod.rs +++ b/pcid/src/pcie/mod.rs @@ -38,7 +38,7 @@ pub struct PcieAlloc { unsafe impl plain::Plain for PcieAlloc {} impl Mcfg { - fn fetch() -> io::Result<&'static Mcfg> { + fn with(f: impl FnOnce(&Mcfg) -> io::Result) -> io::Result { let table_dir = fs::read_dir("acpi:tables")?; for table_direntry in table_dir { @@ -53,24 +53,15 @@ impl Mcfg { let table_filename = table_path.file_name().unwrap().as_encoded_bytes(); if table_filename.get(0..4) == Some(&MCFG_NAME) { let bytes = fs::read(table_path)?.into_boxed_slice(); - if Mcfg::parse(&*bytes).is_none() { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "couldn't find mcfg table", - )); + match Mcfg::parse(&*bytes) { + Some(mcfg) => return f(mcfg), + None => { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "couldn't find mcfg table", + )); + } } - - // Leaking memory is fine as this function is only called once - // and we need this for the entire lifetime of pcid anyway. - let bytes = Box::leak(bytes); - - // This unwrap is fine as we checked that it will return Some above. - let mcfg = Mcfg::parse(bytes).unwrap(); - - // There should only be a single MCFG table and Linux ignores - // all MCFG tables beyond the first too, so doing an early - // return here is fine. - return Ok(mcfg); } } @@ -136,22 +127,22 @@ unsafe impl Sync for Pcie {} impl Pcie { pub fn new(fallback: Arc) -> io::Result { - let mcfg = Mcfg::fetch()?; + Mcfg::with(|mcfg| { + let alloc_maps = (0..=255) + .map(|bus| { + if let Some(alloc) = mcfg.at_bus(bus) { + Some(unsafe { Self::physmap_pcie_bus(alloc, bus) }) + } else { + None + } + }) + .collect(); - let alloc_maps = (0..=255) - .map(|bus| { - if let Some(alloc) = mcfg.at_bus(bus) { - Some(unsafe { Self::physmap_pcie_bus(alloc, bus) }) - } else { - None - } + Ok(Self { + lock: Mutex::new(()), + bus_maps: alloc_maps, + fallback, }) - .collect(); - - Ok(Self { - lock: Mutex::new(()), - bus_maps: alloc_maps, - fallback, }) }