* [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE>
2025-10-16 21:02 [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Zhi Wang
@ 2025-10-16 21:02 ` Zhi Wang
2025-10-17 1:26 ` Bjorn Helgaas
` (4 more replies)
2025-10-16 21:02 ` [PATCH v2 2/5] rust: io: factor out MMIO read/write macros Zhi Wang
` (4 subsequent siblings)
5 siblings, 5 replies; 16+ messages in thread
From: Zhi Wang @ 2025-10-16 21:02 UTC (permalink / raw)
To: rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiw, zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
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 a new Io trait, and moves the MMIO-specific
logic into a dedicated Mmio<SIZE> type implementing that trait. Rename the
IoRaw to MmioRaw and pdate the bus MMIO implementations to use MmioRaw.
No functional change intended.
Cc: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
drivers/gpu/nova-core/regs/macros.rs | 36 +++++------
rust/kernel/io.rs | 89 +++++++++++++++++-----------
rust/kernel/io/mem.rs | 16 ++---
rust/kernel/pci.rs | 10 ++--
4 files changed, 87 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 8058e1696df9..c2a6547d58cd 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -609,7 +609,7 @@ 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>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
{
Self(io.read32($offset))
}
@@ -617,7 +617,7 @@ pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
/// 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>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
{
io.write32(self.0, $offset)
}
@@ -629,7 +629,7 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
io: &T,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io));
@@ -652,7 +652,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
#[allow(unused_variables)]
base: &B,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
@@ -673,7 +673,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
#[allow(unused_variables)]
base: &B,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
@@ -693,7 +693,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
base: &B,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
@@ -717,7 +717,7 @@ pub(crate) fn read<const SIZE: usize, T>(
io: &T,
idx: usize,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
{
build_assert!(idx < Self::SIZE);
@@ -734,7 +734,7 @@ pub(crate) fn write<const SIZE: usize, T>(
io: &T,
idx: usize
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
{
build_assert!(idx < Self::SIZE);
@@ -751,7 +751,7 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
idx: usize,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io, idx));
@@ -767,7 +767,7 @@ pub(crate) fn try_read<const SIZE: usize, T>(
io: &T,
idx: usize,
) -> ::kernel::error::Result<Self> where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
{
if idx < Self::SIZE {
Ok(Self::read(io, idx))
@@ -786,7 +786,7 @@ pub(crate) fn try_write<const SIZE: usize, T>(
io: &T,
idx: usize,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
{
if idx < Self::SIZE {
Ok(self.write(io, idx))
@@ -806,7 +806,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, F>(
idx: usize,
f: F,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
F: ::core::ops::FnOnce(Self) -> Self,
{
if idx < Self::SIZE {
@@ -838,7 +838,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
base: &B,
idx: usize,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
@@ -860,7 +860,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
base: &B,
idx: usize
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
@@ -881,7 +881,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
idx: usize,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
@@ -900,7 +900,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
base: &B,
idx: usize,
) -> ::kernel::error::Result<Self> where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
{
if idx < Self::SIZE {
@@ -922,7 +922,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
base: &B,
idx: usize,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
{
if idx < Self::SIZE {
@@ -945,7 +945,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
idx: usize,
f: F,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index ee182b0b5452..78413dc7ffcc 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -18,16 +18,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);
@@ -62,11 +62,11 @@ pub fn maxsize(&self) -> usize {
/// # Examples
///
/// ```no_run
-/// # use kernel::{bindings, ffi::c_void, io::{Io, IoRaw}};
+/// # use kernel::{bindings, ffi::c_void, io::{Mmio, MmioRaw}};
/// # 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
@@ -81,7 +81,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)?))
/// }
/// }
///
@@ -93,11 +93,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) }
/// }
/// }
///
@@ -111,7 +111,7 @@ 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) => {
@@ -172,32 +172,24 @@ 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() }
- }
-
+/// 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.
+///
+/// Types implementing this trait (e.g. MMIO BARs or PCI config
+/// regions) can share the same accessors.
+pub trait Io<const 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()
- }
+ fn maxsize(&self) -> usize;
+ /// 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 {
+ 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
@@ -206,6 +198,8 @@ const fn offset_valid<U>(offset: usize, size: usize) -> bool {
}
}
+ /// Returns the absolute I/O address for a given `offset`.
+ /// Performs runtime bounds checks using [`offset_valid`]
#[inline]
fn io_addr<U>(&self, offset: usize) -> Result<usize> {
if !Self::offset_valid::<U>(offset, self.maxsize()) {
@@ -217,12 +211,41 @@ 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));
self.addr() + offset
}
+}
+
+impl<const SIZE: usize> Io<SIZE> for Mmio<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> 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!(read8, try_read8, readb -> u8);
define_read!(read16, try_read16, readw -> u16);
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index 6f99510bfc3a..93cad8539b18 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -11,8 +11,8 @@
use crate::io;
use crate::io::resource::Region;
use crate::io::resource::Resource;
-use crate::io::Io;
-use crate::io::IoRaw;
+use crate::io::Mmio;
+use crate::io::MmioRaw;
use crate::prelude::*;
/// An IO request for a specific device and resource.
@@ -195,7 +195,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
@@ -209,10 +209,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> {
@@ -247,7 +247,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)
@@ -270,10 +270,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/pci.rs b/rust/kernel/pci.rs
index 7fcc5f6022c1..77a8eb39ad32 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -10,7 +10,7 @@
devres::Devres,
driver,
error::{from_result, to_result, Result},
- io::{Io, IoRaw},
+ io::{Mmio, MmioRaw},
irq::{self, IrqRequest},
str::CStr,
sync::aref::ARef,
@@ -313,7 +313,7 @@ pub struct Device<Ctx: device::DeviceContext = device::Normal>(
/// 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,
}
@@ -349,7 +349,7 @@ 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:
@@ -403,11 +403,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) }
}
}
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE>
2025-10-16 21:02 ` [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE> Zhi Wang
@ 2025-10-17 1:26 ` Bjorn Helgaas
2025-10-17 10:56 ` Danilo Krummrich
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2025-10-17 1:26 UTC (permalink / raw)
To: Zhi Wang
Cc: rust-for-linux, dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl,
tmgross, linux-pci, linux-kernel, cjia, smitra, ankita, aniketa,
kwankhede, targupta, zhiwang, acourbot, joelagnelf, jhubbard,
markus.probst
On Thu, Oct 16, 2025 at 09:02:46PM +0000, 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 a new Io trait, and moves the MMIO-specific
> logic into a dedicated Mmio<SIZE> type implementing that trait. Rename the
> IoRaw to MmioRaw and pdate the bus MMIO implementations to use MmioRaw.
s/and moves/and move/ to match "Factor" and "Rename"
s/pdate/update/ ?
> +/// 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.
s/any guarantees are given/are any guarantees given/
> + /// Returns a new `MmioRaw` instance ...
> +/// Provides common helpers ...
> + /// Checks whether an access ...
There's a Linux trend toward using "imperative mood", e.g., "Return",
for commit logs and comments, but I notice Rust code more often seems
to use the indicative mood: "Returns", "Provides", "Checks", etc.
Maybe that's part of the Rust style; I dunno if that's intentional or
worth commenting on, just something I notice.
Bjorn
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE>
2025-10-16 21:02 ` [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE> Zhi Wang
2025-10-17 1:26 ` Bjorn Helgaas
@ 2025-10-17 10:56 ` Danilo Krummrich
2025-10-20 11:44 ` Alexandre Courbot
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-10-17 10:56 UTC (permalink / raw)
To: Zhi Wang
Cc: rust-for-linux, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl,
tmgross, linux-pci, linux-kernel, cjia, smitra, ankita, aniketa,
kwankhede, targupta, zhiwang, acourbot, joelagnelf, jhubbard,
markus.probst
On Thu Oct 16, 2025 at 11:02 PM CEST, Zhi Wang wrote:
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
> index 8058e1696df9..c2a6547d58cd 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/drivers/gpu/nova-core/regs/macros.rs
> @@ -609,7 +609,7 @@ 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>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
This should be
T: ::core::ops::Deref<Target = I>,
I: ::kernel::io::Io<SIZE>,
instead, otherwise register!() only works for MMIO, but it should work for any
I/O backend.
> +impl<const SIZE: usize> Io<SIZE> for Mmio<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()
> + }
> +}
The I/O trait should contain the corresponding read/write accessors, otherwise
we can't easily wire up the register!() macro with arbitrary I/O backends.
I think more specific things, such as relaxed operations can remain MMIO
specific, but all the {try_}{read,write}{8,16,32,64} accessors should be part of
the I/O trait.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE>
2025-10-16 21:02 ` [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE> Zhi Wang
2025-10-17 1:26 ` Bjorn Helgaas
2025-10-17 10:56 ` Danilo Krummrich
@ 2025-10-20 11:44 ` Alexandre Courbot
2025-10-20 19:34 ` John Hubbard
2025-10-20 23:36 ` kernel test robot
4 siblings, 0 replies; 16+ messages in thread
From: Alexandre Courbot @ 2025-10-20 11:44 UTC (permalink / raw)
To: Zhi Wang, rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
On Fri Oct 17, 2025 at 6:02 AM JST, Zhi Wang wrote:
<snip>
> +/// 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.
> +///
> +/// Types implementing this trait (e.g. MMIO BARs or PCI config
> +/// regions) can share the same accessors.
> +pub trait Io<const 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()
> - }
> + fn maxsize(&self) -> usize;
>
> + /// 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 {
> + fn offset_valid<U>(offset: usize, size: usize) -> bool {
Is it ok to lose the `const` attribute here?
Since this method doesn't take `self`, and is not supposed to be
overloaded anyway, I'd suggest moving it out of the trait and turning it
into a private function of this module.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE>
2025-10-16 21:02 ` [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE> Zhi Wang
` (2 preceding siblings ...)
2025-10-20 11:44 ` Alexandre Courbot
@ 2025-10-20 19:34 ` John Hubbard
2025-10-20 23:36 ` kernel test robot
4 siblings, 0 replies; 16+ messages in thread
From: John Hubbard @ 2025-10-20 19:34 UTC (permalink / raw)
To: Zhi Wang, rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiwang, acourbot, joelagnelf, markus.probst
On 10/16/25 2:02 PM, 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 a new Io trait, and moves the MMIO-specific
> logic into a dedicated Mmio<SIZE> type implementing that trait. Rename the
> IoRaw to MmioRaw and pdate the bus MMIO implementations to use MmioRaw.
>
> No functional change intended.
>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
> drivers/gpu/nova-core/regs/macros.rs | 36 +++++------
> rust/kernel/io.rs | 89 +++++++++++++++++-----------
> rust/kernel/io/mem.rs | 16 ++---
> rust/kernel/pci.rs | 10 ++--
> 4 files changed, 87 insertions(+), 64 deletions(-)
Hi Zhi,
This fails to build for me. Looking closer, I see that the doctests are
failing, which implies that you must not be building them.
Can I suggest that you set this, so as to avoid this in the future:
CONFIG_RUST_KERNEL_DOCTESTS=y
And here is a diff that fixes the build for me, with that config
option set:
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 10a6a1789854..d35cd39e32bf 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -52,11 +52,11 @@ struct Inner<T: Send> {
/// # Examples
///
/// ```no_run
-/// # use kernel::{bindings, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}};
+/// # use kernel::{bindings, device::{Bound, Device}, devres::Devres, io::{Mmio, MmioRaw}};
/// # 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
@@ -71,7 +71,7 @@ struct Inner<T: Send> {
/// return Err(ENOMEM);
/// }
///
-/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
+/// Ok(IoMem(MmioRaw::new(addr as usize, SIZE)?))
/// }
/// }
///
@@ -83,11 +83,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> {
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index 613eb25047ef..835599085339 100644
--- a/rust/kernel/io/poll.rs
+++ b/rust/kernel/io/poll.rs
@@ -37,12 +37,12 @@
/// # Examples
///
/// ```no_run
-/// use kernel::io::{Io, poll::read_poll_timeout};
+/// use kernel::io::{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<()> {
/// match read_poll_timeout(
/// // The `op` closure reads the value of a specific status register.
/// || io.try_read16(0x1000),
thanks,
--
John Hubbard
>
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
> index 8058e1696df9..c2a6547d58cd 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/drivers/gpu/nova-core/regs/macros.rs
> @@ -609,7 +609,7 @@ 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>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> {
> Self(io.read32($offset))
> }
> @@ -617,7 +617,7 @@ pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
> /// 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>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> {
> io.write32(self.0, $offset)
> }
> @@ -629,7 +629,7 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
> io: &T,
> f: F,
> ) where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> F: ::core::ops::FnOnce(Self) -> Self,
> {
> let reg = f(Self::read(io));
> @@ -652,7 +652,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
> #[allow(unused_variables)]
> base: &B,
> ) -> Self where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> {
> const OFFSET: usize = $name::OFFSET;
> @@ -673,7 +673,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
> #[allow(unused_variables)]
> base: &B,
> ) where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> {
> const OFFSET: usize = $name::OFFSET;
> @@ -693,7 +693,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
> base: &B,
> f: F,
> ) where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> F: ::core::ops::FnOnce(Self) -> Self,
> {
> @@ -717,7 +717,7 @@ pub(crate) fn read<const SIZE: usize, T>(
> io: &T,
> idx: usize,
> ) -> Self where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> {
> build_assert!(idx < Self::SIZE);
>
> @@ -734,7 +734,7 @@ pub(crate) fn write<const SIZE: usize, T>(
> io: &T,
> idx: usize
> ) where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> {
> build_assert!(idx < Self::SIZE);
>
> @@ -751,7 +751,7 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
> idx: usize,
> f: F,
> ) where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> F: ::core::ops::FnOnce(Self) -> Self,
> {
> let reg = f(Self::read(io, idx));
> @@ -767,7 +767,7 @@ pub(crate) fn try_read<const SIZE: usize, T>(
> io: &T,
> idx: usize,
> ) -> ::kernel::error::Result<Self> where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> {
> if idx < Self::SIZE {
> Ok(Self::read(io, idx))
> @@ -786,7 +786,7 @@ pub(crate) fn try_write<const SIZE: usize, T>(
> io: &T,
> idx: usize,
> ) -> ::kernel::error::Result where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> {
> if idx < Self::SIZE {
> Ok(self.write(io, idx))
> @@ -806,7 +806,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, F>(
> idx: usize,
> f: F,
> ) -> ::kernel::error::Result where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> F: ::core::ops::FnOnce(Self) -> Self,
> {
> if idx < Self::SIZE {
> @@ -838,7 +838,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
> base: &B,
> idx: usize,
> ) -> Self where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> {
> build_assert!(idx < Self::SIZE);
> @@ -860,7 +860,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
> base: &B,
> idx: usize
> ) where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> {
> build_assert!(idx < Self::SIZE);
> @@ -881,7 +881,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
> idx: usize,
> f: F,
> ) where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> F: ::core::ops::FnOnce(Self) -> Self,
> {
> @@ -900,7 +900,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
> base: &B,
> idx: usize,
> ) -> ::kernel::error::Result<Self> where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> {
> if idx < Self::SIZE {
> @@ -922,7 +922,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
> base: &B,
> idx: usize,
> ) -> ::kernel::error::Result where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> {
> if idx < Self::SIZE {
> @@ -945,7 +945,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
> idx: usize,
> f: F,
> ) -> ::kernel::error::Result where
> - T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> + T: ::core::ops::Deref<Target = ::kernel::io::Mmio<SIZE>>,
> B: crate::regs::macros::RegisterBase<$base>,
> F: ::core::ops::FnOnce(Self) -> Self,
> {
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index ee182b0b5452..78413dc7ffcc 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -18,16 +18,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);
> @@ -62,11 +62,11 @@ pub fn maxsize(&self) -> usize {
> /// # Examples
> ///
> /// ```no_run
> -/// # use kernel::{bindings, ffi::c_void, io::{Io, IoRaw}};
> +/// # use kernel::{bindings, ffi::c_void, io::{Mmio, MmioRaw}};
> /// # 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
> @@ -81,7 +81,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)?))
> /// }
> /// }
> ///
> @@ -93,11 +93,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) }
> /// }
> /// }
> ///
> @@ -111,7 +111,7 @@ 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) => {
> @@ -172,32 +172,24 @@ 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() }
> - }
> -
> +/// 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.
> +///
> +/// Types implementing this trait (e.g. MMIO BARs or PCI config
> +/// regions) can share the same accessors.
> +pub trait Io<const 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()
> - }
> + fn maxsize(&self) -> usize;
>
> + /// 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 {
> + 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
> @@ -206,6 +198,8 @@ const fn offset_valid<U>(offset: usize, size: usize) -> bool {
> }
> }
>
> + /// Returns the absolute I/O address for a given `offset`.
> + /// Performs runtime bounds checks using [`offset_valid`]
> #[inline]
> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> if !Self::offset_valid::<U>(offset, self.maxsize()) {
> @@ -217,12 +211,41 @@ 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));
>
> self.addr() + offset
> }
> +}
> +
> +impl<const SIZE: usize> Io<SIZE> for Mmio<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> 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!(read8, try_read8, readb -> u8);
> define_read!(read16, try_read16, readw -> u16);
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> index 6f99510bfc3a..93cad8539b18 100644
> --- a/rust/kernel/io/mem.rs
> +++ b/rust/kernel/io/mem.rs
> @@ -11,8 +11,8 @@
> use crate::io;
> use crate::io::resource::Region;
> use crate::io::resource::Resource;
> -use crate::io::Io;
> -use crate::io::IoRaw;
> +use crate::io::Mmio;
> +use crate::io::MmioRaw;
> use crate::prelude::*;
>
> /// An IO request for a specific device and resource.
> @@ -195,7 +195,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
> @@ -209,10 +209,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> {
> @@ -247,7 +247,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)
> @@ -270,10 +270,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/pci.rs b/rust/kernel/pci.rs
> index 7fcc5f6022c1..77a8eb39ad32 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -10,7 +10,7 @@
> devres::Devres,
> driver,
> error::{from_result, to_result, Result},
> - io::{Io, IoRaw},
> + io::{Mmio, MmioRaw},
> irq::{self, IrqRequest},
> str::CStr,
> sync::aref::ARef,
> @@ -313,7 +313,7 @@ pub struct Device<Ctx: device::DeviceContext = device::Normal>(
> /// 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,
> }
>
> @@ -349,7 +349,7 @@ 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:
> @@ -403,11 +403,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) }
> }
> }
>
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE>
2025-10-16 21:02 ` [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE> Zhi Wang
` (3 preceding siblings ...)
2025-10-20 19:34 ` John Hubbard
@ 2025-10-20 23:36 ` kernel test robot
4 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2025-10-20 23:36 UTC (permalink / raw)
To: Zhi Wang, rust-for-linux
Cc: llvm, oe-kbuild-all, dakr, bhelgaas, kwilczynski, ojeda,
alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg,
aliceryhl, tmgross, linux-pci, linux-kernel, cjia, smitra, ankita,
aniketa, kwankhede, targupta, zhiw, zhiwang, acourbot, joelagnelf,
jhubbard, markus.probst
Hi Zhi,
kernel test robot noticed the following build errors:
[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.18-rc2 next-20251020]
[cannot apply to driver-core/driver-core-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Zhi-Wang/rust-io-factor-common-I-O-helpers-into-Io-trait-and-specialize-Mmio-SIZE/20251017-050553
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20251016210250.15932-2-zhiw%40nvidia.com
patch subject: [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE>
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20251021/202510210730.qW10Mhd0-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251021/202510210730.qW10Mhd0-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510210730.qW10Mhd0-lkp@intel.com/
All errors (new ones prefixed by >>):
>> error[E0432]: unresolved import `kernel::io::IoRaw`
--> rust/doctests_kernel_generated.rs:5047:74
|
5047 | use kernel::{bindings, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}};
| ^^^^^ no `IoRaw` in `io`
--
>> error[E0782]: expected a type, found a trait
--> rust/doctests_kernel_generated.rs:7075:46
|
7075 | fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
| ^^^^^^^^
|
= note: `Io<SIZE>` is dyn-incompatible, otherwise a trait object could be used
help: use a new generic type parameter, constrained by `Io<SIZE>`
|
7075 - fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
7075 + fn wait_for_hardware<const SIZE: usize, T: Io<SIZE>>(io: &T) -> Result<()> {
|
help: you can also use an opaque type, but users won't be able to specify the type parameter when calling the `fn`, having to rely exclusively on type inference
|
7075 | fn wait_for_hardware<const SIZE: usize>(io: &impl Io<SIZE>) -> Result<()> {
| ++++
--
>> error[E0782]: expected a type, found a trait
--> rust/doctests_kernel_generated.rs:5078:18
|
5078 | type Target = Io<SIZE>;
| ^^^^^^^^
--
>> error[E0782]: expected a type, found a trait
--> rust/doctests_kernel_generated.rs:5082:18
|
5082 | unsafe { Io::from_raw(&self.0) }
| ^^
|
help: you can add the `dyn` keyword if you want a trait object
|
5082 | unsafe { <dyn Io>::from_raw(&self.0) }
| ++++ +
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/5] rust: io: factor out MMIO read/write macros
2025-10-16 21:02 [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Zhi Wang
2025-10-16 21:02 ` [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE> Zhi Wang
@ 2025-10-16 21:02 ` Zhi Wang
2025-10-17 11:01 ` Danilo Krummrich
2025-10-16 21:02 ` [PATCH v2 3/5] rust: pci: add a helper to query configuration space size Zhi Wang
` (3 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Zhi Wang @ 2025-10-16 21:02 UTC (permalink / raw)
To: rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiw, zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
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.
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
rust/kernel/io.rs | 72 ++++++++++++++++++++++++++++++-----------------
1 file changed, 46 insertions(+), 26 deletions(-)
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 78413dc7ffcc..78f7bbc945ad 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -4,6 +4,8 @@
//!
//! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
+use kernel::error::Error;
+
use crate::error::{code::EINVAL, Result};
use crate::{bindings, build_assert, ffi::c_void};
@@ -113,8 +115,23 @@ pub fn maxsize(&self) -> usize {
#[repr(transparent)]
pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>);
+macro_rules! call_mmio_read {
+ ($c_fn:ident, $self:ident, $offset:expr, $type:ty, $addr:expr) => {
+ // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+ Ok::<$type, Error>(unsafe { bindings::$c_fn($addr as *const c_void) as $type })
+ };
+}
+
+macro_rules! call_mmio_write {
+ ($c_fn:ident, $self:ident, $offset:expr, $ty:ty, $addr:expr, $value:expr) => {
+ // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+ Ok::<(), Error>(unsafe { bindings::$c_fn($value, $addr as *mut c_void) })
+ };
+}
+
macro_rules! define_read {
- ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn:ident -> $type_name:ty) => {
+ ($(#[$attr:meta])* $name:ident, $try_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
@@ -122,10 +139,9 @@ macro_rules! define_read {
$(#[$attr])*
#[inline]
pub fn $name(&self, offset: usize) -> $type_name {
- let addr = self.io_addr_assert::<$type_name>(offset);
+ 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) }
+ $call_macro!($c_fn, self, offset, $type_name, _addr).unwrap_or(!0)
}
/// Read IO data from a given offset.
@@ -134,16 +150,18 @@ pub fn $name(&self, offset: usize) -> $type_name {
/// out of bounds.
$(#[$attr])*
pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
- let addr = self.io_addr::<$type_name>(offset)?;
+ 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) })
+ $call_macro!($c_fn, self, offset, $type_name, _addr)
}
};
}
+pub(crate) use define_read;
macro_rules! define_write {
- ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn:ident <- $type_name:ty) => {
+ ($(#[$attr:meta])* $name:ident, $try_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
@@ -151,10 +169,9 @@ macro_rules! define_write {
$(#[$attr])*
#[inline]
pub fn $name(&self, value: $type_name, offset: usize) {
- let addr = self.io_addr_assert::<$type_name>(offset);
+ 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) }
+ let _ = $call_macro!($c_fn, self, offset, $type_name, _addr, value);
}
/// Write IO data from a given offset.
@@ -163,14 +180,13 @@ pub fn $name(&self, value: $type_name, offset: usize) {
/// out of bounds.
$(#[$attr])*
pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
- let addr = self.io_addr::<$type_name>(offset)?;
+ 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!($c_fn, self, offset, $type_name, _addr, value)
}
};
}
+pub(crate) use define_write;
/// Represents a region of I/O space of a fixed size.
///
@@ -247,43 +263,47 @@ pub unsafe fn from_raw(raw: &MmioRaw<SIZE>) -> &Self {
unsafe { &*core::ptr::from_ref(raw).cast() }
}
- define_read!(read8, try_read8, readb -> u8);
- define_read!(read16, try_read16, readw -> u16);
- define_read!(read32, try_read32, readl -> u32);
+ define_read!(read8, try_read8, call_mmio_read, readb -> u8);
+ define_read!(read16, try_read16, call_mmio_read, readw -> u16);
+ define_read!(read32, try_read32, call_mmio_read, readl -> u32);
define_read!(
#[cfg(CONFIG_64BIT)]
read64,
try_read64,
+ call_mmio_read,
readq -> 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!(read8_relaxed, try_read8_relaxed, call_mmio_read, readb_relaxed -> u8);
+ define_read!(read16_relaxed, try_read16_relaxed, call_mmio_read, readw_relaxed -> u16);
+ define_read!(read32_relaxed, try_read32_relaxed, call_mmio_read, readl_relaxed -> u32);
define_read!(
#[cfg(CONFIG_64BIT)]
read64_relaxed,
try_read64_relaxed,
+ call_mmio_read,
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!(write8, try_write8, call_mmio_write, writeb <- u8);
+ define_write!(write16, try_write16, call_mmio_write, writew <- u16);
+ define_write!(write32, try_write32, call_mmio_write, writel <- u32);
define_write!(
#[cfg(CONFIG_64BIT)]
write64,
try_write64,
+ call_mmio_write,
writeq <- 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!(write8_relaxed, try_write8_relaxed, call_mmio_write, writeb_relaxed <- u8);
+ define_write!(write16_relaxed, try_write16_relaxed, call_mmio_write, writew_relaxed <- u16);
+ define_write!(write32_relaxed, try_write32_relaxed, call_mmio_write, writel_relaxed <- u32);
define_write!(
#[cfg(CONFIG_64BIT)]
write64_relaxed,
try_write64_relaxed,
+ call_mmio_write,
writeq_relaxed <- u64
);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 2/5] rust: io: factor out MMIO read/write macros
2025-10-16 21:02 ` [PATCH v2 2/5] rust: io: factor out MMIO read/write macros Zhi Wang
@ 2025-10-17 11:01 ` Danilo Krummrich
0 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-10-17 11:01 UTC (permalink / raw)
To: Zhi Wang
Cc: rust-for-linux, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl,
tmgross, linux-pci, linux-kernel, cjia, smitra, ankita, aniketa,
kwankhede, targupta, zhiwang, acourbot, joelagnelf, jhubbard,
markus.probst
On Thu Oct 16, 2025 at 11:02 PM CEST, Zhi Wang wrote:
> @@ -122,10 +139,9 @@ macro_rules! define_read {
> $(#[$attr])*
> #[inline]
> pub fn $name(&self, offset: usize) -> $type_name {
> - let addr = self.io_addr_assert::<$type_name>(offset);
> + 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) }
> + $call_macro!($c_fn, self, offset, $type_name, _addr).unwrap_or(!0)
This introduces unnecessary CPU cycles in the infallible case, I think the
implementations of the try and non-try variants should remain separate.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/5] rust: pci: add a helper to query configuration space size
2025-10-16 21:02 [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Zhi Wang
2025-10-16 21:02 ` [PATCH v2 1/5] rust/io: factor common I/O helpers into Io trait and specialize Mmio<SIZE> Zhi Wang
2025-10-16 21:02 ` [PATCH v2 2/5] rust: io: factor out MMIO read/write macros Zhi Wang
@ 2025-10-16 21:02 ` Zhi Wang
2025-10-17 11:09 ` Danilo Krummrich
2025-10-16 21:02 ` [PATCH v2 4/5] rust: pci: add config space read/write support Zhi Wang
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Zhi Wang @ 2025-10-16 21:02 UTC (permalink / raw)
To: rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiw, zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
Expose a safe Rust wrapper for the `cfg_size` field of `struct pci_dev`,
allowing drivers to query the size of a device's configuration space.
This is useful for code that needs to know whether the device supports
extended configuration space (e.g. 256 vs 4096 bytes) when accessing PCI
configuration registers and apply runtime checks.
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
rust/kernel/pci.rs | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 77a8eb39ad32..34729c6f5665 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -514,6 +514,12 @@ 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.
+ pub fn cfg_size(&self) -> i32 {
+ // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+ unsafe { (*self.as_raw()).cfg_size }
+ }
}
impl Device<device::Bound> {
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 3/5] rust: pci: add a helper to query configuration space size
2025-10-16 21:02 ` [PATCH v2 3/5] rust: pci: add a helper to query configuration space size Zhi Wang
@ 2025-10-17 11:09 ` Danilo Krummrich
0 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-10-17 11:09 UTC (permalink / raw)
To: Zhi Wang
Cc: rust-for-linux, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl,
tmgross, linux-pci, linux-kernel, cjia, smitra, ankita, aniketa,
kwankhede, targupta, zhiwang, acourbot, joelagnelf, jhubbard,
markus.probst
On Thu Oct 16, 2025 at 11:02 PM CEST, Zhi Wang wrote:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 77a8eb39ad32..34729c6f5665 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -514,6 +514,12 @@ 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.
> + pub fn cfg_size(&self) -> i32 {
> + // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> + unsafe { (*self.as_raw()).cfg_size }
> + }
> }
This should probably return an enum type:
enum ConfigSize {
Normal = 256,
Extended = 4096,
}
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 4/5] rust: pci: add config space read/write support
2025-10-16 21:02 [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Zhi Wang
` (2 preceding siblings ...)
2025-10-16 21:02 ` [PATCH v2 3/5] rust: pci: add a helper to query configuration space size Zhi Wang
@ 2025-10-16 21:02 ` Zhi Wang
2025-10-16 21:02 ` [PATCH v2 5/5] nova-core: test configuration routine Zhi Wang
2025-10-21 14:07 ` [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Alexandre Courbot
5 siblings, 0 replies; 16+ messages in thread
From: Zhi Wang @ 2025-10-16 21:02 UTC (permalink / raw)
To: rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiw, zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
accessors for PCI configuration space. The new type implements the
`Io` trait to share offset validation and bound-checking logic with
others.
Cc: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
rust/kernel/pci.rs | 65 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 64 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 34729c6f5665..d7e0f18169d7 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -10,7 +10,8 @@
devres::Devres,
driver,
error::{from_result, to_result, Result},
- io::{Mmio, MmioRaw},
+ io::{define_read, define_write},
+ io::{Io, Mmio, MmioRaw},
irq::{self, IrqRequest},
str::CStr,
sync::aref::ARef,
@@ -305,6 +306,63 @@ pub struct Device<Ctx: device::DeviceContext = device::Normal>(
PhantomData<Ctx>,
);
+/// Represents 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). The actual size is obtained
+/// from the underlying `struct pci_dev` via [`Device::cfg_size`].
+pub struct ConfigSpace<'a, const SIZE: usize = 4096> {
+ pdev: &'a Device<device::Bound>,
+}
+
+impl<'a, const SIZE: usize> Io<SIZE> for ConfigSpace<'a, SIZE> {
+ /// Returns the base address of this mapping.
+ #[inline]
+ fn addr(&self) -> usize {
+ 0
+ }
+
+ /// Returns the maximum size of this mapping.
+ #[inline]
+ fn maxsize(&self) -> usize {
+ self.pdev.cfg_size() as usize
+ }
+}
+
+macro_rules! call_config_read {
+ ($c_fn:ident, $self:ident, $offset:expr, $ty:ty, $_addr:expr) => {{
+ let mut val: $ty = 0;
+ let ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $offset as i32, &mut val) };
+ (ret == 0)
+ .then_some(Ok(val))
+ .unwrap_or_else(|| Err(Error::from_errno(ret)))
+ }};
+}
+
+macro_rules! call_config_write {
+ ($c_fn:ident, $self:ident, $offset:expr, $ty:ty, $_addr:expr, $value:expr) => {{
+ let ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $offset as i32, $value) };
+ (ret == 0)
+ .then_some(Ok(()))
+ .unwrap_or_else(|| Err(Error::from_errno(ret)))
+ }};
+}
+
+#[allow(dead_code)]
+impl<'a, const SIZE: usize> ConfigSpace<'a, SIZE> {
+ define_read!(read8, try_read8, call_config_read, pci_read_config_byte -> u8);
+ define_read!(read16, try_read16, call_config_read, pci_read_config_word -> u16);
+ define_read!(read32, try_read32, call_config_read, pci_read_config_dword -> u32);
+
+ define_write!(write8, try_write8, call_config_write, pci_write_config_byte <- u8);
+ define_write!(write16, try_write16, call_config_write, pci_write_config_word <- u16);
+ define_write!(write32, try_write32, call_config_write, pci_write_config_dword <- u32);
+}
+
/// A PCI BAR to perform I/O-Operations on.
///
/// # Invariants
@@ -582,6 +640,11 @@ pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
request, flags, name, handler,
))
}
+
+ /// Return an initialized object.
+ pub fn config_space<'a>(&'a self) -> Result<ConfigSpace<'a>> {
+ Ok(ConfigSpace { pdev: self })
+ }
}
impl Device<device::Core> {
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 5/5] nova-core: test configuration routine.
2025-10-16 21:02 [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Zhi Wang
` (3 preceding siblings ...)
2025-10-16 21:02 ` [PATCH v2 4/5] rust: pci: add config space read/write support Zhi Wang
@ 2025-10-16 21:02 ` Zhi Wang
2025-10-17 4:55 ` Zhi Wang
2025-10-21 14:07 ` [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Alexandre Courbot
5 siblings, 1 reply; 16+ messages in thread
From: Zhi Wang @ 2025-10-16 21:02 UTC (permalink / raw)
To: rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiw, zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
drivers/gpu/nova-core/driver.rs | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index edc72052e27a..48538aa72dfd 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -67,6 +67,10 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
let bar_clone = Arc::clone(&devres_bar);
let bar = bar_clone.access(pdev.as_ref())?;
+ let config = pdev.config_space()?;
+
+ dev_info!(pdev.as_ref(), "Nova Core GPU driver read pci {:x}.\n", config.read16(0));
+
let this = KBox::pin_init(
try_pin_init!(Self {
gpu <- Gpu::new(pdev, devres_bar, bar),
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 5/5] nova-core: test configuration routine.
2025-10-16 21:02 ` [PATCH v2 5/5] nova-core: test configuration routine Zhi Wang
@ 2025-10-17 4:55 ` Zhi Wang
2025-10-17 11:11 ` Danilo Krummrich
0 siblings, 1 reply; 16+ messages in thread
From: Zhi Wang @ 2025-10-17 4:55 UTC (permalink / raw)
To: rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
On Thu, 16 Oct 2025 21:02:50 +0000
Zhi Wang <zhiw@nvidia.com> wrote:
Hi guys:
To avoid misunderstanding, this is just meant for folks to test, not for
merging. I will drop this one in the next re-spin.
Z.
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
> drivers/gpu/nova-core/driver.rs | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/nova-core/driver.rs
> b/drivers/gpu/nova-core/driver.rs index edc72052e27a..48538aa72dfd
> 100644 --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -67,6 +67,10 @@ fn probe(pdev: &pci::Device<Core>, _info:
> &Self::IdInfo) -> Result<Pin<KBox<Self let bar_clone =
> Arc::clone(&devres_bar); let bar = bar_clone.access(pdev.as_ref())?;
>
> + let config = pdev.config_space()?;
> +
> + dev_info!(pdev.as_ref(), "Nova Core GPU driver read pci
> {:x}.\n", config.read16(0)); +
> let this = KBox::pin_init(
> try_pin_init!(Self {
> gpu <- Gpu::new(pdev, devres_bar, bar),
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 5/5] nova-core: test configuration routine.
2025-10-17 4:55 ` Zhi Wang
@ 2025-10-17 11:11 ` Danilo Krummrich
0 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-10-17 11:11 UTC (permalink / raw)
To: Zhi Wang
Cc: rust-for-linux, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl,
tmgross, linux-pci, linux-kernel, cjia, smitra, ankita, aniketa,
kwankhede, targupta, zhiwang, acourbot, joelagnelf, jhubbard,
markus.probst
On Fri Oct 17, 2025 at 6:55 AM CEST, Zhi Wang wrote:
> On Thu, 16 Oct 2025 21:02:50 +0000
> Zhi Wang <zhiw@nvidia.com> wrote:
>
> Hi guys:
>
> To avoid misunderstanding, this is just meant for folks to test, not for
> merging. I will drop this one in the next re-spin.
I suggest adding it to samples/rust/rust_driver_pci.rs instead. :)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/5] rust: pci: add config space read/write support, take 1
2025-10-16 21:02 [PATCH v2 0/5] rust: pci: add config space read/write support, take 1 Zhi Wang
` (4 preceding siblings ...)
2025-10-16 21:02 ` [PATCH v2 5/5] nova-core: test configuration routine Zhi Wang
@ 2025-10-21 14:07 ` Alexandre Courbot
5 siblings, 0 replies; 16+ messages in thread
From: Alexandre Courbot @ 2025-10-21 14:07 UTC (permalink / raw)
To: Zhi Wang, rust-for-linux
Cc: dakr, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, linux-pci,
linux-kernel, cjia, smitra, ankita, aniketa, kwankhede, targupta,
zhiwang, acourbot, joelagnelf, jhubbard, markus.probst
On Fri Oct 17, 2025 at 6:02 AM JST, Zhi Wang wrote:
> 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 abstrations. Bascially, we are thinking of introducing another
> backend for PCI configuration space access similar with Kernel::Io.
>
> This ideas of this series are:
>
> - Factor out a common trait 'Io' 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
> backend.
>
> - Add a helper to query configuration space size. This is mostly for
> runtime check.
>
> - Implement the PCI configuration space access backend in PCI
> abstractions.
>
> 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 implemention to use
> 'MmioRaw'.
>
> - Intorduce pci::Device<Bound>::config_space(). (Danilo)
>
> - Implement both infallible and fallible read/write routines, the device
> driver devicdes which version should be used.
>
> Moving forward:
>
> - Define and use register! macros.
> - Introduce { cap, ecap } search and read.
>
> RFC v1:
> https://lore.kernel.org/all/20251010080330.183559-1-zhiw@nvidia.com/
One small nit: the title of this series
[PATCH v2 0/5] rust: pci: add config space read/write support, take 1
Is a tad confusing. How can this be take 1, if this is a v2? Also there
is no v1, the previous revision was a RFC.
Technically this should have been [PATCH 0/5] or [PATCH v1 0/5]. `b4` is
great to avoid this kind of problems, and a huge time saver generally
speaking. Can't recommend it enough.
^ permalink raw reply [flat|nested] 16+ messages in thread