Message-Id: <DFV3N1SHS8SK.338Z25H89HINU@garyguo.net>
Date: Thu, 22 Jan 2026 11:59:14 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Cc: <dakr@...nel.org>, <aliceryhl@...gle.com>, <bhelgaas@...gle.com>,
 <kwilczynski@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <lossin@...nel.org>, <a.hindborg@...nel.org>, <tmgross@...ch.edu>,
 <markus.probst@...teo.de>, <helgaas@...nel.org>, <cjia@...dia.com>,
 <smitra@...dia.com>, <ankita@...dia.com>, <aniketa@...dia.com>,
 <kwankhede@...dia.com>, <targupta@...dia.com>, <acourbot@...dia.com>,
 <joelagnelf@...dia.com>, <jhubbard@...dia.com>, <zhiwang@...nel.org>,
 <daniel.almeida@...labora.com>
Subject: Re: [PATCH v12 4/5] rust: pci: add config space read/write support

On Wed Jan 21, 2026 at 8:22 PM GMT, Zhi Wang wrote:
> Drivers might need to access PCI config space for querying capability
> structures and access the registers inside the structures.
>
> For Rust drivers need to access PCI config space, the Rust PCI abstraction
> needs to support it in a way that upholds Rust's safety principles.
>
> Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
> accessors for PCI config space. The new type implements the `Io` trait and
> `IoCapable<T>` for u8, u16, and u32 to share offset validation and
> bound-checking logic with other I/O backends.
>
> The `ConfigSpace` type uses marker types (`Normal` and `Extended`) to
> represent configuration space sizes at the type level.
>
> Cc: Alexandre Courbot <acourbot@...dia.com>
> Cc: Danilo Krummrich <dakr@...nel.org>
> Cc: Gary Guo <gary@...yguo.net>
> Cc: Joel Fernandes <joelagnelf@...dia.com>
> Signed-off-by: Zhi Wang <zhiw@...dia.com>
> ---
>  rust/kernel/pci.rs    |   7 +-
>  rust/kernel/pci/io.rs | 167 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 172 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 82e128431f08..9020959ce0c7 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -40,7 +40,12 @@
>      ClassMask,
>      Vendor, //
>  };
> -pub use self::io::Bar;
> +pub use self::io::{
> +    Bar,
> +    ConfigSpaceSize,
> +    Extended,
> +    Normal, //
> +};
>  pub use self::irq::{
>      IrqType,
>      IrqTypes,
> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index e3377397666e..39df41d0eaab 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs
> @@ -8,13 +8,149 @@
>      device,
>      devres::Devres,
>      io::{
> +        define_read,
> +        define_write,
> +        Io,
> +        IoCapable,
> +        IoKnownSize,
>          Mmio,
>          MmioRaw, //
>      },
>      prelude::*,
>      sync::aref::ARef, //
>  };
> -use core::ops::Deref;
> +use core::{
> +    marker::PhantomData,
> +    ops::Deref, //
> +};
> +
> +/// Marker type for normal (256-byte) PCI configuration space.
> +pub struct Normal;
> +
> +/// Marker type for extended (4096-byte) PCIe configuration space.
> +pub struct Extended;
> +
> +/// Trait for PCI configuration space size markers.
> +///
> +/// This trait is implemented by [`Normal`] and [`Extended`] to provide
> +/// compile-time knowledge of the configuration space size.
> +pub trait ConfigSpaceSize {
> +    /// The size of this configuration space in bytes.
> +    const SIZE: usize;
> +}
> +
> +impl ConfigSpaceSize for Normal {
> +    const SIZE: usize = 256;
> +}
> +
> +impl ConfigSpaceSize for Extended {
> +    const SIZE: usize = 4096;
> +}
> +
> +/// 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 parameter `S` indicates the maximum size of the configuration space.
> +/// Use [`Normal`] for 256-byte legacy configuration space or [`Extended`] for
> +/// 4096-byte PCIe extended configuration space (default).
> +pub struct ConfigSpace<'a, S: ConfigSpaceSize = Extended> {
> +    pub(crate) pdev: &'a Device<device::Bound>,
> +    _marker: PhantomData<S>,
> +}
> +
> +/// Internal helper macros used to invoke C PCI configuration space read functions.
> +///
> +/// This macro is intended to be used by higher-level PCI configuration space access macros
> +/// (define_read) and provides a unified expansion for infallible vs. fallible read semantics. It
> +/// emits a direct call into the corresponding C helper and performs the required cast to the Rust
> +/// return type.
> +///
> +/// # Parameters
> +///
> +/// * `$c_fn` – The C function performing the PCI configuration space write.
> +/// * `$self` – The I/O backend object.
> +/// * `$ty` – The type of the value to read.
> +/// * `$addr` – The PCI configuration space offset to read.
> +///
> +/// This macro does not perform any validation; all invariants must be upheld by the higher-level
> +/// abstraction invoking it.
> +macro_rules! call_config_read {
> +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) => {{
> +        let mut val: $ty = 0;
> +        // SAFETY: By the type invariant `$self.pdev` is a valid address.
> +        // CAST: The offset is cast to `i32` because the C functions expect a 32-bit signed offset
> +        // parameter. PCI configuration space size is at most 4096 bytes, so the value always fits
> +        // within `i32` without truncation or sign change.
> +        // Return value from C function is ignored in infallible accessors.
> +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, &mut val) };
> +        val
> +    }};
> +}
> +
> +/// Internal helper macros used to invoke C PCI configuration space write functions.
> +///
> +/// This macro is intended to be used by higher-level PCI configuration space access macros
> +/// (define_write) and provides a unified expansion for infallible vs. fallible read semantics. It
> +/// emits a direct call into the corresponding C helper and performs the required cast to the Rust
> +/// return type.
> +///
> +/// # Parameters
> +///
> +/// * `$c_fn` – The C function performing the PCI configuration space write.
> +/// * `$self` – The I/O backend object.
> +/// * `$ty` – The type of the written value.
> +/// * `$addr` – The configuration space offset to write.
> +/// * `$value` – The value to write.
> +///
> +/// This macro does not perform any validation; all invariants must be upheld by the higher-level
> +/// abstraction invoking it.
> +macro_rules! call_config_write {
> +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
> +        // SAFETY: By the type invariant `$self.pdev` is a valid address.
> +        // CAST: The offset is cast to `i32` because the C functions expect a 32-bit signed offset
> +        // parameter. PCI configuration space size is at most 4096 bytes, so the value always fits
> +        // within `i32` without truncation or sign change.
> +        // Return value from C function is ignored in infallible accessors.
> +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, $value) };
> +    };
> +}
> +
> +// PCI configuration space supports 8, 16, and 32-bit accesses.
> +impl<'a, S: ConfigSpaceSize> IoCapable<u8> for ConfigSpace<'a, S> {}
> +impl<'a, S: ConfigSpaceSize> IoCapable<u16> for ConfigSpace<'a, S> {}
> +impl<'a, S: ConfigSpaceSize> IoCapable<u32> for ConfigSpace<'a, S> {}
> +
> +impl<'a, S: ConfigSpaceSize> Io for ConfigSpace<'a, S> {
> +    const MIN_SIZE: usize = S::SIZE;
> +
> +    /// Returns the base address of the I/O region. It is always 0 for configuration space.
> +    #[inline]
> +    fn addr(&self) -> usize {
> +        0
> +    }
> +
> +    /// Returns the maximum size of the configuration space.
> +    #[inline]
> +    fn maxsize(&self) -> usize {
> +        self.pdev.cfg_size().map_or(0, |v| v)
> +    }
> +
> +    // PCI configuration space does not support fallible operations.
> +    // The default implementations from the Io trait are not used.
> +
> +    define_read!(infallible, read8, call_config_read(pci_read_config_byte) -> u8);
> +    define_read!(infallible, read16, call_config_read(pci_read_config_word) -> u16);
> +    define_read!(infallible, read32, call_config_read(pci_read_config_dword) -> u32);
> +
> +    define_write!(infallible, write8, call_config_write(pci_write_config_byte) <- u8);
> +    define_write!(infallible, write16, call_config_write(pci_write_config_word) <- u16);
> +    define_write!(infallible, write32, call_config_write(pci_write_config_dword) <- u32);
> +}
> +
> +/// Marker trait indicating ConfigSpace has a known size at compile time.
> +impl<'a, S: ConfigSpaceSize> IoKnownSize for ConfigSpace<'a, S> {}
>  
>  /// A PCI BAR to perform I/O-Operations on.
>  ///
> @@ -144,4 +280,33 @@ pub fn iomap_region<'a>(
>      ) -> impl PinInit<Devres<Bar>, Error> + 'a {
>          self.iomap_region_sized::<0>(bar, name)
>      }
> +
> +    /// Returns the size of configuration space in bytes.
> +    fn cfg_size(&self) -> Result<usize> {
> +        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> +        let size = unsafe { (*self.as_raw()).cfg_size };
> +        match size {
> +            256 | 4096 => Ok(size as usize),
> +            _ => {
> +                debug_assert!(false);
> +                Err(EINVAL)
> +            }
> +        }
> +    }

This method is only invoked from maxsize, which turns error into `0`. Do apart
from the debug assertion, the error code is pointless. I think this function
should just return `usize` as it's specified in the device (we should trust the
C side that the value is sensible).

The check, as Alex mentioned, need to be done when ConfigSpace is created in
the first place and is too late when you already hand out `Ok(ConfigSpace)`.

Best,
Gary

> +
> +    /// Return an initialized normal (256-byte) config space object.
> +    pub fn config_space<'a>(&'a self) -> Result<ConfigSpace<'a, Normal>> {
> +        Ok(ConfigSpace {
> +            pdev: self,
> +            _marker: PhantomData,
> +        })
> +    }
> +
> +    /// Return an initialized extended (4096-byte) config space object.
> +    pub fn config_space_extended<'a>(&'a self) -> Result<ConfigSpace<'a, Extended>> {
> +        Ok(ConfigSpace {
> +            pdev: self,
> +            _marker: PhantomData,
> +        })
> +    }
>  }

Powered by blists - more mailing lists