On Thu, Dec 19, 2024 at 06:04:09PM +0100, Danilo Krummrich wrote:
> I/O memory is typically either mapped through direct calls to ioremap()
> or subsystem / bus specific ones such as pci_iomap().
>
> Even though subsystem / bus specific functions to map I/O memory are
> based on ioremap() / iounmap() it is not desirable to re-implement them
> in Rust.
>
> Instead, implement a base type for I/O mapped memory, which generically
> provides the corresponding accessors, such as `Io::readb` or
> `Io:try_readb`.
>
> `Io` supports an optional const generic, such that a driver can indicate
> the minimal expected and required size of the mapping at compile time.
> Correspondingly, calls to the 'non-try' accessors, support compile time
> checks of the I/O memory offset to read / write, while the 'try'
> accessors, provide boundary checks on runtime.
>
> `IoRaw` is meant to be embedded into a structure (e.g. pci::Bar or
> io::IoMem) which creates the actual I/O memory mapping and initializes
> `IoRaw` accordingly.
>
> To ensure that I/O mapped memory can't out-live the device it may be
> bound to, subsystems must embed the corresponding I/O memory type (e.g.
> pci::Bar) into a `Devres` container, such that it gets revoked once the
> device is unbound.
[...]
> +macro_rules! define_read {
> + ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
> + /// Read IO data from a given offset known at compile time.
> + ///
> + /// Bound checks are performed on compile time, hence if the offset is not known at compile
> + /// time, the build will fail.
> + $(#[$attr])*
> + #[inline]
> + pub fn $name(&self, offset: usize) -> $type_name {
> + let addr = self.io_addr_assert::<$type_name>(offset);
I'm rather new to Rust in the kernel but I've been playing
around with the nova-core stub (https://lore.kernel.org/
dri-devel/20250209173048.17398-1-dakr@xxxxxxxxxx/) a bit and the first thing I
tried to do was add a new register access. Of course when doing that I forgot to
update the BAR size definition:
const BAR0_SIZE: usize = 8;
pub(crate) type Bar0 = pci::Bar<BAR0_SIZE>;
That lead to this rather cryptic build error message:
RUSTC [M] drivers/gpu/nova-core/nova_core.o
drivers/gpu/nova-core/nova_core.o: warning: objtool: _RNvXNtCs3LxSlPxFOnk_9nova_core6driverNtB2_8NovaCoreNtNtCsgupvMsqqUAi_6kernel3pci6Driver5probe() falls through to next function _RNvXs_NtCs3LxSlPxFOnk_9nova_core3gpuNtB4_7ChipsetNtNtCs2wKxHFORdeQ_4core3fmt7Display3fmt()
MODPOST Module.symvers
ERROR: modpost: "rust_build_error" [drivers/gpu/nova-core/nova_core.ko] undefined!
Building it into the kernel instead of as a module provides some more hints:
vmlinux.o: warning: objtool: _RNvXNtCs3LxSlPxFOnk_9nova_core6driverNtB2_8NovaCoreNtNtCsgupvMsqqUAi_6kernel3pci6Driver5probe() falls through to next function _RNvXs_NtCs3LxSlPxFOnk_9nova_core3gpuNtB4_7ChipsetNtNtCs2wKxHFORdeQ_4core3fmt7Display3fmt()
OBJCOPY modules.builtin.modinfo
GEN modules.builtin
MODPOST Module.symvers
UPD include/generated/utsversion.h
CC init/version-timestamp.o
KSYMS .tmp_vmlinux0.kallsyms.S
AS .tmp_vmlinux0.kallsyms.o
LD .tmp_vmlinux1
/usr/bin/ld: vmlinux.o: in function `<kernel::io::Io<4>>::io_addr_assert::<u32>':
/data/source/linux/rust/kernel/build_assert.rs:78: undefined reference to `rust_build_error'
That was just enough to let me figure out what I'd done wrong based on the small
change I'd made. However the lack of reference to the actual offending line of
code that triggered the assert still made it more difficult than needed.
Is this a known issue or limitation? Or is this a bug/rough edge that still
needs fixing? Or alternatively am I just doing something wrong? Would appreciate
any insights as figuring out what I'd done wrong here was a bit of a rough
introduction!
- Alistair
> +
> + // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> + unsafe { bindings::$name(addr as _) }
> + }
> +
> + /// Read IO data from a given offset.
> + ///
> + /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
> + /// out of bounds.
> + $(#[$attr])*
> + pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
> + let addr = self.io_addr::<$type_name>(offset)?;
> +
> + // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> + Ok(unsafe { bindings::$name(addr as _) })
> + }
> + };
> +}
> +
> +macro_rules! define_write {
> + ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
> + /// Write IO data from a given offset known at compile time.
> + ///
> + /// Bound checks are performed on compile time, hence if the offset is not known at compile
> + /// time, the build will fail.
> + $(#[$attr])*
> + #[inline]
> + pub fn $name(&self, value: $type_name, offset: usize) {
> + let addr = self.io_addr_assert::<$type_name>(offset);
> +
> + // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> + unsafe { bindings::$name(value, addr as _, ) }
> + }
> +
> + /// Write IO data from a given offset.
> + ///
> + /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
> + /// out of bounds.
> + $(#[$attr])*
> + pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
> + let addr = self.io_addr::<$type_name>(offset)?;
> +
> + // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> + unsafe { bindings::$name(value, addr as _) }
> + Ok(())
> + }
> + };
> +}
> +
> +impl<const SIZE: usize> Io<SIZE> {
> + /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
> + /// `maxsize`.
> + pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self {
> + // SAFETY: `Io` is a transparent wrapper around `IoRaw`.
> + unsafe { &*core::ptr::from_ref(raw).cast() }
> + }
> +
> + /// Returns the base address of this mapping.
> + #[inline]
> + pub fn addr(&self) -> usize {
> + self.0.addr()
> + }
> +
> + /// Returns the maximum size of this mapping.
> + #[inline]
> + pub fn maxsize(&self) -> usize {
> + self.0.maxsize()
> + }
> +
> + #[inline]
> + const fn offset_valid<U>(offset: usize, size: usize) -> bool {
> + let type_size = core::mem::size_of::<U>();
> + if let Some(end) = offset.checked_add(type_size) {
> + end <= size && offset % type_size == 0
> + } else {
> + false
> + }
> + }
> +
> + #[inline]
> + fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> + if !Self::offset_valid::<U>(offset, self.maxsize()) {
> + return Err(EINVAL);
> + }
> +
> + // Probably no need to check, since the safety requirements of `Self::new` guarantee that
> + // this can't overflow.
> + self.addr().checked_add(offset).ok_or(EINVAL)
> + }
> +
> + #[inline]
> + fn io_addr_assert<U>(&self, offset: usize) -> usize {
> + build_assert!(Self::offset_valid::<U>(offset, SIZE));
> +
> + self.addr() + offset
> + }
> +
> + define_read!(readb, try_readb, u8);
> + define_read!(readw, try_readw, u16);
> + define_read!(readl, try_readl, u32);
> + define_read!(
> + #[cfg(CONFIG_64BIT)]
> + readq,
> + try_readq,
> + u64
> + );
> +
> + define_read!(readb_relaxed, try_readb_relaxed, u8);
> + define_read!(readw_relaxed, try_readw_relaxed, u16);
> + define_read!(readl_relaxed, try_readl_relaxed, u32);
> + define_read!(
> + #[cfg(CONFIG_64BIT)]
> + readq_relaxed,
> + try_readq_relaxed,
> + u64
> + );
> +
> + define_write!(writeb, try_writeb, u8);
> + define_write!(writew, try_writew, u16);
> + define_write!(writel, try_writel, u32);
> + define_write!(
> + #[cfg(CONFIG_64BIT)]
> + writeq,
> + try_writeq,
> + u64
> + );
> +
> + define_write!(writeb_relaxed, try_writeb_relaxed, u8);
> + define_write!(writew_relaxed, try_writew_relaxed, u16);
> + define_write!(writel_relaxed, try_writel_relaxed, u32);
> + define_write!(
> + #[cfg(CONFIG_64BIT)]
> + writeq_relaxed,
> + try_writeq_relaxed,
> + u64
> + );
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5702ce32ec8e..6c836ab73771 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -79,6 +79,7 @@
>
> #[doc(hidden)]
> pub use bindings;
> +pub mod io;
> pub use macros;
> pub use uapi;
>
> --
> 2.47.1
>
>
