diff --git a/src/bare-metal/aps/better-uart/bitflags.md b/src/bare-metal/aps/better-uart/bitflags.md index 891cd2e0..ab54412b 100644 --- a/src/bare-metal/aps/better-uart/bitflags.md +++ b/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. diff --git a/src/bare-metal/aps/better-uart/driver.md b/src/bare-metal/aps/better-uart/driver.md index 1661f809..488be573 100644 --- a/src/bare-metal/aps/better-uart/driver.md +++ b/src/bare-metal/aps/better-uart/driver.md @@ -8,7 +8,16 @@ Now let's use the new `Registers` struct in our driver.
-- 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.
diff --git a/src/bare-metal/aps/better-uart/registers.md b/src/bare-metal/aps/better-uart/registers.md index 48f5e5b1..cf95b665 100644 --- a/src/bare-metal/aps/better-uart/registers.md +++ b/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. @@ -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. diff --git a/src/bare-metal/aps/examples/Cargo.lock b/src/bare-metal/aps/examples/Cargo.lock index 28ff5c31..3a6622d1 100644 --- a/src/bare-metal/aps/examples/Cargo.lock +++ b/src/bare-metal/aps/examples/Cargo.lock @@ -31,8 +31,10 @@ dependencies = [ "arm-pl011-uart", "bitflags", "log", + "safe-mmio", "smccc", "spin", + "zerocopy", ] [[package]] diff --git a/src/bare-metal/aps/examples/Cargo.toml b/src/bare-metal/aps/examples/Cargo.toml index 6c91f2ba..555a5ce0 100644 --- a/src/bare-metal/aps/examples/Cargo.toml +++ b/src/bare-metal/aps/examples/Cargo.toml @@ -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" diff --git a/src/bare-metal/aps/examples/src/logger.rs b/src/bare-metal/aps/examples/src/logger.rs index e27ee2b9..6e4c73ad 100644 --- a/src/bare-metal/aps/examples/src/logger.rs +++ b/src/bare-metal/aps/examples/src/logger.rs @@ -21,7 +21,7 @@ use spin::mutex::SpinMutex; static LOGGER: Logger = Logger { uart: SpinMutex::new(None) }; struct Logger { - uart: SpinMutex>, + uart: SpinMutex>>, } 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)?; diff --git a/src/bare-metal/aps/examples/src/main_improved.rs b/src/bare-metal/aps/examples/src/main_improved.rs index 0e262da0..bb266014 100644 --- a/src/bare-metal/aps/examples/src/main_improved.rs +++ b/src/bare-metal/aps/examples/src/main_improved.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 = + 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(); diff --git a/src/bare-metal/aps/examples/src/main_logger.rs b/src/bare-metal/aps/examples/src/main_logger.rs index b90e5668..d036bbdf 100644 --- a/src/bare-metal/aps/examples/src/main_logger.rs +++ b/src/bare-metal/aps/examples/src/main_logger.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 = + 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})"); diff --git a/src/bare-metal/aps/examples/src/pl011.rs b/src/bare-metal/aps/examples/src/pl011.rs index 2aef3621..29eabf7c 100644 --- a/src/bare-metal/aps/examples/src/pl011.rs +++ b/src/bare-metal/aps/examples/src/pl011.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,70 +67,64 @@ bitflags! { } // ANCHOR: Registers +use safe_mmio::fields::{ReadPure, ReadPureWrite, ReadWrite, WriteOnly}; + #[repr(C, align(4))] -struct Registers { - dr: u16, +pub struct Registers { + dr: ReadWrite, _reserved0: [u8; 2], - rsr: ReceiveStatus, + rsr: ReadPure, _reserved1: [u8; 19], - fr: Flags, + fr: ReadPure, _reserved2: [u8; 6], - ilpr: u8, + ilpr: ReadPureWrite, _reserved3: [u8; 3], - ibrd: u16, + ibrd: ReadPureWrite, _reserved4: [u8; 2], - fbrd: u8, + fbrd: ReadPureWrite, _reserved5: [u8; 3], - lcr_h: u8, + lcr_h: ReadPureWrite, _reserved6: [u8; 3], - cr: u16, + cr: ReadPureWrite, _reserved7: [u8; 3], - ifls: u8, + ifls: ReadPureWrite, _reserved8: [u8; 3], - imsc: u16, + imsc: ReadPureWrite, _reserved9: [u8; 2], - ris: u16, + ris: ReadPure, _reserved10: [u8; 2], - mis: u16, + mis: ReadPure, _reserved11: [u8; 2], - icr: u16, + icr: WriteOnly, _reserved12: [u8; 2], - dmacr: u8, + dmacr: ReadPureWrite, _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) {} @@ -133,27 +132,23 @@ impl Uart { /// Reads and returns a pending byte, or `None` if nothing has been /// received. - pub fn read_byte(&self) -> Option { + pub fn read_byte(&mut self) -> Option { 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); @@ -161,7 +156,3 @@ impl Write for Uart { Ok(()) } } - -// Safe because it just contains a pointer to device memory, which can be -// accessed from any context. -unsafe impl Send for Uart {} diff --git a/src/exercises/bare-metal/rtc/Cargo.lock b/src/exercises/bare-metal/rtc/Cargo.lock index f4f17e4e..26501fbe 100644 --- a/src/exercises/bare-metal/rtc/Cargo.lock +++ b/src/exercises/bare-metal/rtc/Cargo.lock @@ -157,8 +157,10 @@ dependencies = [ "bitflags", "chrono", "log", + "safe-mmio", "smccc", "spin", + "zerocopy", ] [[package]] diff --git a/src/exercises/bare-metal/rtc/Cargo.toml b/src/exercises/bare-metal/rtc/Cargo.toml index e8b794f6..afaff5f5 100644 --- a/src/exercises/bare-metal/rtc/Cargo.toml +++ b/src/exercises/bare-metal/rtc/Cargo.toml @@ -14,5 +14,7 @@ arm-pl011-uart = "0.3.1" bitflags = "2.9.1" chrono = { version = "0.4.41", default-features = false } log = "0.4.27" +safe-mmio = "0.2.5" smccc = "0.2.1" spin = "0.10.0" +zerocopy = "0.8.25" diff --git a/src/exercises/bare-metal/rtc/src/main.rs b/src/exercises/bare-metal/rtc/src/main.rs index bdf5595e..2d96b8dc 100644 --- a/src/exercises/bare-metal/rtc/src/main.rs +++ b/src/exercises/bare-metal/rtc/src/main.rs @@ -72,7 +72,8 @@ initial_pagetable!({ // ANCHOR_END: imports /// Base address of the PL031 RTC. -const PL031_BASE_ADDRESS: *mut u32 = 0x901_0000 as _; +const PL031_BASE_ADDRESS: NonNull = + NonNull::new(0x901_0000 as _).unwrap(); /// The IRQ used by the PL031 RTC. const PL031_IRQ: IntId = IntId::spi(2); @@ -96,7 +97,7 @@ fn main(x0: u64, x1: u64, x2: u64, x3: u64) -> ! { // SAFETY: `PL031_BASE_ADDRESS` is the base address of a PL031 device, and // nothing else accesses that address range. - let mut rtc = unsafe { Rtc::new(PL031_BASE_ADDRESS) }; + let mut rtc = unsafe { Rtc::new(UniqueMmioPointer::new(PL031_BASE_ADDRESS)) }; let timestamp = rtc.read(); let time = Utc.timestamp_opt(timestamp.into(), 0).unwrap(); info!("RTC: {time}"); diff --git a/src/exercises/bare-metal/rtc/src/pl031.rs b/src/exercises/bare-metal/rtc/src/pl031.rs index 842e7440..217bb88e 100644 --- a/src/exercises/bare-metal/rtc/src/pl031.rs +++ b/src/exercises/bare-metal/rtc/src/pl031.rs @@ -12,72 +12,63 @@ // See the License for the specific language governing permissions and // limitations under the License. +use safe_mmio::fields::{ReadPure, ReadPureWrite, WriteOnly}; +use safe_mmio::{UniqueMmioPointer, field, field_shared}; + // ANCHOR: solution #[repr(C, align(4))] -struct Registers { +pub struct Registers { /// Data register - dr: u32, + dr: ReadPure, /// Match register - mr: u32, + mr: ReadPureWrite, /// Load register - lr: u32, + lr: ReadPureWrite, /// Control register - cr: u8, + cr: ReadPureWrite, _reserved0: [u8; 3], /// Interrupt Mask Set or Clear register - imsc: u8, + imsc: ReadPureWrite, _reserved1: [u8; 3], /// Raw Interrupt Status - ris: u8, + ris: ReadPure, _reserved2: [u8; 3], /// Masked Interrupt Status - mis: u8, + mis: ReadPure, _reserved3: [u8; 3], /// Interrupt Clear Register - icr: u8, + icr: WriteOnly, _reserved4: [u8; 3], } /// Driver for a PL031 real-time clock. #[derive(Debug)] -pub struct Rtc { - registers: *mut Registers, +pub struct Rtc<'a> { + registers: UniqueMmioPointer<'a, Registers>, } -impl Rtc { - /// Constructs a new instance of the RTC driver for a PL031 device at the - /// given base address. - /// - /// # Safety - /// - /// The given base address must point to the MMIO control registers of a - /// PL031 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> Rtc<'a> { + /// Constructs a new instance of the RTC driver for a PL031 device with the + /// given set of registers. + pub fn new(registers: UniqueMmioPointer<'a, Registers>) -> Self { + Self { registers } } /// Reads the current RTC value. pub fn read(&self) -> u32 { - // SAFETY: We know that self.registers points to the control registers - // of a PL031 device which is appropriately mapped. - unsafe { (&raw const (*self.registers).dr).read_volatile() } + field_shared!(self.registers, dr).read() } /// Writes a match value. When the RTC value matches this then an interrupt /// will be generated (if it is enabled). pub fn set_match(&mut self, value: u32) { - // SAFETY: We know that self.registers points to the control registers - // of a PL031 device which is appropriately mapped. - unsafe { (&raw mut (*self.registers).mr).write_volatile(value) } + field!(self.registers, mr).write(value); } /// Returns whether the match register matches the RTC value, whether or not /// the interrupt is enabled. pub fn matched(&self) -> bool { - // SAFETY: We know that self.registers points to the control registers - // of a PL031 device which is appropriately mapped. - let ris = unsafe { (&raw const (*self.registers).ris).read_volatile() }; + let ris = field_shared!(self.registers, ris).read(); (ris & 0x01) != 0 } @@ -86,10 +77,8 @@ impl Rtc { /// This should be true if and only if `matched` returns true and the /// interrupt is masked. pub fn interrupt_pending(&self) -> bool { - // SAFETY: We know that self.registers points to the control registers - // of a PL031 device which is appropriately mapped. - let ris = unsafe { (&raw const (*self.registers).mis).read_volatile() }; - (ris & 0x01) != 0 + let mis = field_shared!(self.registers, mis).read(); + (mis & 0x01) != 0 } /// Sets or clears the interrupt mask. @@ -98,19 +87,11 @@ impl Rtc { /// interrupt is disabled. pub fn enable_interrupt(&mut self, mask: bool) { let imsc = if mask { 0x01 } else { 0x00 }; - // SAFETY: We know that self.registers points to the control registers - // of a PL031 device which is appropriately mapped. - unsafe { (&raw mut (*self.registers).imsc).write_volatile(imsc) } + field!(self.registers, imsc).write(imsc); } /// Clears a pending interrupt, if any. pub fn clear_interrupt(&mut self) { - // SAFETY: We know that self.registers points to the control registers - // of a PL031 device which is appropriately mapped. - unsafe { (&raw mut (*self.registers).icr).write_volatile(0x01) } + field!(self.registers, icr).write(0x01); } } - -// SAFETY: `Rtc` just contains a pointer to device memory, which can be -// accessed from any context. -unsafe impl Send for Rtc {} diff --git a/src/exercises/bare-metal/solutions-afternoon.md b/src/exercises/bare-metal/solutions-afternoon.md index 38f85d1c..fbb079b6 100644 --- a/src/exercises/bare-metal/solutions-afternoon.md +++ b/src/exercises/bare-metal/solutions-afternoon.md @@ -12,6 +12,6 @@ _main.rs_: _pl031.rs_: -```rust +```rust,compile_fail {{#include rtc/src/pl031.rs:solution}} ``` diff --git a/tests/src/slides/slide-exemptions.list.ts b/tests/src/slides/slide-exemptions.list.ts index 42758387..d8ac8366 100644 --- a/tests/src/slides/slide-exemptions.list.ts +++ b/tests/src/slides/slide-exemptions.list.ts @@ -18,7 +18,6 @@ export const size_exemptions = [ ]; export const playground_size_exemptions = [ - "bare-metal/aps/better-uart/driver.html", "bare-metal/microcontrollers/type-state.html", "concurrency/async-pitfalls/cancellation.html", "iterators/intoiterator.html",