[RFC 0/6] rust: pci: add config space read/write support

[RFC 0/6] rust: pci: add config space read/write support

[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 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 IoRegion 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.

  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).

- Add a helper to query configuration space size. This is mostly for
  runtime check.

- Implement the PCI configuration space access backend in PCI
  Abstractions.

  In detail:

  * `struct ConfigSpace<SIZE>` wrapping a `pdev: ARef<Device>`.
  * `IoRegion` implementation returning the device's `cfg_size`.
  * `call_config_read!` and `call_config_write!` macros bridging to
    the existing C helpers (`pci_read_config_*` /
    `pci_write_config_*`).
  * Read accessors: `read8/16/32` and `try_read8/16/32`.
  * Write accessors: `write8/16/32` and `try_write8/16/32`.

- Introduce an rust wrapper for pci_find_ext_capability(). Thus, the
  rust driver can locate the extended PCI configuration caps.

Open:

The current kernel::Io MMIO read/write doesn't return a failure, because
{read, write}{b, w, l}() are always successful. This is not true in
pci_{read, write}_config{byte, word, dword}() because a PCI device
can be disconnected from the bus. Thus a failure is returned.

- Do we still need a non-fallible version of read/write for config space?
A rust panic in accessing the PCI config space when device is
unexpectedly disconnected seems overkill.

Zhi Wang (6):
  rust: io: refactor Io<SIZE> helpers into IoRegion trait
  rust: io: factor out MMIO read/write macros
  rust: pci: add a helper to query configuration space size
  rust: pci: add config space read/write support
  rust: pci: add helper to find extended capability
  [!UPSTREAM] nova-core: test configuration routine.

 drivers/gpu/nova-core/driver.rs |   4 +
 rust/kernel/io.rs               | 132 +++++++++++++++++++++-----------
 rust/kernel/pci.rs              |  74 ++++++++++++++++++
 3 files changed, 164 insertions(+), 46 deletions(-)

-- 
2.47.3

[RFC 1/6] rust: io: refactor Io<SIZE> helpers into IoRegion trait

[Zhi Wang]

The existing `Io<SIZE>` implementation embedded utility methods such
as `addr()`, `maxsize()`, and offset validation helpers directly on the
struct.

To allow sharing of I/O region check logic across different region types
(e.g. MMIO BARs, PCI config space), move common bound-checking and
address arithmetic into a reusable trait.

No functional change intended.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/io.rs | 65 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 21 deletions(-)

diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 03b467722b86..f4727f3b954e 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -171,32 +171,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 IoRegion<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
@@ -205,6 +197,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()) {
@@ -216,12 +210,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> IoRegion<SIZE> for Io<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> 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() }
+    }
 
     define_read!(read8, try_read8, readb -> u8);
     define_read!(read16, try_read16, readw -> u16);
-- 
2.47.3

[RFC 2/6] rust: io: factor out MMIO read/write macros

[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.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/io.rs | 67 +++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 25 deletions(-)

diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index f4727f3b954e..8d67d46777db 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -112,8 +112,22 @@ pub fn maxsize(&self) -> usize {
 #[repr(transparent)]
 pub struct Io<const SIZE: usize = 0>(IoRaw<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.
+        unsafe { bindings::$c_fn($addr as *const c_void) }
+    };
+}
+
+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.
+        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
@@ -121,10 +135,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)
         }
 
         /// Read IO data from a given offset.
@@ -133,16 +146,17 @@ 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) })
+            Ok($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
@@ -150,10 +164,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) }
+            $call_macro!($c_fn, self, offset, $type_name, _addr, value)
         }
 
         /// Write IO data from a given offset.
@@ -162,14 +175,14 @@ 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) }
+            $call_macro!($c_fn, self, offset, $type_name, _addr, value);
             Ok(())
         }
     };
 }
+pub(crate) use define_write;
 
 /// Represents a region of I/O space of a fixed size.
 ///
@@ -246,43 +259,47 @@ pub unsafe fn from_raw(raw: &IoRaw<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

[RFC 3/6] rust: pci: add a helper to query configuration space size

[Zhi Wang]

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 887ee611b553..7a107015e7d2 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -410,6 +410,12 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
         // - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`.
         Ok(unsafe { bindings::pci_resource_len(self.as_raw(), bar.try_into()?) })
     }
+
+    /// 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

