[v6] rust: pci: add config space read/write support

[PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

Posted by Zhi Wang 3 months ago

Drivers might need to access PCI config space for querying capability
structures and access the registers inside the structures.

For Rust drivers need to access PCI config space, the Rust PCI abstraction
needs to support it in a way that upholds Rust's safety principles.

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

Cc: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/pci.rs    | 41 ++++++++++++++++++++++-
 rust/kernel/pci/io.rs | 75 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 114 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 410b79d46632..d8048c7d0f32 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -39,7 +39,10 @@
     ClassMask,
     Vendor, //
 };
-pub use self::io::Bar;
+pub use self::io::{
+    Bar,
+    ConfigSpace, //
+};
 pub use self::irq::{
     IrqType,
     IrqTypes,
@@ -330,6 +333,28 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
     }
 }
 
+/// Represents the size of a PCI configuration space.
+///
+/// PCI devices can have either a *normal* (legacy) configuration space of 256 bytes,
+/// or an *extended* configuration space of 4096 bytes as defined in the PCI Express
+/// specification.
+#[repr(usize)]
+pub enum ConfigSpaceSize {
+    /// 256-byte legacy PCI configuration space.
+    Normal = 256,
+
+    /// 4096-byte PCIe extended configuration space.
+    Extended = 4096,
+}
+
+impl ConfigSpaceSize {
+    /// Get the raw value of this enum.
+    #[inline(always)]
+    pub const fn as_raw(self) -> usize {
+        self as usize
+    }
+}
+
 impl Device {
     /// Returns the PCI vendor ID as [`Vendor`].
     ///
@@ -426,6 +451,20 @@ pub fn pci_class(&self) -> Class {
         // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
         Class::from_raw(unsafe { (*self.as_raw()).class })
     }
+
+    /// Returns the size of configuration space.
+    fn cfg_size(&self) -> Result<ConfigSpaceSize> {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        let size = unsafe { (*self.as_raw()).cfg_size };
+        match size {
+            256 => Ok(ConfigSpaceSize::Normal),
+            4096 => Ok(ConfigSpaceSize::Extended),
+            _ => {
+                debug_assert!(false);
+                Err(EINVAL)
+            }
+        }
+    }
 }
 
 impl Device<device::Core> {
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
index 2bbb3261198d..bb78a83fe92c 100644
--- a/rust/kernel/pci/io.rs
+++ b/rust/kernel/pci/io.rs
@@ -2,12 +2,19 @@
 
 //! PCI memory-mapped I/O infrastructure.
 
-use super::Device;
+use super::{
+    ConfigSpaceSize,
+    Device, //
+};
 use crate::{
     bindings,
     device,
     devres::Devres,
     io::{
+        define_read,
+        define_write,
+        Io,
+        IoInfallible,
         Mmio,
         MmioRaw, //
     },
@@ -16,6 +23,58 @@
 };
 use core::ops::Deref;
 
+/// The PCI configuration space of a device.
+///
+/// Provides typed read and write accessors for configuration registers
+/// using the standard `pci_read_config_*` and `pci_write_config_*` helpers.
+///
+/// The generic const parameter `SIZE` can be used to indicate the
+/// maximum size of the configuration space (e.g. 256 bytes for legacy,
+/// 4096 bytes for extended config space).
+pub struct ConfigSpace<'a, const SIZE: usize = { ConfigSpaceSize::Extended as usize }> {
+    pub(crate) pdev: &'a Device<device::Bound>,
+}
+
+macro_rules! call_config_read {
+    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) => {{
+        let mut val: $ty = 0;
+        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, &mut val) };
+        val
+    }};
+}
+
+macro_rules! call_config_write {
+    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
+        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, $value) };
+    };
+}
+
+impl<'a, const SIZE: usize> Io for ConfigSpace<'a, SIZE> {
+    const MIN_SIZE: usize = SIZE;
+
+    /// Returns the base address of the I/O region. It is always 0 for configuration space.
+    #[inline]
+    fn addr(&self) -> usize {
+        0
+    }
+
+    /// Returns the maximum size of the configuration space.
+    #[inline]
+    fn maxsize(&self) -> usize {
+        self.pdev.cfg_size().map_or(0, |v| v as usize)
+    }
+}
+
+impl<'a, const SIZE: usize> IoInfallible for ConfigSpace<'a, SIZE> {
+    define_read!(infallible, read8, call_config_read, pci_read_config_byte -> u8);
+    define_read!(infallible, read16, call_config_read, pci_read_config_word -> u16);
+    define_read!(infallible, read32, call_config_read, pci_read_config_dword -> u32);
+
+    define_write!(infallible, write8, call_config_write, pci_write_config_byte <- u8);
+    define_write!(infallible, write16, call_config_write, pci_write_config_word <- u16);
+    define_write!(infallible, write32, call_config_write, pci_write_config_dword <- u32);
+}
+
 /// A PCI BAR to perform I/O-Operations on.
 ///
 /// # Invariants
