diff --git a/Cargo.toml b/Cargo.toml index f45f2f689..97e70dc01 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ quote = { version = "1" } scroll = { version = "0.13", default-features = false, features = ["derive"]} spin = { version = "0.12" } syn = { version = "3" } -uart_16550 = { version = "0.3.2" } +uart_16550 = { version = "0.8" } corosensei = { version = "0.3.4", default-features = false } uuid = { version = "1.23", default-features = false } zerocopy = { version = "0.8" } @@ -90,10 +90,6 @@ suspicious = { level = "warn", priority = -1 } # `the "_support" suffix in the feature name "v1_resource_descriptor_support" is redundant` # This is allowed until the v1_resource_descriptor_support feature is dropped. redundant_feature_names = "allow" -# cargo::multiple_crate_versions violation: -# `multiple versions for dependency `bitflags`: 1.3.2, 2.13.1 ` -# This is allowed until the uart_16550 crate is updated to v0.6+ which removes the bitflags v1 dependency. -multiple_crate_versions = "allow" # pedantic::cast_possible_truncation - Temporarily allowed until all cases are updated (each fix needs a lot of # attention) to determine if the truncation is intentional and safe or not. cast_possible_truncation = "allow" diff --git a/core/patina_debugger/README.md b/core/patina_debugger/README.md index 9e5bff0eb..9af30b88f 100644 --- a/core/patina_debugger/README.md +++ b/core/patina_debugger/README.md @@ -44,7 +44,8 @@ The self-hosted debugger is lightweight and tightly integrated with Patina, offe ## Platform Integration -1. Instantiate a `PatinaDebugger` with the platform UART configuration (for example, `Uart16550::Io { base: 0x3F8 }`). +1. Instantiate a `PatinaDebugger` with the platform UART configuration (for example, + `unsafe { Uart16550::new_io(0x3F8) }`). 2. Apply any policy overrides such as `.with_force_enable`, `.with_log_policy`, or `.with_transport_init` when if the debugger must initialize the transport. 3. Register the debugger using `patina_debugger::set_debugger(&DEBUGGER)` before the Patina DXE Core starts dispatching diff --git a/deny.toml b/deny.toml index d1baba0b5..993fd6a4f 100644 --- a/deny.toml +++ b/deny.toml @@ -170,12 +170,7 @@ deny = [ #exact = true # Certain crates/versions that will be skipped when doing duplicate detection. -skip = [ - #"ansi_term@0.11.0", - #{ crate = "ansi_term@0.11.0", reason = "you can specify a reason why it can't be updated/removed" }, - - { crate = "bitflags", reason = "Need https://github.com/gz/rust-x86/pull/150 to be merged into rust-x86." } -] +skip = [] # Similarly to `skip` allows you to skip certain crates during duplicate # detection. Unlike skip, it also includes the entire tree of transitive # dependencies starting at the specified crate, up to a certain depth, which is diff --git a/docs/src/dev/principles/abstractions.md b/docs/src/dev/principles/abstractions.md index 16aff1e02..8f388528c 100644 --- a/docs/src/dev/principles/abstractions.md +++ b/docs/src/dev/principles/abstractions.md @@ -142,40 +142,47 @@ impl SerialIO for Terminal { } } -use uart_16550::MmioSerialPort; +use core::num::NonZero; +use uart_16550::backend::MmioBackend; +use uart_16550::{Config, Uart16550 as InnerUart16550}; struct Uart16550(usize); impl Uart16550 { fn new(addr: usize) -> Self { Self(addr) } + + /// Returns `None` if the address is rejected or the device can't be detected. + fn port(&self) -> Option> { + let addr = core::ptr::NonNull::::with_exposed_provenance(NonZero::new(self.0)?); + // SAFETY: `self.0` is assumed to be a valid, exclusively-owned MMIO address for this example. + unsafe { InnerUart16550::new_mmio(addr, 1) }.ok() + } } impl SerialIO for Uart16550 { fn init(&self) { - unsafe { MmioSerialPort::new(self.0).init() }; + if let Some(mut port) = self.port() { + let _ = port.init(Config::DEFAULT); + } } fn write(&self, buffer: &[u8]) { - let mut port = unsafe { MmioSerialPort::new(self.0) }; - - for b in buffer { - port.send(*b); + if let Some(mut port) = self.port() { + port.send_bytes_exact(buffer); } } fn read(&self) -> u8 { - let mut port = unsafe { MmioSerialPort::new(self.0) }; - port.receive() + let mut byte = 0u8; + if let Some(mut port) = self.port() { + port.receive_bytes_exact(core::slice::from_mut(&mut byte)); + } + byte } fn try_read(&self) -> Option { - let mut port = unsafe { MmioSerialPort::new(self.0) }; - if let Ok(value) = port.try_receive() { - Some(value) - } else { - None - } + self.port()?.try_receive_byte().ok() } } diff --git a/sdk/patina/src/debug/log.rs b/sdk/patina/src/debug/log.rs index ad47da876..162c67966 100644 --- a/sdk/patina/src/debug/log.rs +++ b/sdk/patina/src/debug/log.rs @@ -3,29 +3,31 @@ //! ## Examples //! //! ```rust ignore -//! use patina::debug::log::SerialLogger; -//! use patina::peripheral::serial::SerialIO; -//! use serial_writer::*; +//! use patina::debug::log::{Format, SerialLogger}; +//! use patina::peripheral::serial::Terminal; +//! use patina::peripheral::serial::uart::{Uart16550, UartPl011}; //! //! let terminal_logger = SerialLogger::new( //! Format::Standard, //! &[("crate1::module", log::LevelFilter::Off)], //! log::LevelFilter::Trace, -//! Terminal, +//! Terminal {}, //! ); //! +//! // SAFETY: 0x3F8 is the standard COM1 I/O port, exclusively owned for the logger's lifetime. //! let uart_16550_logger = SerialLogger::new( //! Format::Standard, //! &[("crate1::module", log::LevelFilter::Off)], //! log::LevelFilter::Trace, -//! Uart16550::new(Interface::Io(0x3F8)), +//! unsafe { Uart16550::new_io(0x3F8) }, //! ); //! +//! // SAFETY: 0x3F8_0000 is a valid, exclusively-owned PL011 MMIO base address on this platform. //! let uart_pl011_logger = SerialLogger::new( //! Format::Standard, //! &[("crate1::module", log::LevelFilter::Off)], //! log::LevelFilter::Trace, -//! UartPl011::new(0x3F8_0000), +//! unsafe { UartPl011::new(0x3F8_0000) }, //! ); //! ``` //! diff --git a/sdk/patina/src/peripheral/serial/uart/uart_16550.rs b/sdk/patina/src/peripheral/serial/uart/uart_16550.rs index 685797db2..5a5d8057e 100644 --- a/sdk/patina/src/peripheral/serial/uart/uart_16550.rs +++ b/sdk/patina/src/peripheral/serial/uart/uart_16550.rs @@ -7,20 +7,116 @@ //! SPDX-License-Identifier: Apache-2.0 //! -use uart_16550::MmioSerialPort; -use uart_16550::SerialPort as IoSerialPort; +use core::ptr::NonNull; + +use crate::log_debug_assert; +use uart_16550::backend::{MmioBackend, PioBackend}; +use uart_16550::{BaudRate, Config, Uart16550 as Inner}; + +/// The configuration applied the first time a `Uart16550` port is used. +const CONFIG: Config = Config { baud_rate: BaudRate::Baud38400, ..Config::DEFAULT }; + +/// A lazily-initialized I/O port-mapped backend. +/// +/// Construction only records the base port. The underlying driver is validated but not +/// hardware-probed on first use (see `driver`). +#[derive(Debug)] +pub struct IoPort { + base: u16, + inner: Option>, +} + +impl IoPort { + const fn new(base: u16) -> Self { + Self { base, inner: None } + } + + /// Returns the constructed driver, building it from the base port on the first call. + /// Returns `None` if the port is rejected (e.g. its register range overflows `u16`). The + /// attempt is retried on every call since there is not much overhead to retry. + /// + /// This only validates the address, it does not probe for hardware presence, so ports that + /// don't implement a full 16550 register file still work for raw reads/writes even though + /// `init` below can't confirm that they're present. + fn driver(&mut self) -> Option<&mut Inner> { + if self.inner.is_none() { + // SAFETY: Forwarded from `Uart16550::new_io`'s caller contract that `self.base` is a + // valid I/O port range, exclusively owned for the lifetime of this `Uart16550`. + self.inner = unsafe { Inner::new_port(self.base) }.ok(); + } + self.inner.as_mut() + } + + /// Applies [`CONFIG`] to the device. Some ports may not have a probeable identity, so a failed + /// presence probe here does not block later reads/writes, it just means the baud rate/format may + /// not match [`CONFIG`]. + fn init(&mut self) { + if let Some(driver) = self.driver() { + let _ = driver.init(CONFIG); + } + } +} + +/// A lazily-initialized Memory Mapped I/O backend. +/// +/// Construction only records the base address and register stride. The underlying driver is +/// validated (including rejecting a null base address) but not hardware-probed on first use (see +/// `driver`). +#[derive(Debug)] +pub struct MmioPort { + base: usize, + reg_stride: u8, + inner: Option>, +} + +impl MmioPort { + const fn new(base: usize, reg_stride: u8) -> Self { + Self { base, reg_stride, inner: None } + } + + /// Returns the constructed driver, building it from the base address/stride on the first + /// call. Returns `None` if the base address is null or the address/stride is otherwise + /// rejected. The attempt is retried on every call since there is not much overhead to retry. + /// + /// This only validates the address, it does not probe for hardware presence, so devices + /// that don't implement a full, probeable 16550 register file still work for raw reads/writes + /// even though `init` below can't confirm they're present. + fn driver(&mut self) -> Option<&mut Inner> { + if self.inner.is_none() { + let base = NonNull::new(core::ptr::with_exposed_provenance_mut::(self.base))?; + // SAFETY: Forwarded from `Uart16550::new_mmio`'s caller contract that `self.base` is a + // valid, exclusively-owned MMIO register range for the lifetime of this `Uart16550`. + self.inner = unsafe { Inner::new_mmio(base, self.reg_stride) }.ok(); + } + self.inner.as_mut() + } + + /// Applies [`CONFIG`] to the device. Some ports may not have a probeable identity, so a failed + /// presence probe here does not block later reads/writes, it just means the baud rate/format may + /// not match [`CONFIG`]. + fn init(&mut self) { + if let Some(driver) = self.driver() { + let _ = driver.init(CONFIG); + } + } +} /// An interface for writing to a Uart16550 device. /// /// Each variant owns the underlying serial port. The owning `&mut` access required by the /// [`SerialIO`](crate::peripheral::serial::SerialIO) methods guarantees exclusive use of the device, so no /// interior mutability or per-operation reconstruction is required. +/// +/// Only the address/stride are validated during construction so it is infallible. The underlying +/// driver is built lazily on first use, and hardware presence is only probed by an explicit call +/// to `SerialIO::init`. A rejected address or a failed presence check degrades to a no-op rather than panicking +/// or blocking boot. #[derive(Debug)] pub enum Uart16550 { /// The I/O port-mapped interface for the Uart16550 serial port. - Io(IoSerialPort), + Io(IoPort), /// The Memory Mapped I/O interface for the Uart16550 serial port. - Mmio(MmioSerialPort), + Mmio(MmioPort), } impl Uart16550 { @@ -29,22 +125,26 @@ impl Uart16550 { /// # Safety /// /// The caller must ensure `base` points to a valid Uart16550 I/O port range and that - /// the caller has the rights to perform the I/O operations on it. + /// the caller has the rights to perform the I/O operations on it, for as long as the + /// returned value is used. pub const unsafe fn new_io(base: u16) -> Self { - // SAFETY: The safety contract is forwarded to the caller of this function. - Uart16550::Io(unsafe { IoSerialPort::new(base) }) + Uart16550::Io(IoPort::new(base)) } /// Creates a new memory-mapped Uart16550 interface at the given base address and /// register stride. /// + /// This will not fail, even for a null or otherwise invalid address. The address and stride + /// are validated lazily on first use. Hardware presence is probed by an explicit call to + /// `SerialIO::init`. + /// /// # Safety /// /// The caller must ensure `base` points to a valid, exclusively-owned Uart16550 MMIO - /// register range with the given `reg_stride` between consecutive registers. - pub const unsafe fn new_mmio(base: usize, reg_stride: usize) -> Self { - // SAFETY: The safety contract is forwarded to the caller of this function. - Uart16550::Mmio(unsafe { MmioSerialPort::new_with_stride(base, reg_stride) }) + /// register range with the given `reg_stride` between consecutive registers, for as long as + /// the returned value is used. + pub const unsafe fn new_mmio(base: usize, reg_stride: u8) -> Self { + Uart16550::Mmio(MmioPort::new(base, reg_stride)) } } @@ -57,31 +157,51 @@ impl crate::peripheral::serial::SerialIO for Uart16550 { } fn write(&mut self, buffer: &[u8]) { + if buffer.is_empty() { + return; + } match self { Uart16550::Io(port) => { - for b in buffer { - port.send(*b); + if let Some(driver) = port.driver() { + driver.send_bytes_exact(buffer); } } Uart16550::Mmio(port) => { - for b in buffer { - port.send(*b); + if let Some(driver) = port.driver() { + driver.send_bytes_exact(buffer); } } } } fn read(&mut self) -> u8 { + // Blocks until a byte is available once the device is constructed. A rejected address + // logs an error and panics in debug builds. In release builds, a `0` sentinel is returned + // instead of spinning forever. + let mut byte = 0u8; match self { - Uart16550::Io(port) => port.receive(), - Uart16550::Mmio(port) => port.receive(), + Uart16550::Io(port) => { + if let Some(driver) = port.driver() { + driver.receive_bytes_exact(core::slice::from_mut(&mut byte)); + } else { + log_debug_assert!("Uart16550::read on a rejected I/O port. Returning a 0 sentinel byte"); + } + } + Uart16550::Mmio(port) => { + if let Some(driver) = port.driver() { + driver.receive_bytes_exact(core::slice::from_mut(&mut byte)); + } else { + log_debug_assert!("Uart16550::read on a rejected MMIO port. Returning a 0 sentinel byte"); + } + } } + byte } fn try_read(&mut self) -> Option { match self { - Uart16550::Io(port) => port.try_receive().ok(), - Uart16550::Mmio(port) => port.try_receive().ok(), + Uart16550::Io(port) => port.driver()?.try_receive_byte().ok(), + Uart16550::Mmio(port) => port.driver()?.try_receive_byte().ok(), } } } @@ -93,7 +213,8 @@ mod tests { use crate::peripheral::serial::SerialIO; struct FakeMmio { - regs: [u8; 6], + // The size of the full 8-register file (NUM_REGISTERS). + regs: [u8; 8], } impl FakeMmio { @@ -104,14 +225,22 @@ mod tests { const LINE_CTRL: usize = 3; const MODEM_CTRL: usize = 4; const LINE_STS: usize = 5; + const MODEM_STS: usize = 6; // Line status register bits. const INPUT_FULL: u8 = 1; const OUTPUT_EMPTY: u8 = 1 << 5; + const TRANSMITTER_EMPTY: u8 = 1 << 6; + + // Modem status register bits. + const CLEAR_TO_SEND: u8 = 1 << 4; fn new() -> Self { - let mut regs = [0u8; 6]; - regs[Self::LINE_STS] = Self::OUTPUT_EMPTY; + let mut regs = [0u8; 8]; + // init() spins on TRANSMITTER_EMPTY. MSR::CTS is set too, though CONFIG doesn't + // enable check_cts_before_sending, so ready_to_send() doesn't require it. + regs[Self::LINE_STS] = Self::OUTPUT_EMPTY | Self::TRANSMITTER_EMPTY; + regs[Self::MODEM_STS] = Self::CLEAR_TO_SEND; FakeMmio { regs } } @@ -122,7 +251,7 @@ mod tests { fn uart(&mut self) -> Uart16550 { let base = self.regs.as_mut_ptr() as usize; - // SAFETY: `base` points to a six-byte register file (stride 1) that outlives the + // SAFETY: `base` points to an eight-byte register file (stride 1) that outlives the // returned interface and is exclusively owned for the duration of the test. unsafe { Uart16550::new_mmio(base, 1) } } @@ -149,7 +278,8 @@ mod tests { // Values written by the 16550 default configuration (38400/8-N-1). assert_eq!(fake.regs[FakeMmio::DATA], 0x03); - assert_eq!(fake.regs[FakeMmio::INT_EN], 0x01); + // `Config::DEFAULT` does not enable interrupts, so `IER` stays clear. + assert_eq!(fake.regs[FakeMmio::INT_EN], 0x00); assert_eq!(fake.regs[FakeMmio::FIFO_CTRL], 0xC7); assert_eq!(fake.regs[FakeMmio::LINE_CTRL], 0x03); assert_eq!(fake.regs[FakeMmio::MODEM_CTRL], 0x0B); @@ -177,6 +307,17 @@ mod tests { assert_eq!(fake.regs[FakeMmio::DATA], 0); } + #[test] + fn test_uart_16550_mmio_write_works_without_explicit_init() { + let mut fake = FakeMmio::new(); + fake.uart().write(b"A"); + assert_eq!(fake.regs[FakeMmio::DATA], b'A'); + assert_eq!(fake.regs[FakeMmio::INT_EN], 0); + assert_eq!(fake.regs[FakeMmio::FIFO_CTRL], 0); + assert_eq!(fake.regs[FakeMmio::LINE_CTRL], 0); + assert_eq!(fake.regs[FakeMmio::MODEM_CTRL], 0); + } + #[test] fn test_uart_16550_mmio_read_returns_available_byte() { let mut fake = FakeMmio::new(); @@ -196,4 +337,38 @@ mod tests { fake.set_rx_byte(b'!'); assert_eq!(fake.uart().try_read(), Some(b'!')); } + + #[test] + fn test_uart_16550_mmio_null_base_never_panics() { + // SAFETY: Intentionally violates the constructor's contract (a null base address) to + // verify the driver no-ops instead of panicking. + let mut uart = unsafe { Uart16550::new_mmio(0, 1) }; + uart.init(); + uart.write(b"unreachable"); + assert_eq!(uart.try_read(), None); + assert_eq!(uart.read(), 0); + } + + #[test] + fn test_uart_16550_mmio_invalid_stride_never_panics() { + let mut fake = FakeMmio::new(); + let base = fake.regs.as_mut_ptr() as usize; + // SAFETY: Intentionally passes an invalid stride to verify the driver no-ops instead of + // panicking. + let mut uart = unsafe { Uart16550::new_mmio(base, 0) }; + uart.init(); + uart.write(b"unreachable"); + assert_eq!(uart.try_read(), None); + assert_eq!(uart.read(), 0); + } + + #[test] + fn test_uart_16550_io_invalid_base_never_panics() { + // SAFETY: `u16::MAX` overflows the device's register range. + let mut uart = unsafe { Uart16550::new_io(u16::MAX) }; + uart.init(); + uart.write(b"unreachable"); + assert_eq!(uart.try_read(), None); + assert_eq!(uart.read(), 0); + } } diff --git a/supply-chain/config.toml b/supply-chain/config.toml index 7ce7296ba..ed125c80a 100644 --- a/supply-chain/config.toml +++ b/supply-chain/config.toml @@ -537,7 +537,7 @@ version = "1.19.0" criteria = "safe-to-run" [[exemptions.uart_16550]] -version = "0.3.2" +version = "0.8.0" criteria = "safe-to-deploy" [[exemptions.uint]]