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

["Gary Guo" <gary@garyguo.net>]

On Thu Jan 22, 2026 at 12:40 PM GMT, Danilo Krummrich 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:
>>> +    /// 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).
>
> That seems reasonable, but I also think we should keep the enum ConfigSpaceSize
> we had before and call the new trait ConfigSpaceKind instead, such that this
> method becomes:
>
> 	fn cfg_size(&self) -> ConfigSpaceSize;
>
>> 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)`.
>
> We need the check for config_space_extended(), but not for config_space(), as it
> represents the minimum size, i.e. it's always valid.
>
> Here's a diff of what I think this should look like on top of this series.

The proposal looks good to me. Some comments below.

Reviewed-by: Gary Guo <gary@garyguo.net>

>
> (@Zhi: If we all agree on the diff and nothing else comes up you don't need to
> resend. :)
>
> 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)]

When `PartialEq` is derived, I would also derive `Eq` unless there's no
reflexivity in comparison.

> +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 {

If you keep this `fn` instead of `pub fn`, then the `ConfigSpaceSize` type being
`pub` is not very useful as it cannot be invoked by the user.

>          // 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> {

Nice. Less failing path = good.

> +        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(),
>              }))