From 381218446ec98344978d0a6301f93a657108ba9c Mon Sep 17 00:00:00 2001 From: Michael Kubacki Date: Thu, 6 Aug 2026 11:56:01 -0400 Subject: [PATCH 1/2] Update and integrate uart_16550 to 0.8 The 0.3.2 version of uart_16550 depends on the unmaintained x86 crate (not updated in ~4 years), which pulls in an old 1.x bitflags release that duplicates the version used everywhere else in Patina. This uart_16550 update was raised before in PR 1560, but the duplicate dependency has become enough of a problem to make the update now. Bumping to 0.8 lets us drop the bitflags skip entry in deny.toml and reduce the copies of bitflags in the dependency tree. In this crate update, the old `SerialPort` and `MmioSerialPort` types are replaced with a generic `Uart16550` driver. Construction is now fallible because it validates the address range and performs real hardware presence checks before applying the baud rate and format. Because the constructors are fallible, there is a tendency to `expect()` on the return value which panics if the address is bad or the probe fails. We generally don't want to panic because a UART probe check failed as that could boot a device. In reality, it will be obvious in most platforms if UART fails because there will be no output. `new_io()` and `new_mmio()` are `const fn` and do not fail. They just record basic information and defer everything else. The underlying driver is built lazily the first time it is actually needed. Presence probing and configuration happen through the explicit `SerialIO::init` call, and a failed probe there does not block reads or writes afterward. The goal is to make UART safe while reducing potential panics and allowing graceful degradation in cases where a UART might not behave as expected. Signed-off-by: Michael Kubacki --- Cargo.toml | 2 +- core/patina_debugger/README.md | 3 +- deny.toml | 7 +- docs/src/dev/principles/abstractions.md | 35 +-- sdk/patina/src/debug/log.rs | 14 +- .../src/peripheral/serial/uart/uart_16550.rs | 223 ++++++++++++++++-- supply-chain/config.toml | 2 +- 7 files changed, 233 insertions(+), 53 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f45f2f689..d89d1f62d 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" } 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]] From 599c67aea94dda30e91cf0905a1e6cda153a4fb6 Mon Sep 17 00:00:00 2001 From: Michael Kubacki Date: Mon, 17 Aug 2026 17:18:03 -0400 Subject: [PATCH 2/2] Cargo.toml: No longer allow multiple_crate_versions This was enabled until the uart_16550 crate could be updated so the crate dependency on bitflags v1 could be removed. Since that is now done, the lint no longer needs to be allowed. Signed-off-by: Michael Kubacki --- Cargo.toml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d89d1f62d..97e70dc01 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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"