Use safe-mmio crate in PL011 UART driver example and RTC exercise solution (#2752)

Author: qwandor

src/bare-metal/aps/better-uart/bitflags.md

@@ -11,5 +11,7 @@ with bitflags.
 
 - The `bitflags!` macro creates a newtype something like `Flags(u16)`, along
   with a bunch of method implementations to get and set flags.
+- We need to derive `FromBytes` and `IntoBytes` for use with `safe-mmio`, which
+  we'll see on the next page.
 
 </details>

src/bare-metal/aps/better-uart/driver.md

@@ -8,7 +8,16 @@ Now let's use the new `Registers` struct in our driver.
 
 <details>
 
-- Note the use of `&raw const` / `&raw mut` to get pointers to individual fields
-  without creating an intermediate reference, which would be unsound.
+- `UniqueMmioPointer` is a wrapper around a raw pointer to an MMIO device or
+  register. The caller of `UniqueMmioPointer::new` promises that it is valid and
+  unique for the given lifetime, so it can provide safe methods to read and
+  write fields.
+- These MMIO accesses are generally a wrapper around `read_volatile` and
+  `write_volatile`, though on aarch64 they are instead implemented in assembly
+  to work around a bug where the compiler can emit instructions that prevent
+  MMIO virtualisation.
+- The `field!` and `field_shared!` macros internally use `&raw mut` and
+  `&raw const` to get pointers to individual fields without creating an
+  intermediate reference, which would be unsound.
 
 </details>

src/bare-metal/aps/better-uart/registers.md

@@ -1,6 +1,8 @@
 # Multiple registers
 
-We can use a struct to represent the memory layout of the UART's registers.
+We can use a struct to represent the memory layout of the UART's registers,
+using types from the `safe-mmio` crate to wrap ones which can be read or written
+safely.
 
 <!-- mdbook-xgettext: skip -->
 
@@ -15,5 +17,12 @@ We can use a struct to represent the memory layout of the UART's registers.
   rules as C. This is necessary for our struct to have a predictable layout, as
   default Rust representation allows the compiler to (among other things)
   reorder fields however it sees fit.
+- There are a number of different crates providing safe abstractions around MMIO
+  operations; we recommend the `safe-mmio` crate.
+- The difference between `ReadPure` or `ReadOnly` (and likewise between
+  `ReadPureWrite` and `ReadWrite`) is whether reading a register can have
+  side-effects which change the state of the device. E.g. reading the data
+  register pops a byte from the receive FIFO. `ReadPure` means that reads have
+  no side-effects, they are purely reading data.
 
 </details>

src/bare-metal/aps/examples/Cargo.lock

@@ -12,8 +12,10 @@ aarch64-rt = "0.1.3"
 arm-pl011-uart = "0.3.1"
 bitflags = "2.9.1"
 log = "0.4.27"
+safe-mmio = "0.2.5"
 smccc = "0.2.0"
 spin = "0.10.0"
+zerocopy = "0.8.25"
 
 [[bin]]
 name = "improved"

src/bare-metal/aps/examples/Cargo.toml

@@ -21,7 +21,7 @@ use spin::mutex::SpinMutex;
 static LOGGER: Logger = Logger { uart: SpinMutex::new(None) };
 
 struct Logger {
-    uart: SpinMutex<Option<Uart>>,
+    uart: SpinMutex<Option<Uart<'static>>>,
 }
 
 impl Log for Logger {
@@ -43,7 +43,10 @@ impl Log for Logger {
 }
 
 /// Initialises UART logger.
-pub fn init(uart: Uart, max_level: LevelFilter) -> Result<(), SetLoggerError> {
+pub fn init(
+    uart: Uart<'static>,
+    max_level: LevelFilter,
+) -> Result<(), SetLoggerError> {
     LOGGER.uart.lock().replace(uart);
 
     log::set_logger(&LOGGER)?;

src/bare-metal/aps/examples/src/logger.rs

@@ -22,19 +22,22 @@ mod pl011;
 use crate::pl011::Uart;
 use core::fmt::Write;
 use core::panic::PanicInfo;
+use core::ptr::NonNull;
 use log::error;
+use safe_mmio::UniqueMmioPointer;
 use smccc::Hvc;
 use smccc::psci::system_off;
 
 /// Base address of the primary PL011 UART.
-const PL011_BASE_ADDRESS: *mut u32 = 0x900_0000 as _;
+const PL011_BASE_ADDRESS: NonNull<pl011::Registers> =
+    NonNull::new(0x900_0000 as _).unwrap();
 
 // SAFETY: There is no other global function of this name.
 #[unsafe(no_mangle)]
 extern "C" fn main(x0: u64, x1: u64, x2: u64, x3: u64) {
     // SAFETY: `PL011_BASE_ADDRESS` is the base address of a PL011 device, and
     // nothing else accesses that address range.
-    let mut uart = unsafe { Uart::new(PL011_BASE_ADDRESS) };
+    let mut uart = unsafe { Uart::new(UniqueMmioPointer::new(PL011_BASE_ADDRESS)) };
 
     writeln!(uart, "main({x0:#x}, {x1:#x}, {x2:#x}, {x3:#x})").unwrap();
 

src/bare-metal/aps/examples/src/main_improved.rs

@@ -22,19 +22,22 @@ mod pl011;
 
 use crate::pl011::Uart;
 use core::panic::PanicInfo;
+use core::ptr::NonNull;
 use log::{LevelFilter, error, info};
+use safe_mmio::UniqueMmioPointer;
 use smccc::Hvc;
 use smccc::psci::system_off;
 
 /// Base address of the primary PL011 UART.
-const PL011_BASE_ADDRESS: *mut u32 = 0x900_0000 as _;
+const PL011_BASE_ADDRESS: NonNull<pl011::Registers> =
+    NonNull::new(0x900_0000 as _).unwrap();
 
 // SAFETY: There is no other global function of this name.
 #[unsafe(no_mangle)]
 extern "C" fn main(x0: u64, x1: u64, x2: u64, x3: u64) {
     // SAFETY: `PL011_BASE_ADDRESS` is the base address of a PL011 device, and
     // nothing else accesses that address range.
-    let uart = unsafe { Uart::new(PL011_BASE_ADDRESS) };
+    let uart = unsafe { Uart::new(UniqueMmioPointer::new(PL011_BASE_ADDRESS)) };
     logger::init(uart, LevelFilter::Trace).unwrap();
 
     info!("main({x0:#x}, {x1:#x}, {x2:#x}, {x3:#x})");

src/bare-metal/aps/examples/src/main_logger.rs

@@ -17,12 +17,15 @@ use core::fmt::{self, Write};
 
 // ANCHOR: Flags
 use bitflags::bitflags;
+use zerocopy::{FromBytes, IntoBytes};
+
+/// Flags from the UART flag register.
+#[repr(transparent)]
+#[derive(Copy, Clone, Debug, Eq, FromBytes, IntoBytes, PartialEq)]
+struct Flags(u16);
 
 bitflags! {
-    /// Flags from the UART flag register.
-    #[repr(transparent)]
-    #[derive(Copy, Clone, Debug, Eq, PartialEq)]
-    struct Flags: u16 {
+    impl Flags: u16 {
         /// Clear to send.
         const CTS = 1 << 0;
         /// Data set ready.
@@ -45,11 +48,13 @@ bitflags! {
 }
 // ANCHOR_END: Flags
 
+/// Flags from the UART Receive Status Register / Error Clear Register.
+#[repr(transparent)]
+#[derive(Copy, Clone, Debug, Eq, FromBytes, IntoBytes, PartialEq)]
+struct ReceiveStatus(u16);
+
 bitflags! {
-    /// Flags from the UART Receive Status Register / Error Clear Register.
-    #[repr(transparent)]
-    #[derive(Copy, Clone, Debug, Eq, PartialEq)]
-    struct ReceiveStatus: u16 {
+    impl ReceiveStatus: u16 {
         /// Framing error.
         const FE = 1 << 0;
         /// Parity error.
@@ -62,106 +67,92 @@ bitflags! {
 }
 
 // ANCHOR: Registers
+use safe_mmio::fields::{ReadPure, ReadPureWrite, ReadWrite, WriteOnly};
+
 #[repr(C, align(4))]
-struct Registers {
-    dr: u16,
+pub struct Registers {
+    dr: ReadWrite<u16>,
     _reserved0: [u8; 2],
-    rsr: ReceiveStatus,
+    rsr: ReadPure<ReceiveStatus>,
     _reserved1: [u8; 19],
-    fr: Flags,
+    fr: ReadPure<Flags>,
     _reserved2: [u8; 6],
-    ilpr: u8,
+    ilpr: ReadPureWrite<u8>,
     _reserved3: [u8; 3],
-    ibrd: u16,
+    ibrd: ReadPureWrite<u16>,
     _reserved4: [u8; 2],
-    fbrd: u8,
+    fbrd: ReadPureWrite<u8>,
     _reserved5: [u8; 3],
-    lcr_h: u8,
+    lcr_h: ReadPureWrite<u8>,
     _reserved6: [u8; 3],
-    cr: u16,
+    cr: ReadPureWrite<u16>,
     _reserved7: [u8; 3],
-    ifls: u8,
+    ifls: ReadPureWrite<u8>,
     _reserved8: [u8; 3],
-    imsc: u16,
+    imsc: ReadPureWrite<u16>,
     _reserved9: [u8; 2],
-    ris: u16,
+    ris: ReadPure<u16>,
     _reserved10: [u8; 2],
-    mis: u16,
+    mis: ReadPure<u16>,
     _reserved11: [u8; 2],
-    icr: u16,
+    icr: WriteOnly<u16>,
     _reserved12: [u8; 2],
-    dmacr: u8,
+    dmacr: ReadPureWrite<u8>,
     _reserved13: [u8; 3],
 }
 // ANCHOR_END: Registers
 
 // ANCHOR: Uart
+use safe_mmio::{UniqueMmioPointer, field, field_shared};
+
 /// Driver for a PL011 UART.
 #[derive(Debug)]
-pub struct Uart {
-    registers: *mut Registers,
+pub struct Uart<'a> {
+    registers: UniqueMmioPointer<'a, Registers>,
 }
 
-impl Uart {
-    /// Constructs a new instance of the UART driver for a PL011 device at the
-    /// given base address.
-    ///
-    /// # Safety
-    ///
-    /// The given base address must point to the 8 MMIO control registers of a
-    /// PL011 device, which must be mapped into the address space of the process
-    /// as device memory and not have any other aliases.
-    pub unsafe fn new(base_address: *mut u32) -> Self {
-        Self { registers: base_address as *mut Registers }
+impl<'a> Uart<'a> {
+    /// Constructs a new instance of the UART driver for a PL011 device with the
+    /// given set of registers.
+    pub fn new(registers: UniqueMmioPointer<'a, Registers>) -> Self {
+        Self { registers }
     }
 
     /// Writes a single byte to the UART.
-    pub fn write_byte(&self, byte: u8) {
+    pub fn write_byte(&mut self, byte: u8) {
         // Wait until there is room in the TX buffer.
         while self.read_flag_register().contains(Flags::TXFF) {}
 
-        // SAFETY: We know that self.registers points to the control registers
-        // of a PL011 device which is appropriately mapped.
-        unsafe {
-            // Write to the TX buffer.
-            (&raw mut (*self.registers).dr).write_volatile(byte.into());
-        }
+        // Write to the TX buffer.
+        field!(self.registers, dr).write(byte.into());
 
         // Wait until the UART is no longer busy.
         while self.read_flag_register().contains(Flags::BUSY) {}
     }
 
     /// Reads and returns a pending byte, or `None` if nothing has been
     /// received.
-    pub fn read_byte(&self) -> Option<u8> {
+    pub fn read_byte(&mut self) -> Option<u8> {
         if self.read_flag_register().contains(Flags::RXFE) {
             None
         } else {
-            // SAFETY: We know that self.registers points to the control
-            // registers of a PL011 device which is appropriately mapped.
-            let data = unsafe { (&raw const (*self.registers).dr).read_volatile() };
+            let data = field!(self.registers, dr).read();
             // TODO: Check for error conditions in bits 8-11.
             Some(data as u8)
         }
     }
 
     fn read_flag_register(&self) -> Flags {
-        // SAFETY: We know that self.registers points to the control registers
-        // of a PL011 device which is appropriately mapped.
-        unsafe { (&raw const (*self.registers).fr).read_volatile() }
+        field_shared!(self.registers, fr).read()
     }
 }
 // ANCHOR_END: Uart
 
-impl Write for Uart {
+impl Write for Uart<'_> {
     fn write_str(&mut self, s: &str) -> fmt::Result {
         for c in s.as_bytes() {
             self.write_byte(*c);
         }
         Ok(())
     }
 }
-
-// Safe because it just contains a pointer to device memory, which can be
-// accessed from any context.
-unsafe impl Send for Uart {}