From 545596f296f86680239f02441c09bb7faa80c00e Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 22 Jan 2024 17:54:38 +0100 Subject: [PATCH 1/3] Don't try to fetch BARs unless there is a matching driver Fetching BARs will temporarily invalidate them, which could cause crashes if a driver happens to access it at the same time. As we don't yet use a single pcid instance, we don't know which devices have a driver attached or not. As such approximate this as the set of devices for which the current pcid instance would load a driver. --- pcid/src/main.rs | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/pcid/src/main.rs b/pcid/src/main.rs index 5789d21263..de7bbe0508 100644 --- a/pcid/src/main.rs +++ b/pcid/src/main.rs @@ -207,9 +207,8 @@ pub struct State { } fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, header: PciHeader) { - let raw_class: u8 = header.class().into(); let mut string = format!("PCI {} {:>04X}:{:>04X} {:>02X}.{:>02X}.{:>02X}.{:>02X} {:?}", - addr, header.vendor_id(), header.device_id(), raw_class, + addr, header.vendor_id(), header.device_id(), header.class(), header.subclass(), header.interface(), header.revision(), header.class()); let device_type = DeviceType::from((header.class(), header.subclass())); match device_type { @@ -229,17 +228,6 @@ fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, he }, _ => (), } - - let bars = header.bars(&state.pcie); - for (i, bar) in bars.iter().enumerate() { - match bar { - PciBar::None => {}, - PciBar::Memory32{addr,..} => string.push_str(&format!(" {i}={addr:08X}")), - PciBar::Memory64{addr,..} => string.push_str(&format!(" {i}={addr:016X}")), - PciBar::Port(port) => string.push_str(&format!(" {i}=P{port:04X}")), - } - } - info!("{}", string); for driver in config.drivers.iter() { @@ -251,6 +239,21 @@ fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, he continue; }; + let mut string = String::new(); + let bars = header.bars(&state.pcie); + for (i, bar) in bars.iter().enumerate() { + match bar { + PciBar::None => {}, + PciBar::Memory32 { addr, .. } => string.push_str(&format!(" {i}={addr:08X}")), + PciBar::Memory64 { addr, .. } => string.push_str(&format!(" {i}={addr:016X}")), + PciBar::Port(port) => string.push_str(&format!(" {i}=P{port:04X}")), + } + } + + if !string.is_empty() { + info!(" BAR{}", string); + } + // Enable bus mastering, memory space, and I/O space unsafe { let mut data = state.pcie.read(addr, 0x04); From 623d12f2765b7d7f9fceb43c1f16d6d5509a5f0c Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 22 Jan 2024 18:03:46 +0100 Subject: [PATCH 2/3] Only try to load drivers for endpoint functions For bridge functions we don't have any existing drivers and we currently panic if we would try to load a driver for them. --- pcid/src/main.rs | 24 ++++++++++++++---- pcid/src/pci_header.rs | 55 ++++++++++++++++++------------------------ 2 files changed, 42 insertions(+), 37 deletions(-) diff --git a/pcid/src/main.rs b/pcid/src/main.rs index de7bbe0508..b4778dbbaf 100644 --- a/pcid/src/main.rs +++ b/pcid/src/main.rs @@ -16,7 +16,7 @@ use crate::config::Config; use crate::pci::{PciBar, PciFunc}; use crate::pci::cap::Capability as PciCapability; use crate::pci::func::{ConfigReader, ConfigWriter}; -use crate::pci_header::{PciHeader, PciHeaderError}; +use crate::pci_header::{PciEndpointHeader, PciHeader, PciHeaderError}; mod cfg_access; mod config; @@ -206,7 +206,7 @@ pub struct State { pcie: Pcie, } -fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, header: PciHeader) { +fn print_pci_function(addr: PciAddress, header: &PciHeader) { let mut string = format!("PCI {} {:>04X}:{:>04X} {:>02X}.{:>02X}.{:>02X}.{:>02X} {:?}", addr, header.vendor_id(), header.device_id(), header.class(), header.subclass(), header.interface(), header.revision(), header.class()); @@ -229,7 +229,9 @@ fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, he _ => (), } info!("{}", string); +} +fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, header: PciEndpointHeader) { for driver in config.drivers.iter() { if !driver.match_function(header.full_device_id()) { continue; @@ -470,9 +472,21 @@ fn main(args: Args) { let func_addr = PciAddress::new(0, bus_num, dev_num, func_num); match PciHeader::from_reader(&state.pcie, func_addr) { Ok(header) => { - handle_parsed_header(Arc::clone(&state), &config, func_addr, header); - if let PciHeader::PciToPci { secondary_bus_num, .. } = header { - bus_nums.push(secondary_bus_num); + print_pci_function(func_addr, &header); + match header { + PciHeader::General(endpoint_header) => { + handle_parsed_header( + Arc::clone(&state), + &config, + func_addr, + endpoint_header, + ); + } + PciHeader::PciToPci { + secondary_bus_num, .. + } => { + bus_nums.push(secondary_bus_num); + } } } Err(PciHeaderError::NoDevice) => { diff --git a/pcid/src/pci_header.rs b/pcid/src/pci_header.rs index 9905af380e..136042b387 100644 --- a/pcid/src/pci_header.rs +++ b/pcid/src/pci_header.rs @@ -21,14 +21,17 @@ pub struct SharedPciHeader { addr: PciAddress, } +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct PciEndpointHeader { + shared: SharedPciHeader, + subsystem_vendor_id: u16, + subsystem_id: u16, + cap_pointer: u8, +} + #[derive(Clone, Copy, Debug, PartialEq)] pub enum PciHeader { - General { - shared: SharedPciHeader, - subsystem_vendor_id: u16, - subsystem_id: u16, - cap_pointer: u8, - }, + General(PciEndpointHeader), PciToPci { shared: SharedPciHeader, secondary_bus_num: u8, @@ -73,12 +76,12 @@ impl PciHeader { let endpoint_header = EndpointHeader::from_header(header, access).unwrap(); let (subsystem_id, subsystem_vendor_id) = endpoint_header.subsystem(access); let cap_pointer = (unsafe { access.read(addr, 0x34) } & 0xff) as u8; - Ok(PciHeader::General { + Ok(PciHeader::General(PciEndpointHeader { shared, subsystem_vendor_id, subsystem_id, cap_pointer, - }) + })) } HeaderType::PciPciBridge => { let bridge_header = PciPciBridgeHeader::from_header(header, access).unwrap(); @@ -97,14 +100,14 @@ impl PciHeader { /// Return all identifying information of the PCI function. pub fn full_device_id(&self) -> &FullDeviceId { match self { - PciHeader::General { + PciHeader::General(PciEndpointHeader { shared: SharedPciHeader { full_device_id: device_id, .. }, .. - } + }) | PciHeader::PciToPci { shared: SharedPciHeader { @@ -145,17 +148,18 @@ impl PciHeader { pub fn class(&self) -> u8 { self.full_device_id().class } +} + +impl PciEndpointHeader { + pub fn full_device_id(&self) -> &FullDeviceId { + &self.shared.full_device_id + } /// Return the Headers BARs. // FIXME use pci_types::Bar instead pub fn bars(&self, access: &impl ConfigRegionAccess) -> [PciBar; 6] { - let endpoint_header = match *self { - PciHeader::General { - shared: SharedPciHeader { addr, .. }, - .. - } => EndpointHeader::from_header(TyPciHeader::new(addr), access).unwrap(), - PciHeader::PciToPci { .. } => unreachable!(), - }; + let endpoint_header = + EndpointHeader::from_header(TyPciHeader::new(self.shared.addr), access).unwrap(); let mut bars = [PciBar::None; 6]; let mut skip = false; @@ -196,24 +200,11 @@ impl PciHeader { } pub fn status(&self) -> u16 { - match self { - &PciHeader::General { - shared: SharedPciHeader { status, .. }, - .. - } - | &PciHeader::PciToPci { - shared: SharedPciHeader { status, .. }, - .. - } => status, - } + self.shared.status } pub fn cap_pointer(&self) -> u8 { - match self { - &PciHeader::General { cap_pointer, .. } | &PciHeader::PciToPci { cap_pointer, .. } => { - cap_pointer - } - } + self.cap_pointer } } From e97204756787efa9dfc6a113827712a84c59c41d Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 22 Jan 2024 18:12:37 +0100 Subject: [PATCH 3/3] Use EndpointHeader methods for the command and status registers --- pcid/src/main.rs | 16 +++++++++------- pcid/src/pci_header.rs | 18 +++++------------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/pcid/src/main.rs b/pcid/src/main.rs index b4778dbbaf..2858433a43 100644 --- a/pcid/src/main.rs +++ b/pcid/src/main.rs @@ -6,7 +6,7 @@ use std::thread; use std::sync::{Arc, Mutex}; use pci_types::device_type::DeviceType; -use pci_types::{ConfigRegionAccess, PciAddress}; +use pci_types::{CommandRegister, ConfigRegionAccess, PciAddress}; use structopt::StructOpt; use log::{debug, error, info, warn, trace}; use redox_log::{OutputBuilder, RedoxLogger}; @@ -256,12 +256,14 @@ fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, he info!(" BAR{}", string); } + let endpoint_header = header.endpoint_header(&state.pcie); + // Enable bus mastering, memory space, and I/O space - unsafe { - let mut data = state.pcie.read(addr, 0x04); - data |= 7; - state.pcie.write(addr, 0x04, data); - } + endpoint_header.update_command(&state.pcie, |cmd| { + cmd | CommandRegister::BUS_MASTER_ENABLE + | CommandRegister::MEMORY_ENABLE + | CommandRegister::IO_ENABLE + }); // Set IRQ line to 9 if not set let mut irq; @@ -278,7 +280,7 @@ fn handle_parsed_header(state: Arc, config: &Config, addr: PciAddress, he state.pcie.write(addr, 0x3C, data); }; - let capabilities = if header.status() & (1 << 4) != 0 { + let capabilities = if endpoint_header.status(&state.pcie).has_capability_list() { let func = PciFunc { pci: &state.pcie, addr diff --git a/pcid/src/pci_header.rs b/pcid/src/pci_header.rs index 136042b387..2b3a64e4c5 100644 --- a/pcid/src/pci_header.rs +++ b/pcid/src/pci_header.rs @@ -16,8 +16,6 @@ pub enum PciHeaderError { #[derive(Clone, Copy, Debug, PartialEq)] pub struct SharedPciHeader { full_device_id: FullDeviceId, - command: u16, - status: u16, addr: PciAddress, } @@ -52,9 +50,6 @@ impl PciHeader { let header = TyPciHeader::new(addr); let (vendor_id, device_id) = header.id(access); - let command_and_status = unsafe { access.read(addr, 4) }; - let command = (command_and_status & 0xffff) as u16; - let status = (command_and_status >> 16) as u16; let (revision, class, subclass, interface) = header.revision_and_class(access); let header_type = header.header_type(access); let shared = SharedPciHeader { @@ -66,8 +61,6 @@ impl PciHeader { interface, revision, }, - command, - status, addr, }; @@ -151,6 +144,10 @@ impl PciHeader { } impl PciEndpointHeader { + pub fn endpoint_header(&self, access: &impl ConfigRegionAccess) -> EndpointHeader { + EndpointHeader::from_header(TyPciHeader::new(self.shared.addr), access).unwrap() + } + pub fn full_device_id(&self) -> &FullDeviceId { &self.shared.full_device_id } @@ -158,8 +155,7 @@ impl PciEndpointHeader { /// Return the Headers BARs. // FIXME use pci_types::Bar instead pub fn bars(&self, access: &impl ConfigRegionAccess) -> [PciBar; 6] { - let endpoint_header = - EndpointHeader::from_header(TyPciHeader::new(self.shared.addr), access).unwrap(); + let endpoint_header = self.endpoint_header(access); let mut bars = [PciBar::None; 6]; let mut skip = false; @@ -199,10 +195,6 @@ impl PciEndpointHeader { bars } - pub fn status(&self) -> u16 { - self.shared.status - } - pub fn cap_pointer(&self) -> u8 { self.cap_pointer }