@@ -141,4 +200,18 @@ pub fn iomap_region<'a>(
     ) -> impl PinInit<Devres<Bar>, Error> + 'a {
         self.iomap_region_sized::<0>(bar, name)
     }
+
+    /// Return an initialized config space object.
+    pub fn config_space<'a>(
+        &'a self,
+    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw() }>> {
+        Ok(ConfigSpace { pdev: self })
+    }
+
+    /// Return an initialized config space object.
+    pub fn config_space_exteneded<'a>(
+        &'a self,
+    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
+        Ok(ConfigSpace { pdev: self })
+    }
 }
-- 
2.51.0

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

Posted by Joel Fernandes 2 months, 3 weeks ago

Hi Zhi,

On Mon, Nov 10, 2025 at 10:41:18PM +0200, Zhi Wang wrote:
[..]  
>  impl Device<device::Core> {
> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index 2bbb3261198d..bb78a83fe92c 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs
> @@ -2,12 +2,19 @@
>  
>  //! PCI memory-mapped I/O infrastructure.
>  
> -use super::Device;
> +use super::{
> +    ConfigSpaceSize,
> +    Device, //
> +};
>  use crate::{
>      bindings,
>      device,
>      devres::Devres,
>      io::{
> +        define_read,
> +        define_write,
> +        Io,
> +        IoInfallible,
>          Mmio,
>          MmioRaw, //
>      },
> @@ -16,6 +23,58 @@
>  };
>  use core::ops::Deref;
>  
> +/// The PCI configuration space of a device.
> +///
> +/// Provides typed read and write accessors for configuration registers
> +/// using the standard `pci_read_config_*` and `pci_write_config_*` helpers.
> +///
> +/// The generic const parameter `SIZE` can be used to indicate the
> +/// maximum size of the configuration space (e.g. 256 bytes for legacy,
> +/// 4096 bytes for extended config space).
> +pub struct ConfigSpace<'a, const SIZE: usize = { ConfigSpaceSize::Extended as usize }> {
> +    pub(crate) pdev: &'a Device<device::Bound>,
> +}
> +
> +macro_rules! call_config_read {
> +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) => {{
> +        let mut val: $ty = 0;
> +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, &mut val) };
> +        val
> +    }};
> +}
> +
> +macro_rules! call_config_write {
> +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
> +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, $value) };

unsafe block needs safety comments, also I understand 'as' to convert is
generally forbidden without a CAST: comment or using ::from() for conversion
because it can by a lossy conversion.

Also we should have a comment on why its safe for _ret to be ignored.
Basically what guarantees that the call is really infallible? Anything we can
do to ensure errors are not silently ignored? Let me know if I missed
something.

[..]

> +
> +    /// Return an initialized config space object.
> +    pub fn config_space_exteneded<'a>(

typo in func name.

thanks,

 - Joel

> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
>  }
> -- 
> 2.51.0
>

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

Posted by Danilo Krummrich 2 months, 3 weeks ago

On Fri Nov 14, 2025 at 1:20 PM NZDT, Joel Fernandes wrote:
> Also we should have a comment on why its safe for _ret to be ignored.
> Basically what guarantees that the call is really infallible? Anything we can
> do to ensure errors are not silently ignored? Let me know if I missed
> something.

