From 23ab2f3ce4380f6aef595b808f04b48441e3884a Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 24 Mar 2026 18:01:48 +0100 Subject: [PATCH 1/2] tests: add api test Having such tests is useful. Without it, it could happen that `Backend` isn't exported anymore - which would still enable to create instances via the constructors but one can't create bindings anymore. This test therefore protects against that. The test succeeds if the test compiles. --- tests/api.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 tests/api.rs diff --git a/tests/api.rs b/tests/api.rs new file mode 100644 index 0000000..28007e9 --- /dev/null +++ b/tests/api.rs @@ -0,0 +1,24 @@ +use uart_16550::backend::{Backend, MmioBackend}; +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +use uart_16550::backend::PioBackend; +use uart_16550::Uart16550; + + +/// This ensures that all necessary helper types to create bindings are publicly +/// exported. +/// +/// This test succeeds if it compiles. +#[test] +fn test_public_api() { + fn consume(_device: Uart16550) { } + + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + { + // SAFETY: This is a synthetic example and the hardware is not accessed. + let device: Uart16550 = unsafe { Uart16550::new_port(0x3f8) }.unwrap(); + consume(device); + } + // SAFETY: This is a synthetic example and the hardware is not accessed. + let device: Uart16550 = unsafe { Uart16550::new_mmio(0x1000 as *mut _, 1) }.unwrap(); + consume(device); +} From 1934f2e3b45380600d3dcd5527238efe62262fe2 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 24 Mar 2026 18:23:00 +0100 Subject: [PATCH 2/2] lib: require NonNull for new_mmio() constructors Make the MMIO constructors take `NonNull` instead of `*mut u8`. This encodes the non-null invariant in the type system and removes the runtime null check, so `InvalidAddressError::Null` is no longer needed. The current strict pointer provenance APIs and understanding are still shaping. From my understanding, using ptr::with_exposed_provenance_mut() is the correct way to tell Rust: This pointer comes from outside the Rust abstract machine and uses the provenance of that - which is the correct thing for MMIO. This is a breaking change: callers must now pass `NonNull` to `new_mmio()` and `Uart16550Tty::new_mmio()`. --- CHANGELOG.md | 10 +++++++++ src/error.rs | 5 ----- src/lib.rs | 59 ++++++++++++++++++++++++++++++++++++---------------- src/tty.rs | 9 ++++++-- tests/api.rs | 12 ++++++----- 5 files changed, 65 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ec02ba..018c75d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,16 @@ - Documentation improvements - Internal safety fixes (there was no UB, just making the internal code more robust) +- `Uart16550::new_mmio()` and `Uart16550Ttty::new_mmio()` now accept a + `NonNull` instead of a `*mut u8`. The recommended way to construct the + MMIO address is to use: \ + ```rust + fn main() { + let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); + let mmio_address = NonNull::new(mmio_address).unwrap(); + let mut uart = unsafe { Uart16550::new_mmio(mmio_address, 4).unwrap() }; + } + ``` ## 0.5.0 - 2026-03-20 diff --git a/src/error.rs b/src/error.rs index 0f2abcf..f24abd0 100644 --- a/src/error.rs +++ b/src/error.rs @@ -16,8 +16,6 @@ use core::fmt::Display; /// [NUM_REGISTERS]: crate::spec::NUM_REGISTERS #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub enum InvalidAddressError { - /// The specified address is null. - Null, /// The given base address is invalid, e.g., it cannot accommodate /// [NUM_REGISTERS] - 1 consecutive addresses. /// @@ -33,9 +31,6 @@ pub enum InvalidAddressError { impl Display for InvalidAddressError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::Null => { - write!(f, "address is null") - } Self::InvalidBaseAddress(addr) => { write!(f, "invalid register address: {addr:x?}") } diff --git a/src/lib.rs b/src/lib.rs index 0f29a3d..5d28af3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,9 +77,13 @@ //! //! ```rust,no_run //! use uart_16550::{Config, Uart16550}; +//! use core::ptr::{self, NonNull}; +//! +//! let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); +//! let mmio_address = NonNull::new(mmio_address).unwrap(); //! //! // SAFETY: The address is valid and we have exclusive access. -//! let mut uart = unsafe { Uart16550::new_mmio(0x1000 as *mut _, 4).unwrap() }; +//! let mut uart = unsafe { Uart16550::new_mmio(mmio_address, 4).unwrap() }; //! uart.init(Config::default()).expect("should init device successfully"); //! uart.send_bytes_exact(b"hello world!"); //! ``` @@ -88,9 +92,13 @@ //! //! ```rust,no_run //! use uart_16550::{Config, Uart16550}; +//! use core::ptr::{self, NonNull}; +//! +//! let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); +//! let mmio_address = NonNull::new(mmio_address).unwrap(); //! //! // SAFETY: The address is valid and we have exclusive access. -//! let mut uart = unsafe { Uart16550::new_mmio(0x1000 as *mut _, 4).expect("should be valid port") }; +//! let mut uart = unsafe { Uart16550::new_mmio(mmio_address, 4).expect("should be valid port") }; //! // ^ or `new_port(0x3f8)` //! uart.init(Config::default()).expect("should init device successfully"); //! uart.test_loopback().expect("should have working loopback mode"); @@ -214,9 +222,13 @@ mod tty; /// /// ```rust,no_run /// use uart_16550::{Config, Uart16550}; +/// use core::ptr::{self, NonNull}; +/// +/// let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); +/// let mmio_address = NonNull::new(mmio_address).unwrap(); /// /// // SAFETY: The address is valid and we have exclusive access. -/// let mut uart = unsafe { Uart16550::new_mmio(0x1000 as *mut _, 4).unwrap() }; +/// let mut uart = unsafe { Uart16550::new_mmio(mmio_address, 4).unwrap() }; /// uart.init(Config::default()).expect("should init device successfully"); /// uart.send_bytes_exact(b"hello world!"); /// ``` @@ -225,9 +237,13 @@ mod tty; /// /// ```rust,no_run /// use uart_16550::{Config, Uart16550}; +/// use core::ptr::{self, NonNull}; +/// +/// let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); +/// let mmio_address = NonNull::new(mmio_address).unwrap(); /// /// // SAFETY: The address is valid and we have exclusive access. -/// let mut uart = unsafe { Uart16550::new_mmio(0x1000 as *mut _, 4).expect("should be valid port") }; +/// let mut uart = unsafe { Uart16550::new_mmio(mmio_address, 4).expect("should be valid port") }; /// // ^ or `new_port(0x3f8)` /// uart.init(Config::default()).expect("should init device successfully"); /// uart.test_loopback().expect("should have working loopback mode"); @@ -333,10 +349,9 @@ impl Uart16550 { /// the **whole lifetime** of the device. Further, all [`NUM_REGISTERS`] /// registers must be safely reachable from the base address. pub unsafe fn new_mmio( - base_address: *mut u8, + base_address: NonNull, stride: u8, ) -> Result> { - let base_address = NonNull::new(base_address).ok_or(InvalidAddressError::Null)?; let base_address = MmioAddress(base_address); if stride == 0 || !stride.is_power_of_two() { @@ -989,6 +1004,7 @@ impl ConfigRegisterDump { #[cfg(test)] mod tests { use super::*; + use core::ptr; #[test] fn constructors() { @@ -1004,34 +1020,37 @@ mod tests { // SAFETY: We just test the constructor but do not access the device. unsafe { - assert2::assert!(let Ok(_) = Uart16550::new_mmio(0x1000 as *mut _, 1)); - assert2::assert!(let Ok(_) = Uart16550::new_mmio(0x1000 as *mut _, 2)); - assert2::assert!(let Ok(_) = Uart16550::new_mmio(0x1000 as *mut _, 4)); - assert2::assert!(let Ok(_) = Uart16550::new_mmio(0x1000 as *mut _, 8)); + let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); + let mmio_address = NonNull::new(mmio_address).unwrap(); + + assert2::assert!(let Ok(_) = Uart16550::new_mmio(mmio_address, 1)); + assert2::assert!(let Ok(_) = Uart16550::new_mmio(mmio_address, 2)); + assert2::assert!(let Ok(_) = Uart16550::new_mmio(mmio_address, 4)); + assert2::assert!(let Ok(_) = Uart16550::new_mmio(mmio_address, 8)); assert2::assert!( let Err(InvalidAddressError::InvalidStride(0)) = - Uart16550::new_mmio(0x1000 as *mut _, 0) + Uart16550::new_mmio(mmio_address, 0) ); assert2::assert!( let Err(InvalidAddressError::InvalidStride(3)) = - Uart16550::new_mmio(0x1000 as *mut _, 3) + Uart16550::new_mmio(mmio_address, 3) ); assert2::assert!( let Err(InvalidAddressError::InvalidStride(5)) = - Uart16550::new_mmio(0x1000 as *mut _, 5) + Uart16550::new_mmio(mmio_address, 5) ); assert2::assert!( let Err(InvalidAddressError::InvalidStride(6)) = - Uart16550::new_mmio(0x1000 as *mut _, 6) + Uart16550::new_mmio(mmio_address, 6) ); assert2::assert!( let Err(InvalidAddressError::InvalidStride(7)) = - Uart16550::new_mmio(0x1000 as *mut _, 7) + Uart16550::new_mmio(mmio_address, 7) ); assert2::assert!( let Err(InvalidAddressError::InvalidStride(9)) = - Uart16550::new_mmio(0x1000 as *mut _, 9) + Uart16550::new_mmio(mmio_address, 9) ); } } @@ -1062,8 +1081,10 @@ mod tests { // Unblock init() memory[offsets::LSR] = LSR::TRANSMITTER_EMPTY.bits(); + let mmio_address = NonNull::new(memory.as_mut_ptr()).unwrap(); + // SAFETY: We are operating on valid memory. - let mut uart = unsafe { Uart16550::new_mmio(memory.as_mut_ptr(), STRIDE as u8) } + let mut uart = unsafe { Uart16550::new_mmio(mmio_address, STRIDE as u8) } .expect("should be able to create the dummy MMIO"); uart.init(config.clone()) @@ -1083,8 +1104,10 @@ mod tests { // Unblock init() memory[offsets::LSR * STRIDE] = LSR::TRANSMITTER_EMPTY.bits(); + let mmio_address = NonNull::new(memory.as_mut_ptr()).unwrap(); + // SAFETY: We are operating on valid memory. - let mut uart = unsafe { Uart16550::new_mmio(memory.as_mut_ptr(), STRIDE as u8) } + let mut uart = unsafe { Uart16550::new_mmio(mmio_address, STRIDE as u8) } .expect("should be able to create the dummy MMIO"); uart.init(config) diff --git a/src/tty.rs b/src/tty.rs index 056f0ca..f504f4d 100644 --- a/src/tty.rs +++ b/src/tty.rs @@ -17,6 +17,7 @@ use crate::backend::{PioBackend, PortIoAddress}; use crate::{Config, InitError, InvalidAddressError, LoopbackError, Uart16550}; use core::error::Error; use core::fmt::{self, Display, Formatter}; +use core::ptr::NonNull; /// Errors that [`Uart16550Tty::new_port()`] and [`Uart16550Tty::new_mmio()`] may /// return. @@ -88,9 +89,13 @@ impl Error for Uart16550TtyError { /// ```rust,no_run /// use uart_16550::{Config, Uart16550Tty}; /// use core::fmt::Write; +/// use core::ptr::{self, NonNull}; +/// +/// let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); +/// let mmio_address = NonNull::new(mmio_address).unwrap(); /// /// // SAFETY: The address is valid and we have exclusive access. -/// let mut uart = unsafe { Uart16550Tty::new_mmio(0x1000 as *mut _, 4, Config::default()).expect("should initialize device") }; +/// let mut uart = unsafe { Uart16550Tty::new_mmio(mmio_address, 4, Config::default()).expect("should initialize device") }; /// uart.write_str("hello world\nhow's it going?"); /// ``` /// @@ -164,7 +169,7 @@ impl Uart16550Tty { /// /// [`NUM_REGISTERS`]: crate::spec::NUM_REGISTERS pub unsafe fn new_mmio( - base_address: *mut u8, + base_address: NonNull, stride: u8, config: Config, ) -> Result> { diff --git a/tests/api.rs b/tests/api.rs index 28007e9..b198e35 100644 --- a/tests/api.rs +++ b/tests/api.rs @@ -1,8 +1,8 @@ -use uart_16550::backend::{Backend, MmioBackend}; +use core::ptr::{self, NonNull}; +use uart_16550::Uart16550; #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] use uart_16550::backend::PioBackend; -use uart_16550::Uart16550; - +use uart_16550::backend::{Backend, MmioBackend}; /// This ensures that all necessary helper types to create bindings are publicly /// exported. @@ -10,7 +10,7 @@ use uart_16550::Uart16550; /// This test succeeds if it compiles. #[test] fn test_public_api() { - fn consume(_device: Uart16550) { } + fn consume(_device: Uart16550) {} #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] { @@ -18,7 +18,9 @@ fn test_public_api() { let device: Uart16550 = unsafe { Uart16550::new_port(0x3f8) }.unwrap(); consume(device); } + let mmio_address = ptr::with_exposed_provenance_mut::(0x1000); + let mmio_address = NonNull::new(mmio_address).unwrap(); // SAFETY: This is a synthetic example and the hardware is not accessed. - let device: Uart16550 = unsafe { Uart16550::new_mmio(0x1000 as *mut _, 1) }.unwrap(); + let device: Uart16550 = unsafe { Uart16550::new_mmio(mmio_address, 1) }.unwrap(); consume(device); }