* [PATCH v7 0/6] rust: pci: add config space read/write support
@ 2025-11-19 11:21 Zhi Wang
2025-11-19 11:21 ` [PATCH v7 1/6] samples: rust: rust_driver_pci: use "kernel vertical" style for imports Zhi Wang
` (5 more replies)
0 siblings, 6 replies; 28+ messages in thread
From: Zhi Wang @ 2025-11-19 11:21 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang
In the NVIDIA vGPU RFC [1], the PCI configuration space access is
required in nova-core for preparing gspVFInfo when vGPU support is
enabled. This series is the following up of the discussion with Danilo
for how to introduce support of PCI configuration space access in Rust
PCI abstractions.
The repo with the patches can be found at [2].
v7:
- Rebase to latest driver-core-testing branch.
- Introduce Io64 trait. (Alice)
- Add docs for call_{mmio, config}_{read, write}() macros. (Alex)
- Improve the define_{read, write} macros. (Alex)
- Add SAFETY/CAST in call_config_{read, write}. (Joel)
- Fix typo of method name. (Alex/Joel)
v6:
- Implement config_space() and config_space_extended() in device::Bound
lifecycle. (Danilo)
- Fix the "use" in the comment for generating proper rust docs, verify
the output of rustdoc. (Miguel)
- Improve the comments of PCI configuration space when checking the
output of rustdoc.
v5:
- Remove fallible accessors of PCI configuration space. (Danilo)
- Add #[repr(usize)] for enum ConfigSpace. (Danilo)
- Refine the handling of return value in read accessors. (Danilo)
- Add debug_assert!() in pdev::cfg_size(). (Danilo)
- Add ConfigSpace.as_raw() for extracting the raw value. (Danilo)
- Rebase the patches on top of driver-core-testing branch.
- Convert imports touched by this series to vertical style.
v4:
- Refactor the SIZE constant to be an associated constant. (Alice)
- Remove the default method implementations in the Io trait. (Alice)
- Make cfg_size() private. (Danilo/Bjorn)
- Implement the infallible accessors of ConfigSpace. (Danilo)
- Create a new Io64 trait specifically for 64-bit accessors. (Danilo)
- Provide two separate methods for driver: config_space() and
config_space_extended(). (Danilo)
- Update the sample driver to test the infallible accessors. (Danilo)
v3:
- Turn offset_valid() into a private function of kernel::io:Io. (Alex)
- Separate try and non-try variants. (Danilo)
- Move all the {try_}{read,write}{8,16,32,64} accessors to the I/O trait.
(Danilo)
- Replace the hardcoded MMIO type constraint with a generic trait bound
so that register! macro can be used in other places. (Danilo)
- Fix doctest. (John)
- Add an enum for PCI configuration space size. (Danilo)
- Refine the patch comments. (Bjorn)
v2:
- Factor out common trait as 'Io' and keep the rest routines in original
'Io' as 'Mmio'. (Danilo)
- Rename 'IoRaw' to 'MmioRaw'. Update the bus MMIO implementation to use
'MmioRaw'.
- Introduce pci::Device<Bound>::config_space(). (Danilo)
- Implement both infallible and fallible read/write routines, the device
driver decicdes which version should be used.
This ideas of this series are:
- Factor out common traits for other accessors to share the same
compiling/runtime check like before.
- Factor the MMIO read/write macros from the define_read! and
define_write! macros. Thus, define_{read, write}! can be used in other
backends.
In detail:
* Introduce `call_mmio_read!` and `call_mmio_write!` helper macros
to encapsulate the unsafe FFI calls.
* Update `define_read!` and `define_write!` macros to delegate to
the call macros.
* Export `define_read` and `define_write` so they can be reused
for other I/O backends (e.g. PCI config space).
- Implement the PCI configuration space access backend in PCI
abstractions.
- Add tests for config space routines in rust PCI sample driver.
[1] https://lore.kernel.org/all/20250903221111.3866249-1-zhiw@nvidia.com/
[2] https://github.com/zhiwang-nvidia/nova-core/tree/rust-for-linux/pci-configuration-space-v7
Zhi Wang (6):
samples: rust: rust_driver_pci: use "kernel vertical" style for
imports
rust: devres: style for imports
rust: io: factor common I/O helpers into Io trait
rust: io: factor out MMIO read/write macros
rust: pci: add config space read/write support
sample: rust: pci: add tests for config space routines
drivers/gpu/nova-core/regs/macros.rs | 90 ++++---
drivers/gpu/nova-core/vbios.rs | 1 +
rust/kernel/devres.rs | 57 +++--
rust/kernel/io.rs | 366 +++++++++++++++++++++------
rust/kernel/io/mem.rs | 16 +-
rust/kernel/io/poll.rs | 8 +-
rust/kernel/pci.rs | 43 +++-
rust/kernel/pci/io.rs | 128 +++++++++-
samples/rust/rust_driver_pci.rs | 38 ++-
9 files changed, 593 insertions(+), 154 deletions(-)
--
2.51.0
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v7 1/6] samples: rust: rust_driver_pci: use "kernel vertical" style for imports
2025-11-19 11:21 [PATCH v7 0/6] rust: pci: add config space read/write support Zhi Wang
@ 2025-11-19 11:21 ` Zhi Wang
2025-11-19 11:21 ` [PATCH v7 2/6] rust: devres: " Zhi Wang
` (4 subsequent siblings)
5 siblings, 0 replies; 28+ messages in thread
From: Zhi Wang @ 2025-11-19 11:21 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang
Convert all imports in the rust_driver_pci to use "kernel vertical" style.
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
samples/rust/rust_driver_pci.rs | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 5823787bea8e..ee6248b8cda5 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -4,7 +4,14 @@
//!
//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
-use kernel::{c_str, device::Core, devres::Devres, pci, prelude::*, sync::aref::ARef};
+use kernel::{
+ c_str,
+ device::Core,
+ devres::Devres,
+ pci,
+ prelude::*,
+ sync::aref::ARef, //
+};
struct Regs;
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v7 2/6] rust: devres: style for imports
2025-11-19 11:21 [PATCH v7 0/6] rust: pci: add config space read/write support Zhi Wang
2025-11-19 11:21 ` [PATCH v7 1/6] samples: rust: rust_driver_pci: use "kernel vertical" style for imports Zhi Wang
@ 2025-11-19 11:21 ` Zhi Wang
2025-11-19 11:21 ` [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait Zhi Wang
` (3 subsequent siblings)
5 siblings, 0 replies; 28+ messages in thread
From: Zhi Wang @ 2025-11-19 11:21 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang,
Miguel Ojeda
Convert all imports in the devres to use "kernel vertical" style. Drop
unnecessary imports covered by prelude::*.
Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
rust/kernel/devres.rs | 43 ++++++++++++++++++++++++++++++++++---------
1 file changed, 34 insertions(+), 9 deletions(-)
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index e01e0d36702d..659f513a72a6 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -8,13 +8,28 @@
use crate::{
alloc::Flags,
bindings,
- device::{Bound, Device},
- error::{to_result, Error, Result},
- ffi::c_void,
+ device::{
+ Bound,
+ Device, //
+ },
+ error::{
+ to_result, //
+ },
prelude::*,
- revocable::{Revocable, RevocableGuard},
- sync::{aref::ARef, rcu, Completion},
- types::{ForeignOwnable, Opaque, ScopeGuard},
+ revocable::{
+ Revocable,
+ RevocableGuard, //
+ },
+ sync::{
+ aref::ARef,
+ rcu,
+ Completion, //
+ },
+ types::{
+ ForeignOwnable,
+ Opaque,
+ ScopeGuard, //
+ },
};
use pin_init::Wrapper;
@@ -241,8 +256,12 @@ pub fn device(&self) -> &Device {
/// # Examples
///
/// ```no_run
- /// # #![cfg(CONFIG_PCI)]
- /// # use kernel::{device::Core, devres::Devres, pci};
+ /// #![cfg(CONFIG_PCI)]
+ /// use kernel::{
+ /// device::Core,
+ /// devres::Devres,
+ /// pci, //
+ /// };
///
/// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result {
/// let bar = devres.access(dev.as_ref())?;
@@ -345,7 +364,13 @@ fn register_foreign<P>(dev: &Device<Bound>, data: P) -> Result
/// # Examples
///
/// ```no_run
-/// use kernel::{device::{Bound, Device}, devres};
+/// use kernel::{
+/// device::{
+/// Bound,
+/// Device, //
+/// },
+/// devres, //
+/// };
///
/// /// Registration of e.g. a class device, IRQ, etc.
/// struct Registration;
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-19 11:21 [PATCH v7 0/6] rust: pci: add config space read/write support Zhi Wang
2025-11-19 11:21 ` [PATCH v7 1/6] samples: rust: rust_driver_pci: use "kernel vertical" style for imports Zhi Wang
2025-11-19 11:21 ` [PATCH v7 2/6] rust: devres: " Zhi Wang
@ 2025-11-19 11:21 ` Zhi Wang
2025-11-21 14:20 ` Alice Ryhl
2025-11-25 14:09 ` Alexandre Courbot
2025-11-19 11:21 ` [PATCH v7 4/6] rust: io: factor out MMIO read/write macros Zhi Wang
` (2 subsequent siblings)
5 siblings, 2 replies; 28+ messages in thread
From: Zhi Wang @ 2025-11-19 11:21 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang
The previous Io<SIZE> type combined both the generic I/O access helpers
and MMIO implementation details in a single struct.
To establish a cleaner layering between the I/O interface and its concrete
backends, paving the way for supporting additional I/O mechanisms in the
future, Io<SIZE> need to be factored.
Factor the common helpers into new {Io, Io64} traits, and move the
MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
to use MmioRaw.
No functional change intended.
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
drivers/gpu/nova-core/regs/macros.rs | 90 +++++----
drivers/gpu/nova-core/vbios.rs | 1 +
rust/kernel/devres.rs | 14 +-
rust/kernel/io.rs | 283 ++++++++++++++++++++-------
rust/kernel/io/mem.rs | 16 +-
rust/kernel/io/poll.rs | 8 +-
rust/kernel/pci/io.rs | 12 +-
samples/rust/rust_driver_pci.rs | 2 +
8 files changed, 295 insertions(+), 131 deletions(-)
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 8058e1696df9..39b1069a3429 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -608,16 +608,18 @@ impl $name {
/// Read the register from its address in `io`.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ pub(crate) fn read<T, I>(io: &T) -> Self where
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
Self(io.read32($offset))
}
/// Write the value contained in `self` to the register address in `io`.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ pub(crate) fn write<T, I>(self, io: &T) where
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
io.write32(self.0, $offset)
}
@@ -625,11 +627,12 @@ pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
/// Read the register from its address in `io` and run `f` on its value to obtain a new
/// value to write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, F>(
+ pub(crate) fn alter<T, I, F>(
io: &T,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io));
@@ -647,12 +650,13 @@ impl $name {
/// Read the register from `io`, using the base address provided by `base` and adding
/// the register's offset to it.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T, B>(
+ pub(crate) fn read<T, I, B>(
io: &T,
#[allow(unused_variables)]
base: &B,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
@@ -667,13 +671,14 @@ pub(crate) fn read<const SIZE: usize, T, B>(
/// Write the value contained in `self` to `io`, using the base address provided by
/// `base` and adding the register's offset to it.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T, B>(
+ pub(crate) fn write<T, I, B>(
self,
io: &T,
#[allow(unused_variables)]
base: &B,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
@@ -688,12 +693,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
/// the register's offset to it, then run `f` on its value to obtain a new value to
/// write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, B, F>(
+ pub(crate) fn alter<T, I, B, F>(
io: &T,
base: &B,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
@@ -713,11 +719,12 @@ impl $name {
/// Read the array register at index `idx` from its address in `io`.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T>(
+ pub(crate) fn read<T, I>(
io: &T,
idx: usize,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
build_assert!(idx < Self::SIZE);
@@ -729,12 +736,13 @@ pub(crate) fn read<const SIZE: usize, T>(
/// Write the value contained in `self` to the array register with index `idx` in `io`.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T>(
+ pub(crate) fn write<T, I>(
self,
io: &T,
idx: usize
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
build_assert!(idx < Self::SIZE);
@@ -746,12 +754,13 @@ pub(crate) fn write<const SIZE: usize, T>(
/// Read the array register at index `idx` in `io` and run `f` on its value to obtain a
/// new value to write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, F>(
+ pub(crate) fn alter<T, I, F>(
io: &T,
idx: usize,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io, idx));
@@ -763,11 +772,12 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_read<const SIZE: usize, T>(
+ pub(crate) fn try_read<T, I>(
io: &T,
idx: usize,
) -> ::kernel::error::Result<Self> where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
if idx < Self::SIZE {
Ok(Self::read(io, idx))
@@ -781,12 +791,13 @@ pub(crate) fn try_read<const SIZE: usize, T>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_write<const SIZE: usize, T>(
+ pub(crate) fn try_write<T, I>(
self,
io: &T,
idx: usize,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
if idx < Self::SIZE {
Ok(self.write(io, idx))
@@ -801,12 +812,13 @@ pub(crate) fn try_write<const SIZE: usize, T>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_alter<const SIZE: usize, T, F>(
+ pub(crate) fn try_alter<T, I, F>(
io: &T,
idx: usize,
f: F,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
F: ::core::ops::FnOnce(Self) -> Self,
{
if idx < Self::SIZE {
@@ -832,13 +844,14 @@ impl $name {
/// Read the array register at index `idx` from `io`, using the base address provided
/// by `base` and adding the register's offset to it.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T, B>(
+ pub(crate) fn read<T, I, B>(
io: &T,
#[allow(unused_variables)]
base: &B,
idx: usize,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
@@ -853,14 +866,15 @@ pub(crate) fn read<const SIZE: usize, T, B>(
/// Write the value contained in `self` to `io`, using the base address provided by
/// `base` and adding the offset of array register `idx` to it.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T, B>(
+ pub(crate) fn write<T, I, B>(
self,
io: &T,
#[allow(unused_variables)]
base: &B,
idx: usize
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
@@ -875,13 +889,14 @@ pub(crate) fn write<const SIZE: usize, T, B>(
/// by `base` and adding the register's offset to it, then run `f` on its value to
/// obtain a new value to write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, B, F>(
+ pub(crate) fn alter<T, I, B, F>(
io: &T,
base: &B,
idx: usize,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
@@ -895,12 +910,13 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_read<const SIZE: usize, T, B>(
+ pub(crate) fn try_read<T, I, B>(
io: &T,
base: &B,
idx: usize,
) -> ::kernel::error::Result<Self> where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
if idx < Self::SIZE {
@@ -916,13 +932,14 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_write<const SIZE: usize, T, B>(
+ pub(crate) fn try_write<T, I, B>(
self,
io: &T,
base: &B,
idx: usize,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
if idx < Self::SIZE {
@@ -939,13 +956,14 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
+ pub(crate) fn try_alter<T, I, B, F>(
io: &T,
base: &B,
idx: usize,
f: F,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 71fbe71b84db..7a0121ab9b09 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -8,6 +8,7 @@
use core::convert::TryFrom;
use kernel::device;
use kernel::error::Result;
+use kernel::io::IoFallible;
use kernel::prelude::*;
use kernel::ptr::{Alignable, Alignment};
use kernel::types::ARef;
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 659f513a72a6..ad72943adbbe 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -75,15 +75,16 @@ struct Inner<T: Send> {
/// },
/// devres::Devres,
/// io::{
-/// Io,
-/// IoRaw,
+/// IoInfallible,
+/// Mmio,
+/// MmioRaw,
/// PhysAddr,
/// },
/// };
/// use core::ops::Deref;
///
/// // See also [`pci::Bar`] for a real example.
-/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
+/// struct IoMem<const SIZE: usize>(MmioRaw<SIZE>);
///
/// impl<const SIZE: usize> IoMem<SIZE> {
/// /// # Safety
@@ -98,7 +99,7 @@ struct Inner<T: Send> {
/// return Err(ENOMEM);
/// }
///
-/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
+/// Ok(IoMem(MmioRaw::new(addr as usize, SIZE)?))
/// }
/// }
///
@@ -110,11 +111,11 @@ struct Inner<T: Send> {
/// }
///
/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
-/// type Target = Io<SIZE>;
+/// type Target = Mmio<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) }
+/// unsafe { Mmio::from_raw(&self.0) }
/// }
/// }
/// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
@@ -260,6 +261,7 @@ pub fn device(&self) -> &Device {
/// use kernel::{
/// device::Core,
/// devres::Devres,
+ /// io::IoInfallible,
/// pci, //
/// };
///
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 98e8b84e68d1..3e6ea29835ae 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -32,16 +32,16 @@
/// 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> {
+/// Instead, the bus specific MMIO implementation must convert this raw representation into an
+/// `Mmio` instance providing the actual memory accessors. Only by the conversion into an `Mmio`
+/// structure any guarantees are given.
+pub struct MmioRaw<const SIZE: usize = 0> {
addr: usize,
maxsize: usize,
}
-impl<const SIZE: usize> IoRaw<SIZE> {
- /// Returns a new `IoRaw` instance on success, an error otherwise.
+impl<const SIZE: usize> MmioRaw<SIZE> {
+ /// Returns a new `MmioRaw` instance on success, an error otherwise.
pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
if maxsize < SIZE {
return Err(EINVAL);
@@ -80,15 +80,17 @@ pub fn maxsize(&self) -> usize {
/// bindings,
/// ffi::c_void,
/// io::{
-/// Io,
-/// IoRaw,
+/// IoFallible,
+/// IoInfallible,
+/// Mmio,
+/// MmioRaw,
/// PhysAddr,
/// },
/// };
/// use core::ops::Deref;
///
/// // See also [`pci::Bar`] for a real example.
-/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
+/// struct IoMem<const SIZE: usize>(MmioRaw<SIZE>);
///
/// impl<const SIZE: usize> IoMem<SIZE> {
/// /// # Safety
@@ -103,7 +105,7 @@ pub fn maxsize(&self) -> usize {
/// return Err(ENOMEM);
/// }
///
-/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
+/// Ok(IoMem(MmioRaw::new(addr as usize, SIZE)?))
/// }
/// }
///
@@ -115,11 +117,11 @@ pub fn maxsize(&self) -> usize {
/// }
///
/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
-/// type Target = Io<SIZE>;
+/// type Target = Mmio<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) }
+/// unsafe { Mmio::from_raw(&self.0) }
/// }
/// }
///
@@ -133,29 +135,31 @@ pub fn maxsize(&self) -> usize {
/// # }
/// ```
#[repr(transparent)]
-pub struct Io<const SIZE: usize = 0>(IoRaw<SIZE>);
+pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>);
macro_rules! define_read {
- ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn:ident -> $type_name:ty) => {
+ (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn: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 {
+ $vis 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::$c_fn(addr as *const c_void) }
}
+ };
+ (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident -> $type_name:ty) => {
/// 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> {
+ $vis 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.
@@ -165,26 +169,28 @@ pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
}
macro_rules! define_write {
- ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn:ident <- $type_name:ty) => {
+ (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn: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) {
+ $vis 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::$c_fn(value, addr as *mut c_void) }
}
+ };
+ (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident <- $type_name:ty) => {
/// 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 {
+ $vis 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.
@@ -194,43 +200,38 @@ pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
};
}
-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() }
+/// Checks whether an access of type `U` at the given `offset`
+/// is valid within this region.
+#[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
}
+}
+
+/// Represents a region of I/O space of a fixed size.
+///
+/// Provides common helpers for offset validation and address
+/// calculation on top of a base address and maximum size.
+///
+pub trait Io {
+ /// Minimum usable size of this region.
+ const MIN_SIZE: usize;
/// Returns the base address of this mapping.
- #[inline]
- pub fn addr(&self) -> usize {
- self.0.addr()
- }
+ fn addr(&self) -> usize;
/// 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
- }
- }
+ fn maxsize(&self) -> usize;
+ /// Returns the absolute I/O address for a given `offset`,
+ /// performing runtime bound checks.
#[inline]
fn io_addr<U>(&self, offset: usize) -> Result<usize> {
- if !Self::offset_valid::<U>(offset, self.maxsize()) {
+ if !offset_valid::<U>(offset, self.maxsize()) {
return Err(EINVAL);
}
@@ -239,50 +240,190 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
self.addr().checked_add(offset).ok_or(EINVAL)
}
+ /// Returns the absolute I/O address for a given `offset`,
+ /// performing compile-time bound checks.
#[inline]
fn io_addr_assert<U>(&self, offset: usize) -> usize {
- build_assert!(Self::offset_valid::<U>(offset, SIZE));
+ build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
self.addr() + offset
}
+}
+
+/// Types implementing this trait (e.g. MMIO BARs or PCI config
+/// regions) can share the same Infallible accessors.
+pub trait IoInfallible: Io {
+ /// Infallible 8-bit read with compile-time bounds check.
+ fn read8(&self, offset: usize) -> u8;
+
+ /// Infallible 16-bit read with compile-time bounds check.
+ fn read16(&self, offset: usize) -> u16;
+
+ /// Infallible 32-bit read with compile-time bounds check.
+ fn read32(&self, offset: usize) -> u32;
+
+ /// Infallible 8-bit write with compile-time bounds check.
+ fn write8(&self, value: u8, offset: usize);
+
+ /// Infallible 16-bit write with compile-time bounds check.
+ fn write16(&self, value: u16, offset: usize);
+
+ /// Infallible 32-bit write with compile-time bounds check.
+ fn write32(&self, value: u32, offset: usize);
+}
+
+/// Types implementing this trait (e.g. MMIO BARs or PCI config
+/// regions) can share the same Fallible accessors.
+pub trait IoFallible: Io {
+ /// Fallible 8-bit read with runtime bounds check.
+ fn try_read8(&self, offset: usize) -> Result<u8>;
+
+ /// Fallible 16-bit read with runtime bounds check.
+ fn try_read16(&self, offset: usize) -> Result<u16>;
+
+ /// Fallible 32-bit read with runtime bounds check.
+ fn try_read32(&self, offset: usize) -> Result<u32>;
+
+ /// Fallible 8-bit write with runtime bounds check.
+ fn try_write8(&self, value: u8, offset: usize) -> Result;
+
+ /// Fallible 16-bit write with runtime bounds check.
+ fn try_write16(&self, value: u16, offset: usize) -> Result;
+
+ /// Fallible 32-bit write with runtime bounds check.
+ fn try_write32(&self, value: u32, offset: usize) -> Result;
+}
+
+/// Represents a region of I/O space of a fixed size with 64-bit accessors.
+///
+/// Provides common helpers for offset validation and address
+/// calculation on top of a base address and maximum size.
+///
+#[cfg(CONFIG_64BIT)]
+pub trait Io64: Io {}
- define_read!(read8, try_read8, readb -> u8);
- define_read!(read16, try_read16, readw -> u16);
- define_read!(read32, try_read32, readl -> u32);
+/// Types implementing this trait can share the same Infallible accessors.
+#[cfg(CONFIG_64BIT)]
+pub trait IoInfallible64: Io64 {
+ /// Infallible 64-bit read with compile-time bounds check.
+ fn read64(&self, offset: usize) -> u64;
+
+ /// Infallible 64-bit write with compile-time bounds check.
+ fn write64(&self, value: u64, offset: usize);
+}
+
+/// Types implementing this trait can share the same Infallible accessors.
+#[cfg(CONFIG_64BIT)]
+pub trait IoFallible64: Io64 {
+ /// Fallible 64-bit read with runtime bounds check.
+ fn try_read64(&self, offset: usize) -> Result<u64>;
+
+ /// Fallible 64-bit write with runtime bounds check.
+ fn try_write64(&self, value: u64, offset: usize) -> Result;
+}
+
+impl<const SIZE: usize> Io for Mmio<SIZE> {
+ const MIN_SIZE: usize = SIZE;
+
+ /// Returns the base address of this mapping.
+ #[inline]
+ fn addr(&self) -> usize {
+ self.0.addr()
+ }
+
+ /// Returns the maximum size of this mapping.
+ #[inline]
+ fn maxsize(&self) -> usize {
+ self.0.maxsize()
+ }
+}
+
+impl<const SIZE: usize> IoInfallible for Mmio<SIZE> {
+ define_read!(infallible, read8, readb -> u8);
+ define_read!(infallible, read16, readw -> u16);
+ define_read!(infallible, read32, readl -> u32);
+
+ define_write!(infallible, write8, writeb <- u8);
+ define_write!(infallible, write16, writew <- u16);
+ define_write!(infallible, write32, writel <- u32);
+}
+
+impl<const SIZE: usize> IoFallible for Mmio<SIZE> {
+ define_read!(fallible, try_read8, readb -> u8);
+ define_read!(fallible, try_read16, readw -> u16);
+ define_read!(fallible, try_read32, readl -> u32);
+
+ define_write!(fallible, try_write8, writeb <- u8);
+ define_write!(fallible, try_write16, writew <- u16);
+ define_write!(fallible, try_write32, writel <- u32);
+}
+
+#[cfg(CONFIG_64BIT)]
+impl<const SIZE: usize> Io64 for Mmio<SIZE> {}
+
+#[cfg(CONFIG_64BIT)]
+impl<const SIZE: usize> IoInfallible64 for Mmio<SIZE> {
+ define_read!(infallible, read64, readq -> u64);
+
+ define_write!(infallible, write64, writeq <- u64);
+}
+
+#[cfg(CONFIG_64BIT)]
+impl<const SIZE: usize> IoFallible64 for Mmio<SIZE> {
+ define_read!(fallible, try_read64, readq -> u64);
+
+ define_write!(fallible, try_write64, writeq <- u64);
+}
+
+impl<const SIZE: usize> Mmio<SIZE> {
+ /// Converts an `MmioRaw` into an `Mmio` 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: &MmioRaw<SIZE>) -> &Self {
+ // SAFETY: `Mmio` is a transparent wrapper around `MmioRaw`.
+ unsafe { &*core::ptr::from_ref(raw).cast() }
+ }
+
+ define_read!(infallible, pub read8_relaxed, readb_relaxed -> u8);
+ define_read!(infallible, pub read16_relaxed, readw_relaxed -> u16);
+ define_read!(infallible, pub read32_relaxed, readl_relaxed -> u32);
define_read!(
+ infallible,
#[cfg(CONFIG_64BIT)]
- read64,
- try_read64,
- readq -> u64
+ pub read64_relaxed,
+ readq_relaxed -> u64
);
- define_read!(read8_relaxed, try_read8_relaxed, readb_relaxed -> u8);
- define_read!(read16_relaxed, try_read16_relaxed, readw_relaxed -> u16);
- define_read!(read32_relaxed, try_read32_relaxed, readl_relaxed -> u32);
+ define_read!(fallible, pub try_read8_relaxed, readb_relaxed -> u8);
+ define_read!(fallible, pub try_read16_relaxed, readw_relaxed -> u16);
+ define_read!(fallible, pub try_read32_relaxed, readl_relaxed -> u32);
define_read!(
+ fallible,
#[cfg(CONFIG_64BIT)]
- read64_relaxed,
- try_read64_relaxed,
+ pub try_read64_relaxed,
readq_relaxed -> u64
);
- define_write!(write8, try_write8, writeb <- u8);
- define_write!(write16, try_write16, writew <- u16);
- define_write!(write32, try_write32, writel <- u32);
+ define_write!(infallible, pub write8_relaxed, writeb_relaxed <- u8);
+ define_write!(infallible, pub write16_relaxed, writew_relaxed <- u16);
+ define_write!(infallible, pub write32_relaxed, writel_relaxed <- u32);
define_write!(
+ infallible,
#[cfg(CONFIG_64BIT)]
- write64,
- try_write64,
- writeq <- u64
+ pub write64_relaxed,
+ writeq_relaxed <- u64
);
- define_write!(write8_relaxed, try_write8_relaxed, writeb_relaxed <- u8);
- define_write!(write16_relaxed, try_write16_relaxed, writew_relaxed <- u16);
- define_write!(write32_relaxed, try_write32_relaxed, writel_relaxed <- u32);
+ define_write!(fallible, pub try_write8_relaxed, writeb_relaxed <- u8);
+ define_write!(fallible, pub try_write16_relaxed, writew_relaxed <- u16);
+ define_write!(fallible, pub try_write32_relaxed, writel_relaxed <- u32);
define_write!(
+ fallible,
#[cfg(CONFIG_64BIT)]
- write64_relaxed,
- try_write64_relaxed,
+ pub try_write64_relaxed,
writeq_relaxed <- u64
);
}
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index b03b82cd531b..5dcd7c901427 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -17,8 +17,8 @@
Region,
Resource, //
},
- Io,
- IoRaw, //
+ Mmio,
+ MmioRaw, //
},
prelude::*,
};
@@ -203,7 +203,7 @@ pub fn new<'a>(io_request: IoRequest<'a>) -> impl PinInit<Devres<Self>, Error> +
}
impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
- type Target = Io<SIZE>;
+ type Target = Mmio<SIZE>;
fn deref(&self) -> &Self::Target {
&self.iomem
@@ -217,10 +217,10 @@ fn deref(&self) -> &Self::Target {
///
/// # Invariants
///
-/// [`IoMem`] always holds an [`IoRaw`] instance that holds a valid pointer to the
+/// [`IoMem`] always holds an [`MmioRaw`] instance that holds a valid pointer to the
/// start of the I/O memory mapped region.
pub struct IoMem<const SIZE: usize = 0> {
- io: IoRaw<SIZE>,
+ io: MmioRaw<SIZE>,
}
impl<const SIZE: usize> IoMem<SIZE> {
@@ -255,7 +255,7 @@ fn ioremap(resource: &Resource) -> Result<Self> {
return Err(ENOMEM);
}
- let io = IoRaw::new(addr as usize, size)?;
+ let io = MmioRaw::new(addr as usize, size)?;
let io = IoMem { io };
Ok(io)
@@ -278,10 +278,10 @@ fn drop(&mut self) {
}
impl<const SIZE: usize> Deref for IoMem<SIZE> {
- type Target = Io<SIZE>;
+ type Target = Mmio<SIZE>;
fn deref(&self) -> &Self::Target {
// SAFETY: Safe as by the invariant of `IoMem`.
- unsafe { Io::from_raw(&self.io) }
+ unsafe { Mmio::from_raw(&self.io) }
}
}
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index b1a2570364f4..543a4b7cea0d 100644
--- a/rust/kernel/io/poll.rs
+++ b/rust/kernel/io/poll.rs
@@ -45,12 +45,12 @@
/// # Examples
///
/// ```no_run
-/// use kernel::io::{Io, poll::read_poll_timeout};
+/// use kernel::io::{IoFallible, Mmio, poll::read_poll_timeout};
/// use kernel::time::Delta;
///
/// const HW_READY: u16 = 0x01;
///
-/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
+/// fn wait_for_hardware<const SIZE: usize>(io: &Mmio<SIZE>) -> Result {
/// read_poll_timeout(
/// // The `op` closure reads the value of a specific status register.
/// || io.try_read16(0x1000),
@@ -128,12 +128,12 @@ pub fn read_poll_timeout<Op, Cond, T>(
/// # Examples
///
/// ```no_run
-/// use kernel::io::{poll::read_poll_timeout_atomic, Io};
+/// use kernel::io::{poll::read_poll_timeout_atomic, IoFallible, Mmio};
/// use kernel::time::Delta;
///
/// const HW_READY: u16 = 0x01;
///
-/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
+/// fn wait_for_hardware<const SIZE: usize>(io: &Mmio<SIZE>) -> Result {
/// read_poll_timeout_atomic(
/// // The `op` closure reads the value of a specific status register.
/// || io.try_read16(0x1000),
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
index 0d55c3139b6f..2bbb3261198d 100644
--- a/rust/kernel/pci/io.rs
+++ b/rust/kernel/pci/io.rs
@@ -8,8 +8,8 @@
device,
devres::Devres,
io::{
- Io,
- IoRaw, //
+ Mmio,
+ MmioRaw, //
},
prelude::*,
sync::aref::ARef, //
@@ -24,7 +24,7 @@
/// memory mapped PCI BAR and its size.
pub struct Bar<const SIZE: usize = 0> {
pdev: ARef<Device>,
- io: IoRaw<SIZE>,
+ io: MmioRaw<SIZE>,
num: i32,
}
@@ -60,7 +60,7 @@ pub(super) fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {
return Err(ENOMEM);
}
- let io = match IoRaw::new(ioptr, len as usize) {
+ let io = match MmioRaw::new(ioptr, len as usize) {
Ok(io) => io,
Err(err) => {
// SAFETY:
@@ -114,11 +114,11 @@ fn drop(&mut self) {
}
impl<const SIZE: usize> Deref for Bar<SIZE> {
- type Target = Io<SIZE>;
+ type Target = Mmio<SIZE>;
fn deref(&self) -> &Self::Target {
// SAFETY: By the type invariant of `Self`, the MMIO range in `self.io` is properly mapped.
- unsafe { Io::from_raw(&self.io) }
+ unsafe { Mmio::from_raw(&self.io) }
}
}
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index ee6248b8cda5..74b93ca7c338 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -8,6 +8,8 @@
c_str,
device::Core,
devres::Devres,
+ io::IoFallible,
+ io::IoInfallible,
pci,
prelude::*,
sync::aref::ARef, //
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v7 4/6] rust: io: factor out MMIO read/write macros
2025-11-19 11:21 [PATCH v7 0/6] rust: pci: add config space read/write support Zhi Wang
` (2 preceding siblings ...)
2025-11-19 11:21 ` [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait Zhi Wang
@ 2025-11-19 11:21 ` Zhi Wang
2025-11-19 11:21 ` [PATCH v7 5/6] rust: pci: add config space read/write support Zhi Wang
2025-11-19 11:21 ` [PATCH v7 6/6] sample: rust: pci: add tests for config space routines Zhi Wang
5 siblings, 0 replies; 28+ messages in thread
From: Zhi Wang @ 2025-11-19 11:21 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang
Refactor the existing MMIO accessors to use common call macros
instead of inlining the bindings calls in each `define_{read,write}!`
expansion.
This factoring separates the common offset/bounds checks from the
low-level call pattern, making it easier to add additional I/O accessor
families.
No functional change intended.
Cc: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
rust/kernel/io.rs | 149 ++++++++++++++++++++++++++++++++--------------
1 file changed, 104 insertions(+), 45 deletions(-)
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 3e6ea29835ae..6e0daaf8dc6a 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -137,8 +137,65 @@ pub fn maxsize(&self) -> usize {
#[repr(transparent)]
pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>);
+/// Internal helper macros used to invoke C MMIO read functions.
+///
+/// This macro is intended to be used by higher-level MMIO access macros (define_read) and provides
+/// a unified expansion for infallible vs. fallible read semantics. It emits a direct call into the
+/// corresponding C helper and performs the required cast to the Rust return type.
+///
+/// # Parameters
+///
+/// * `$c_fn` – The C function performing the MMIO write.
+/// * `$self` – The I/O backend object.
+/// * `$ty` – The type of the value to be read.
+/// * `$addr` – The MMIO address to read.
+///
+/// This macro does not perform any validation; all invariants must be upheld by the higher-level
+/// abstraction invoking it.
+macro_rules! call_mmio_read {
+ (infallible, $c_fn:ident, $self:ident, $type:ty, $addr:expr) => {
+ // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+ unsafe { bindings::$c_fn($addr as *const c_void) as $type }
+ };
+
+ (fallible, $c_fn:ident, $self:ident, $type:ty, $addr:expr) => {{
+ // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+ Ok(unsafe { bindings::$c_fn($addr as *const c_void) as $type })
+ }};
+}
+
+/// Internal helper macros used to invoke C MMIO write functions.
+///
+/// This macro is intended to be used by higher-level MMIO access macros (define_write) and provides
+/// a unified expansion for infallible vs. fallible read semantics. It emits a direct call into the
+/// corresponding C helper and performs the required cast to the Rust return type.
+///
+/// # Parameters
+///
+/// * `$c_fn` – The C function performing the MMIO write.
+/// * `$self` – The I/O backend object.
+/// * `$ty` – The type of the written value.
+/// * `$addr` – The MMIO address to write.
+/// * `$value` – The value to write.
+///
+/// This macro does not perform any validation; all invariants must be upheld by the higher-level
+/// abstraction invoking it.
+macro_rules! call_mmio_write {
+ (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
+ // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+ unsafe { bindings::$c_fn($value, $addr as *mut c_void) }
+ };
+
+ (fallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {{
+ // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+ unsafe { bindings::$c_fn($value, $addr as *mut c_void) };
+ Ok(())
+ }};
+}
+
macro_rules! define_read {
- (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn:ident -> $type_name:ty) => {
+ (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $call_macro:ident($c_fn: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
@@ -148,12 +205,13 @@ macro_rules! define_read {
$vis 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::$c_fn(addr as *const c_void) }
+ // SAFETY: By the type invariant `addr` is a valid address for IO operations.
+ $call_macro!(infallible, $c_fn, self, $type_name, addr)
}
};
- (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident -> $type_name:ty) => {
+ (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $call_macro:ident($c_fn:ident) ->
+ $type_name:ty) => {
/// Read IO data from a given offset.
///
/// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
@@ -162,14 +220,16 @@ macro_rules! define_read {
$vis 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::$c_fn(addr as *const c_void) })
+ // SAFETY: By the type invariant `addr` is a valid address for IO operations.
+ $call_macro!(fallible, $c_fn, self, $type_name, addr)
}
};
}
+pub(crate) use define_read;
macro_rules! define_write {
- (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn:ident <- $type_name:ty) => {
+ (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $call_macro:ident($c_fn: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
@@ -179,12 +239,12 @@ macro_rules! define_write {
$vis 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::$c_fn(value, addr as *mut c_void) }
+ $call_macro!(infallible, $c_fn, self, $type_name, addr, value);
}
};
- (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident <- $type_name:ty) => {
+ (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $call_macro:ident($c_fn:ident) <-
+ $type_name:ty) => {
/// Write IO data from a given offset.
///
/// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
@@ -193,12 +253,11 @@ macro_rules! define_write {
$vis 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::$c_fn(value, addr as *mut c_void) }
- Ok(())
+ $call_macro!(fallible, $c_fn, self, $type_name, addr, value)
}
};
}
+pub(crate) use define_write;
/// Checks whether an access of type `U` at the given `offset`
/// is valid within this region.
@@ -339,23 +398,23 @@ fn maxsize(&self) -> usize {
}
impl<const SIZE: usize> IoInfallible for Mmio<SIZE> {
- define_read!(infallible, read8, readb -> u8);
- define_read!(infallible, read16, readw -> u16);
- define_read!(infallible, read32, readl -> u32);
+ define_read!(infallible, read8, call_mmio_read(readb) -> u8);
+ define_read!(infallible, read16, call_mmio_read(readw) -> u16);
+ define_read!(infallible, read32, call_mmio_read(readl) -> u32);
- define_write!(infallible, write8, writeb <- u8);
- define_write!(infallible, write16, writew <- u16);
- define_write!(infallible, write32, writel <- u32);
+ define_write!(infallible, write8, call_mmio_write(writeb) <- u8);
+ define_write!(infallible, write16, call_mmio_write(writew) <- u16);
+ define_write!(infallible, write32, call_mmio_write(writel) <- u32);
}
impl<const SIZE: usize> IoFallible for Mmio<SIZE> {
- define_read!(fallible, try_read8, readb -> u8);
- define_read!(fallible, try_read16, readw -> u16);
- define_read!(fallible, try_read32, readl -> u32);
+ define_read!(fallible, try_read8, call_mmio_read(readb) -> u8);
+ define_read!(fallible, try_read16, call_mmio_read(readw) -> u16);
+ define_read!(fallible, try_read32, call_mmio_read(readl) -> u32);
- define_write!(fallible, try_write8, writeb <- u8);
- define_write!(fallible, try_write16, writew <- u16);
- define_write!(fallible, try_write32, writel <- u32);
+ define_write!(fallible, try_write8, call_mmio_write(writeb) <- u8);
+ define_write!(fallible, try_write16, call_mmio_write(writew) <- u16);
+ define_write!(fallible, try_write32, call_mmio_write(writel) <- u32);
}
#[cfg(CONFIG_64BIT)]
@@ -363,16 +422,16 @@ impl<const SIZE: usize> Io64 for Mmio<SIZE> {}
#[cfg(CONFIG_64BIT)]
impl<const SIZE: usize> IoInfallible64 for Mmio<SIZE> {
- define_read!(infallible, read64, readq -> u64);
+ define_read!(infallible, read64, call_mmio_read(readq) -> u64);
- define_write!(infallible, write64, writeq <- u64);
+ define_write!(infallible, write64, call_mmio_write(writeq) <- u64);
}
#[cfg(CONFIG_64BIT)]
impl<const SIZE: usize> IoFallible64 for Mmio<SIZE> {
- define_read!(fallible, try_read64, readq -> u64);
+ define_read!(fallible, try_read64, call_mmio_read(readq) -> u64);
- define_write!(fallible, try_write64, writeq <- u64);
+ define_write!(fallible, try_write64, call_mmio_write(writeq) <- u64);
}
impl<const SIZE: usize> Mmio<SIZE> {
@@ -387,43 +446,43 @@ pub unsafe fn from_raw(raw: &MmioRaw<SIZE>) -> &Self {
unsafe { &*core::ptr::from_ref(raw).cast() }
}
- define_read!(infallible, pub read8_relaxed, readb_relaxed -> u8);
- define_read!(infallible, pub read16_relaxed, readw_relaxed -> u16);
- define_read!(infallible, pub read32_relaxed, readl_relaxed -> u32);
+ define_read!(infallible, pub read8_relaxed, call_mmio_read(readb_relaxed) -> u8);
+ define_read!(infallible, pub read16_relaxed, call_mmio_read(readw_relaxed) -> u16);
+ define_read!(infallible, pub read32_relaxed, call_mmio_read(readl_relaxed) -> u32);
define_read!(
infallible,
#[cfg(CONFIG_64BIT)]
pub read64_relaxed,
- readq_relaxed -> u64
+ call_mmio_read(readq_relaxed) -> u64
);
- define_read!(fallible, pub try_read8_relaxed, readb_relaxed -> u8);
- define_read!(fallible, pub try_read16_relaxed, readw_relaxed -> u16);
- define_read!(fallible, pub try_read32_relaxed, readl_relaxed -> u32);
+ define_read!(fallible, pub try_read8_relaxed, call_mmio_read(readb_relaxed) -> u8);
+ define_read!(fallible, pub try_read16_relaxed, call_mmio_read(readw_relaxed) -> u16);
+ define_read!(fallible, pub try_read32_relaxed, call_mmio_read(readl_relaxed) -> u32);
define_read!(
fallible,
#[cfg(CONFIG_64BIT)]
pub try_read64_relaxed,
- readq_relaxed -> u64
+ call_mmio_read(readq_relaxed) -> u64
);
- define_write!(infallible, pub write8_relaxed, writeb_relaxed <- u8);
- define_write!(infallible, pub write16_relaxed, writew_relaxed <- u16);
- define_write!(infallible, pub write32_relaxed, writel_relaxed <- u32);
+ define_write!(infallible, pub write8_relaxed, call_mmio_write(writeb_relaxed) <- u8);
+ define_write!(infallible, pub write16_relaxed, call_mmio_write(writew_relaxed) <- u16);
+ define_write!(infallible, pub write32_relaxed, call_mmio_write(writel_relaxed) <- u32);
define_write!(
infallible,
#[cfg(CONFIG_64BIT)]
pub write64_relaxed,
- writeq_relaxed <- u64
+ call_mmio_write(writeq_relaxed) <- u64
);
- define_write!(fallible, pub try_write8_relaxed, writeb_relaxed <- u8);
- define_write!(fallible, pub try_write16_relaxed, writew_relaxed <- u16);
- define_write!(fallible, pub try_write32_relaxed, writel_relaxed <- u32);
+ define_write!(fallible, pub try_write8_relaxed, call_mmio_write(writeb_relaxed) <- u8);
+ define_write!(fallible, pub try_write16_relaxed, call_mmio_write(writew_relaxed) <- u16);
+ define_write!(fallible, pub try_write32_relaxed, call_mmio_write(writel_relaxed) <- u32);
define_write!(
fallible,
#[cfg(CONFIG_64BIT)]
pub try_write64_relaxed,
- writeq_relaxed <- u64
+ call_mmio_write(writeq_relaxed) <- u64
);
}
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v7 5/6] rust: pci: add config space read/write support
2025-11-19 11:21 [PATCH v7 0/6] rust: pci: add config space read/write support Zhi Wang
` (3 preceding siblings ...)
2025-11-19 11:21 ` [PATCH v7 4/6] rust: io: factor out MMIO read/write macros Zhi Wang
@ 2025-11-19 11:21 ` Zhi Wang
2025-11-25 14:20 ` Alexandre Courbot
2025-11-19 11:21 ` [PATCH v7 6/6] sample: rust: pci: add tests for config space routines Zhi Wang
5 siblings, 1 reply; 28+ messages in thread
From: Zhi Wang @ 2025-11-19 11:21 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang
Drivers might need to access PCI config space for querying capability
structures and access the registers inside the structures.
For Rust drivers need to access PCI config space, the Rust PCI abstraction
needs to support it in a way that upholds Rust's safety principles.
Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
accessors for PCI config space. The new type implements the `Io` trait to
share offset validation and bound-checking logic with others.
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
rust/kernel/pci.rs | 43 ++++++++++++++-
rust/kernel/pci/io.rs | 118 +++++++++++++++++++++++++++++++++++++++++-
2 files changed, 159 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 82e128431f08..f373413e8a84 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -40,7 +40,10 @@
ClassMask,
Vendor, //
};
-pub use self::io::Bar;
+pub use self::io::{
+ Bar,
+ ConfigSpace, //
+};
pub use self::irq::{
IrqType,
IrqTypes,
@@ -331,6 +334,30 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
}
}
+/// Represents the size of a PCI configuration space.
+///
+/// PCI devices can have either a *normal* (legacy) configuration space of 256 bytes,
+/// or an *extended* configuration space of 4096 bytes as defined in the PCI Express
+/// specification.
+#[repr(usize)]
+pub enum ConfigSpaceSize {
+ /// 256-byte legacy PCI configuration space.
+ Normal = 256,
+
+ /// 4096-byte PCIe extended configuration space.
+ Extended = 4096,
+}
+
+impl ConfigSpaceSize {
+ /// Get the raw value of this enum.
+ #[inline(always)]
+ pub const fn as_raw(self) -> usize {
+ // CAST: PCI configuration space size is at most 4096 bytes, so the value always fits
+ // within `usize` without truncation or sign change.
+ self as usize
+ }
+}
+
impl Device {
/// Returns the PCI vendor ID as [`Vendor`].
///
@@ -427,6 +454,20 @@ pub fn pci_class(&self) -> Class {
// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
Class::from_raw(unsafe { (*self.as_raw()).class })
}
+
+ /// Returns the size of configuration space.
+ fn cfg_size(&self) -> Result<ConfigSpaceSize> {
+ // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+ let size = unsafe { (*self.as_raw()).cfg_size };
+ match size {
+ 256 => Ok(ConfigSpaceSize::Normal),
+ 4096 => Ok(ConfigSpaceSize::Extended),
+ _ => {
+ debug_assert!(false);
+ Err(EINVAL)
+ }
+ }
+ }
}
impl Device<device::Core> {
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
index 2bbb3261198d..72a83d4b6ea4 100644
--- a/rust/kernel/pci/io.rs
+++ b/rust/kernel/pci/io.rs
@@ -2,12 +2,19 @@
//! PCI memory-mapped I/O infrastructure.
-use super::Device;
+use super::{
+ ConfigSpaceSize,
+ Device, //
+};
use crate::{
bindings,
device,
devres::Devres,
io::{
+ define_read,
+ define_write,
+ Io,
+ IoInfallible,
Mmio,
MmioRaw, //
},
@@ -16,6 +23,101 @@
};
use core::ops::Deref;
+/// The PCI configuration space of a device.
+///
+/// Provides typed read and write accessors for configuration registers
+/// using the standard `pci_read_config_*` and `pci_write_config_*` helpers.
+///
+/// The generic const parameter `SIZE` can be used to indicate the
+/// maximum size of the configuration space (e.g. 256 bytes for legacy,
+/// 4096 bytes for extended config space).
+pub struct ConfigSpace<'a, const SIZE: usize = { ConfigSpaceSize::Extended as usize }> {
+ pub(crate) pdev: &'a Device<device::Bound>,
+}
+
+/// Internal helper macros used to invoke C PCI configuration space read functions.
+///
+/// This macro is intended to be used by higher-level PCI configuration space access macros
+/// (define_read) and provides a unified expansion for infallible vs. fallible read semantics. It
+/// emits a direct call into the corresponding C helper and performs the required cast to the Rust
+/// return type.
+///
+/// # Parameters
+///
+/// * `$c_fn` – The C function performing the PCI configuration space write.
+/// * `$self` – The I/O backend object.
+/// * `$ty` – The type of the value to read.
+/// * `$addr` – The PCI configuration space offset to read.
+///
+/// This macro does not perform any validation; all invariants must be upheld by the higher-level
+/// abstraction invoking it.
+macro_rules! call_config_read {
+ (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) => {{
+ let mut val: $ty = 0;
+ // SAFETY: By the type invariant `$self.pdev` is a valid address.
+ // CAST: The offset is cast to `i32` because the C functions expect a 32-bit signed offset
+ // parameter. PCI configuration space size is at most 4096 bytes, so the value always fits
+ // within `i32` without truncation or sign change.
+ // Return value from C function is ignored in infallible accessors.
+ let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, &mut val) };
+ val
+ }};
+}
+
+/// Internal helper macros used to invoke C PCI configuration space write functions.
+///
+/// This macro is intended to be used by higher-level PCI configuration space access macros
+/// (define_write) and provides a unified expansion for infallible vs. fallible read semantics. It
+/// emits a direct call into the corresponding C helper and performs the required cast to the Rust
+/// return type.
+///
+/// # Parameters
+///
+/// * `$c_fn` – The C function performing the PCI configuration space write.
+/// * `$self` – The I/O backend object.
+/// * `$ty` – The type of the written value.
+/// * `$addr` – The configuration space offset to write.
+/// * `$value` – The value to write.
+///
+/// This macro does not perform any validation; all invariants must be upheld by the higher-level
+/// abstraction invoking it.
+macro_rules! call_config_write {
+ (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
+ // SAFETY: By the type invariant `$self.pdev` is a valid address.
+ // CAST: The offset is cast to `i32` because the C functions expect a 32-bit signed offset
+ // parameter. PCI configuration space size is at most 4096 bytes, so the value always fits
+ // within `i32` without truncation or sign change.
+ // Return value from C function is ignored in infallible accessors.
+ let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, $value) };
+ };
+}
+
+impl<'a, const SIZE: usize> Io for ConfigSpace<'a, SIZE> {
+ const MIN_SIZE: usize = SIZE;
+
+ /// Returns the base address of the I/O region. It is always 0 for configuration space.
+ #[inline]
+ fn addr(&self) -> usize {
+ 0
+ }
+
+ /// Returns the maximum size of the configuration space.
+ #[inline]
+ fn maxsize(&self) -> usize {
+ self.pdev.cfg_size().map_or(0, |v| v as usize)
+ }
+}
+
+impl<'a, const SIZE: usize> IoInfallible for ConfigSpace<'a, SIZE> {
+ define_read!(infallible, read8, call_config_read(pci_read_config_byte) -> u8);
+ define_read!(infallible, read16, call_config_read(pci_read_config_word) -> u16);
+ define_read!(infallible, read32, call_config_read(pci_read_config_dword) -> u32);
+
+ define_write!(infallible, write8, call_config_write(pci_write_config_byte) <- u8);
+ define_write!(infallible, write16, call_config_write(pci_write_config_word) <- u16);
+ define_write!(infallible, write32, call_config_write(pci_write_config_dword) <- u32);
+}
+
/// A PCI BAR to perform I/O-Operations on.
///
/// # Invariants
@@ -141,4 +243,18 @@ pub fn iomap_region<'a>(
) -> impl PinInit<Devres<Bar>, Error> + 'a {
self.iomap_region_sized::<0>(bar, name)
}
+
+ /// Return an initialized config space object.
+ pub fn config_space<'a>(
+ &'a self,
+ ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw() }>> {
+ Ok(ConfigSpace { pdev: self })
+ }
+
+ /// Return an initialized config space object.
+ pub fn config_space_extended<'a>(
+ &'a self,
+ ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
+ Ok(ConfigSpace { pdev: self })
+ }
}
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v7 6/6] sample: rust: pci: add tests for config space routines
2025-11-19 11:21 [PATCH v7 0/6] rust: pci: add config space read/write support Zhi Wang
` (4 preceding siblings ...)
2025-11-19 11:21 ` [PATCH v7 5/6] rust: pci: add config space read/write support Zhi Wang
@ 2025-11-19 11:21 ` Zhi Wang
5 siblings, 0 replies; 28+ messages in thread
From: Zhi Wang @ 2025-11-19 11:21 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang
Add tests exercising the PCI configuration space helpers.
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
samples/rust/rust_driver_pci.rs | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 74b93ca7c338..8f549b2100bc 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -67,6 +67,32 @@ fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
Ok(bar.read32(Regs::COUNT))
}
+
+ fn config_space(pdev: &pci::Device<Core>) -> Result {
+ let config = pdev.config_space()?;
+
+ // TODO: use the register!() macro for defining PCI configuration space registers once it
+ // has been move out of nova-core.
+ dev_info!(
+ pdev.as_ref(),
+ "pci-testdev config space read8 rev ID: {:x}\n",
+ config.read8(0x8)
+ );
+
+ dev_info!(
+ pdev.as_ref(),
+ "pci-testdev config space read16 vendor ID: {:x}\n",
+ config.read16(0)
+ );
+
+ dev_info!(
+ pdev.as_ref(),
+ "pci-testdev config space read32 BAR 0: {:x}\n",
+ config.read32(0x10)
+ );
+
+ Ok(())
+ }
}
impl pci::Driver for SampleDriver {
@@ -98,6 +124,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Er
"pci-testdev data-match count: {}\n",
Self::testdev(info, bar)?
);
+ Self::config_space(pdev)?;
},
pdev: pdev.into(),
}))
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-19 11:21 ` [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait Zhi Wang
@ 2025-11-21 14:20 ` Alice Ryhl
2025-11-24 10:08 ` Zhi Wang
2025-11-25 13:44 ` Alexandre Courbot
2025-11-25 14:09 ` Alexandre Courbot
1 sibling, 2 replies; 28+ messages in thread
From: Alice Ryhl @ 2025-11-21 14:20 UTC (permalink / raw)
To: Zhi Wang
Cc: rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf,
jhubbard, zhiwang
On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
> The previous Io<SIZE> type combined both the generic I/O access helpers
> and MMIO implementation details in a single struct.
>
> To establish a cleaner layering between the I/O interface and its concrete
> backends, paving the way for supporting additional I/O mechanisms in the
> future, Io<SIZE> need to be factored.
>
> Factor the common helpers into new {Io, Io64} traits, and move the
> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
> to use MmioRaw.
>
> No functional change intended.
>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
I said this on a previous version, but I still don't buy the split
into IoFallible and IoInfallible.
For one, we're never going to have a method that can accept any Io - we
will always want to accept either IoInfallible or IoFallible, so the
base Io trait serves no purpose.
For another, the docs explain that the distinction between them is
whether the bounds check is done at compile-time or runtime. That is not
the kind of capability one normally uses different traits to distinguish
between. It makes sense to have additional traits to distinguish
between e.g.:
* Whether IO ops can fail for reasons *other* than bounds checks.
* Whether 64-bit IO ops are possible.
Well ... I guess one could distinguish between whether it's possible to
check bounds at compile-time at all. But if you can check them at
compile-time, it should always be possible to check at runtime too, so
one should be a sub-trait of the other if you want to distinguish
them. (And then a trait name of KnownSizeIo would be more idiomatic.)
And I'm not really convinced that the current compile-time checked
traits are a good idea at all. See:
https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
If we want to have a compile-time checked trait, then the idiomatic way
to do that in Rust would be to have a new integer type that's guaranteed
to only contain integers <= the size. For example, the Bounded integer
being added elsewhere.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-21 14:20 ` Alice Ryhl
@ 2025-11-24 10:08 ` Zhi Wang
2025-11-24 10:20 ` Alice Ryhl
2025-11-25 13:44 ` Alexandre Courbot
1 sibling, 1 reply; 28+ messages in thread
From: Zhi Wang @ 2025-11-24 10:08 UTC (permalink / raw)
To: Alice Ryhl
Cc: rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf,
jhubbard, zhiwang
On Fri, 21 Nov 2025 14:20:13 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
> > The previous Io<SIZE> type combined both the generic I/O access
> > helpers and MMIO implementation details in a single struct.
> >
> > To establish a cleaner layering between the I/O interface and its
> > concrete backends, paving the way for supporting additional I/O
> > mechanisms in the future, Io<SIZE> need to be factored.
> >
> > Factor the common helpers into new {Io, Io64} traits, and move the
> > MMIO-specific logic into a dedicated Mmio<SIZE> type implementing
> > that trait. Rename the IoRaw to MmioRaw and update the bus MMIO
> > implementations to use MmioRaw.
> >
> > No functional change intended.
> >
> > Cc: Alexandre Courbot <acourbot@nvidia.com>
> > Cc: Alice Ryhl <aliceryhl@google.com>
> > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>
> I said this on a previous version, but I still don't buy the split
> into IoFallible and IoInfallible.
>
> For one, we're never going to have a method that can accept any Io -
> we will always want to accept either IoInfallible or IoFallible, so
> the base Io trait serves no purpose.
>
> For another, the docs explain that the distinction between them is
> whether the bounds check is done at compile-time or runtime. That is
> not the kind of capability one normally uses different traits to
> distinguish between. It makes sense to have additional traits to
> distinguish between e.g.:
>
> * Whether IO ops can fail for reasons *other* than bounds checks.
> * Whether 64-bit IO ops are possible.
>
> Well ... I guess one could distinguish between whether it's possible
> to check bounds at compile-time at all. But if you can check them at
> compile-time, it should always be possible to check at runtime too, so
> one should be a sub-trait of the other if you want to distinguish
> them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>
Thanks a lot for the detailed feedback. Agree with the points. Let's
keep the IoFallible and IoInfallible traits but not just tie them to the
bound checks in the docs.
> And I'm not really convinced that the current compile-time checked
> traits are a good idea at all. See:
> https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>
> If we want to have a compile-time checked trait, then the idiomatic
> way to do that in Rust would be to have a new integer type that's
> guaranteed to only contain integers <= the size. For example, the
> Bounded integer being added elsewhere.
>
Oops, this is a interesting bug. :) I think we can apply the bound
integer to IoFallible and IoInfallible to avoid possible problems
mentioned above. E.g. constructing a Bounded interger when constructing
Mmio and ConfigSpace objects and use them in the boundary checks in the
trait methods.
I saw Alex had already had an implementation of Bounded integer [1] in
rust-next. While my patchset is against driver-core-testing
branch. Would it be OK that we move on without it and switch to Bounded
integer when it is landed to driver-core-testing? I am open to
suggestions. :)
Z.
[1]
https://lore.kernel.org/all/CANiq72nV1zwoCCcHuizdfqWF=e8hvd6RO1CBXTEt73eqe4ayaA@mail.gmail.com/
> Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-24 10:08 ` Zhi Wang
@ 2025-11-24 10:20 ` Alice Ryhl
2025-11-24 13:32 ` Zhi Wang
0 siblings, 1 reply; 28+ messages in thread
From: Alice Ryhl @ 2025-11-24 10:20 UTC (permalink / raw)
To: Zhi Wang
Cc: rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf,
jhubbard, zhiwang
On Mon, Nov 24, 2025 at 12:08:46PM +0200, Zhi Wang wrote:
> On Fri, 21 Nov 2025 14:20:13 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
> > > The previous Io<SIZE> type combined both the generic I/O access
> > > helpers and MMIO implementation details in a single struct.
> > >
> > > To establish a cleaner layering between the I/O interface and its
> > > concrete backends, paving the way for supporting additional I/O
> > > mechanisms in the future, Io<SIZE> need to be factored.
> > >
> > > Factor the common helpers into new {Io, Io64} traits, and move the
> > > MMIO-specific logic into a dedicated Mmio<SIZE> type implementing
> > > that trait. Rename the IoRaw to MmioRaw and update the bus MMIO
> > > implementations to use MmioRaw.
> > >
> > > No functional change intended.
> > >
> > > Cc: Alexandre Courbot <acourbot@nvidia.com>
> > > Cc: Alice Ryhl <aliceryhl@google.com>
> > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > Cc: John Hubbard <jhubbard@nvidia.com>
> > > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> >
> > I said this on a previous version, but I still don't buy the split
> > into IoFallible and IoInfallible.
> >
> > For one, we're never going to have a method that can accept any Io -
> > we will always want to accept either IoInfallible or IoFallible, so
> > the base Io trait serves no purpose.
> >
> > For another, the docs explain that the distinction between them is
> > whether the bounds check is done at compile-time or runtime. That is
> > not the kind of capability one normally uses different traits to
> > distinguish between. It makes sense to have additional traits to
> > distinguish between e.g.:
> >
> > * Whether IO ops can fail for reasons *other* than bounds checks.
> > * Whether 64-bit IO ops are possible.
> >
> > Well ... I guess one could distinguish between whether it's possible
> > to check bounds at compile-time at all. But if you can check them at
> > compile-time, it should always be possible to check at runtime too, so
> > one should be a sub-trait of the other if you want to distinguish
> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >
>
> Thanks a lot for the detailed feedback. Agree with the points. Let's
> keep the IoFallible and IoInfallible traits but not just tie them to the
> bound checks in the docs.
What do you plan to write in the docs instead?
> > And I'm not really convinced that the current compile-time checked
> > traits are a good idea at all. See:
> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >
> > If we want to have a compile-time checked trait, then the idiomatic
> > way to do that in Rust would be to have a new integer type that's
> > guaranteed to only contain integers <= the size. For example, the
> > Bounded integer being added elsewhere.
> >
>
> Oops, this is a interesting bug. :) I think we can apply the bound
> integer to IoFallible and IoInfallible to avoid possible problems
> mentioned above. E.g. constructing a Bounded interger when constructing
> Mmio and ConfigSpace objects and use them in the boundary checks in the
> trait methods.
>
> I saw Alex had already had an implementation of Bounded integer [1] in
> rust-next. While my patchset is against driver-core-testing
> branch. Would it be OK that we move on without it and switch to Bounded
> integer when it is landed to driver-core-testing? I am open to
> suggestions. :)
The last -rc of this cycle is already out, so I don't think you need to
worry about branch issues - you won't land it in time for that.
But there is another problem: Bounded only supports the case where the
bound is a power of two, so I don't think it's usable here. You can have
Io regions whose size is not a power of two.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-24 10:20 ` Alice Ryhl
@ 2025-11-24 13:32 ` Zhi Wang
2025-11-24 13:53 ` Alexandre Courbot
0 siblings, 1 reply; 28+ messages in thread
From: Zhi Wang @ 2025-11-24 13:32 UTC (permalink / raw)
To: Alice Ryhl
Cc: rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf,
jhubbard, zhiwang
On Mon, 24 Nov 2025 10:20:41 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> On Mon, Nov 24, 2025 at 12:08:46PM +0200, Zhi Wang wrote:
> > On Fri, 21 Nov 2025 14:20:13 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
> > > > The previous Io<SIZE> type combined both the generic I/O access
> > > > helpers and MMIO implementation details in a single struct.
> > > >
> > > > To establish a cleaner layering between the I/O interface and
> > > > its concrete backends, paving the way for supporting additional
> > > > I/O mechanisms in the future, Io<SIZE> need to be factored.
> > > >
> > > > Factor the common helpers into new {Io, Io64} traits, and move
> > > > the MMIO-specific logic into a dedicated Mmio<SIZE> type
> > > > implementing that trait. Rename the IoRaw to MmioRaw and update
> > > > the bus MMIO implementations to use MmioRaw.
> > > >
> > > > No functional change intended.
> > > >
> > > > Cc: Alexandre Courbot <acourbot@nvidia.com>
> > > > Cc: Alice Ryhl <aliceryhl@google.com>
> > > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > > Cc: John Hubbard <jhubbard@nvidia.com>
> > > > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> > >
> > > I said this on a previous version, but I still don't buy the split
> > > into IoFallible and IoInfallible.
> > >
> > > For one, we're never going to have a method that can accept any
> > > Io - we will always want to accept either IoInfallible or
> > > IoFallible, so the base Io trait serves no purpose.
> > >
> > > For another, the docs explain that the distinction between them is
> > > whether the bounds check is done at compile-time or runtime. That
> > > is not the kind of capability one normally uses different traits
> > > to distinguish between. It makes sense to have additional traits
> > > to distinguish between e.g.:
> > >
> > > * Whether IO ops can fail for reasons *other* than bounds checks.
> > > * Whether 64-bit IO ops are possible.
> > >
> > > Well ... I guess one could distinguish between whether it's
> > > possible to check bounds at compile-time at all. But if you can
> > > check them at compile-time, it should always be possible to check
> > > at runtime too, so one should be a sub-trait of the other if you
> > > want to distinguish them. (And then a trait name of KnownSizeIo
> > > would be more idiomatic.)
> > >
> >
> > Thanks a lot for the detailed feedback. Agree with the points. Let's
> > keep the IoFallible and IoInfallible traits but not just tie them
> > to the bound checks in the docs.
>
> What do you plan to write in the docs instead?
>
What I understad according to the discussion:
1. Infallible vs Fallible:
- Infallible indicates the I/O operation can will not return error from
the API level, and doesn't guarentee the hardware status from device
level.
- Fallible indicates the I/O operation can return error from the
API level.
2. compiling-time check vs run-time check:
- driver specifies a known-valid-size I/O region, we go compiling-time
check (saves the cost of run-time check).
- driver is not able to specifiy a known-valid-size I/O region, we
should go run-time check.
For IoInfallible, I would write the doc as:
A trait for I/O accessors that are guaranteed to succeed at the API
level.
Implementations of this trait provide I/O operations that do
not return errors to the caller. From the perspective of the I/O
API, the I/O operation is always considered successful.
Note that this does *not* mean that the underlying device is guaranteed
to be in a healthy state. Hardware-specific exceptional states must be
detected and handled by the driver or subsystem managing the device.
For Iofallible,
A trait for I/O accessors that may return an error.
This trait represents I/O operations where the API can intentionally
return an error. The error typically reflects issues detected by the
subsystem.
> > > And I'm not really convinced that the current compile-time checked
> > > traits are a good idea at all. See:
> > > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
snip
> The last -rc of this cycle is already out, so I don't think you need
> to worry about branch issues - you won't land it in time for that.
>
I am mostly refering to the dependances if I have to implement this on
top of bounded integer on driver-core-testing.
>
> But there is another problem: Bounded only supports the case where the
> bound is a power of two, so I don't think it's usable here. You can
> have Io regions whose size is not a power of two.
Any suggestion on this? :) Should I implement something like
BoundedOffset? Also would like to hear some inputs from Danilo as well.
Z.
>
> Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-24 13:32 ` Zhi Wang
@ 2025-11-24 13:53 ` Alexandre Courbot
0 siblings, 0 replies; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-24 13:53 UTC (permalink / raw)
To: Zhi Wang, Alice Ryhl
Cc: rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf,
jhubbard, zhiwang
On Mon Nov 24, 2025 at 10:32 PM JST, Zhi Wang wrote:
<snip>
>> But there is another problem: Bounded only supports the case where the
>> bound is a power of two, so I don't think it's usable here. You can
>> have Io regions whose size is not a power of two.
>
> Any suggestion on this? :) Should I implement something like
> BoundedOffset? Also would like to hear some inputs from Danilo as well.
Bounded integers were written with bitfields in mind and that is
reflected in the current implementation, but provided one can devise a
way to check for upper and lower bounds that is simple and works for
both signed and unsigned ints, there should be little standing in the
way of making it more granular and accepting any valid range as the
bounds constraint.
If this proves useful here and we can keep things simple for the
bitfield use-case, then making them more flexible is definitely an
option! :)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-21 14:20 ` Alice Ryhl
2025-11-24 10:08 ` Zhi Wang
@ 2025-11-25 13:44 ` Alexandre Courbot
2025-11-25 14:58 ` Alice Ryhl
1 sibling, 1 reply; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-25 13:44 UTC (permalink / raw)
To: Alice Ryhl, Zhi Wang
Cc: rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf,
jhubbard, zhiwang
On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>> The previous Io<SIZE> type combined both the generic I/O access helpers
>> and MMIO implementation details in a single struct.
>>
>> To establish a cleaner layering between the I/O interface and its concrete
>> backends, paving the way for supporting additional I/O mechanisms in the
>> future, Io<SIZE> need to be factored.
>>
>> Factor the common helpers into new {Io, Io64} traits, and move the
>> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>> to use MmioRaw.
>>
>> No functional change intended.
>>
>> Cc: Alexandre Courbot <acourbot@nvidia.com>
>> Cc: Alice Ryhl <aliceryhl@google.com>
>> Cc: Bjorn Helgaas <helgaas@kernel.org>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>
> I said this on a previous version, but I still don't buy the split
> into IoFallible and IoInfallible.
>
> For one, we're never going to have a method that can accept any Io - we
> will always want to accept either IoInfallible or IoFallible, so the
> base Io trait serves no purpose.
>
> For another, the docs explain that the distinction between them is
> whether the bounds check is done at compile-time or runtime. That is not
> the kind of capability one normally uses different traits to distinguish
> between. It makes sense to have additional traits to distinguish
> between e.g.:
>
> * Whether IO ops can fail for reasons *other* than bounds checks.
> * Whether 64-bit IO ops are possible.
>
> Well ... I guess one could distinguish between whether it's possible to
> check bounds at compile-time at all. But if you can check them at
> compile-time, it should always be possible to check at runtime too, so
> one should be a sub-trait of the other if you want to distinguish
> them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>
> And I'm not really convinced that the current compile-time checked
> traits are a good idea at all. See:
> https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>
> If we want to have a compile-time checked trait, then the idiomatic way
> to do that in Rust would be to have a new integer type that's guaranteed
> to only contain integers <= the size. For example, the Bounded integer
> being added elsewhere.
Would that be so different from using an associated const value though?
IIUC the bounded integer type would play the same role, only slightly
differently - by that I mean that if the offset is expressed by an
expression that is not const (such as an indexed access), then the
bounded integer still needs to rely on `build_assert` to be built.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-19 11:21 ` [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait Zhi Wang
2025-11-21 14:20 ` Alice Ryhl
@ 2025-11-25 14:09 ` Alexandre Courbot
2025-11-25 14:14 ` Alexandre Courbot
1 sibling, 1 reply; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-25 14:09 UTC (permalink / raw)
To: Zhi Wang, rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang
On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
<snip>
> -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() }
> +/// Checks whether an access of type `U` at the given `offset`
> +/// is valid within this region.
> +#[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
> }
> +}
> +
> +/// Represents a region of I/O space of a fixed size.
> +///
> +/// Provides common helpers for offset validation and address
> +/// calculation on top of a base address and maximum size.
> +///
> +pub trait Io {
> + /// Minimum usable size of this region.
> + const MIN_SIZE: usize;
This associated constant should probably be part of `IoInfallible` -
otherwise what value should it take if some type only implement
`IoFallible`?
>
> /// Returns the base address of this mapping.
> - #[inline]
> - pub fn addr(&self) -> usize {
> - self.0.addr()
> - }
> + fn addr(&self) -> usize;
>
> /// 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
> - }
> - }
> + fn maxsize(&self) -> usize;
>
> + /// Returns the absolute I/O address for a given `offset`,
> + /// performing runtime bound checks.
> #[inline]
> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> - if !Self::offset_valid::<U>(offset, self.maxsize()) {
> + if !offset_valid::<U>(offset, self.maxsize()) {
> return Err(EINVAL);
> }
Similarly I cannot find any context where `maxsize` and `io_addr` are
used outside of `IoFallible`, hinting that these should be part of this
trait.
>
> @@ -239,50 +240,190 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> self.addr().checked_add(offset).ok_or(EINVAL)
> }
>
> + /// Returns the absolute I/O address for a given `offset`,
> + /// performing compile-time bound checks.
> #[inline]
> fn io_addr_assert<U>(&self, offset: usize) -> usize {
> - build_assert!(Self::offset_valid::<U>(offset, SIZE));
> + build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
>
> self.addr() + offset
> }
... and `io_addr_assert` is only used from `IoFallible`.
So if my gut feeling is correct, we can disband `Io` entirely and only
rely on `IoFallible` and `IoInfallible`, which is exactly what Alice
said in her review. It makes sense to me as well because as she
mentioned, users need to specify one or the other already if they want
to call I/O methods - so why keep part of their non-shared functionality
in another trait.
This design would also reflect the fact that they perform the same
checks, except one does them at compile-time and the other at runtime.
Another point that Alice mentioned is that if you can do the check at
compile-time, you should be able to do it at runtime as well, and some
(most) non-bus specific APIs will probably only expect an `IoFallible`.
For these cases, rather than imposing a hierarchy on the traits, I'd
suggest a e.g. `IoFallibleAdapter` that wraps a reference to a
`IoInfallible` and exposes the fallible API.
<snip>
> +impl<const SIZE: usize> Io for Mmio<SIZE> {
> + const MIN_SIZE: usize = SIZE;
> +
> + /// Returns the base address of this mapping.
> + #[inline]
> + fn addr(&self) -> usize {
> + self.0.addr()
> + }
> +
> + /// Returns the maximum size of this mapping.
> + #[inline]
> + fn maxsize(&self) -> usize {
> + self.0.maxsize()
> + }
Nit: when implementing a trait, you don't need to repeat the doccomment
of its methods - LSP will pick them from the source.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-25 14:09 ` Alexandre Courbot
@ 2025-11-25 14:14 ` Alexandre Courbot
2025-11-25 14:22 ` Alice Ryhl
0 siblings, 1 reply; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-25 14:14 UTC (permalink / raw)
To: Alexandre Courbot, Zhi Wang, rust-for-linux, linux-pci,
linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, joelagnelf, jhubbard, zhiwang
On Tue Nov 25, 2025 at 11:09 PM JST, Alexandre Courbot wrote:
> On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
> <snip>
>> -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() }
>> +/// Checks whether an access of type `U` at the given `offset`
>> +/// is valid within this region.
>> +#[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
>> }
>> +}
>> +
>> +/// Represents a region of I/O space of a fixed size.
>> +///
>> +/// Provides common helpers for offset validation and address
>> +/// calculation on top of a base address and maximum size.
>> +///
>> +pub trait Io {
>> + /// Minimum usable size of this region.
>> + const MIN_SIZE: usize;
>
> This associated constant should probably be part of `IoInfallible` -
> otherwise what value should it take if some type only implement
> `IoFallible`?
>
>>
>> /// Returns the base address of this mapping.
>> - #[inline]
>> - pub fn addr(&self) -> usize {
>> - self.0.addr()
>> - }
>> + fn addr(&self) -> usize;
>>
>> /// 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
>> - }
>> - }
>> + fn maxsize(&self) -> usize;
>>
>> + /// Returns the absolute I/O address for a given `offset`,
>> + /// performing runtime bound checks.
>> #[inline]
>> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> - if !Self::offset_valid::<U>(offset, self.maxsize()) {
>> + if !offset_valid::<U>(offset, self.maxsize()) {
>> return Err(EINVAL);
>> }
>
> Similarly I cannot find any context where `maxsize` and `io_addr` are
> used outside of `IoFallible`, hinting that these should be part of this
> trait.
>
>>
>> @@ -239,50 +240,190 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> self.addr().checked_add(offset).ok_or(EINVAL)
>> }
>>
>> + /// Returns the absolute I/O address for a given `offset`,
>> + /// performing compile-time bound checks.
>> #[inline]
>> fn io_addr_assert<U>(&self, offset: usize) -> usize {
>> - build_assert!(Self::offset_valid::<U>(offset, SIZE));
>> + build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
>>
>> self.addr() + offset
>> }
>
> ... and `io_addr_assert` is only used from `IoFallible`.
>
> So if my gut feeling is correct, we can disband `Io` entirely and only
... except that we can't due to `addr`, unless we find a better way to
provide this base. But even if we need to keep `Io`, the compile-time
and runtime members should be moved to their respective traits.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 5/6] rust: pci: add config space read/write support
2025-11-19 11:21 ` [PATCH v7 5/6] rust: pci: add config space read/write support Zhi Wang
@ 2025-11-25 14:20 ` Alexandre Courbot
0 siblings, 0 replies; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-25 14:20 UTC (permalink / raw)
To: Zhi Wang, rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang
On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
> Drivers might need to access PCI config space for querying capability
> structures and access the registers inside the structures.
>
> For Rust drivers need to access PCI config space, the Rust PCI abstraction
> needs to support it in a way that upholds Rust's safety principles.
>
> Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
> accessors for PCI config space. The new type implements the `Io` trait to
> share offset validation and bound-checking logic with others.
>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
> rust/kernel/pci.rs | 43 ++++++++++++++-
> rust/kernel/pci/io.rs | 118 +++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 159 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 82e128431f08..f373413e8a84 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -40,7 +40,10 @@
> ClassMask,
> Vendor, //
> };
> -pub use self::io::Bar;
> +pub use self::io::{
> + Bar,
> + ConfigSpace, //
> +};
> pub use self::irq::{
> IrqType,
> IrqTypes,
> @@ -331,6 +334,30 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
> }
> }
>
> +/// Represents the size of a PCI configuration space.
> +///
> +/// PCI devices can have either a *normal* (legacy) configuration space of 256 bytes,
> +/// or an *extended* configuration space of 4096 bytes as defined in the PCI Express
> +/// specification.
> +#[repr(usize)]
> +pub enum ConfigSpaceSize {
> + /// 256-byte legacy PCI configuration space.
> + Normal = 256,
> +
> + /// 4096-byte PCIe extended configuration space.
> + Extended = 4096,
> +}
> +
> +impl ConfigSpaceSize {
> + /// Get the raw value of this enum.
> + #[inline(always)]
> + pub const fn as_raw(self) -> usize {
> + // CAST: PCI configuration space size is at most 4096 bytes, so the value always fits
> + // within `usize` without truncation or sign change.
> + self as usize
While correct, an even more solid guarantee is the fact that `self` is
itself represented by a `usize`.
> + }
> +}
> +
> impl Device {
> /// Returns the PCI vendor ID as [`Vendor`].
> ///
> @@ -427,6 +454,20 @@ pub fn pci_class(&self) -> Class {
> // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> Class::from_raw(unsafe { (*self.as_raw()).class })
> }
> +
> + /// Returns the size of configuration space.
> + fn cfg_size(&self) -> Result<ConfigSpaceSize> {
> + // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> + let size = unsafe { (*self.as_raw()).cfg_size };
> + match size {
> + 256 => Ok(ConfigSpaceSize::Normal),
> + 4096 => Ok(ConfigSpaceSize::Extended),
> + _ => {
> + debug_assert!(false);
> + Err(EINVAL)
> + }
Should we implement `TryFrom` for `ConfigSpaceSize` and use it here?
(can't wait for the `TryFrom` derive macro to come and remove the need
for this kind of boilerplate!)
<snip>
> @@ -141,4 +243,18 @@ pub fn iomap_region<'a>(
> ) -> impl PinInit<Devres<Bar>, Error> + 'a {
> self.iomap_region_sized::<0>(bar, name)
> }
> +
> + /// Return an initialized config space object.
> + pub fn config_space<'a>(
> + &'a self,
> + ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw() }>> {
> + Ok(ConfigSpace { pdev: self })
> + }
> +
> + /// Return an initialized config space object.
> + pub fn config_space_extended<'a>(
> + &'a self,
> + ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
> + Ok(ConfigSpace { pdev: self })
> + }
> }
This still has the problem that one can create an extended config space
object even if the device's `cfg_size` is `256`. Both
`config_space` and `config_space_extended` should fail if `cfg_size`
returns an invalid value, and `config_space_extended` should also fail
if the returned size less than `ConfigSpaceSize::Extended`.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-25 14:14 ` Alexandre Courbot
@ 2025-11-25 14:22 ` Alice Ryhl
2025-11-25 14:46 ` Alexandre Courbot
0 siblings, 1 reply; 28+ messages in thread
From: Alice Ryhl @ 2025-11-25 14:22 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Tue, Nov 25, 2025 at 3:15 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> On Tue Nov 25, 2025 at 11:09 PM JST, Alexandre Courbot wrote:
> > On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
> > <snip>
> >> -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() }
> >> +/// Checks whether an access of type `U` at the given `offset`
> >> +/// is valid within this region.
> >> +#[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
> >> }
> >> +}
> >> +
> >> +/// Represents a region of I/O space of a fixed size.
> >> +///
> >> +/// Provides common helpers for offset validation and address
> >> +/// calculation on top of a base address and maximum size.
> >> +///
> >> +pub trait Io {
> >> + /// Minimum usable size of this region.
> >> + const MIN_SIZE: usize;
> >
> > This associated constant should probably be part of `IoInfallible` -
> > otherwise what value should it take if some type only implement
> > `IoFallible`?
> >
> >>
> >> /// Returns the base address of this mapping.
> >> - #[inline]
> >> - pub fn addr(&self) -> usize {
> >> - self.0.addr()
> >> - }
> >> + fn addr(&self) -> usize;
> >>
> >> /// 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
> >> - }
> >> - }
> >> + fn maxsize(&self) -> usize;
> >>
> >> + /// Returns the absolute I/O address for a given `offset`,
> >> + /// performing runtime bound checks.
> >> #[inline]
> >> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> >> - if !Self::offset_valid::<U>(offset, self.maxsize()) {
> >> + if !offset_valid::<U>(offset, self.maxsize()) {
> >> return Err(EINVAL);
> >> }
> >
> > Similarly I cannot find any context where `maxsize` and `io_addr` are
> > used outside of `IoFallible`, hinting that these should be part of this
> > trait.
> >
> >>
> >> @@ -239,50 +240,190 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> >> self.addr().checked_add(offset).ok_or(EINVAL)
> >> }
> >>
> >> + /// Returns the absolute I/O address for a given `offset`,
> >> + /// performing compile-time bound checks.
> >> #[inline]
> >> fn io_addr_assert<U>(&self, offset: usize) -> usize {
> >> - build_assert!(Self::offset_valid::<U>(offset, SIZE));
> >> + build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
> >>
> >> self.addr() + offset
> >> }
> >
> > ... and `io_addr_assert` is only used from `IoFallible`.
> >
> > So if my gut feeling is correct, we can disband `Io` entirely and only
>
> ... except that we can't due to `addr`, unless we find a better way to
> provide this base. But even if we need to keep `Io`, the compile-time
> and runtime members should be moved to their respective traits.
If you have IoInfallible depend on IoFallible, then you can place
`addr` on IoFallible.
(And I still think you should rename IoFallible to Io and IoInfallible
to IoKnownSize.)
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-25 14:22 ` Alice Ryhl
@ 2025-11-25 14:46 ` Alexandre Courbot
2025-11-25 19:19 ` John Hubbard
0 siblings, 1 reply; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-25 14:46 UTC (permalink / raw)
To: Alice Ryhl, Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Tue Nov 25, 2025 at 11:22 PM JST, Alice Ryhl wrote:
> On Tue, Nov 25, 2025 at 3:15 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> On Tue Nov 25, 2025 at 11:09 PM JST, Alexandre Courbot wrote:
>> > On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
>> > <snip>
>> >> -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() }
>> >> +/// Checks whether an access of type `U` at the given `offset`
>> >> +/// is valid within this region.
>> >> +#[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
>> >> }
>> >> +}
>> >> +
>> >> +/// Represents a region of I/O space of a fixed size.
>> >> +///
>> >> +/// Provides common helpers for offset validation and address
>> >> +/// calculation on top of a base address and maximum size.
>> >> +///
>> >> +pub trait Io {
>> >> + /// Minimum usable size of this region.
>> >> + const MIN_SIZE: usize;
>> >
>> > This associated constant should probably be part of `IoInfallible` -
>> > otherwise what value should it take if some type only implement
>> > `IoFallible`?
>> >
>> >>
>> >> /// Returns the base address of this mapping.
>> >> - #[inline]
>> >> - pub fn addr(&self) -> usize {
>> >> - self.0.addr()
>> >> - }
>> >> + fn addr(&self) -> usize;
>> >>
>> >> /// 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
>> >> - }
>> >> - }
>> >> + fn maxsize(&self) -> usize;
>> >>
>> >> + /// Returns the absolute I/O address for a given `offset`,
>> >> + /// performing runtime bound checks.
>> >> #[inline]
>> >> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> >> - if !Self::offset_valid::<U>(offset, self.maxsize()) {
>> >> + if !offset_valid::<U>(offset, self.maxsize()) {
>> >> return Err(EINVAL);
>> >> }
>> >
>> > Similarly I cannot find any context where `maxsize` and `io_addr` are
>> > used outside of `IoFallible`, hinting that these should be part of this
>> > trait.
>> >
>> >>
>> >> @@ -239,50 +240,190 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> >> self.addr().checked_add(offset).ok_or(EINVAL)
>> >> }
>> >>
>> >> + /// Returns the absolute I/O address for a given `offset`,
>> >> + /// performing compile-time bound checks.
>> >> #[inline]
>> >> fn io_addr_assert<U>(&self, offset: usize) -> usize {
>> >> - build_assert!(Self::offset_valid::<U>(offset, SIZE));
>> >> + build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
>> >>
>> >> self.addr() + offset
>> >> }
>> >
>> > ... and `io_addr_assert` is only used from `IoFallible`.
>> >
>> > So if my gut feeling is correct, we can disband `Io` entirely and only
>>
>> ... except that we can't due to `addr`, unless we find a better way to
>> provide this base. But even if we need to keep `Io`, the compile-time
>> and runtime members should be moved to their respective traits.
>
> If you have IoInfallible depend on IoFallible, then you can place
> `addr` on IoFallible.
Indeed. Maybe we could even make `IoInfallible` automatically
implemented, since it just needs to `unwrap_unchecked` the fallible
implementation if the range is valid.
> (And I still think you should rename IoFallible to Io and IoInfallible
> to IoKnownSize.)
Agreed, there are other reasons for I/O to fail than a bad index so this
should not be part of the name of these traits.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-25 13:44 ` Alexandre Courbot
@ 2025-11-25 14:58 ` Alice Ryhl
2025-11-26 0:43 ` Alexandre Courbot
2025-11-26 7:52 ` Alexandre Courbot
0 siblings, 2 replies; 28+ messages in thread
From: Alice Ryhl @ 2025-11-25 14:58 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
> >> The previous Io<SIZE> type combined both the generic I/O access helpers
> >> and MMIO implementation details in a single struct.
> >>
> >> To establish a cleaner layering between the I/O interface and its concrete
> >> backends, paving the way for supporting additional I/O mechanisms in the
> >> future, Io<SIZE> need to be factored.
> >>
> >> Factor the common helpers into new {Io, Io64} traits, and move the
> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
> >> to use MmioRaw.
> >>
> >> No functional change intended.
> >>
> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
> >> Cc: Alice Ryhl <aliceryhl@google.com>
> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
> >> Cc: Danilo Krummrich <dakr@kernel.org>
> >> Cc: John Hubbard <jhubbard@nvidia.com>
> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> >
> > I said this on a previous version, but I still don't buy the split
> > into IoFallible and IoInfallible.
> >
> > For one, we're never going to have a method that can accept any Io - we
> > will always want to accept either IoInfallible or IoFallible, so the
> > base Io trait serves no purpose.
> >
> > For another, the docs explain that the distinction between them is
> > whether the bounds check is done at compile-time or runtime. That is not
> > the kind of capability one normally uses different traits to distinguish
> > between. It makes sense to have additional traits to distinguish
> > between e.g.:
> >
> > * Whether IO ops can fail for reasons *other* than bounds checks.
> > * Whether 64-bit IO ops are possible.
> >
> > Well ... I guess one could distinguish between whether it's possible to
> > check bounds at compile-time at all. But if you can check them at
> > compile-time, it should always be possible to check at runtime too, so
> > one should be a sub-trait of the other if you want to distinguish
> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >
> > And I'm not really convinced that the current compile-time checked
> > traits are a good idea at all. See:
> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >
> > If we want to have a compile-time checked trait, then the idiomatic way
> > to do that in Rust would be to have a new integer type that's guaranteed
> > to only contain integers <= the size. For example, the Bounded integer
> > being added elsewhere.
>
> Would that be so different from using an associated const value though?
> IIUC the bounded integer type would play the same role, only slightly
> differently - by that I mean that if the offset is expressed by an
> expression that is not const (such as an indexed access), then the
> bounded integer still needs to rely on `build_assert` to be built.
I mean something like this:
trait Io {
const SIZE: usize;
fn write(&mut self, i: Bounded<Self::SIZE>);
}
You know that Bounded<SIZE> contains a number less than SIZE, so you
know it's in-bounds without any build_assert required.
Yes, if there's a constructor for Bounded that utilizes build_assert,
then you end up with a build_assert to create it. But I think in many
cases it's avoidable depending on where the index comes from.
For example if you iterate all indices 0..SIZE, there could be a way to
directly create Bounded<SIZE> values from an iterator. Or if the index
comes from an ioctl, then you probably runtime check the integer at the
ioctl entrypoint, in which case you want the runtime-checked
constructor.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-25 14:46 ` Alexandre Courbot
@ 2025-11-25 19:19 ` John Hubbard
0 siblings, 0 replies; 28+ messages in thread
From: John Hubbard @ 2025-11-25 19:19 UTC (permalink / raw)
To: Alexandre Courbot, Alice Ryhl
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, zhiwang
On 11/25/25 6:46 AM, Alexandre Courbot wrote:
> On Tue Nov 25, 2025 at 11:22 PM JST, Alice Ryhl wrote:
>> On Tue, Nov 25, 2025 at 3:15 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
...
>> If you have IoInfallible depend on IoFallible, then you can place
>> `addr` on IoFallible.
>
> Indeed. Maybe we could even make `IoInfallible` automatically
> implemented, since it just needs to `unwrap_unchecked` the fallible
> implementation if the range is valid.
>
>> (And I still think you should rename IoFallible to Io and IoInfallible
>> to IoKnownSize.)
>
> Agreed, there are other reasons for I/O to fail than a bad index so this
> should not be part of the name of these traits.
Great! That neatly fixes the naming problem that was bothering me about
Io*, too.
thanks,
--
John Hubbard
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-25 14:58 ` Alice Ryhl
@ 2025-11-26 0:43 ` Alexandre Courbot
2025-11-26 7:52 ` Alexandre Courbot
1 sibling, 0 replies; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-26 0:43 UTC (permalink / raw)
To: Alice Ryhl, Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>> >> The previous Io<SIZE> type combined both the generic I/O access helpers
>> >> and MMIO implementation details in a single struct.
>> >>
>> >> To establish a cleaner layering between the I/O interface and its concrete
>> >> backends, paving the way for supporting additional I/O mechanisms in the
>> >> future, Io<SIZE> need to be factored.
>> >>
>> >> Factor the common helpers into new {Io, Io64} traits, and move the
>> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>> >> to use MmioRaw.
>> >>
>> >> No functional change intended.
>> >>
>> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
>> >> Cc: Alice Ryhl <aliceryhl@google.com>
>> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
>> >> Cc: Danilo Krummrich <dakr@kernel.org>
>> >> Cc: John Hubbard <jhubbard@nvidia.com>
>> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>> >
>> > I said this on a previous version, but I still don't buy the split
>> > into IoFallible and IoInfallible.
>> >
>> > For one, we're never going to have a method that can accept any Io - we
>> > will always want to accept either IoInfallible or IoFallible, so the
>> > base Io trait serves no purpose.
>> >
>> > For another, the docs explain that the distinction between them is
>> > whether the bounds check is done at compile-time or runtime. That is not
>> > the kind of capability one normally uses different traits to distinguish
>> > between. It makes sense to have additional traits to distinguish
>> > between e.g.:
>> >
>> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> > * Whether 64-bit IO ops are possible.
>> >
>> > Well ... I guess one could distinguish between whether it's possible to
>> > check bounds at compile-time at all. But if you can check them at
>> > compile-time, it should always be possible to check at runtime too, so
>> > one should be a sub-trait of the other if you want to distinguish
>> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >
>> > And I'm not really convinced that the current compile-time checked
>> > traits are a good idea at all. See:
>> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >
>> > If we want to have a compile-time checked trait, then the idiomatic way
>> > to do that in Rust would be to have a new integer type that's guaranteed
>> > to only contain integers <= the size. For example, the Bounded integer
>> > being added elsewhere.
>>
>> Would that be so different from using an associated const value though?
>> IIUC the bounded integer type would play the same role, only slightly
>> differently - by that I mean that if the offset is expressed by an
>> expression that is not const (such as an indexed access), then the
>> bounded integer still needs to rely on `build_assert` to be built.
>
> I mean something like this:
>
> trait Io {
> const SIZE: usize;
> fn write(&mut self, i: Bounded<Self::SIZE>);
> }
>
> You know that Bounded<SIZE> contains a number less than SIZE, so you
> know it's in-bounds without any build_assert required.
>
> Yes, if there's a constructor for Bounded that utilizes build_assert,
> then you end up with a build_assert to create it. But I think in many
> cases it's avoidable depending on where the index comes from.
>
> For example if you iterate all indices 0..SIZE, there could be a way to
> directly create Bounded<SIZE> values from an iterator. Or if the index
> comes from an ioctl, then you probably runtime check the integer at the
> ioctl entrypoint, in which case you want the runtime-checked
> constructor.
Thanks for elaborating. I really like this idea.
You are right that is would drastically reduce the number of times we
need to rely on `build_assert`, as well as concentrating its use to a
single point (bounded int constructor) instead of having to sprinkle
extra invocations in the Io module.
Now I would also like to also keep the ability to define "integers which
only X LSBs represent the value", so I guess we could have a distinct
type for "integers within a lower and higher bound", since the latter
requires two generic constants vs one for the former.
Maybe we could derive the current `Bounded` and that new type from a
more generic "constrained integer" type, which constraint rule itself is
given as a generic argument? From this we could then easily build all
sort of funny things. That could turn into quite an undertaking though.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-25 14:58 ` Alice Ryhl
2025-11-26 0:43 ` Alexandre Courbot
@ 2025-11-26 7:52 ` Alexandre Courbot
2025-11-26 9:50 ` Alice Ryhl
1 sibling, 1 reply; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-26 7:52 UTC (permalink / raw)
To: Alice Ryhl, Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>> >> The previous Io<SIZE> type combined both the generic I/O access helpers
>> >> and MMIO implementation details in a single struct.
>> >>
>> >> To establish a cleaner layering between the I/O interface and its concrete
>> >> backends, paving the way for supporting additional I/O mechanisms in the
>> >> future, Io<SIZE> need to be factored.
>> >>
>> >> Factor the common helpers into new {Io, Io64} traits, and move the
>> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>> >> to use MmioRaw.
>> >>
>> >> No functional change intended.
>> >>
>> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
>> >> Cc: Alice Ryhl <aliceryhl@google.com>
>> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
>> >> Cc: Danilo Krummrich <dakr@kernel.org>
>> >> Cc: John Hubbard <jhubbard@nvidia.com>
>> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>> >
>> > I said this on a previous version, but I still don't buy the split
>> > into IoFallible and IoInfallible.
>> >
>> > For one, we're never going to have a method that can accept any Io - we
>> > will always want to accept either IoInfallible or IoFallible, so the
>> > base Io trait serves no purpose.
>> >
>> > For another, the docs explain that the distinction between them is
>> > whether the bounds check is done at compile-time or runtime. That is not
>> > the kind of capability one normally uses different traits to distinguish
>> > between. It makes sense to have additional traits to distinguish
>> > between e.g.:
>> >
>> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> > * Whether 64-bit IO ops are possible.
>> >
>> > Well ... I guess one could distinguish between whether it's possible to
>> > check bounds at compile-time at all. But if you can check them at
>> > compile-time, it should always be possible to check at runtime too, so
>> > one should be a sub-trait of the other if you want to distinguish
>> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >
>> > And I'm not really convinced that the current compile-time checked
>> > traits are a good idea at all. See:
>> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >
>> > If we want to have a compile-time checked trait, then the idiomatic way
>> > to do that in Rust would be to have a new integer type that's guaranteed
>> > to only contain integers <= the size. For example, the Bounded integer
>> > being added elsewhere.
>>
>> Would that be so different from using an associated const value though?
>> IIUC the bounded integer type would play the same role, only slightly
>> differently - by that I mean that if the offset is expressed by an
>> expression that is not const (such as an indexed access), then the
>> bounded integer still needs to rely on `build_assert` to be built.
>
> I mean something like this:
>
> trait Io {
> const SIZE: usize;
> fn write(&mut self, i: Bounded<Self::SIZE>);
> }
I have experimented a bit with this idea, and unfortunately expressing
`Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
not doable as of today.
Bounding an integer with an upper/lower bound also proves to be more
demanding than the current `Bounded` design. For the `MIN` and `MAX`
constants must be of the same type as the wrapped `T` type, which again
makes rustc unhappy ("the type of const parameters must not depend on
other generic parameters"). A workaround would be to use a macro to
define individual types for each integer type we want to support - or to
just limit this to `usize`.
But the requirement for generic_const_exprs makes this a non-starter I'm
afraid. :/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-26 7:52 ` Alexandre Courbot
@ 2025-11-26 9:50 ` Alice Ryhl
2025-11-26 13:37 ` Alexandre Courbot
0 siblings, 1 reply; 28+ messages in thread
From: Alice Ryhl @ 2025-11-26 9:50 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
> >> >> The previous Io<SIZE> type combined both the generic I/O access helpers
> >> >> and MMIO implementation details in a single struct.
> >> >>
> >> >> To establish a cleaner layering between the I/O interface and its concrete
> >> >> backends, paving the way for supporting additional I/O mechanisms in the
> >> >> future, Io<SIZE> need to be factored.
> >> >>
> >> >> Factor the common helpers into new {Io, Io64} traits, and move the
> >> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
> >> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
> >> >> to use MmioRaw.
> >> >>
> >> >> No functional change intended.
> >> >>
> >> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
> >> >> Cc: Alice Ryhl <aliceryhl@google.com>
> >> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
> >> >> Cc: Danilo Krummrich <dakr@kernel.org>
> >> >> Cc: John Hubbard <jhubbard@nvidia.com>
> >> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> >> >
> >> > I said this on a previous version, but I still don't buy the split
> >> > into IoFallible and IoInfallible.
> >> >
> >> > For one, we're never going to have a method that can accept any Io - we
> >> > will always want to accept either IoInfallible or IoFallible, so the
> >> > base Io trait serves no purpose.
> >> >
> >> > For another, the docs explain that the distinction between them is
> >> > whether the bounds check is done at compile-time or runtime. That is not
> >> > the kind of capability one normally uses different traits to distinguish
> >> > between. It makes sense to have additional traits to distinguish
> >> > between e.g.:
> >> >
> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
> >> > * Whether 64-bit IO ops are possible.
> >> >
> >> > Well ... I guess one could distinguish between whether it's possible to
> >> > check bounds at compile-time at all. But if you can check them at
> >> > compile-time, it should always be possible to check at runtime too, so
> >> > one should be a sub-trait of the other if you want to distinguish
> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >> >
> >> > And I'm not really convinced that the current compile-time checked
> >> > traits are a good idea at all. See:
> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >> >
> >> > If we want to have a compile-time checked trait, then the idiomatic way
> >> > to do that in Rust would be to have a new integer type that's guaranteed
> >> > to only contain integers <= the size. For example, the Bounded integer
> >> > being added elsewhere.
> >>
> >> Would that be so different from using an associated const value though?
> >> IIUC the bounded integer type would play the same role, only slightly
> >> differently - by that I mean that if the offset is expressed by an
> >> expression that is not const (such as an indexed access), then the
> >> bounded integer still needs to rely on `build_assert` to be built.
> >
> > I mean something like this:
> >
> > trait Io {
> > const SIZE: usize;
> > fn write(&mut self, i: Bounded<Self::SIZE>);
> > }
>
> I have experimented a bit with this idea, and unfortunately expressing
> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
> not doable as of today.
>
> Bounding an integer with an upper/lower bound also proves to be more
> demanding than the current `Bounded` design. For the `MIN` and `MAX`
> constants must be of the same type as the wrapped `T` type, which again
> makes rustc unhappy ("the type of const parameters must not depend on
> other generic parameters"). A workaround would be to use a macro to
> define individual types for each integer type we want to support - or to
> just limit this to `usize`.
>
> But the requirement for generic_const_exprs makes this a non-starter I'm
> afraid. :/
Can you try this?
trait Io {
type IdxInt: Int;
fn write(&mut self, i: Self::IdxInt);
}
then implementers would write:
impl Io for MyIo {
type IdxInt = Bounded<17>;
}
instead of:
impl Io for MyIo {
const SIZE = 17;
}
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-26 9:50 ` Alice Ryhl
@ 2025-11-26 13:37 ` Alexandre Courbot
2025-11-26 13:39 ` Alexandre Courbot
2025-12-01 11:57 ` Alexandre Courbot
0 siblings, 2 replies; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-26 13:37 UTC (permalink / raw)
To: Alice Ryhl, Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>> >> >> The previous Io<SIZE> type combined both the generic I/O access helpers
>> >> >> and MMIO implementation details in a single struct.
>> >> >>
>> >> >> To establish a cleaner layering between the I/O interface and its concrete
>> >> >> backends, paving the way for supporting additional I/O mechanisms in the
>> >> >> future, Io<SIZE> need to be factored.
>> >> >>
>> >> >> Factor the common helpers into new {Io, Io64} traits, and move the
>> >> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>> >> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>> >> >> to use MmioRaw.
>> >> >>
>> >> >> No functional change intended.
>> >> >>
>> >> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
>> >> >> Cc: Alice Ryhl <aliceryhl@google.com>
>> >> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
>> >> >> Cc: Danilo Krummrich <dakr@kernel.org>
>> >> >> Cc: John Hubbard <jhubbard@nvidia.com>
>> >> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>> >> >
>> >> > I said this on a previous version, but I still don't buy the split
>> >> > into IoFallible and IoInfallible.
>> >> >
>> >> > For one, we're never going to have a method that can accept any Io - we
>> >> > will always want to accept either IoInfallible or IoFallible, so the
>> >> > base Io trait serves no purpose.
>> >> >
>> >> > For another, the docs explain that the distinction between them is
>> >> > whether the bounds check is done at compile-time or runtime. That is not
>> >> > the kind of capability one normally uses different traits to distinguish
>> >> > between. It makes sense to have additional traits to distinguish
>> >> > between e.g.:
>> >> >
>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> >> > * Whether 64-bit IO ops are possible.
>> >> >
>> >> > Well ... I guess one could distinguish between whether it's possible to
>> >> > check bounds at compile-time at all. But if you can check them at
>> >> > compile-time, it should always be possible to check at runtime too, so
>> >> > one should be a sub-trait of the other if you want to distinguish
>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >> >
>> >> > And I'm not really convinced that the current compile-time checked
>> >> > traits are a good idea at all. See:
>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >> >
>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>> >> > to only contain integers <= the size. For example, the Bounded integer
>> >> > being added elsewhere.
>> >>
>> >> Would that be so different from using an associated const value though?
>> >> IIUC the bounded integer type would play the same role, only slightly
>> >> differently - by that I mean that if the offset is expressed by an
>> >> expression that is not const (such as an indexed access), then the
>> >> bounded integer still needs to rely on `build_assert` to be built.
>> >
>> > I mean something like this:
>> >
>> > trait Io {
>> > const SIZE: usize;
>> > fn write(&mut self, i: Bounded<Self::SIZE>);
>> > }
>>
>> I have experimented a bit with this idea, and unfortunately expressing
>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>> not doable as of today.
>>
>> Bounding an integer with an upper/lower bound also proves to be more
>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>> constants must be of the same type as the wrapped `T` type, which again
>> makes rustc unhappy ("the type of const parameters must not depend on
>> other generic parameters"). A workaround would be to use a macro to
>> define individual types for each integer type we want to support - or to
>> just limit this to `usize`.
>>
>> But the requirement for generic_const_exprs makes this a non-starter I'm
>> afraid. :/
>
> Can you try this?
>
> trait Io {
> type IdxInt: Int;
> fn write(&mut self, i: Self::IdxInt);
> }
>
> then implementers would write:
>
> impl Io for MyIo {
> type IdxInt = Bounded<17>;
> }
>
> instead of:
> impl Io for MyIo {
> const SIZE = 17;
> }
The following builds (using the existing `Bounded` type for
demonstration purposes):
trait Io {
// Type containing an index guaranteed to be valid for this IO.
type IdxInt: Into<usize>;
fn write(&mut self, i: Self::IdxInt);
}
struct FooIo;
impl Io for FooIo {
type IdxInt = Bounded<usize, 8>;
fn write(&mut self, i: Self::IdxInt) {
let idx: usize = i.into();
// Now do the IO knowing that `idx` is a valid index.
}
}
That looks promising, and I like how we can effectively use a wider set
of index types - even, say, a `u16` if a particular I/O happens to have
a guaranteed size of 65536!
I suspect it also changes how we would design the Io interfaces, but I
am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
either unwrapping the result of its fallible methods or using some
`unchecked` accessors?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-26 13:37 ` Alexandre Courbot
@ 2025-11-26 13:39 ` Alexandre Courbot
2025-12-01 11:57 ` Alexandre Courbot
1 sibling, 0 replies; 28+ messages in thread
From: Alexandre Courbot @ 2025-11-26 13:39 UTC (permalink / raw)
To: Alexandre Courbot, Alice Ryhl
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
> On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
>> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>>> >> >> The previous Io<SIZE> type combined both the generic I/O access helpers
>>> >> >> and MMIO implementation details in a single struct.
>>> >> >>
>>> >> >> To establish a cleaner layering between the I/O interface and its concrete
>>> >> >> backends, paving the way for supporting additional I/O mechanisms in the
>>> >> >> future, Io<SIZE> need to be factored.
>>> >> >>
>>> >> >> Factor the common helpers into new {Io, Io64} traits, and move the
>>> >> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>>> >> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>>> >> >> to use MmioRaw.
>>> >> >>
>>> >> >> No functional change intended.
>>> >> >>
>>> >> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
>>> >> >> Cc: Alice Ryhl <aliceryhl@google.com>
>>> >> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
>>> >> >> Cc: Danilo Krummrich <dakr@kernel.org>
>>> >> >> Cc: John Hubbard <jhubbard@nvidia.com>
>>> >> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>>> >> >
>>> >> > I said this on a previous version, but I still don't buy the split
>>> >> > into IoFallible and IoInfallible.
>>> >> >
>>> >> > For one, we're never going to have a method that can accept any Io - we
>>> >> > will always want to accept either IoInfallible or IoFallible, so the
>>> >> > base Io trait serves no purpose.
>>> >> >
>>> >> > For another, the docs explain that the distinction between them is
>>> >> > whether the bounds check is done at compile-time or runtime. That is not
>>> >> > the kind of capability one normally uses different traits to distinguish
>>> >> > between. It makes sense to have additional traits to distinguish
>>> >> > between e.g.:
>>> >> >
>>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>>> >> > * Whether 64-bit IO ops are possible.
>>> >> >
>>> >> > Well ... I guess one could distinguish between whether it's possible to
>>> >> > check bounds at compile-time at all. But if you can check them at
>>> >> > compile-time, it should always be possible to check at runtime too, so
>>> >> > one should be a sub-trait of the other if you want to distinguish
>>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>>> >> >
>>> >> > And I'm not really convinced that the current compile-time checked
>>> >> > traits are a good idea at all. See:
>>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>>> >> >
>>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>>> >> > to only contain integers <= the size. For example, the Bounded integer
>>> >> > being added elsewhere.
>>> >>
>>> >> Would that be so different from using an associated const value though?
>>> >> IIUC the bounded integer type would play the same role, only slightly
>>> >> differently - by that I mean that if the offset is expressed by an
>>> >> expression that is not const (such as an indexed access), then the
>>> >> bounded integer still needs to rely on `build_assert` to be built.
>>> >
>>> > I mean something like this:
>>> >
>>> > trait Io {
>>> > const SIZE: usize;
>>> > fn write(&mut self, i: Bounded<Self::SIZE>);
>>> > }
>>>
>>> I have experimented a bit with this idea, and unfortunately expressing
>>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>>> not doable as of today.
>>>
>>> Bounding an integer with an upper/lower bound also proves to be more
>>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>>> constants must be of the same type as the wrapped `T` type, which again
>>> makes rustc unhappy ("the type of const parameters must not depend on
>>> other generic parameters"). A workaround would be to use a macro to
>>> define individual types for each integer type we want to support - or to
>>> just limit this to `usize`.
>>>
>>> But the requirement for generic_const_exprs makes this a non-starter I'm
>>> afraid. :/
>>
>> Can you try this?
>>
>> trait Io {
>> type IdxInt: Int;
>> fn write(&mut self, i: Self::IdxInt);
>> }
>>
>> then implementers would write:
>>
>> impl Io for MyIo {
>> type IdxInt = Bounded<17>;
>> }
>>
>> instead of:
>> impl Io for MyIo {
>> const SIZE = 17;
>> }
>
> The following builds (using the existing `Bounded` type for
> demonstration purposes):
>
> trait Io {
> // Type containing an index guaranteed to be valid for this IO.
> type IdxInt: Into<usize>;
>
> fn write(&mut self, i: Self::IdxInt);
> }
>
> struct FooIo;
>
> impl Io for FooIo {
> type IdxInt = Bounded<usize, 8>;
>
> fn write(&mut self, i: Self::IdxInt) {
> let idx: usize = i.into();
>
> // Now do the IO knowing that `idx` is a valid index.
> }
> }
>
> That looks promising, and I like how we can effectively use a wider set
> of index types - even, say, a `u16` if a particular I/O happens to have
> a guaranteed size of 65536!
>
> I suspect it also changes how we would design the Io interfaces, but I
> am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
> either unwrapping the result of its fallible methods or using some
> `unchecked` accessors?
That last sentence was ambiguous - for it to make sense, please rename
`Io` to `IoKnownSize` in the code sample above.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-11-26 13:37 ` Alexandre Courbot
2025-11-26 13:39 ` Alexandre Courbot
@ 2025-12-01 11:57 ` Alexandre Courbot
2025-12-01 12:23 ` Alice Ryhl
1 sibling, 1 reply; 28+ messages in thread
From: Alexandre Courbot @ 2025-12-01 11:57 UTC (permalink / raw)
To: Zhi Wang, Alice Ryhl
Cc: rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
> On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
>> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>>> >> >> The previous Io<SIZE> type combined both the generic I/O access helpers
>>> >> >> and MMIO implementation details in a single struct.
>>> >> >>
>>> >> >> To establish a cleaner layering between the I/O interface and its concrete
>>> >> >> backends, paving the way for supporting additional I/O mechanisms in the
>>> >> >> future, Io<SIZE> need to be factored.
>>> >> >>
>>> >> >> Factor the common helpers into new {Io, Io64} traits, and move the
>>> >> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>>> >> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>>> >> >> to use MmioRaw.
>>> >> >>
>>> >> >> No functional change intended.
>>> >> >>
>>> >> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
>>> >> >> Cc: Alice Ryhl <aliceryhl@google.com>
>>> >> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
>>> >> >> Cc: Danilo Krummrich <dakr@kernel.org>
>>> >> >> Cc: John Hubbard <jhubbard@nvidia.com>
>>> >> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>>> >> >
>>> >> > I said this on a previous version, but I still don't buy the split
>>> >> > into IoFallible and IoInfallible.
>>> >> >
>>> >> > For one, we're never going to have a method that can accept any Io - we
>>> >> > will always want to accept either IoInfallible or IoFallible, so the
>>> >> > base Io trait serves no purpose.
>>> >> >
>>> >> > For another, the docs explain that the distinction between them is
>>> >> > whether the bounds check is done at compile-time or runtime. That is not
>>> >> > the kind of capability one normally uses different traits to distinguish
>>> >> > between. It makes sense to have additional traits to distinguish
>>> >> > between e.g.:
>>> >> >
>>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>>> >> > * Whether 64-bit IO ops are possible.
>>> >> >
>>> >> > Well ... I guess one could distinguish between whether it's possible to
>>> >> > check bounds at compile-time at all. But if you can check them at
>>> >> > compile-time, it should always be possible to check at runtime too, so
>>> >> > one should be a sub-trait of the other if you want to distinguish
>>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>>> >> >
>>> >> > And I'm not really convinced that the current compile-time checked
>>> >> > traits are a good idea at all. See:
>>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>>> >> >
>>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>>> >> > to only contain integers <= the size. For example, the Bounded integer
>>> >> > being added elsewhere.
>>> >>
>>> >> Would that be so different from using an associated const value though?
>>> >> IIUC the bounded integer type would play the same role, only slightly
>>> >> differently - by that I mean that if the offset is expressed by an
>>> >> expression that is not const (such as an indexed access), then the
>>> >> bounded integer still needs to rely on `build_assert` to be built.
>>> >
>>> > I mean something like this:
>>> >
>>> > trait Io {
>>> > const SIZE: usize;
>>> > fn write(&mut self, i: Bounded<Self::SIZE>);
>>> > }
>>>
>>> I have experimented a bit with this idea, and unfortunately expressing
>>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>>> not doable as of today.
>>>
>>> Bounding an integer with an upper/lower bound also proves to be more
>>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>>> constants must be of the same type as the wrapped `T` type, which again
>>> makes rustc unhappy ("the type of const parameters must not depend on
>>> other generic parameters"). A workaround would be to use a macro to
>>> define individual types for each integer type we want to support - or to
>>> just limit this to `usize`.
>>>
>>> But the requirement for generic_const_exprs makes this a non-starter I'm
>>> afraid. :/
>>
>> Can you try this?
>>
>> trait Io {
>> type IdxInt: Int;
>> fn write(&mut self, i: Self::IdxInt);
>> }
>>
>> then implementers would write:
>>
>> impl Io for MyIo {
>> type IdxInt = Bounded<17>;
>> }
>>
>> instead of:
>> impl Io for MyIo {
>> const SIZE = 17;
>> }
>
> The following builds (using the existing `Bounded` type for
> demonstration purposes):
>
> trait Io {
> // Type containing an index guaranteed to be valid for this IO.
> type IdxInt: Into<usize>;
>
> fn write(&mut self, i: Self::IdxInt);
> }
>
> struct FooIo;
>
> impl Io for FooIo {
> type IdxInt = Bounded<usize, 8>;
>
> fn write(&mut self, i: Self::IdxInt) {
> let idx: usize = i.into();
>
> // Now do the IO knowing that `idx` is a valid index.
> }
> }
>
> That looks promising, and I like how we can effectively use a wider set
> of index types - even, say, a `u16` if a particular I/O happens to have
> a guaranteed size of 65536!
>
> I suspect it also changes how we would design the Io interfaces, but I
> am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
> either unwrapping the result of its fallible methods or using some
> `unchecked` accessors?
Mmm, one important point I have neglected is that the index type will
have to validate not only the range, but also the alignment of the
index! And the valid alignment is dependent on the access width. So
getting this right is going to take some time and some experimenting I'm
afraid.
Meanwhile, it would be great if we could agree (and proceed) with the
split of the I/O interface into a trait, as other work depends on it.
Changing the index type of compile-time checked bounds is I think an
improvement that is orthogonal to this task.
I have been thinking a bit (too much? ^_^;) about the general design for
this interface, how it would work with the `register!` macro, and how we
could maybe limit the boilerplate. Sorry in advance for this is going to
be a long post.
IIUC there are several aspects we need to tackle with the I/O interface:
- Raw IO access. Given an address, perform the IO operation without any
check. Depending on the bus, this might return the data directly (e.g.
`Mmio`), or a `Result` (e.g. the PCI `ConfigSpace`). The current
implementation ignores the bus error, which we probably shouldn't.
Also the raw access is reimplemented twice, in both the build-time and
runtime accessors, a fact that is mostly hidden by the use of macros.
- Access with dynamic bounds check. This can return `EINVAL` if the
provided index is invalid (out-of-bounds or not aligned), on top of
the bus errors, if any.
- Access with build-time index check. Same as above, but the error
occurs at build time if the index is invalid. Otherwise the return
type of the raw IO accessor is returned.
At the moment we have two traits for build-time and runtime index
checks. What bothers me a bit about them is that they basically
re-implement the same raw I/O accessors. This strongly hints that we
should implement the raw accessors as a base trait, which the
user-facing API would call into:
pub trait Io {
/// Error type returned by IO accessors. Can be `Infallible` for e.g. `Mmio`.
type Error: Into<Error>;
/// Returns the base address of this mapping.
fn addr(&self) -> usize;
/// Returns the maximum size of this mapping.
fn maxsize(&self) -> usize;
unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error>;
unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error>;
// etc. for 16/32 bits accessors.
}
Then we could build the current `IoFallible` trait on top of it:
pub trait IoFallible: Io {
fn io_addr<U>(&self, offset: usize) -> Result<usize> {
if !offset_valid::<U>(offset, self.maxsize()) {
return Err(EINVAL);
}
self.addr().checked_add(offset).ok_or(EINVAL)
}
/// 8-bit read with runtime bounds check.
fn try_read8_checked(&self, offset: usize) -> Result<u8> {
let addr = self.io_addr::<u8>(offset)?;
unsafe { self.try_read8_unchecked(addr) }.map_err(Into::into)
}
/// 8-bit write with runtime bounds check.
fn try_write8_checked(&self, value: u8, offset: usize) -> Result {
let addr = self.io_addr::<u8>(offset)?;
unsafe { self.try_write8_unchecked(value, addr) }.map_err(Into::into)
}
}
Note how this trait is now auto-implemented. Making it available for all
implementers of `Io` is as simple as:
impl<IO: Io> IoFallible for IO {}
(... which hints that maybe it should simply be folded into `Io`, as
Alice previously suggested)
`IoKnownSize` also calls into the base `Io` trait:
/// Trait for IO with a build-time known valid range.
pub unsafe trait IoKnownSize: Io {
/// Minimum usable size of this region.
const MIN_SIZE: usize;
#[inline(always)]
fn io_addr_assert<U>(&self, offset: usize) -> usize {
build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
self.addr() + offset
}
/// 8-bit read with compile-time bounds check.
#[inline(always)]
fn try_read8(&self, offset: usize) -> Result<u8, Self::Error> {
let addr = self.io_addr_assert::<u8>(offset);
unsafe { self.try_read8_unchecked(addr) }
}
/// 8-bit write with compile-time bounds check.
#[inline(always)]
fn try_write8(&self, value: u8, offset: usize) -> Result<(), Self::Error> {
let addr = self.io_addr_assert::<u8>(offset);
unsafe { self.try_write8_unchecked(value, addr) }
}
}
Its accessors now return the error type of the bus, which is good for
safety, but not for ergonomics when dealing with e.g. code that works
with `Mmio`, which we know is infallible. But we can provide an extra
set of methods in this trait for this case:
/// Infallible 8-bit read with compile-time bounds check.
#[inline(always)]
fn read8(&self, offset: usize) -> u8
where
Self: Io<Error = Infallible>,
{
self.read8(offset).unwrap_or_else(|e| match e {})
}
/// Infallible 8-bit write with compile-time bounds check.
#[inline(always)]
fn write8(&self, value: u8, offset: usize)
where
Self: Io<Error = Infallible>,
{
self.write8(value, offset).unwrap_or_else(|e| match e {})
}
`Mmio`'s impl blocks are now reduced to the following:
impl<const SIZE: usize> Io for Mmio<SIZE> {
type Error = core::convert::Infallible;
#[inline]
fn addr(&self) -> usize {
self.0.addr()
}
#[inline]
fn maxsize(&self) -> usize {
self.0.maxsize()
}
unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
Ok(unsafe { bindings::readb(addr as *const c_void) })
}
unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
Ok(unsafe { bindings::writeb(value, addr as *mut c_void) })
}
}
unsafe impl<const SIZE: usize> IoKnownSize for Mmio<SIZE> {
const MIN_SIZE: usize = SIZE;
}
... and that's enough to provide everything we had so far - all of the
accessors called by users are already implemented in the base traits.
Note also the lack of macros.
Another point that I noticed was the relaxed MMIO accessors. They are
currently implemented as a set of dedicated methods (e.g.
`read8_relaxed`) that are not part of a trait. This results in a lot of
additional methods, and limits their usefulness as the same generic
function could not be used with both regular and relaxed accesses.
So I'd propose to implement them using a relaxed wrapper type:
/// Wrapper for [`Mmio`] that performs relaxed I/O accesses.
pub struct RelaxedMmio<'a, const SIZE: usize>(&'a Mmio<SIZE>);
impl<'a, const SIZE: usize> RelaxedMmio<'a, SIZE> {
pub fn new(mmio: &'a Mmio<SIZE>) -> Self {
Self(mmio)
}
}
impl<'a, const SIZE: usize> Io for RelaxedMmio<'a, SIZE> {
fn addr(&self) -> usize {
self.0.addr()
}
fn maxsize(&self) -> usize {
self.0.maxsize()
}
type Error = <Mmio as Io>::Error;
unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
Ok(unsafe { bindings::readb_relaxed(addr as *const c_void) })
}
unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
Ok(unsafe { bindings::writeb_relaxed(value, addr as *mut c_void) })
}
// SAFETY: `MIN_SIZE` is the same as the wrapped type, which also implements `IoKnownSize`.
unsafe impl<'a, const SIZE: usize> IoKnownSize for RelaxedMmio<'a, SIZE> {
const MIN_SIZE: usize = SIZE;
}
}
This way, when you need to do I/O using a register, you can either pass
the `Mmio` instance or derive a `RelaxedMmio` from it, if that access
pattern is more adequate.
How does this sound? I can share a branch with a basic implementation
of this idea if that helps.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-12-01 11:57 ` Alexandre Courbot
@ 2025-12-01 12:23 ` Alice Ryhl
2025-12-03 13:32 ` Alexandre Courbot
0 siblings, 1 reply; 28+ messages in thread
From: Alice Ryhl @ 2025-12-01 12:23 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Mon, Dec 01, 2025 at 08:57:09PM +0900, Alexandre Courbot wrote:
> On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
> > On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
> >> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
> >>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> >>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
> >>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> >>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
> >>> >> >> The previous Io<SIZE> type combined both the generic I/O access helpers
> >>> >> >> and MMIO implementation details in a single struct.
> >>> >> >>
> >>> >> >> To establish a cleaner layering between the I/O interface and its concrete
> >>> >> >> backends, paving the way for supporting additional I/O mechanisms in the
> >>> >> >> future, Io<SIZE> need to be factored.
> >>> >> >>
> >>> >> >> Factor the common helpers into new {Io, Io64} traits, and move the
> >>> >> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
> >>> >> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
> >>> >> >> to use MmioRaw.
> >>> >> >>
> >>> >> >> No functional change intended.
> >>> >> >>
> >>> >> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
> >>> >> >> Cc: Alice Ryhl <aliceryhl@google.com>
> >>> >> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
> >>> >> >> Cc: Danilo Krummrich <dakr@kernel.org>
> >>> >> >> Cc: John Hubbard <jhubbard@nvidia.com>
> >>> >> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> >>> >> >
> >>> >> > I said this on a previous version, but I still don't buy the split
> >>> >> > into IoFallible and IoInfallible.
> >>> >> >
> >>> >> > For one, we're never going to have a method that can accept any Io - we
> >>> >> > will always want to accept either IoInfallible or IoFallible, so the
> >>> >> > base Io trait serves no purpose.
> >>> >> >
> >>> >> > For another, the docs explain that the distinction between them is
> >>> >> > whether the bounds check is done at compile-time or runtime. That is not
> >>> >> > the kind of capability one normally uses different traits to distinguish
> >>> >> > between. It makes sense to have additional traits to distinguish
> >>> >> > between e.g.:
> >>> >> >
> >>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
> >>> >> > * Whether 64-bit IO ops are possible.
> >>> >> >
> >>> >> > Well ... I guess one could distinguish between whether it's possible to
> >>> >> > check bounds at compile-time at all. But if you can check them at
> >>> >> > compile-time, it should always be possible to check at runtime too, so
> >>> >> > one should be a sub-trait of the other if you want to distinguish
> >>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >>> >> >
> >>> >> > And I'm not really convinced that the current compile-time checked
> >>> >> > traits are a good idea at all. See:
> >>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >>> >> >
> >>> >> > If we want to have a compile-time checked trait, then the idiomatic way
> >>> >> > to do that in Rust would be to have a new integer type that's guaranteed
> >>> >> > to only contain integers <= the size. For example, the Bounded integer
> >>> >> > being added elsewhere.
> >>> >>
> >>> >> Would that be so different from using an associated const value though?
> >>> >> IIUC the bounded integer type would play the same role, only slightly
> >>> >> differently - by that I mean that if the offset is expressed by an
> >>> >> expression that is not const (such as an indexed access), then the
> >>> >> bounded integer still needs to rely on `build_assert` to be built.
> >>> >
> >>> > I mean something like this:
> >>> >
> >>> > trait Io {
> >>> > const SIZE: usize;
> >>> > fn write(&mut self, i: Bounded<Self::SIZE>);
> >>> > }
> >>>
> >>> I have experimented a bit with this idea, and unfortunately expressing
> >>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
> >>> not doable as of today.
> >>>
> >>> Bounding an integer with an upper/lower bound also proves to be more
> >>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
> >>> constants must be of the same type as the wrapped `T` type, which again
> >>> makes rustc unhappy ("the type of const parameters must not depend on
> >>> other generic parameters"). A workaround would be to use a macro to
> >>> define individual types for each integer type we want to support - or to
> >>> just limit this to `usize`.
> >>>
> >>> But the requirement for generic_const_exprs makes this a non-starter I'm
> >>> afraid. :/
> >>
> >> Can you try this?
> >>
> >> trait Io {
> >> type IdxInt: Int;
> >> fn write(&mut self, i: Self::IdxInt);
> >> }
> >>
> >> then implementers would write:
> >>
> >> impl Io for MyIo {
> >> type IdxInt = Bounded<17>;
> >> }
> >>
> >> instead of:
> >> impl Io for MyIo {
> >> const SIZE = 17;
> >> }
> >
> > The following builds (using the existing `Bounded` type for
> > demonstration purposes):
> >
> > trait Io {
> > // Type containing an index guaranteed to be valid for this IO.
> > type IdxInt: Into<usize>;
> >
> > fn write(&mut self, i: Self::IdxInt);
> > }
> >
> > struct FooIo;
> >
> > impl Io for FooIo {
> > type IdxInt = Bounded<usize, 8>;
> >
> > fn write(&mut self, i: Self::IdxInt) {
> > let idx: usize = i.into();
> >
> > // Now do the IO knowing that `idx` is a valid index.
> > }
> > }
> >
> > That looks promising, and I like how we can effectively use a wider set
> > of index types - even, say, a `u16` if a particular I/O happens to have
> > a guaranteed size of 65536!
> >
> > I suspect it also changes how we would design the Io interfaces, but I
> > am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
> > either unwrapping the result of its fallible methods or using some
> > `unchecked` accessors?
>
> Mmm, one important point I have neglected is that the index type will
> have to validate not only the range, but also the alignment of the
> index! And the valid alignment is dependent on the access width. So
> getting this right is going to take some time and some experimenting I'm
> afraid.
>
> Meanwhile, it would be great if we could agree (and proceed) with the
> split of the I/O interface into a trait, as other work depends on it.
> Changing the index type of compile-time checked bounds is I think an
> improvement that is orthogonal to this task.
> I have been thinking a bit (too much? ^_^;) about the general design for
> this interface, how it would work with the `register!` macro, and how we
> could maybe limit the boilerplate. Sorry in advance for this is going to
> be a long post.
>
> IIUC there are several aspects we need to tackle with the I/O interface:
>
> - Raw IO access. Given an address, perform the IO operation without any
> check. Depending on the bus, this might return the data directly (e.g.
> `Mmio`), or a `Result` (e.g. the PCI `ConfigSpace`). The current
> implementation ignores the bus error, which we probably shouldn't.
> Also the raw access is reimplemented twice, in both the build-time and
> runtime accessors, a fact that is mostly hidden by the use of macros.
> - Access with dynamic bounds check. This can return `EINVAL` if the
> provided index is invalid (out-of-bounds or not aligned), on top of
> the bus errors, if any.
> - Access with build-time index check. Same as above, but the error
> occurs at build time if the index is invalid. Otherwise the return
> type of the raw IO accessor is returned.
>
> At the moment we have two traits for build-time and runtime index
> checks. What bothers me a bit about them is that they basically
> re-implement the same raw I/O accessors. This strongly hints that we
> should implement the raw accessors as a base trait, which the
> user-facing API would call into:
>
> pub trait Io {
> /// Error type returned by IO accessors. Can be `Infallible` for e.g. `Mmio`.
> type Error: Into<Error>;
>
> /// Returns the base address of this mapping.
> fn addr(&self) -> usize;
>
> /// Returns the maximum size of this mapping.
> fn maxsize(&self) -> usize;
>
> unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error>;
> unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error>;
> // etc. for 16/32 bits accessors.
> }
>
> Then we could build the current `IoFallible` trait on top of it:
>
> pub trait IoFallible: Io {
> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> if !offset_valid::<U>(offset, self.maxsize()) {
> return Err(EINVAL);
> }
>
> self.addr().checked_add(offset).ok_or(EINVAL)
> }
>
> /// 8-bit read with runtime bounds check.
> fn try_read8_checked(&self, offset: usize) -> Result<u8> {
> let addr = self.io_addr::<u8>(offset)?;
>
> unsafe { self.try_read8_unchecked(addr) }.map_err(Into::into)
> }
>
> /// 8-bit write with runtime bounds check.
> fn try_write8_checked(&self, value: u8, offset: usize) -> Result {
> let addr = self.io_addr::<u8>(offset)?;
>
> unsafe { self.try_write8_unchecked(value, addr) }.map_err(Into::into)
> }
> }
>
> Note how this trait is now auto-implemented. Making it available for all
> implementers of `Io` is as simple as:
>
> impl<IO: Io> IoFallible for IO {}
>
> (... which hints that maybe it should simply be folded into `Io`, as
> Alice previously suggested)
Yes, it probably should. At the very least, it should be an extension
trait, which means that it should never be used in trait bounds, since
T: IoFallible is equivalent to T: Io. But in this case, probably just
fold it into Io.
> `IoKnownSize` also calls into the base `Io` trait:
>
> /// Trait for IO with a build-time known valid range.
> pub unsafe trait IoKnownSize: Io {
> /// Minimum usable size of this region.
> const MIN_SIZE: usize;
>
> #[inline(always)]
> fn io_addr_assert<U>(&self, offset: usize) -> usize {
> build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
>
> self.addr() + offset
> }
>
> /// 8-bit read with compile-time bounds check.
> #[inline(always)]
> fn try_read8(&self, offset: usize) -> Result<u8, Self::Error> {
> let addr = self.io_addr_assert::<u8>(offset);
>
> unsafe { self.try_read8_unchecked(addr) }
> }
>
> /// 8-bit write with compile-time bounds check.
> #[inline(always)]
> fn try_write8(&self, value: u8, offset: usize) -> Result<(), Self::Error> {
> let addr = self.io_addr_assert::<u8>(offset);
>
> unsafe { self.try_write8_unchecked(value, addr) }
> }
> }
>
> Its accessors now return the error type of the bus, which is good for
> safety, but not for ergonomics when dealing with e.g. code that works
> with `Mmio`, which we know is infallible. But we can provide an extra
> set of methods in this trait for this case:
>
> /// Infallible 8-bit read with compile-time bounds check.
> #[inline(always)]
> fn read8(&self, offset: usize) -> u8
> where
> Self: Io<Error = Infallible>,
> {
> self.read8(offset).unwrap_or_else(|e| match e {})
> }
>
> /// Infallible 8-bit write with compile-time bounds check.
> #[inline(always)]
> fn write8(&self, value: u8, offset: usize)
> where
> Self: Io<Error = Infallible>,
> {
> self.write8(value, offset).unwrap_or_else(|e| match e {})
> }
>
> `Mmio`'s impl blocks are now reduced to the following:
>
> impl<const SIZE: usize> Io for Mmio<SIZE> {
> type Error = core::convert::Infallible;
>
> #[inline]
> fn addr(&self) -> usize {
> self.0.addr()
> }
>
> #[inline]
> fn maxsize(&self) -> usize {
> self.0.maxsize()
> }
>
> unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
> Ok(unsafe { bindings::readb(addr as *const c_void) })
> }
>
> unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
> Ok(unsafe { bindings::writeb(value, addr as *mut c_void) })
> }
> }
>
> unsafe impl<const SIZE: usize> IoKnownSize for Mmio<SIZE> {
> const MIN_SIZE: usize = SIZE;
> }
>
> ... and that's enough to provide everything we had so far - all of the
> accessors called by users are already implemented in the base traits.
> Note also the lack of macros.
>
> Another point that I noticed was the relaxed MMIO accessors. They are
> currently implemented as a set of dedicated methods (e.g.
> `read8_relaxed`) that are not part of a trait. This results in a lot of
> additional methods, and limits their usefulness as the same generic
> function could not be used with both regular and relaxed accesses.
>
> So I'd propose to implement them using a relaxed wrapper type:
>
> /// Wrapper for [`Mmio`] that performs relaxed I/O accesses.
> pub struct RelaxedMmio<'a, const SIZE: usize>(&'a Mmio<SIZE>);
>
> impl<'a, const SIZE: usize> RelaxedMmio<'a, SIZE> {
> pub fn new(mmio: &'a Mmio<SIZE>) -> Self {
> Self(mmio)
> }
> }
>
> impl<'a, const SIZE: usize> Io for RelaxedMmio<'a, SIZE> {
> fn addr(&self) -> usize {
> self.0.addr()
> }
>
> fn maxsize(&self) -> usize {
> self.0.maxsize()
> }
>
> type Error = <Mmio as Io>::Error;
>
> unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
> Ok(unsafe { bindings::readb_relaxed(addr as *const c_void) })
> }
>
> unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
> Ok(unsafe { bindings::writeb_relaxed(value, addr as *mut c_void) })
> }
>
> // SAFETY: `MIN_SIZE` is the same as the wrapped type, which also implements `IoKnownSize`.
> unsafe impl<'a, const SIZE: usize> IoKnownSize for RelaxedMmio<'a, SIZE> {
> const MIN_SIZE: usize = SIZE;
> }
> }
>
> This way, when you need to do I/O using a register, you can either pass
> the `Mmio` instance or derive a `RelaxedMmio` from it, if that access
> pattern is more adequate.
This sounds like a reasonable way to handle relaxed mmio.
> How does this sound? I can share a branch with a basic implementation
> of this idea if that helps.
My main thoughts are:
First, we need to think some more about the naming. Currently you have
several methods with the same name. For example, you have a read8 method
implemented in terms of a different read8 method. It'd be nice with a
summary of the meaning of:
* try_ prefix
* _unchecked suffix
* _checked suffix (not the same as try_ prefix?)
Second, I think we need to think a bit more about the error types.
Perhaps the trait could define:
/// Error type used by `*_unchecked` methods.
type Error;
/// Error type that can be either `Self::Error` or a failed bounds
/// check.
type TryError: From<Self::Error> + From<BoundsError>;
where BoundsError is a new zero-sized error type we can define
somewhere. This way, Mmio can use these errors:
type Error = Infallible;
type TryError = BoundsError;
wheres cases that can fail with an IO error can use kernel::error::Error
for both cases.
Third, if we're going to postpone the custom integer type for
IoKnownSize, then I think we should get rid of build-checked IO ops
entirely for now.
Fourth, I didn't know about the alignment requirement. I would like to
know how that fits in with the rest of this. Is it treated like a bounds
check? That could make sense, and we could also have a custom integer
type that both has a max value and alignment invariant. But what about
the *_unchecked and runtime bounds-checked methods?
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait
2025-12-01 12:23 ` Alice Ryhl
@ 2025-12-03 13:32 ` Alexandre Courbot
0 siblings, 0 replies; 28+ messages in thread
From: Alexandre Courbot @ 2025-12-03 13:32 UTC (permalink / raw)
To: Alice Ryhl, Alexandre Courbot
Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, bhelgaas,
kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra,
ankita, aniketa, kwankhede, targupta, joelagnelf, jhubbard,
zhiwang
On Mon Dec 1, 2025 at 9:23 PM JST, Alice Ryhl wrote:
> On Mon, Dec 01, 2025 at 08:57:09PM +0900, Alexandre Courbot wrote:
>> On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
>> > On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
>> >> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>> >>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>> >>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> >>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> >>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>> >>> >> >> The previous Io<SIZE> type combined both the generic I/O access helpers
>> >>> >> >> and MMIO implementation details in a single struct.
>> >>> >> >>
>> >>> >> >> To establish a cleaner layering between the I/O interface and its concrete
>> >>> >> >> backends, paving the way for supporting additional I/O mechanisms in the
>> >>> >> >> future, Io<SIZE> need to be factored.
>> >>> >> >>
>> >>> >> >> Factor the common helpers into new {Io, Io64} traits, and move the
>> >>> >> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>> >>> >> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>> >>> >> >> to use MmioRaw.
>> >>> >> >>
>> >>> >> >> No functional change intended.
>> >>> >> >>
>> >>> >> >> Cc: Alexandre Courbot <acourbot@nvidia.com>
>> >>> >> >> Cc: Alice Ryhl <aliceryhl@google.com>
>> >>> >> >> Cc: Bjorn Helgaas <helgaas@kernel.org>
>> >>> >> >> Cc: Danilo Krummrich <dakr@kernel.org>
>> >>> >> >> Cc: John Hubbard <jhubbard@nvidia.com>
>> >>> >> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
>> >>> >> >
>> >>> >> > I said this on a previous version, but I still don't buy the split
>> >>> >> > into IoFallible and IoInfallible.
>> >>> >> >
>> >>> >> > For one, we're never going to have a method that can accept any Io - we
>> >>> >> > will always want to accept either IoInfallible or IoFallible, so the
>> >>> >> > base Io trait serves no purpose.
>> >>> >> >
>> >>> >> > For another, the docs explain that the distinction between them is
>> >>> >> > whether the bounds check is done at compile-time or runtime. That is not
>> >>> >> > the kind of capability one normally uses different traits to distinguish
>> >>> >> > between. It makes sense to have additional traits to distinguish
>> >>> >> > between e.g.:
>> >>> >> >
>> >>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> >>> >> > * Whether 64-bit IO ops are possible.
>> >>> >> >
>> >>> >> > Well ... I guess one could distinguish between whether it's possible to
>> >>> >> > check bounds at compile-time at all. But if you can check them at
>> >>> >> > compile-time, it should always be possible to check at runtime too, so
>> >>> >> > one should be a sub-trait of the other if you want to distinguish
>> >>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >>> >> >
>> >>> >> > And I'm not really convinced that the current compile-time checked
>> >>> >> > traits are a good idea at all. See:
>> >>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >>> >> >
>> >>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>> >>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>> >>> >> > to only contain integers <= the size. For example, the Bounded integer
>> >>> >> > being added elsewhere.
>> >>> >>
>> >>> >> Would that be so different from using an associated const value though?
>> >>> >> IIUC the bounded integer type would play the same role, only slightly
>> >>> >> differently - by that I mean that if the offset is expressed by an
>> >>> >> expression that is not const (such as an indexed access), then the
>> >>> >> bounded integer still needs to rely on `build_assert` to be built.
>> >>> >
>> >>> > I mean something like this:
>> >>> >
>> >>> > trait Io {
>> >>> > const SIZE: usize;
>> >>> > fn write(&mut self, i: Bounded<Self::SIZE>);
>> >>> > }
>> >>>
>> >>> I have experimented a bit with this idea, and unfortunately expressing
>> >>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>> >>> not doable as of today.
>> >>>
>> >>> Bounding an integer with an upper/lower bound also proves to be more
>> >>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>> >>> constants must be of the same type as the wrapped `T` type, which again
>> >>> makes rustc unhappy ("the type of const parameters must not depend on
>> >>> other generic parameters"). A workaround would be to use a macro to
>> >>> define individual types for each integer type we want to support - or to
>> >>> just limit this to `usize`.
>> >>>
>> >>> But the requirement for generic_const_exprs makes this a non-starter I'm
>> >>> afraid. :/
>> >>
>> >> Can you try this?
>> >>
>> >> trait Io {
>> >> type IdxInt: Int;
>> >> fn write(&mut self, i: Self::IdxInt);
>> >> }
>> >>
>> >> then implementers would write:
>> >>
>> >> impl Io for MyIo {
>> >> type IdxInt = Bounded<17>;
>> >> }
>> >>
>> >> instead of:
>> >> impl Io for MyIo {
>> >> const SIZE = 17;
>> >> }
>> >
>> > The following builds (using the existing `Bounded` type for
>> > demonstration purposes):
>> >
>> > trait Io {
>> > // Type containing an index guaranteed to be valid for this IO.
>> > type IdxInt: Into<usize>;
>> >
>> > fn write(&mut self, i: Self::IdxInt);
>> > }
>> >
>> > struct FooIo;
>> >
>> > impl Io for FooIo {
>> > type IdxInt = Bounded<usize, 8>;
>> >
>> > fn write(&mut self, i: Self::IdxInt) {
>> > let idx: usize = i.into();
>> >
>> > // Now do the IO knowing that `idx` is a valid index.
>> > }
>> > }
>> >
>> > That looks promising, and I like how we can effectively use a wider set
>> > of index types - even, say, a `u16` if a particular I/O happens to have
>> > a guaranteed size of 65536!
>> >
>> > I suspect it also changes how we would design the Io interfaces, but I
>> > am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
>> > either unwrapping the result of its fallible methods or using some
>> > `unchecked` accessors?
>>
>> Mmm, one important point I have neglected is that the index type will
>> have to validate not only the range, but also the alignment of the
>> index! And the valid alignment is dependent on the access width. So
>> getting this right is going to take some time and some experimenting I'm
>> afraid.
>>
>> Meanwhile, it would be great if we could agree (and proceed) with the
>> split of the I/O interface into a trait, as other work depends on it.
>> Changing the index type of compile-time checked bounds is I think an
>> improvement that is orthogonal to this task.
>
>
>
>> I have been thinking a bit (too much? ^_^;) about the general design for
>> this interface, how it would work with the `register!` macro, and how we
>> could maybe limit the boilerplate. Sorry in advance for this is going to
>> be a long post.
>>
>> IIUC there are several aspects we need to tackle with the I/O interface:
>>
>> - Raw IO access. Given an address, perform the IO operation without any
>> check. Depending on the bus, this might return the data directly (e.g.
>> `Mmio`), or a `Result` (e.g. the PCI `ConfigSpace`). The current
>> implementation ignores the bus error, which we probably shouldn't.
>> Also the raw access is reimplemented twice, in both the build-time and
>> runtime accessors, a fact that is mostly hidden by the use of macros.
>> - Access with dynamic bounds check. This can return `EINVAL` if the
>> provided index is invalid (out-of-bounds or not aligned), on top of
>> the bus errors, if any.
>> - Access with build-time index check. Same as above, but the error
>> occurs at build time if the index is invalid. Otherwise the return
>> type of the raw IO accessor is returned.
>>
>> At the moment we have two traits for build-time and runtime index
>> checks. What bothers me a bit about them is that they basically
>> re-implement the same raw I/O accessors. This strongly hints that we
>> should implement the raw accessors as a base trait, which the
>> user-facing API would call into:
>>
>> pub trait Io {
>> /// Error type returned by IO accessors. Can be `Infallible` for e.g. `Mmio`.
>> type Error: Into<Error>;
>>
>> /// Returns the base address of this mapping.
>> fn addr(&self) -> usize;
>>
>> /// Returns the maximum size of this mapping.
>> fn maxsize(&self) -> usize;
>>
>> unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error>;
>> unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error>;
>> // etc. for 16/32 bits accessors.
>> }
>>
>> Then we could build the current `IoFallible` trait on top of it:
>>
>> pub trait IoFallible: Io {
>> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> if !offset_valid::<U>(offset, self.maxsize()) {
>> return Err(EINVAL);
>> }
>>
>> self.addr().checked_add(offset).ok_or(EINVAL)
>> }
>>
>> /// 8-bit read with runtime bounds check.
>> fn try_read8_checked(&self, offset: usize) -> Result<u8> {
>> let addr = self.io_addr::<u8>(offset)?;
>>
>> unsafe { self.try_read8_unchecked(addr) }.map_err(Into::into)
>> }
>>
>> /// 8-bit write with runtime bounds check.
>> fn try_write8_checked(&self, value: u8, offset: usize) -> Result {
>> let addr = self.io_addr::<u8>(offset)?;
>>
>> unsafe { self.try_write8_unchecked(value, addr) }.map_err(Into::into)
>> }
>> }
>>
>> Note how this trait is now auto-implemented. Making it available for all
>> implementers of `Io` is as simple as:
>>
>> impl<IO: Io> IoFallible for IO {}
>>
>> (... which hints that maybe it should simply be folded into `Io`, as
>> Alice previously suggested)
>
> Yes, it probably should. At the very least, it should be an extension
> trait, which means that it should never be used in trait bounds, since
> T: IoFallible is equivalent to T: Io. But in this case, probably just
> fold it into Io.
Agreed, I don't see any benefit in keeping these separate although I
remember Danilo prefering to keep them parted.
<snip>
>> How does this sound? I can share a branch with a basic implementation
>> of this idea if that helps.
>
> My main thoughts are:
>
> First, we need to think some more about the naming. Currently you have
> several methods with the same name. For example, you have a read8 method
> implemented in terms of a different read8 method. It'd be nice with a
> summary of the meaning of:
>
> * try_ prefix
> * _unchecked suffix
> * _checked suffix (not the same as try_ prefix?)
I guess it shows that I quickly tried to adopt a naming pattern as I was
writing my email. :)
The logic was roughly:
- `_unchecked` suffix for unsafe ops, like the standard library,
- `try_` prefix if the accessor returns an bus error,
- `_checked` suffix if the bounds are checked at run-time by the
accessor.
So `try_read8_checked` checks the bounds at runtime and returns a
`Result`, `try_read8` checks the bounds at build time and returns a
`Result` which only errors are bus ones, and `read8` is for the case
where the bus' error type is infallible, and we can know at build time
that no error will ever be returned.
... this naming pattern can probably be improved.
> Second, I think we need to think a bit more about the error types.
> Perhaps the trait could define:
>
> /// Error type used by `*_unchecked` methods.
> type Error;
>
> /// Error type that can be either `Self::Error` or a failed bounds
> /// check.
> type TryError: From<Self::Error> + From<BoundsError>;
>
> where BoundsError is a new zero-sized error type we can define
> somewhere. This way, Mmio can use these errors:
>
> type Error = Infallible;
> type TryError = BoundsError;
>
> wheres cases that can fail with an IO error can use kernel::error::Error
> for both cases.
I thought we could roughly achieve this by using the regular kernel
`Error` as the type returned by all accessors - does this extra
associated type bring something more?
> Third, if we're going to postpone the custom integer type for
> IoKnownSize, then I think we should get rid of build-checked IO ops
> entirely for now.
That would be a pretty disruptive change. I dread the Nova patch that
would be required. ^_^;
I'd also say it looks out-of-scope for this change? Zhi's goal is just
to extract the Io API into a trait, and this design doesn't
fundamentally change it, nor does it adds the build-checked IO ops -
they are already here, and this doesn't make the situation worse. I
agree this front should be improved, but that's a different effort.
Should we drop these ops, every single register access in Nova would
become fallible, and we would need to add error handling code for errors
we know for a fact cannot happen. I'd really like to avoid that.
> Fourth, I didn't know about the alignment requirement. I would like to
> know how that fits in with the rest of this. Is it treated like a bounds
> check? That could make sense, and we could also have a custom integer
> type that both has a max value and alignment invariant. But what about
> the *_unchecked and runtime bounds-checked methods?
Basically we would need the custom integer type to hold as invariant
that the current `offset_valid` is always true:
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
}
}
I initially thought that we could maybe turn this into a class of
primitive types for which a custom invariant to hold could be passed as
e.g. a closure, but of course it's not that simple. I'm still hopeful we
can reach a breakthrough somehow though.
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2025-12-03 13:33 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-19 11:21 [PATCH v7 0/6] rust: pci: add config space read/write support Zhi Wang
2025-11-19 11:21 ` [PATCH v7 1/6] samples: rust: rust_driver_pci: use "kernel vertical" style for imports Zhi Wang
2025-11-19 11:21 ` [PATCH v7 2/6] rust: devres: " Zhi Wang
2025-11-19 11:21 ` [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait Zhi Wang
2025-11-21 14:20 ` Alice Ryhl
2025-11-24 10:08 ` Zhi Wang
2025-11-24 10:20 ` Alice Ryhl
2025-11-24 13:32 ` Zhi Wang
2025-11-24 13:53 ` Alexandre Courbot
2025-11-25 13:44 ` Alexandre Courbot
2025-11-25 14:58 ` Alice Ryhl
2025-11-26 0:43 ` Alexandre Courbot
2025-11-26 7:52 ` Alexandre Courbot
2025-11-26 9:50 ` Alice Ryhl
2025-11-26 13:37 ` Alexandre Courbot
2025-11-26 13:39 ` Alexandre Courbot
2025-12-01 11:57 ` Alexandre Courbot
2025-12-01 12:23 ` Alice Ryhl
2025-12-03 13:32 ` Alexandre Courbot
2025-11-25 14:09 ` Alexandre Courbot
2025-11-25 14:14 ` Alexandre Courbot
2025-11-25 14:22 ` Alice Ryhl
2025-11-25 14:46 ` Alexandre Courbot
2025-11-25 19:19 ` John Hubbard
2025-11-19 11:21 ` [PATCH v7 4/6] rust: io: factor out MMIO read/write macros Zhi Wang
2025-11-19 11:21 ` [PATCH v7 5/6] rust: pci: add config space read/write support Zhi Wang
2025-11-25 14:20 ` Alexandre Courbot
2025-11-19 11:21 ` [PATCH v7 6/6] sample: rust: pci: add tests for config space routines Zhi Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).