Re: [PATCH v12 4/5] rust: pci: add config space read/write support

[Zhi Wang <zhiw@nvidia.com>]

On Thu, 22 Jan 2026 13:40:22 +0100
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Thu Jan 22, 2026 at 12:59 PM CET, Gary Guo wrote:
> > On Wed Jan 21, 2026 at 8:22 PM GMT, Zhi Wang wrote:

> (@Zhi: If we all agree on the diff and nothing else comes up you don't
> need to resend. :)
> 

Hi Danilo:

Thanks so much for this. I went through the code. It looks good to me.

Z.

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 9020959ce0c7..1d1a253e5d5d 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -42,6 +42,7 @@
>  };
>  pub use self::io::{
>      Bar,
> +    ConfigSpaceKind,
>      ConfigSpaceSize,
>      Extended,
>      Normal, //
> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index 39df41d0eaab..5dbdfe516418 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs
> @@ -24,6 +24,31 @@
>      ops::Deref, //
>  };
> 
> +/// 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)]
> +#[derive(PartialEq)]
> +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 into_raw(self) -> usize {
> +        // CAST: PCI configuration space size is at most 4096 bytes, so
> the value always fits
> +        // within `usize` without truncation or sign change.
> +        self as usize
> +    }
> +}
> +
>  /// Marker type for normal (256-byte) PCI configuration space.
>  pub struct Normal;
> 
> @@ -34,16 +59,16 @@
>  ///
>  /// This trait is implemented by [`Normal`] and [`Extended`] to provide
>  /// compile-time knowledge of the configuration space size.
> -pub trait ConfigSpaceSize {
> +pub trait ConfigSpaceKind {
>      /// The size of this configuration space in bytes.
>      const SIZE: usize;
>  }
> 
> -impl ConfigSpaceSize for Normal {
> +impl ConfigSpaceKind for Normal {
>      const SIZE: usize = 256;
>  }
> 
> -impl ConfigSpaceSize for Extended {
> +impl ConfigSpaceKind for Extended {
>      const SIZE: usize = 4096;
>  }
> 
> @@ -55,7 +80,7 @@ impl ConfigSpaceSize for Extended {
>  /// 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 struct ConfigSpace<'a, S:
> ConfigSpaceKind = Extended> { pub(crate) pdev: &'a Device<device::Bound>,
>      _marker: PhantomData<S>,
>  }
> @@ -118,11 +143,11 @@ macro_rules! call_config_write {
>  }
> 
>  // 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: ConfigSpaceKind> IoCapable<u8> for ConfigSpace<'a, S> {}
> +impl<'a, S: ConfigSpaceKind> IoCapable<u16> for ConfigSpace<'a, S> {}
> +impl<'a, S: ConfigSpaceKind> IoCapable<u32> for ConfigSpace<'a, S> {}
> 
> -impl<'a, S: ConfigSpaceSize> Io for ConfigSpace<'a, S> {
> +impl<'a, S: ConfigSpaceKind> 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. @@ -134,7 +159,7 @@ fn addr(&self) -> usize {
>      /// Returns the maximum size of the configuration space.
>      #[inline]
>      fn maxsize(&self) -> usize {
> -        self.pdev.cfg_size().map_or(0, |v| v)
> +        self.pdev.cfg_size().into_raw()
>      }
> 
>      // PCI configuration space does not support fallible operations.
> @@ -150,7 +175,7 @@ fn maxsize(&self) -> usize {
>  }
> 
>  /// Marker trait indicating ConfigSpace has a known size at compile
> time. -impl<'a, S: ConfigSpaceSize> IoKnownSize for ConfigSpace<'a, S> {}
> +impl<'a, S: ConfigSpaceKind> IoKnownSize for ConfigSpace<'a, S> {}
> 
>  /// A PCI BAR to perform I/O-Operations on.
>  ///
> @@ -281,29 +306,35 @@ pub fn iomap_region<'a>(
>          self.iomap_region_sized::<0>(bar, name)
>      }
> 
> -    /// Returns the size of configuration space in bytes.
> -    fn cfg_size(&self) -> Result<usize> {
> +    /// Returns the size of configuration space.
> +    fn cfg_size(&self) -> 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 | 4096 => Ok(size as usize),
> +            256 => ConfigSpaceSize::Normal,
> +            4096 => ConfigSpaceSize::Extended,
>              _ => {
> -                debug_assert!(false);
> -                Err(EINVAL)
> +                // PANIC: The PCI subsystem only ever reports the
> configuration space size as either
> +                // `ConfigSpaceSize::Normal` or
> `ConfigSpaceSize::Extended`.
> +                unreachable!();
>              }
>          }
>      }
> 
>      /// Return an initialized normal (256-byte) config space object.
> -    pub fn config_space<'a>(&'a self) -> Result<ConfigSpace<'a,
> Normal>> {
> -        Ok(ConfigSpace {
> +    pub fn config_space<'a>(&'a self) -> ConfigSpace<'a, Normal> {
> +        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>> {
> +        if self.cfg_size() != ConfigSpaceSize::Extended {
> +            return Err(EINVAL);
> +        }
> +
>          Ok(ConfigSpace {
>              pdev: self,
>              _marker: PhantomData,
> diff --git a/samples/rust/rust_driver_pci.rs
> b/samples/rust/rust_driver_pci.rs index 1bc5bd1a8df5..8eea79e858a2 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -67,8 +67,8 @@ fn testdev(index: &TestIndex, bar: &Bar0) ->
> Result<u32> { Ok(bar.read32(Regs::COUNT))
>      }
> 
> -    fn config_space(pdev: &pci::Device<Bound>) -> Result {
> -        let config = pdev.config_space()?;
> +    fn config_space(pdev: &pci::Device<Bound>) {
> +        let config = pdev.config_space();
> 
>          // TODO: use the register!() macro for defining PCI
> configuration space registers once it // has been move out of nova-core.
> @@ -89,8 +89,6 @@ fn config_space(pdev: &pci::Device<Bound>) -> Result {
>              "pci-testdev config space read32 BAR 0: {:x}\n",
>              config.read32(0x10)
>          );
> -
> -        Ok(())
>      }
>  }
> 
> @@ -123,7 +121,7 @@ fn probe(pdev: &pci::Device<Core>, info:
> &Self::IdInfo) -> impl PinInit<Self, Er "pci-testdev data-match count:
> {}\n", Self::testdev(info, bar)?
>                      );
> -                    Self::config_space(pdev)?;
> +                    Self::config_space(pdev);
>                  },
>                  pdev: pdev.into(),
>              }))
>