[RFC 4/6] rust: pci: add config space read/write support

[Zhi Wang]

Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
accessors for PCI configuration space. The new type implements the
`IoRegion` trait to share offset validation and bound-checking logic with
MMIO regions.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/pci.rs | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 7a107015e7d2..2f94b370fc99 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -12,6 +12,8 @@
     error::{from_result, to_result, Result},
     io::Io,
     io::IoRaw,
+    io::IoRegion,
+    io::{define_read, define_write},
     str::CStr,
     types::{ARef, Opaque},
     ThisModule,
@@ -275,6 +277,65 @@ 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<const SIZE: usize = 0> {
+    pdev: ARef<Device>,
+}
+
+impl<const SIZE: usize> IoRegion<SIZE> for ConfigSpace<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) };
+        val
+    }};
+}
+
+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) };
+    }};
+}
+
+#[allow(dead_code)]
+impl<const SIZE: usize> ConfigSpace<SIZE> {
+    /// Return an initialized object.
+    pub fn new(pdev: &Device) -> Result<Self> {
+        Ok(ConfigSpace {
+            pdev: pdev.into(),
+        })
+    }
+
+    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
-- 
2.47.3

[RFC 5/6] rust: pci: add helper to find extended capability

[Zhi Wang]

Add a safe wrapper for `pci_find_ext_capability()` that returns an
`Option<u16>` indicating the offset of a given PCIe extended capability.

This allows Rust drivers to query extended capabilities without dealing
with raw pointers or integer return codes. The method returns `None`
when the capability is not present.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/pci.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 2f94b370fc99..5d9c5eef5c85 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -477,6 +477,13 @@ 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 }
     }
