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.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
rust/helpers/helpers.c | 1 +
rust/helpers/io.c | 101 ++++++++++++++++
rust/kernel/io.rs | 260 +++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
4 files changed, 363 insertions(+)
create mode 100644 rust/helpers/io.c
create mode 100644 rust/kernel/io.rs
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 060750af6524..63f9b1da179f 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -14,6 +14,7 @@
#include "cred.c"
#include "err.c"
#include "fs.c"
+#include "io.c"
#include "jump_label.c"
#include "kunit.c"
#include "mutex.c"
diff --git a/rust/helpers/io.c b/rust/helpers/io.c
new file mode 100644
index 000000000000..4c2401ccd720
--- /dev/null
+++ b/rust/helpers/io.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/io.h>
+
+void __iomem *rust_helper_ioremap(phys_addr_t offset, size_t size)
+{
+ return ioremap(offset, size);
+}
+
+void rust_helper_iounmap(volatile void __iomem *addr)
+{
+ iounmap(addr);
+}
+
+u8 rust_helper_readb(const volatile void __iomem *addr)
+{
+ return readb(addr);
+}
+
+u16 rust_helper_readw(const volatile void __iomem *addr)
+{
+ return readw(addr);
+}
+
+u32 rust_helper_readl(const volatile void __iomem *addr)
+{
+ return readl(addr);
+}
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq(const volatile void __iomem *addr)
+{
+ return readq(addr);
+}
+#endif
+
+void rust_helper_writeb(u8 value, volatile void __iomem *addr)
+{
+ writeb(value, addr);
+}
+
+void rust_helper_writew(u16 value, volatile void __iomem *addr)
+{
+ writew(value, addr);
+}
+
+void rust_helper_writel(u32 value, volatile void __iomem *addr)
+{
+ writel(value, addr);
+}
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq(u64 value, volatile void __iomem *addr)
+{
+ writeq(value, addr);
+}
+#endif
+
+u8 rust_helper_readb_relaxed(const volatile void __iomem *addr)
+{
+ return readb_relaxed(addr);
+}
+
+u16 rust_helper_readw_relaxed(const volatile void __iomem *addr)
+{
+ return readw_relaxed(addr);
+}
+
+u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
+{
+ return readl_relaxed(addr);
+}
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
+{
+ return readq_relaxed(addr);
+}
+#endif
+
+void rust_helper_writeb_relaxed(u8 value, volatile void __iomem *addr)
+{
+ writeb_relaxed(value, addr);
+}
+
+void rust_helper_writew_relaxed(u16 value, volatile void __iomem *addr)
+{
+ writew_relaxed(value, addr);
+}
+
+void rust_helper_writel_relaxed(u32 value, volatile void __iomem *addr)
+{
+ writel_relaxed(value, addr);
+}
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
+{
+ writeq_relaxed(value, addr);
+}
+#endif
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
new file mode 100644
index 000000000000..d4a73e52e3ee
--- /dev/null
+++ b/rust/kernel/io.rs
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Memory-mapped IO.
+//!
+//! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
+
+use crate::error::{code::EINVAL, Result};
+use crate::{bindings, build_assert};
+
+/// Raw representation of an MMIO region.
+///
+/// By itself, the existence of an instance of this structure does not provide any guarantees that
+/// the represented MMIO region does exist or is properly mapped.
+///
+/// Instead, the bus specific MMIO implementation must convert this raw representation into an `Io`
+/// instance providing the actual memory accessors. Only by the conversion into an `Io` structure
+/// any guarantees are given.
+pub struct IoRaw<const SIZE: usize = 0> {
+ addr: usize,
+ maxsize: usize,
+}
+
+impl<const SIZE: usize> IoRaw<SIZE> {
+ /// Returns a new `IoRaw` instance on success, an error otherwise.
+ pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
+ if maxsize < SIZE {
+ return Err(EINVAL);
+ }
+
+ Ok(Self { addr, maxsize })
+ }
+
+ /// Returns the base address of the MMIO region.
+ #[inline]
+ pub fn addr(&self) -> usize {
+ self.addr
+ }
+
+ /// Returns the maximum size of the MMIO region.
+ #[inline]
+ pub fn maxsize(&self) -> usize {
+ self.maxsize
+ }
+}
+
+/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
+///
+/// The creator (usually a subsystem / bus such as PCI) is responsible for creating the
+/// mapping, performing an additional region request etc.
+///
+/// # Invariant
+///
+/// `addr` is the start and `maxsize` the length of valid I/O mapped memory region of size
+/// `maxsize`.
+///
+/// # Examples
+///
+/// ```no_run
+/// # use kernel::{bindings, io::{Io, IoRaw}};
+/// # use core::ops::Deref;
+///
+/// // See also [`pci::Bar`] for a real example.
+/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
+///
+/// impl<const SIZE: usize> IoMem<SIZE> {
+/// /// # Safety
+/// ///
+/// /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
+/// /// virtual address space.
+/// unsafe fn new(paddr: usize) -> Result<Self>{
+/// // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
+/// // valid for `ioremap`.
+/// let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
+/// if addr.is_null() {
+/// return Err(ENOMEM);
+/// }
+///
+/// Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
+/// }
+/// }
+///
+/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
+/// fn drop(&mut self) {
+/// // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
+/// unsafe { bindings::iounmap(self.0.addr() as _); };
+/// }
+/// }
+///
+/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
+/// type Target = Io<SIZE>;
+///
+/// fn deref(&self) -> &Self::Target {
+/// // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
+/// unsafe { Io::from_raw(&self.0) }
+/// }
+/// }
+///
+///# fn no_run() -> Result<(), Error> {
+/// // SAFETY: Invalid usage for example purposes.
+/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
+/// iomem.writel(0x42, 0x0);
+/// assert!(iomem.try_writel(0x42, 0x0).is_ok());
+/// assert!(iomem.try_writel(0x42, 0x4).is_err());
+/// # Ok(())
+/// # }
+/// ```
+#[repr(transparent)]
+pub struct Io<const SIZE: usize = 0>(IoRaw<SIZE>);
+
+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);
+
+ // 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.1On 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@kernel.org/) 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
>
>
Hi Alistair,
On Fri, Feb 21, 2025 at 2:20 AM Alistair Popple <apopple@nvidia.com> wrote:
>
> 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!
Yeah, it is a result of our `build_assert!` machinery:
https://rust.docs.kernel.org/kernel/macro.build_assert.html
which works by producing a build (link) error rather than the usual
compiler error and thus the bad error message.
`build_assert!` is really the biggest hammer we have to assert
something is true at build time, since it may rely on the optimizer.
For instance, if `static_assert!` is usable in that context, it should
be instead of `build_assert!`.
Ideally we would have a way to get the message propagated somehow,
because it is indeed confusing.
I hope that helps.
Cheers,
Miguel
On Fri, Feb 21, 2025 at 04:58:59AM +0100, Miguel Ojeda wrote: > Hi Alistair, > > On Fri, Feb 21, 2025 at 2:20 AM Alistair Popple <apopple@nvidia.com> wrote: > > > > 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! > > Yeah, it is a result of our `build_assert!` machinery: > > https://rust.docs.kernel.org/kernel/macro.build_assert.html > > which works by producing a build (link) error rather than the usual > compiler error and thus the bad error message. > > `build_assert!` is really the biggest hammer we have to assert > something is true at build time, since it may rely on the optimizer. > For instance, if `static_assert!` is usable in that context, it should > be instead of `build_assert!`. > > Ideally we would have a way to get the message propagated somehow, > because it is indeed confusing. Are there any proposals or ideas for how we could do that? > I hope that helps. Kind of, but given the current state of build_assert's and the impossiblity of debugging them should we avoid adding them until they can be fixed? Unless the code absolutely cannot compile without them I think it would be better to turn them into runtime errors that can at least hint at what might have gone wrong. For example I think a run-time check would have been much more appropriate and easy to debug here, rather than having to bisect my changes. I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes, but testing with that also didn't yeild great results - it creates a backtrace but that doesn't seem to point anywhere terribly close to where the bad access was, I'm guessing maybe due to inlining and other optimisations - or is decode_stacktrace.sh not the right tool for this job? Thanks. - Alistair > Cheers, > Miguel
On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote:
> Kind of, but given the current state of build_assert's and the impossiblity of
> debugging them should we avoid adding them until they can be fixed?
If you build the module as built-in the linker gives you some more hints, but
I agree it's still not great.
I think build_assert() is not widely used yet and, until the situation improves,
we could also keep a list of common pitfalls if that helps?
> Unless the code absolutely cannot compile without them I think it would be
> better to turn them into runtime errors that can at least hint at what might
> have gone wrong. For example I think a run-time check would have been much more
> appropriate and easy to debug here, rather than having to bisect my changes.
No, especially for I/O the whole purpose of the non-try APIs is to ensure that
boundary checks happen at compile time.
> I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes,
> but testing with that also didn't yeild great results - it creates a backtrace
> but that doesn't seem to point anywhere terribly close to where the bad access
> was, I'm guessing maybe due to inlining and other optimisations - or is
> decode_stacktrace.sh not the right tool for this job?
I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will
make the kernel panic when hitting a build_assert().
I gave this a quick try with [1] in qemu and it lead to the following hint,
right before the oops:
[ 0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9:
Seeing this immediately tells me that I'm trying to do out of bound I/O accesses
in my driver, which indeed doesn't tell me the exact line (in case things are
inlined too much to gather it from the backtrace of the oops), but it should be
good enough, no?
[1]
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 1fb6e44f3395..2ff3af11d711 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -13,7 +13,7 @@ impl Regs {
const OFFSET: usize = 0x4;
const DATA: usize = 0x8;
const COUNT: usize = 0xC;
- const END: usize = 0x10;
+ const END: usize = 0x2;
}
type Bar0 = pci::Bar<{ Regs::END }>;
On Tue, Feb 25, 2025 at 12:04:35PM +0100, Danilo Krummrich wrote:
> On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote:
> > Kind of, but given the current state of build_assert's and the impossiblity of
> > debugging them should we avoid adding them until they can be fixed?
>
> If you build the module as built-in the linker gives you some more hints, but
> I agree it's still not great.
Yeah, that is how I eventually figured it out as a result of trying to resolve
the "undefined symbol" build error.
> I think build_assert() is not widely used yet and, until the situation improves,
> we could also keep a list of common pitfalls if that helps?
I've asked a few times, but are there any plans/ideas on how to improve the
situation? I'm kind of suprised we're building things on top of a fairly broken
feature without an idea of how we might make that feature work. I'd love to
help, but being new to R4L no immediately useful ideas come to mind.
At the very least if we could produce something more informative in the output
than the objtool "falls through to next function" warning and the undefined
reference that would help. I'm not sure if there is something we could do in the
build system to detect that and print a build-time hint along the lines of "hey,
you probably hit a build assert".
> > Unless the code absolutely cannot compile without them I think it would be
> > better to turn them into runtime errors that can at least hint at what might
> > have gone wrong. For example I think a run-time check would have been much more
> > appropriate and easy to debug here, rather than having to bisect my changes.
>
> No, especially for I/O the whole purpose of the non-try APIs is to ensure that
> boundary checks happen at compile time.
To be honest I don't really understand the utility here because the compile-time
check can't be a definitive check. You're always going to have to fallback to
a run-time check because at least for PCI (and likely others) you can't know
for at compile time if the IO region is big enough or matches the compile-time
constraint.
So this seems more like a quiz for developers to check if they really do want
to access the given offset. It's not really doing any useful compile-time bounds
check that is preventing something bad from happening, becasue that has to
happen at run-time. Especially as the whole BAR is mapped anyway.
Hence why I think an obvious run-time error instead of an obtuse and difficult
to figure out build error would be better. But maybe I'm missing some usecase
here that makes this more useful.
> > I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes,
> > but testing with that also didn't yeild great results - it creates a backtrace
> > but that doesn't seem to point anywhere terribly close to where the bad access
> > was, I'm guessing maybe due to inlining and other optimisations - or is
> > decode_stacktrace.sh not the right tool for this job?
>
> I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will
> make the kernel panic when hitting a build_assert().
>
> I gave this a quick try with [1] in qemu and it lead to the following hint,
> right before the oops:
>
> [ 0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9:
>
> Seeing this immediately tells me that I'm trying to do out of bound I/O accesses
> in my driver, which indeed doesn't tell me the exact line (in case things are
> inlined too much to gather it from the backtrace of the oops), but it should be
> good enough, no?
*smacks forehead*
Yes. So to answer this question:
> or is decode_stacktrace.sh not the right tool for this job?
No, it isn't. Just reading the kernel logs properly would have been a better
option! I guess coming from C I'm just too used to jumping straight to the stack
trace in the case of BUG_ON(), etc. Thanks for point that out.
> [1]
>
> diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> index 1fb6e44f3395..2ff3af11d711 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -13,7 +13,7 @@ impl Regs {
> const OFFSET: usize = 0x4;
> const DATA: usize = 0x8;
> const COUNT: usize = 0xC;
> - const END: usize = 0x10;
> + const END: usize = 0x2;
> }
>
> type Bar0 = pci::Bar<{ Regs::END }>;
On Thu, Feb 27, 2025 at 1:26 AM Alistair Popple <apopple@nvidia.com> wrote: > > I've asked a few times, but are there any plans/ideas on how to improve the > situation? I'm kind of suprised we're building things on top of a fairly broken > feature without an idea of how we might make that feature work. I'd love to > help, but being new to R4L no immediately useful ideas come to mind. It is not "broken" -- after all, it works as it was intended/designed when it was introduced, though it is definitely a hack and thus indeed the message could be improved greatly. :) As for how to improve it, e.g. Gary suggested the other day to use the DWARF information to locate the call site. I guess another way would be to generate different symbol names per call site, so that we can embed the path and line number into it (more or less), so that the user at least has a hint, though that may have disadvantages. Cheers, Miguel
On Thu, Feb 27, 2025 at 11:25:55AM +1100, Alistair Popple wrote: > On Tue, Feb 25, 2025 at 12:04:35PM +0100, Danilo Krummrich wrote: > > On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote: > > > I think build_assert() is not widely used yet and, until the situation improves, > > we could also keep a list of common pitfalls if that helps? > > I've asked a few times, but are there any plans/ideas on how to improve the > situation? I just proposed a few ones. If the limitation can be resolved I don't know. There are two different cases. (1) build_assert() is evaluated in const context to false (2) the compiler can't guarantee that build_assert() is evaluated to true at compile time For (1) you get a proper backtrace by the compiler. For (2) there's currently only the option to make the linker fail, which doesn't produce the most useful output. If we wouldn't do (2) we'd cause a kernel panic on runtime, which can be enforced with CONFIG_RUST_BUILD_ASSERT_ALLOW=y. > I'm kind of suprised we're building things on top of a fairly broken > feature without an idea of how we might make that feature work. I'd love to > help, but being new to R4L no immediately useful ideas come to mind. The feature is not broken at all, it works perfectly fine. It's just that for (2) it has an ergonomic limitation. > > > Unless the code absolutely cannot compile without them I think it would be > > > better to turn them into runtime errors that can at least hint at what might > > > have gone wrong. For example I think a run-time check would have been much more > > > appropriate and easy to debug here, rather than having to bisect my changes. > > > > No, especially for I/O the whole purpose of the non-try APIs is to ensure that > > boundary checks happen at compile time. > > To be honest I don't really understand the utility here because the compile-time > check can't be a definitive check. You're always going to have to fallback to > a run-time check because at least for PCI (and likely others) you can't know > for at compile time if the IO region is big enough or matches the compile-time > constraint. That's not true, let me explain. When you write a driver, you absolutely have to know the register layout. This means that you also know what the minimum PCI bar size has to be for your driver to work. If it would be smaller than what your driver expects, it can't function anyways. In Rust we make use of this fact. When you map a PCI bar through `pdev.iomap_region_sized` you pass in a const generic (`SIZE`) representing the *expected* PCI bar size. This can indeed fail on run-time, but that's fine, as mentioned, if the bar is smaller than what your driver expect, it's useless anyways. If the call succeeds, it means that the actual PCI bar size is greater or equal to `SIZE`. Since `SIZE` is known at compile time all subsequent I/O operations can be boundary checked against `SIZE` at compile time, which additionally makes the call infallible. This works for most I/O operations drivers do. However, sometimes we need to do I/O ops at a PCI bar offset that is only known at run-time. In this case you can use the `try_*` variants, such as `try_read32()`. Those do boundary checks against the actual size of the PCI bar, which is only known at run-time and hence they're fallible. > > So this seems more like a quiz for developers to check if they really do want > to access the given offset. It's not really doing any useful compile-time bounds > check that is preventing something bad from happening, becasue that has to > happen at run-time. Especially as the whole BAR is mapped anyway. See the explanation above. > > Hence why I think an obvious run-time error instead of an obtuse and difficult > to figure out build error would be better. But maybe I'm missing some usecase > here that makes this more useful. No, failing the boundary check at compile time (if possible) is always better than failing it at run-time for obvious reasons. > > > > I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes, > > > but testing with that also didn't yeild great results - it creates a backtrace > > > but that doesn't seem to point anywhere terribly close to where the bad access > > > was, I'm guessing maybe due to inlining and other optimisations - or is > > > decode_stacktrace.sh not the right tool for this job? > > > > I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will > > make the kernel panic when hitting a build_assert(). > > > > I gave this a quick try with [1] in qemu and it lead to the following hint, > > right before the oops: > > > > [ 0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9: > > > > Seeing this immediately tells me that I'm trying to do out of bound I/O accesses > > in my driver, which indeed doesn't tell me the exact line (in case things are > > inlined too much to gather it from the backtrace of the oops), but it should be > > good enough, no? > > *smacks forehead* > > Yes. So to answer this question: > > > or is decode_stacktrace.sh not the right tool for this job? > > No, it isn't. Just reading the kernel logs properly would have been a better > option! I guess coming from C I'm just too used to jumping straight to the stack > trace in the case of BUG_ON(), etc. Thanks for point that out. Happy I could help.
On Thu, Feb 27, 2025 at 11:01:55AM +0100, Danilo Krummrich wrote: > On Thu, Feb 27, 2025 at 11:25:55AM +1100, Alistair Popple wrote: > > On Tue, Feb 25, 2025 at 12:04:35PM +0100, Danilo Krummrich wrote: > > > On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote: > > > > > I think build_assert() is not widely used yet and, until the situation improves, > > > we could also keep a list of common pitfalls if that helps? > > > > I've asked a few times, but are there any plans/ideas on how to improve the > > situation? > > I just proposed a few ones. If the limitation can be resolved I don't know. Thanks. The limitation is what I was asking about. The work around for (2) works, but is not terribly discoverable. I will see if I can come up with something better to help with discoverability that at least. > There are two different cases. > > (1) build_assert() is evaluated in const context to false > (2) the compiler can't guarantee that build_assert() is evaluated to > true at compile time > > For (1) you get a proper backtrace by the compiler. For (2) there's currently > only the option to make the linker fail, which doesn't produce the most useful > output. > > If we wouldn't do (2) we'd cause a kernel panic on runtime, which can be > enforced with CONFIG_RUST_BUILD_ASSERT_ALLOW=y. > > > I'm kind of suprised we're building things on top of a fairly broken > > feature without an idea of how we might make that feature work. I'd love to > > help, but being new to R4L no immediately useful ideas come to mind. > > The feature is not broken at all, it works perfectly fine. It's just that for > (2) it has an ergonomic limitation. I'm not sure I agree it works perfectly fine. Developer ergonomics are an important aspect of any build environment, and I'd argue the ergonomic limitation for (2) means it is at least somewhat broken and needs fixing. > > > > Unless the code absolutely cannot compile without them I think it would be > > > > better to turn them into runtime errors that can at least hint at what might > > > > have gone wrong. For example I think a run-time check would have been much more > > > > appropriate and easy to debug here, rather than having to bisect my changes. > > > > > > No, especially for I/O the whole purpose of the non-try APIs is to ensure that > > > boundary checks happen at compile time. > > > > To be honest I don't really understand the utility here because the compile-time > > check can't be a definitive check. You're always going to have to fallback to > > a run-time check because at least for PCI (and likely others) you can't know > > for at compile time if the IO region is big enough or matches the compile-time > > constraint. > > That's not true, let me explain. > > When you write a driver, you absolutely have to know the register layout. This > means that you also know what the minimum PCI bar size has to be for your driver > to work. If it would be smaller than what your driver expects, it can't function > anyways. In Rust we make use of this fact. > > When you map a PCI bar through `pdev.iomap_region_sized` you pass in a const > generic (`SIZE`) representing the *expected* PCI bar size. This can indeed fail > on run-time, but that's fine, as mentioned, if the bar is smaller than what your > driver expect, it's useless anyways. > > If the call succeeds, it means that the actual PCI bar size is greater or equal > to `SIZE`. Since `SIZE` is known at compile time all subsequent I/O operations > can be boundary checked against `SIZE` at compile time, which additionally makes > the call infallible. This works for most I/O operations drivers do. Argh! That's the piece I was missing - that this makes the IO call infallible and thus removes the need to write run-time error handling code. Sadly of course that's not actually true, because I/O operations can always fail for reasons other than what can be checked at compile time (eg. in particular PCI devices can fall off the bus and return all 0xF's). But I guess existing drivers don't really handle those cases either. Anyway thanks for your time and detailed explainations, I really just started this thread as I think reducing friction for existing kernel developers to start looking at Rust in the kernel is important. So I wanted to highlight that the build_assert as linker error is really confusing when coming from C, and that it's an area that I think needs to improve to make Rust more successful in the kernel. Sadly I don't have any immediately ideas but if I do I will post them. > However, sometimes we need to do I/O ops at a PCI bar offset that is only known > at run-time. In this case you can use the `try_*` variants, such as > `try_read32()`. Those do boundary checks against the actual size of the PCI bar, > which is only known at run-time and hence they're fallible. > > > > > So this seems more like a quiz for developers to check if they really do want > > to access the given offset. It's not really doing any useful compile-time bounds > > check that is preventing something bad from happening, becasue that has to > > happen at run-time. Especially as the whole BAR is mapped anyway. > > See the explanation above. > > > > > Hence why I think an obvious run-time error instead of an obtuse and difficult > > to figure out build error would be better. But maybe I'm missing some usecase > > here that makes this more useful. > > No, failing the boundary check at compile time (if possible) is always better > than failing it at run-time for obvious reasons. > > > > > > > I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes, > > > > but testing with that also didn't yeild great results - it creates a backtrace > > > > but that doesn't seem to point anywhere terribly close to where the bad access > > > > was, I'm guessing maybe due to inlining and other optimisations - or is > > > > decode_stacktrace.sh not the right tool for this job? > > > > > > I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will > > > make the kernel panic when hitting a build_assert(). > > > > > > I gave this a quick try with [1] in qemu and it lead to the following hint, > > > right before the oops: > > > > > > [ 0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9: > > > > > > Seeing this immediately tells me that I'm trying to do out of bound I/O accesses > > > in my driver, which indeed doesn't tell me the exact line (in case things are > > > inlined too much to gather it from the backtrace of the oops), but it should be > > > good enough, no? > > > > *smacks forehead* > > > > Yes. So to answer this question: > > > > > or is decode_stacktrace.sh not the right tool for this job? > > > > No, it isn't. Just reading the kernel logs properly would have been a better > > option! I guess coming from C I'm just too used to jumping straight to the stack > > trace in the case of BUG_ON(), etc. Thanks for point that out. > > Happy I could help.
On Fri, Feb 28, 2025 at 6:29 AM Alistair Popple <apopple@nvidia.com> wrote: > > I'm not sure I agree it works perfectly fine. Developer ergonomics are > an important aspect of any build environment, and I'd argue the ergonomic > limitation for (2) means it is at least somewhat broken and needs fixing. > > Anyway thanks for your time and detailed explainations, I really just started > this thread as I think reducing friction for existing kernel developers to start > looking at Rust in the kernel is important. +1, it is indeed very, very important. But, just to clarify, we have been caring about ergonomics and reducing friction for kernel developers since the very beginning, including asking upstream Rust for features and so on when applicable. In general, it has been a factor in most topics we have discussed in the team since 2020, not just for source code or debugging features, but also docs, KUnit, and so on. That is why we would like to improve it and why we have it in our upstream Rust wishlist and so on. In other words, it is not that we do not see the issue! I hope that clarifies. Cheers, Miguel
On Fri, Feb 28, 2025 at 04:29:04PM +1100, Alistair Popple wrote: > On Thu, Feb 27, 2025 at 11:01:55AM +0100, Danilo Krummrich wrote: > > On Thu, Feb 27, 2025 at 11:25:55AM +1100, Alistair Popple wrote: > > > > To be honest I don't really understand the utility here because the compile-time > > > check can't be a definitive check. You're always going to have to fallback to > > > a run-time check because at least for PCI (and likely others) you can't know > > > for at compile time if the IO region is big enough or matches the compile-time > > > constraint. > > > > That's not true, let me explain. > > > > When you write a driver, you absolutely have to know the register layout. This > > means that you also know what the minimum PCI bar size has to be for your driver > > to work. If it would be smaller than what your driver expects, it can't function > > anyways. In Rust we make use of this fact. > > > > When you map a PCI bar through `pdev.iomap_region_sized` you pass in a const > > generic (`SIZE`) representing the *expected* PCI bar size. This can indeed fail > > on run-time, but that's fine, as mentioned, if the bar is smaller than what your > > driver expect, it's useless anyways. > > > > If the call succeeds, it means that the actual PCI bar size is greater or equal > > to `SIZE`. Since `SIZE` is known at compile time all subsequent I/O operations > > can be boundary checked against `SIZE` at compile time, which additionally makes > > the call infallible. This works for most I/O operations drivers do. > > Argh! That's the piece I was missing - that this makes the IO call infallible > and thus removes the need to write run-time error handling code. Sadly of course > that's not actually true, because I/O operations can always fail for reasons > other than what can be checked at compile time (eg. in particular PCI devices > can fall off the bus and return all 0xF's). But I guess existing drivers don't > really handle those cases either. We handle this case too by giving out a Devres<pci::Bar> rather than just a pci::Bar. The former gets revoked when the device falls off the bus.
On 19 Dec 2024, at 18:04, 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.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> rust/helpers/helpers.c | 1 +
> rust/helpers/io.c | 101 ++++++++++++++++
> rust/kernel/io.rs | 260 +++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 4 files changed, 363 insertions(+)
> create mode 100644 rust/helpers/io.c
> create mode 100644 rust/kernel/io.rs
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 060750af6524..63f9b1da179f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -14,6 +14,7 @@
> #include "cred.c"
> #include "err.c"
> #include "fs.c"
> +#include "io.c"
> #include "jump_label.c"
> #include "kunit.c"
> #include "mutex.c"
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> new file mode 100644
> index 000000000000..4c2401ccd720
> --- /dev/null
> +++ b/rust/helpers/io.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/io.h>
> +
> +void __iomem *rust_helper_ioremap(phys_addr_t offset, size_t size)
> +{
> + return ioremap(offset, size);
> +}
> +
> +void rust_helper_iounmap(volatile void __iomem *addr)
> +{
> + iounmap(addr);
> +}
> +
> +u8 rust_helper_readb(const volatile void __iomem *addr)
> +{
> + return readb(addr);
> +}
> +
> +u16 rust_helper_readw(const volatile void __iomem *addr)
> +{
> + return readw(addr);
> +}
> +
> +u32 rust_helper_readl(const volatile void __iomem *addr)
> +{
> + return readl(addr);
> +}
> +
> +#ifdef CONFIG_64BIT
> +u64 rust_helper_readq(const volatile void __iomem *addr)
> +{
> + return readq(addr);
> +}
> +#endif
> +
> +void rust_helper_writeb(u8 value, volatile void __iomem *addr)
> +{
> + writeb(value, addr);
> +}
> +
> +void rust_helper_writew(u16 value, volatile void __iomem *addr)
> +{
> + writew(value, addr);
> +}
> +
> +void rust_helper_writel(u32 value, volatile void __iomem *addr)
> +{
> + writel(value, addr);
> +}
> +
> +#ifdef CONFIG_64BIT
> +void rust_helper_writeq(u64 value, volatile void __iomem *addr)
> +{
> + writeq(value, addr);
> +}
> +#endif
> +
> +u8 rust_helper_readb_relaxed(const volatile void __iomem *addr)
> +{
> + return readb_relaxed(addr);
> +}
> +
> +u16 rust_helper_readw_relaxed(const volatile void __iomem *addr)
> +{
> + return readw_relaxed(addr);
> +}
> +
> +u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
> +{
> + return readl_relaxed(addr);
> +}
> +
> +#ifdef CONFIG_64BIT
> +u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
> +{
> + return readq_relaxed(addr);
> +}
> +#endif
> +
> +void rust_helper_writeb_relaxed(u8 value, volatile void __iomem *addr)
> +{
> + writeb_relaxed(value, addr);
> +}
> +
> +void rust_helper_writew_relaxed(u16 value, volatile void __iomem *addr)
> +{
> + writew_relaxed(value, addr);
> +}
> +
> +void rust_helper_writel_relaxed(u32 value, volatile void __iomem *addr)
> +{
> + writel_relaxed(value, addr);
> +}
> +
> +#ifdef CONFIG_64BIT
> +void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
> +{
> + writeq_relaxed(value, addr);
> +}
> +#endif
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> new file mode 100644
> index 000000000000..d4a73e52e3ee
> --- /dev/null
> +++ b/rust/kernel/io.rs
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Memory-mapped IO.
> +//!
> +//! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
> +
> +use crate::error::{code::EINVAL, Result};
> +use crate::{bindings, build_assert};
> +
> +/// Raw representation of an MMIO region.
> +///
> +/// By itself, the existence of an instance of this structure does not provide any guarantees that
> +/// the represented MMIO region does exist or is properly mapped.
> +///
> +/// Instead, the bus specific MMIO implementation must convert this raw representation into an `Io`
> +/// instance providing the actual memory accessors. Only by the conversion into an `Io` structure
> +/// any guarantees are given.
> +pub struct IoRaw<const SIZE: usize = 0> {
> + addr: usize,
> + maxsize: usize,
> +}
> +
> +impl<const SIZE: usize> IoRaw<SIZE> {
> + /// Returns a new `IoRaw` instance on success, an error otherwise.
> + pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
> + if maxsize < SIZE {
> + return Err(EINVAL);
> + }
> +
> + Ok(Self { addr, maxsize })
> + }
> +
> + /// Returns the base address of the MMIO region.
> + #[inline]
> + pub fn addr(&self) -> usize {
> + self.addr
> + }
> +
> + /// Returns the maximum size of the MMIO region.
> + #[inline]
> + pub fn maxsize(&self) -> usize {
> + self.maxsize
> + }
> +}
> +
> +/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
> +///
> +/// The creator (usually a subsystem / bus such as PCI) is responsible for creating the
> +/// mapping, performing an additional region request etc.
> +///
> +/// # Invariant
> +///
> +/// `addr` is the start and `maxsize` the length of valid I/O mapped memory region of size
> +/// `maxsize`.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// # use kernel::{bindings, io::{Io, IoRaw}};
> +/// # use core::ops::Deref;
> +///
> +/// // See also [`pci::Bar`] for a real example.
> +/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
> +///
> +/// impl<const SIZE: usize> IoMem<SIZE> {
> +/// /// # Safety
> +/// ///
> +/// /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
> +/// /// virtual address space.
> +/// unsafe fn new(paddr: usize) -> Result<Self>{
> +/// // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
> +/// // valid for `ioremap`.
> +/// let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
> +/// if addr.is_null() {
> +/// return Err(ENOMEM);
> +/// }
> +///
> +/// Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
> +/// }
> +/// }
> +///
> +/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +/// fn drop(&mut self) {
> +/// // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
> +/// unsafe { bindings::iounmap(self.0.addr() as _); };
> +/// }
> +/// }
> +///
> +/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +/// type Target = Io<SIZE>;
> +///
> +/// fn deref(&self) -> &Self::Target {
> +/// // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
> +/// unsafe { Io::from_raw(&self.0) }
> +/// }
> +/// }
> +///
> +///# fn no_run() -> Result<(), Error> {
> +/// // SAFETY: Invalid usage for example purposes.
> +/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
> +/// iomem.writel(0x42, 0x0);
> +/// assert!(iomem.try_writel(0x42, 0x0).is_ok());
> +/// assert!(iomem.try_writel(0x42, 0x4).is_err());
> +/// # Ok(())
> +/// # }
> +/// ```
> +#[repr(transparent)]
> +pub struct Io<const SIZE: usize = 0>(IoRaw<SIZE>);
> +
> +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);
> +
> + // 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
> + }
Currently reworking the portmem abstractions I wrote for the LED/SE10 driver.
Right now I’m wondering if it would make sense to move the 3 functions above (`offset_valid`, `io_addr` and `io_addr_assert` into IoRaw),
as I’m considering reusing the IoRaw in the portmem and then just offer the outb/outw/outl functions on a wraping type around IoRaw.
For this I would also use the same functions to check bounds.
Thanks,
Fiona
> +
> + 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
On 1/16/25 11:31 AM, Fiona Behrens wrote:
> On 19 Dec 2024, at 18:04, Danilo Krummrich wrote:
>> +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
>> + }
>
> Currently reworking the portmem abstractions I wrote for the LED/SE10 driver.
> Right now I’m wondering if it would make sense to move the 3 functions above (`offset_valid`, `io_addr` and `io_addr_assert` into IoRaw),
> as I’m considering reusing the IoRaw in the portmem and then just offer the outb/outw/outl functions on a wraping type around IoRaw.
> For this I would also use the same functions to check bounds.
Sure, feel free to move them. I think I should have moved those functions to
IoRaw from the get-go.
- Danilo
© 2016 - 2025 Red Hat, Inc.