Please also see [1].

[1] https://lore.kernel.org/all/DE8PBRC48D14.IX6FUPOLLVHR@kernel.org/

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

Posted by Zhi Wang 2 months, 3 weeks ago

On Thu, 13 Nov 2025 19:20:05 -0500
Joel Fernandes <joelagnelf@nvidia.com> wrote:

> Hi Zhi,
> 
> On Mon, Nov 10, 2025 at 10:41:18PM +0200, Zhi Wang wrote:
> [..]  
> >  impl Device<device::Core> {
> > diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> > index 2bbb3261198d..bb78a83fe92c 100644
> > --- a/rust/kernel/pci/io.rs
> > +++ b/rust/kernel/pci/io.rs
> > @@ -2,12 +2,19 @@
> >  
> >  //! PCI memory-mapped I/O infrastructure.
> >  
> > -use super::Device;
> > +use super::{
> > +    ConfigSpaceSize,
> > +    Device, //
> > +};
> >  use crate::{
> >      bindings,
> >      device,
> >      devres::Devres,
> >      io::{
> > +        define_read,
> > +        define_write,
> > +        Io,
> > +        IoInfallible,
> >          Mmio,
> >          MmioRaw, //
> >      },
> > @@ -16,6 +23,58 @@
> >  };
> >  use core::ops::Deref;
> >  
> > +/// The PCI configuration space of a device.
> > +///
> > +/// Provides typed read and write accessors for configuration
> > registers +/// using the standard `pci_read_config_*` and
> > `pci_write_config_*` helpers. +///
> > +/// The generic const parameter `SIZE` can be used to indicate the
> > +/// maximum size of the configuration space (e.g. 256 bytes for
> > legacy, +/// 4096 bytes for extended config space).
> > +pub struct ConfigSpace<'a, const SIZE: usize = {
> > ConfigSpaceSize::Extended as usize }> {
> > +    pub(crate) pdev: &'a Device<device::Bound>,
> > +}
> > +
> > +macro_rules! call_config_read {
> > +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) =>
> > {{
> > +        let mut val: $ty = 0;
> > +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(),
> > $addr as i32, &mut val) };
> > +        val
> > +    }};
> > +}
> > +
> > +macro_rules! call_config_write {
> > +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr,
> > $value:expr) => {
> > +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(),
> > $addr as i32, $value) };
> 
> unsafe block needs safety comments, also I understand 'as' to convert

snip

> is generally forbidden without a CAST: comment or using ::from() for
> conversion because it can by a lossy conversion.
> 

Let me take a look, basically this was similar with the define_{read,
mmio} macros, which has originally been from the mainline. Might have to
fix that part as well.

> Also we should have a comment on why its safe for _ret to be ignored.
> Basically what guarantees that the call is really infallible?
> Anything we can do to ensure errors are not silently ignored? Let me
> know if I missed something.
> 

This was discussed with Danilo on Zulip. The driver will observe a
!0 value on read when an error occurs in the infallible accessors. For
writes, I think the driver should read the value back. (similar with
MMIO cases)

Besides those, I think the driver needs to use fallible ones if it
really cares about the return.

Z.

> [..]
> 
> > +
> > +    /// Return an initialized config space object.
> > +    pub fn config_space_exteneded<'a>(
> 
> typo in func name.
> 
> thanks,
> 
>  - Joel
> 
> > +        &'a self,
> > +    ) -> Result<ConfigSpace<'a, {
> > ConfigSpaceSize::Extended.as_raw() }>> {
> > +        Ok(ConfigSpace { pdev: self })
> > +    }
> >  }
> > -- 
> > 2.51.0
> >

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

Posted by Alexandre Courbot 2 months, 4 weeks ago