+
+    /// Find the extended capability
+    pub fn find_ext_capability(&self, cap: i32) -> Option<u16> {
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct pci_dev`.
+        let offset = unsafe { bindings::pci_find_ext_capability(self.as_raw(), cap) };
+        (offset != 0).then(|| offset as u16)
+    }
 }
 
 impl Device<device::Bound> {
-- 
2.47.3

[RFC 6/6] [!UPSTREAM] nova-core: test configuration routine.

[Zhi Wang]

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 1380b47617f7..605d9146113c 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -44,6 +44,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 = pci::ConfigSpace::<{bindings::PCI_CFG_SPACE_EXP_SIZE as usize}>::new(pdev)?;
+
+        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

Re: [RFC 0/6] rust: pci: add config space read/write support

[Danilo Krummrich]

Hi Zhi,

(Cc: Alex, Joel, John, Markus)

On Fri Oct 10, 2025 at 10:03 AM CEST, Zhi Wang wrote:
> This ideas of this series are:
>
> - Factor out a common trait IoRegion for other accessors to share the
>   same compiling/runtime check like before.

Yes, this is something we want to have in general:

Currently, we have a single I/O backend (struct Io) which is used for generic
MMIO. However, we should make Io a trait instead and require a new MMIO type to
implement the trait, where the trait methods would remain to be
{try_}{read,write}{8,16,..}().

We need the same thing for other I/O backends, such as I2C, etc.

@Markus: Most of the design aspects for the PCI configuration space below should
apply to I2C I/O accessors as well.

>   In detail:
>
>   * `struct ConfigSpace<SIZE>` wrapping a `pdev: ARef<Device>`.

There are two cases:

  (1) I/O backends that embedd a dedicated device resource. For instance, a
      pci::Bar embedds an iomapped pointer that must be wrapped with Devres to
      ensure it can't outlive the driver being bound to its corresponding
      device.

      In this case we have a method pci::Device<Bound>::iomap_region(), which
      returns a Devres<pci::Bar>.

  (2) I/O backends that don't need to embedd a dedicated device resource because
      the resource is already attached to the device itself. This is the case
      with the PCI configuration space; drivers don't need to create their own
      mapping, but can access it directly through the device.

      For this case we want a method pci::Device<Bound>::config_space() that
      returns a ConfigSpace<'a>, where 'a is the lifetime of the
      &'a Device<Bound> given to config_space().

      This ensures that the ConfigSpace structure still serves as I/O backend
      for the types generated by the register!() macro, but at the same time
      can't outlife the scope of the bound device.

      (Drivers shouldn't be able to write the PCI configuration space of a
      device they're not bound to.)

Besides that, we should also use the register!() macro to create the common
configuration space register types in the PCI core for driver to use.

Of course, there is no need to (re-)implement the following one, but it's a
good example:

	register!(PCI_CONFIG_ID @ 0x0 {
	    31:16   device_id ?=> pci::DeviceId, "Device ID";
	    15:0    vendor_id ?=> pci::VendorId, "Vendor ID";
	});

	// Assume we have a `&pci::Device<Bound>`, e.g. from probe().
	let pdev: &pci::Device<Bound> = ...;

	// Grab the configuration space I/O backend; lives as long as `pdev`.
	let config_space = pdev.config_space();

	// Read the first standard register of the configuration space header.
	let id = PCI_CONFIG_ID::read(config_space);

	// `id.vendor()` returns a `pci::Vendor`. Since it implements `Display`
	// the `dev_info()` call prints the actual vendor string.
	dev_info!(pdev.as_ref(), "VendorId={}\n", id.vendor());

> Open:
>
> The current kernel::Io MMIO read/write doesn't return a failure, because
> {read, write}{b, w, l}() are always successful. This is not true in
> pci_{read, write}_config{byte, word, dword}() because a PCI device
> can be disconnected from the bus. Thus a failure is returned.

This is in fact also true for the PCI configuration space. The PCI configuration
space has a minimum size that is known at compile time. All registers within
this minimum size can be access in an infallible way with the non try_*()
methods.

The main idea behind the fallible and infallible accessors is that you can
assert a minimum expected size of an I/O backend (e.g. a PCI bar). I.e. drivers
know their minimum requirements of the size of the I/O region. If the I/O
backend can fulfill the request we can be sure about the minimum size and hence
accesses with offsets that are known at compile time can be infallible (because
we know the minimum accepted size of the I/O backend at compile time as well).

Accesses with offsets that are not known at compile time still remain fallible
of course. That's why we have both, e.g. write32() and try_write32().

Re: [RFC 0/6] rust: pci: add config space read/write support

[Zhi Wang]

On Mon, 13 Oct 2025 17:39:41 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> Hi Zhi,
> 
> (Cc: Alex, Joel, John, Markus)
> 
> On Fri Oct 10, 2025 at 10:03 AM CEST, Zhi Wang wrote:
> > This ideas of this series are:
> >
> > - Factor out a common trait IoRegion for other accessors to share
> > the same compiling/runtime check like before.  
> 
> Yes, this is something we want to have in general:
> 
> Currently, we have a single I/O backend (struct Io) which is used for
> generic MMIO. However, we should make Io a trait instead and require
> a new MMIO type to implement the trait, where the trait methods would
> remain to be {try_}{read,write}{8,16,..}().
> 

I was considering the same when writing this series. The concern is
mostly about having to change the drivers' MMIO code to adapt to the
re-factor.

IMHO, if we are seeing the necessity of this re-factor, we should do it
before it got more usage. This could be the part 1 of the next spin.

and adding pci::Device<Bound>::config_space() could be part 2 and
register! marco could be part 3.

ditto

> 
> > The current kernel::Io MMIO read/write doesn't return a failure,
> > because {read, write}{b, w, l}() are always successful. This is not
> > true in pci_{read, write}_config{byte, word, dword}() because a PCI
> > device can be disconnected from the bus. Thus a failure is
> > returned.  
> 
> This is in fact also true for the PCI configuration space. The PCI
> configuration space has a minimum size that is known at compile time.
> All registers within this minimum size can be access in an infallible
> way with the non try_*() methods.
> 
> The main idea behind the fallible and infallible accessors is that
> you can assert a minimum expected size of an I/O backend (e.g. a PCI
> bar). I.e. drivers know their minimum requirements of the size of the
> I/O region. If the I/O backend can fulfill the request we can be sure
> about the minimum size and hence accesses with offsets that are known
> at compile time can be infallible (because we know the minimum
> accepted size of the I/O backend at compile time as well).
> 

For PCI configuration space. Standard configuration space should be
readable and to access the extended configuration space, the MCFG
should be enabled beforehand and the enabling is system-wide.

I think the size of standard configuration space falls in "falliable
accessors", and the extended configuration space falls in "infalliable"
parts

But for the "infallible" part in PCI configuration space, the device
can be disconnected from the PCI bus. E.g. unresponsive device. In that
case, the current PCI core will mark the device as "disconnected" before
they causes more problems and any access to the configuration space
will fail with an error code. This can also happen on access to
"infalliable" part.

How should we handle this case in "infallible" accessors of PCI
configuration space? Returning Result<> seems doesn't fit the concept
of "infallible", but causing a rust panic seems overkill...

Z.

Re: [RFC 0/6] rust: pci: add config space read/write support

[Danilo Krummrich]

On Mon Oct 13, 2025 at 8:25 PM CEST, Zhi Wang wrote:
> I was considering the same when writing this series. The concern is
> mostly about having to change the drivers' MMIO code to adapt to the
> re-factor.

For this you need to adjust the register macro to take something that
dereferences to `T: Io` instead of something that dereferences to `Io`.

This change should be trivial.

> IMHO, if we are seeing the necessity of this re-factor, we should do it
> before it got more usage. This could be the part 1 of the next spin.

Yes, you can do it in a separete series. But I'd also be fine if you do both in
a single one. The required code changes shouldn't be crazy.

> and adding pci::Device<Bound>::config_space() could be part 2 and
> register! marco could be part 3.

Part 3 has to happen with part 1 anyways, otherwise it breaks compilation.

> I think the size of standard configuration space falls in "falliable
> accessors", and the extended configuration space falls in "infalliable"
> parts

Both can be infallible. The standard configuration space size is constant, hence
all accesses to the standard configuration space with constant offsets can be
infallible.

For the extended space it depends what a driver can assert, just like for any
MMIO space.

However, you seem to talk about whether a physical device is still present.

> But for the "infallible" part in PCI configuration space, the device
> can be disconnected from the PCI bus. E.g. unresponsive device. In that
> case, the current PCI core will mark the device as "disconnected" before
> they causes more problems and any access to the configuration space
> will fail with an error code. This can also happen on access to
> "infalliable" part.
>
> How should we handle this case in "infallible" accessors of PCI
> configuration space? Returning Result<> seems doesn't fit the concept
> of "infallible", but causing a rust panic seems overkill...

Panics are for the "the machine is unrecoverably dead" case, this clearly isn't
one of them. :)

I think we should do the same as with "normal" MMIO and just return the value,
i.e. all bits set (PCI_ERROR_RESPONSE).

The window between physical unplug and the driver core unbinds the driver should
be pretty small and drivers have to be able to deal with garbage values read
from registers anyways.

If we really want to handle it, you can only implement the try_*() methods and
for the non-try_*() methods throw a compile time error, but I don't see a reason
for that.

Re: [RFC 0/6] rust: pci: add config space read/write support

[Zhi Wang]

On Mon, 13 Oct 2025 22:02:55 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

ditto

> > But for the "infallible" part in PCI configuration space, the device
> > can be disconnected from the PCI bus. E.g. unresponsive device. In
> > that case, the current PCI core will mark the device as
> > "disconnected" before they causes more problems and any access to
> > the configuration space will fail with an error code. This can also
> > happen on access to "infalliable" part.
> >
> > How should we handle this case in "infallible" accessors of PCI
> > configuration space? Returning Result<> seems doesn't fit the
> > concept of "infallible", but causing a rust panic seems overkill...
> 
> Panics are for the "the machine is unrecoverably dead" case, this
> clearly isn't one of them. :)
> 
> I think we should do the same as with "normal" MMIO and just return
> the value, i.e. all bits set (PCI_ERROR_RESPONSE).
> 
> The window between physical unplug and the driver core unbinds the
> driver should be pretty small and drivers have to be able to deal
> with garbage values read from registers anyways.
> 

Was thinking about this these days. Panic seems overkill. Given the
current semantics of "infallible" (non-try) and "fallible" (try) is
decided by the driver, I think we can do the same for PCI configuration
space with the case of "PCI device could be disconnected".

- We implement both for PCI configuration space.

- The driver decides to use "non-try" or "try" according to the device
  characteristic. E.g. if the device is simple, hardly to be
  unresponsive, not supporting hotplug, or the driver is in a context
  that the device is surely responsive. The driver can be confident
  to use the "infallible" version and tolerate garbage values in the
  rare situation. Otherwise the driver can use "faillble" version to
  capture the error code if it is sure the device can sometimes be
  unresponsive.

> If we really want to handle it, you can only implement the try_*()
> methods and for the non-try_*() methods throw a compile time error,
> but I don't see a reason for that.