From 5c55c4bb168ddff12d773fefaa373cb47ba8cee3 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 15 Jun 2024 21:16:38 +0200 Subject: [PATCH] Use pci_allocate_interrupt_vector in nvmed --- audio/ihdad/src/main.rs | 6 +- net/rtl8139d/src/main.rs | 6 +- net/rtl8168d/src/main.rs | 6 +- pcid/src/driver_interface/irq_helpers.rs | 72 +++++++++++-- storage/nvmed/src/main.rs | 111 ++------------------ storage/nvmed/src/nvme/mod.rs | 123 ++++++----------------- 6 files changed, 110 insertions(+), 214 deletions(-) diff --git a/audio/ihdad/src/main.rs b/audio/ihdad/src/main.rs index c1f4cd681f..687b7745d5 100755 --- a/audio/ihdad/src/main.rs +++ b/audio/ihdad/src/main.rs @@ -76,7 +76,7 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { .unwrap(); event_queue .subscribe( - irq_file.as_raw_fd() as usize, + irq_file.irq_handle().as_raw_fd() as usize, Source::Irq, event::EventFlags::READ, ) @@ -93,12 +93,12 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { match event { Source::Irq => { let mut irq = [0; 8]; - irq_file.read(&mut irq).unwrap(); + irq_file.irq_handle().read(&mut irq).unwrap(); if !device.borrow_mut().irq() { continue; } - irq_file.write(&mut irq).unwrap(); + irq_file.irq_handle().write(&mut irq).unwrap(); readiness_based .poll_all_requests(|| device.borrow_mut()) diff --git a/net/rtl8139d/src/main.rs b/net/rtl8139d/src/main.rs index bdd150982e..2f3533978e 100644 --- a/net/rtl8139d/src/main.rs +++ b/net/rtl8139d/src/main.rs @@ -75,7 +75,7 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { let event_queue = EventQueue::::new().expect("rtl8139d: Could not create event queue."); event_queue .subscribe( - irq_file.as_raw_fd() as usize, + irq_file.irq_handle().as_raw_fd() as usize, Source::Irq, event::EventFlags::READ, ) @@ -96,10 +96,10 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { match event.user_data { Source::Irq => { let mut irq = [0; 8]; - irq_file.read(&mut irq).unwrap(); + irq_file.irq_handle().read(&mut irq).unwrap(); //TODO: This may be causing spurious interrupts if unsafe { scheme.adapter_mut().irq() } { - irq_file.write(&mut irq).unwrap(); + irq_file.irq_handle().write(&mut irq).unwrap(); scheme.tick().unwrap(); } diff --git a/net/rtl8168d/src/main.rs b/net/rtl8168d/src/main.rs index c477c0aaeb..f89c9d9825 100644 --- a/net/rtl8168d/src/main.rs +++ b/net/rtl8168d/src/main.rs @@ -75,7 +75,7 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { let event_queue = EventQueue::::new().expect("rtl8168d: Could not create event queue."); event_queue .subscribe( - irq_file.as_raw_fd() as usize, + irq_file.irq_handle().as_raw_fd() as usize, Source::Irq, event::EventFlags::READ, ) @@ -96,10 +96,10 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { match event.user_data { Source::Irq => { let mut irq = [0; 8]; - irq_file.read(&mut irq).unwrap(); + irq_file.irq_handle().read(&mut irq).unwrap(); //TODO: This may be causing spurious interrupts if unsafe { scheme.adapter_mut().irq() } { - irq_file.write(&mut irq).unwrap(); + irq_file.irq_handle().write(&mut irq).unwrap(); scheme.tick().unwrap(); } diff --git a/pcid/src/driver_interface/irq_helpers.rs b/pcid/src/driver_interface/irq_helpers.rs index 3db1c97ce2..7ed060bce0 100644 --- a/pcid/src/driver_interface/irq_helpers.rs +++ b/pcid/src/driver_interface/irq_helpers.rs @@ -8,7 +8,7 @@ use std::fs::{self, File}; use std::io::{self, prelude::*}; use std::num::NonZeroU8; -use crate::driver_interface::msi::MsiAddrAndData; +use crate::driver_interface::msi::{MsiAddrAndData, MsixTableEntry}; /// Read the local APIC ID of the bootstrap processor. pub fn read_bsp_apic_id() -> io::Result { @@ -228,24 +228,59 @@ pub fn allocate_first_msi_interrupt_on_bsp( interrupt_handle } +pub struct InterruptVector { + irq_handle: File, + vector: u16, + kind: InterruptVectorKind, +} + +enum InterruptVectorKind { + Legacy, + Msi, + MsiX { table_entry: *mut MsixTableEntry }, +} + +impl InterruptVector { + pub fn irq_handle(&self) -> &File { + &self.irq_handle + } + + pub fn vector(&self) -> u16 { + self.vector + } + + pub fn set_masked_if_fast(&mut self, masked: bool) -> bool { + match self.kind { + InterruptVectorKind::Legacy | InterruptVectorKind::Msi => false, + InterruptVectorKind::MsiX { table_entry } => { + unsafe { (*table_entry).set_masked(masked) }; + true + } + } + } +} + /// Get the most optimal supported interrupt mechanism: either (in the order of preference): -/// MSI-X, MSI, and INTx# pin. Returns the handles to the interrupts. +/// MSI-X, MSI, and INTx# pin. Returns both runtime interrupt structures (MSI/MSI-X capability +/// structures), and the handles to the interrupts. +// FIXME allow allocating multiple interrupt vectors +// FIXME move MSI-X IRQ allocation to pcid #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] pub fn pci_allocate_interrupt_vector( pcid_handle: &mut crate::driver_interface::PciFunctionHandle, driver: &str, -) -> File { +) -> InterruptVector { let features = pcid_handle.fetch_all_features(); let has_msi = features.iter().any(|feature| feature.is_msi()); let has_msix = features.iter().any(|feature| feature.is_msix()); if has_msix { - let capability_struct = match pcid_handle.feature_info(super::PciFeature::MsiX) { + let msix_info = match pcid_handle.feature_info(super::PciFeature::MsiX) { super::PciFeatureInfo::MsiX(msix) => msix, _ => unreachable!(), }; - let mut info = unsafe { capability_struct.map_and_mask_all(pcid_handle) }; + let mut info = unsafe { msix_info.map_and_mask_all(pcid_handle) }; pcid_handle.enable_feature(crate::driver_interface::PciFeature::MsiX); @@ -257,12 +292,24 @@ pub fn pci_allocate_interrupt_vector( entry.write_addr_and_data(msg_addr_and_data); entry.unmask(); - irq_handle + InterruptVector { + irq_handle, + vector: 0, + kind: InterruptVectorKind::MsiX { table_entry: entry }, + } } else if has_msi { - allocate_first_msi_interrupt_on_bsp(pcid_handle) + InterruptVector { + irq_handle: allocate_first_msi_interrupt_on_bsp(pcid_handle), + vector: 0, + kind: InterruptVectorKind::Msi, + } } else if let Some(irq) = pcid_handle.config().func.legacy_interrupt_line { // INTx# pin based interrupts. - irq.irq_handle(driver) + InterruptVector { + irq_handle: irq.irq_handle(driver), + vector: 0, + kind: InterruptVectorKind::Legacy, + } } else { panic!("{driver}: no interrupts supported at all") } @@ -270,13 +317,16 @@ pub fn pci_allocate_interrupt_vector( // FIXME support MSI on non-x86 systems #[cfg(not(any(target_arch = "x86", target_arch = "x86_64")))] -fn pci_allocate_interrupt_vector( +pub fn pci_allocate_interrupt_vector( pcid_handle: &mut crate::driver_interface::PciFunctionHandle, driver: &str, -) -> Vec { +) -> InterruptVector { if let Some(irq) = pcid_handle.config().func.legacy_interrupt_line { // INTx# pin based interrupts. - irq.irq_handle(driver) + InterruptVector { + irq_handle: irq.irq_handle(driver), + kind: InterruptVectorKind::Legacy, + } } else { panic!("{driver}: no interrupts supported at all") } diff --git a/storage/nvmed/src/main.rs b/storage/nvmed/src/main.rs index ed83b7809c..4437ddde3c 100644 --- a/storage/nvmed/src/main.rs +++ b/storage/nvmed/src/main.rs @@ -7,101 +7,14 @@ use std::sync::Arc; use std::usize; use driver_block::{Disk, DiskScheme}; -use pcid_interface::{PciFeature, PciFeatureInfo, PciFunction, PciFunctionHandle}; -use syscall::Result; +use pcid_interface::{irq_helpers, PciFunctionHandle}; use crate::nvme::NvmeNamespace; -use self::nvme::{InterruptMethod, InterruptSources, Nvme}; +use self::nvme::Nvme; mod nvme; -/// Get the most optimal yet functional interrupt mechanism: either (in the order of preference): -/// MSI-X, MSI, and INTx# pin. Returns both runtime interrupt structures (MSI/MSI-X capability -/// structures), and the handles to the interrupts. -#[cfg(target_arch = "x86_64")] -fn get_int_method( - pcid_handle: &mut PciFunctionHandle, - function: &PciFunction, -) -> Result<(InterruptMethod, InterruptSources)> { - log::trace!("Begin get_int_method"); - use pcid_interface::irq_helpers; - - let features = pcid_handle.fetch_all_features(); - - let has_msi = features.iter().any(|feature| feature.is_msi()); - let has_msix = features.iter().any(|feature| feature.is_msix()); - - // TODO: Allocate more than one vector when possible and useful. - if has_msix { - // Extended message signaled interrupts. - - let msix_info = match pcid_handle.feature_info(PciFeature::MsiX) { - PciFeatureInfo::MsiX(msix) => msix, - _ => unreachable!(), - }; - let mut info = unsafe { msix_info.map_and_mask_all(pcid_handle) }; - - pcid_handle.enable_feature(PciFeature::MsiX); - - let (msix_vector_number, irq_handle) = { - let entry = info.table_entry_pointer(0); - - let bsp_cpu_id = - irq_helpers::read_bsp_apic_id().expect("nvmed: failed to read APIC ID"); - let (msg_addr_and_data, irq_handle) = - irq_helpers::allocate_single_interrupt_vector_for_msi(bsp_cpu_id); - entry.write_addr_and_data(msg_addr_and_data); - entry.unmask(); - - (0, irq_handle) - }; - - let interrupt_method = InterruptMethod::MsiX(info); - let interrupt_sources = - InterruptSources::MsiX(std::iter::once((msix_vector_number, irq_handle)).collect()); - - log::trace!("Using MSI-X"); - Ok((interrupt_method, interrupt_sources)) - } else if has_msi { - // Message signaled interrupts. - - let msi_vector_number = 0; - let irq_handle = irq_helpers::allocate_first_msi_interrupt_on_bsp(pcid_handle); - - let interrupt_method = InterruptMethod::Msi; - let interrupt_sources = - InterruptSources::Msi(std::iter::once((msi_vector_number, irq_handle)).collect()); - - pcid_handle.enable_feature(PciFeature::Msi); - - log::trace!("Using MSI"); - Ok((interrupt_method, interrupt_sources)) - } else if let Some(irq) = function.legacy_interrupt_line { - // INTx# pin based interrupts. - let irq_handle = irq.irq_handle("nvmed"); - log::trace!("Using legacy interrupts"); - Ok((InterruptMethod::Intx, InterruptSources::Intx(irq_handle))) - } else { - panic!("nvmed: no interrupts supported at all") - } -} - -//TODO: MSI on non-x86_64? -#[cfg(not(target_arch = "x86_64"))] -fn get_int_method( - pcid_handle: &mut PciFunctionHandle, - function: &PciFunction, -) -> Result<(InterruptMethod, InterruptSources)> { - if let Some(irq) = function.legacy_interrupt_line { - // INTx# pin based interrupts. - let irq_handle = irq.irq_handle("nvmed"); - Ok((InterruptMethod::Intx, InterruptSources::Intx(irq_handle))) - } else { - panic!("nvmed: no interrupts supported at all") - } -} - struct NvmeDisk { nvme: Arc, ns: NvmeNamespace, @@ -164,26 +77,18 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { let address = unsafe { pcid_handle.map_bar(0).ptr }; - let (interrupt_method, interrupt_sources) = get_int_method(&mut pcid_handle, &pci_config.func) - .expect("nvmed: failed to find a suitable interrupt method"); - let mut nvme = Nvme::new(address.as_ptr() as usize, interrupt_method, pcid_handle) + let interrupt_vector = irq_helpers::pci_allocate_interrupt_vector(&mut pcid_handle, "nvmed"); + let iv = interrupt_vector.vector(); + let irq_handle = interrupt_vector.irq_handle().try_clone().unwrap(); + + let mut nvme = Nvme::new(address.as_ptr() as usize, interrupt_vector, pcid_handle) .expect("nvmed: failed to allocate driver data"); unsafe { nvme.init().expect("nvmed: failed to init") } log::debug!("Finished base initialization"); let nvme = Arc::new(nvme); - let executor = { - let (intx, (iv, irq_handle)) = match interrupt_sources { - InterruptSources::Msi(mut vectors) => ( - false, - vectors.pop_first().map(|(a, b)| (u16::from(a), b)).unwrap(), - ), - InterruptSources::MsiX(mut vectors) => (false, vectors.pop_first().unwrap()), - InterruptSources::Intx(file) => (true, (0, file)), - }; - nvme::executor::init(Arc::clone(&nvme), iv, intx, irq_handle) - }; + let executor = nvme::executor::init(Arc::clone(&nvme), iv, false /* FIXME */, irq_handle); let mut time_handle = File::open(&format!("/scheme/time/{}", libredox::flag::CLOCK_MONOTONIC)) .expect("failed to open time handle"); diff --git a/storage/nvmed/src/nvme/mod.rs b/storage/nvmed/src/nvme/mod.rs index e7050392ec..682ee93329 100644 --- a/storage/nvmed/src/nvme/mod.rs +++ b/storage/nvmed/src/nvme/mod.rs @@ -1,12 +1,12 @@ use std::cell::RefCell; use std::collections::{BTreeMap, HashMap}; use std::convert::TryFrom; -use std::fs::File; use std::iter; use std::sync::atomic::AtomicU16; use std::sync::Arc; use parking_lot::{Mutex, ReentrantMutex, RwLock}; +use pcid_interface::irq_helpers::InterruptVector; use common::io::{Io, Mmio}; use common::timeout::Timeout; @@ -22,28 +22,8 @@ pub mod queues; use self::executor::NvmeExecutor; pub use self::queues::{NvmeCmd, NvmeCmdQueue, NvmeComp, NvmeCompQueue}; -use pcid_interface::msi::MappedMsixRegs; use pcid_interface::PciFunctionHandle; -/// Used in conjunction with `InterruptMethod`, primarily by the CQ executor. -#[derive(Debug)] -pub enum InterruptSources { - MsiX(BTreeMap), - Msi(BTreeMap), - Intx(File), -} - -/// The way interrupts are sent. Unlike other PCI-based interfaces, like XHCI, it doesn't seem like -/// NVME supports operating with interrupts completely disabled. -pub enum InterruptMethod { - /// Traditional level-triggered, INTx# interrupt pins. - Intx, - /// Message signaled interrupts - Msi, - /// Extended message signaled interrupts - MsiX(MappedMsixRegs), -} - #[repr(C, packed)] pub struct NvmeRegs { /// Controller Capabilities @@ -93,7 +73,7 @@ pub type AtomicCmdId = AtomicU16; pub type Iv = u16; pub struct Nvme { - interrupt_method: Mutex, + interrupt_vector: Mutex, pcid_interface: Mutex, regs: RwLock<&'static mut NvmeRegs>, @@ -132,7 +112,7 @@ pub enum FullSqHandling { impl Nvme { pub fn new( address: usize, - interrupt_method: InterruptMethod, + interrupt_vector: InterruptVector, pcid_interface: PciFunctionHandle, ) -> Result { Ok(Nvme { @@ -156,7 +136,7 @@ impl Nvme { cq_ivs: RwLock::new(iter::once((0, 0)).collect()), sq_ivs: RwLock::new(iter::once((0, 0)).collect()), - interrupt_method: Mutex::new(interrupt_method), + interrupt_vector: Mutex::new(interrupt_vector), pcid_interface: Mutex::new(pcid_interface), // TODO @@ -222,14 +202,9 @@ impl Nvme { } } - match self.interrupt_method.get_mut() { - &mut InterruptMethod::Intx | InterruptMethod::Msi { .. } => { - self.regs.get_mut().intms.write(0xFFFF_FFFF); - self.regs.get_mut().intmc.write(0x0000_0001); - } - &mut InterruptMethod::MsiX(ref mut cfg) => { - cfg.table_entry_pointer(0).unmask(); - } + if !self.interrupt_vector.get_mut().set_masked_if_fast(false) { + self.regs.get_mut().intms.write(0xFFFF_FFFF); + self.regs.get_mut().intmc.write(0x0000_0001); } for (qid, iv) in self.cq_ivs.get_mut().iter_mut() { @@ -296,73 +271,39 @@ impl Nvme { Ok(()) } - /// Masks or unmasks multiple vectors. - /// - /// # Panics - /// Will panic if the same vector is called twice with different mask flags. - pub fn set_vectors_masked(&self, vectors: impl IntoIterator) { - let mut interrupt_method_guard = self.interrupt_method.lock(); + pub fn set_vector_masked(&self, vector: u16, masked: bool) { + let mut interrupt_vector_guard = (&self).interrupt_vector.lock(); - match &mut *interrupt_method_guard { - &mut InterruptMethod::Intx => { - let mut iter = vectors.into_iter(); - let (vector, mask) = match iter.next() { - Some(f) => f, - None => return, - }; - assert_eq!( - iter.next(), - None, - "nvmed: internal error: multiple vectors on INTx#" + if !interrupt_vector_guard.set_masked_if_fast(masked) { + let mut to_mask = 0x0000_0000; + let mut to_clear = 0x0000_0000; + + let vector = vector as u8; + + if masked { + assert_ne!( + to_clear & (1 << vector), + (1 << vector), + "nvmed: internal error: cannot both mask and set" ); - assert_eq!(vector, 0, "nvmed: internal error: nonzero vector on INTx#"); - if mask { - self.regs.write().intms.write(0x0000_0001); - } else { - self.regs.write().intmc.write(0x0000_0001); - } + to_mask |= 1 << vector; + } else { + assert_ne!( + to_mask & (1 << vector), + (1 << vector), + "nvmed: internal error: cannot both mask and set" + ); + to_clear |= 1 << vector; } - &mut InterruptMethod::Msi => { - let mut to_mask = 0x0000_0000; - let mut to_clear = 0x0000_0000; - for (vector, mask) in vectors { - let vector = vector as u8; - - if mask { - assert_ne!( - to_clear & (1 << vector), - (1 << vector), - "nvmed: internal error: cannot both mask and set" - ); - to_mask |= 1 << vector; - } else { - assert_ne!( - to_mask & (1 << vector), - (1 << vector), - "nvmed: internal error: cannot both mask and set" - ); - to_clear |= 1 << vector; - } - } - - if to_mask != 0 { - self.regs.write().intms.write(to_mask); - } - if to_clear != 0 { - self.regs.write().intmc.write(to_clear); - } + if to_mask != 0 { + (&self).regs.write().intms.write(to_mask); } - &mut InterruptMethod::MsiX(ref mut cfg) => { - for (vector, mask) in vectors { - cfg.table_entry_pointer(vector.into()).set_masked(mask); - } + if to_clear != 0 { + (&self).regs.write().intmc.write(to_clear); } } } - pub fn set_vector_masked(&self, vector: u16, masked: bool) { - self.set_vectors_masked(std::iter::once((vector, masked))) - } pub async fn submit_and_complete_command( &self,