On Tue Nov 11, 2025 at 5:41 AM JST, Zhi Wang wrote:
> Drivers might need to access PCI config space for querying capability
> structures and access the registers inside the structures.
>
> For Rust drivers need to access PCI config space, the Rust PCI abstraction
> needs to support it in a way that upholds Rust's safety principles.
>
> Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
> accessors for PCI config space. The new type implements the `Io` trait to
> share offset validation and bound-checking logic with others.
>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
>  rust/kernel/pci.rs    | 41 ++++++++++++++++++++++-
>  rust/kernel/pci/io.rs | 75 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 114 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 410b79d46632..d8048c7d0f32 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -39,7 +39,10 @@
>      ClassMask,
>      Vendor, //
>  };
> -pub use self::io::Bar;
> +pub use self::io::{
> +    Bar,
> +    ConfigSpace, //
> +};
>  pub use self::irq::{
>      IrqType,
>      IrqTypes,
> @@ -330,6 +333,28 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>      }
>  }
>  
> +/// Represents the size of a PCI configuration space.
> +///
> +/// PCI devices can have either a *normal* (legacy) configuration space of 256 bytes,
> +/// or an *extended* configuration space of 4096 bytes as defined in the PCI Express
> +/// specification.

The comment says this is either, but below we have:

> @@ -141,4 +200,18 @@ pub fn iomap_region<'a>(
>      ) -> impl PinInit<Devres<Bar>, Error> + 'a {
>          self.iomap_region_sized::<0>(bar, name)
>      }
> +
> +    /// Return an initialized config space object.
> +    pub fn config_space<'a>(
> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
> +
> +    /// Return an initialized config space object.
> +    pub fn config_space_exteneded<'a>(
> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
>  }

(typo on "exteneded" btw)

Which means that a caller can infallibly (both methods return a `Result`
but can never fail, for some reason) create a `ConfigSpace` that does
not match its actual size.

That's particularly a problem is `cfg_size` returns `256` but we call
`config_space_extended`, as the returned `ConfigSpace` will have a
`maxsize` that is smaller than its `MIN_SIZE`...

I guess we should either validate the size using `cfg_size` before
creating and returning the `ConfigSpace`, or have a single method that
returns a Result containing an enum which variants are the supported
sizes?

I am also wondering whether we want `ConfigSpace` to be generic over a
constant - it makes it possible to have instances or arbitrary size, and
makes them a bit cumbersome to define as we need to do something like
`ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>`.

Maybe we can use types instead, e.g.

pub trait ConfigSpaceSize {
    const MIN_SIZE: usize;
}

pub enum Normal {}
impl ConfigSpaceSize for Normal {
    const MIN_SIZE: usize = 256;
}

pub enum Extended {}
impl ConfigSpaceSize for Extended {
    const MIN_SIZE: usize = 4096;
}

pub struct ConfigSpace<'a, S: ConfigSpaceSize> {
    ...

impl<'a, const SIZE: usize> Io for ConfigSpace<'a, S: ConfigSpaceSize> {
    const MIN_SIZE: usize = S::MIN_SIZE;
    ...

And then `cfg_size` could be replaced by just `config_space` which
returns an enum of the possible `ConfigSpace`s supported, that the
caller needs to match against.

Just an idea for your consideration, I don't know whether that would
actually work better. :)

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

Posted by Zhi Wang 2 months, 3 weeks ago

