| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DFV6RV4QOS48.25MHV8QDDCHNV@garyguo.net>
Date: Thu, 22 Jan 2026 14:26:35 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Danilo Krummrich" <dakr@...nel.org>, "Gary Guo" <gary@...yguo.net>
Cc: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<aliceryhl@...gle.com>, <bhelgaas@...gle.com>, <kwilczynski@...nel.org>,
<ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
<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 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@...yguo.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(),
> }))
Powered by blists - more mailing lists