On Thu, 13 Nov 2025 16:56:28 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> On Tue Nov 11, 2025 at 5:41 AM JST, Zhi Wang wrote:
> > Drivers might need to access PCI config space for querying
> > capability structures and access the registers inside the
> > structures.
> >
> > For Rust drivers need to access PCI config space, the Rust PCI
> > abstraction needs to support it in a way that upholds Rust's safety
> > principles.
> >
> > Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to
> > provide safe accessors for PCI config space. The new type
> > implements the `Io` trait to share offset validation and
> > bound-checking logic with others.
> >
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> > ---
> >  rust/kernel/pci.rs    | 41 ++++++++++++++++++++++-
> >  rust/kernel/pci/io.rs | 75
> > ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 114
> > insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 410b79d46632..d8048c7d0f32 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -39,7 +39,10 @@
> >      ClassMask,
> >      Vendor, //
> >  };
> > -pub use self::io::Bar;
> > +pub use self::io::{
> > +    Bar,
> > +    ConfigSpace, //
> > +};
> >  pub use self::irq::{
> >      IrqType,
> >      IrqTypes,
> > @@ -330,6 +333,28 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
> >      }
> >  }
> >  
> > +/// Represents the size of a PCI configuration space.
> > +///
> > +/// PCI devices can have either a *normal* (legacy) configuration
> > space of 256 bytes, +/// or an *extended* configuration space of
> > 4096 bytes as defined in the PCI Express +/// specification.
> 
> The comment says this is either, but below we have:
> 
> > @@ -141,4 +200,18 @@ pub fn iomap_region<'a>(
> >      ) -> impl PinInit<Devres<Bar>, Error> + 'a {
> >          self.iomap_region_sized::<0>(bar, name)
> >      }
> > +
> > +    /// Return an initialized config space object.
> > +    pub fn config_space<'a>(
> > +        &'a self,
> > +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw()
> > }>> {
> > +        Ok(ConfigSpace { pdev: self })
> > +    }
> > +
> > +    /// Return an initialized config space object.
> > +    pub fn config_space_exteneded<'a>(
> > +        &'a self,
> > +    ) -> Result<ConfigSpace<'a, {
> > ConfigSpaceSize::Extended.as_raw() }>> {
> > +        Ok(ConfigSpace { pdev: self })
> > +    }
> >  }
> 
> (typo on "exteneded" btw)
> 
> Which means that a caller can infallibly (both methods return a
> `Result` but can never fail, for some reason) create a `ConfigSpace`
> that does not match its actual size.
> 
> That's particularly a problem is `cfg_size` returns `256` but we call
> `config_space_extended`, as the returned `ConfigSpace` will have a
> `maxsize` that is smaller than its `MIN_SIZE`...
> 
> I guess we should either validate the size using `cfg_size` before
> creating and returning the `ConfigSpace`, or have a single method that
> returns a Result containing an enum which variants are the supported
> sizes?
> 

AFAIU, this was intentional according to usage model of the Io trait.
It has two checking paths, one is at build time and one is at run time.
Pretty much similar with MMIO traits:

- The driver assumes a minimum/expected working region size at build
  time. In PCI configuration space case, the driver knows if its device
  have a extended area or not, and choose
  config_space()/config_space_extended() accordingly.

- The actual available region size is decided at runtime, which will be
  from maxsize() method in the trait. So accessing the region will be
  checked 

The fallible accessors will do runtime check, while infallible
accessors will do build time check.

To following that model,

- cfg_size is only known at runtime. So I didn't get it invovled
  in the config_space()/config_space_extended() path, which is for
  runtime path.

- If a driver chooses the wrong config_space()/config_space_extended()
  which doesn't match its device nature at build time and compiling
  passes:

  a. device has extended area, but driver chooses config_space():
	- the infallible accessors only allows to acccess the legacy
	  256-byte area at build time. If the driver uses the fallible
	  accessors, it still can access the extended area at runtime.

  b. device doesn't have extended area, but driver chooses
  config_space_extended():

	- In this case, the driver can use the infallible accessors to
	  reach the unexpected area and slipped away from the build
	  time check (I think it is the similar situation in MMIO path
	  since it is device specific.). The driver will get !0 at
	  runtime if it reads extended area.

	- Infallible path is not affected. 

> Just an idea for your consideration, I don't know whether that would
> actually work better. :)

It is always good to know new and nice tricks. :)

Z.

[PATCH v6 RESEND 1/7] samples: rust: rust_driver_pci: use "kernel vertical" style for imports
[PATCH v6 RESEND 2/7] rust: devres: style for imports
[PATCH v6 RESEND 3/7] rust: io: style for imports
[PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait
[PATCH v6 RESEND 5/7] rust: io: factor out MMIO read/write macros
[PATCH v6 RESEND 6/7] rust: pci: add config space read/write support
[PATCH v6 RESNED 7/7] sample: rust: pci: add tests for config space routines