Message-ID: <20251114185941.59717d52.zhiw@nvidia.com>
Date: Fri, 14 Nov 2025 18:59:41 +0200
From: Zhi Wang <zhiw@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
CC: <rust-for-linux@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <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>,
	<joelagnelf@...dia.com>, <jhubbard@...dia.com>, <zhiwang@...nel.org>
Subject: Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write
 support

On Thu, 13 Nov 2025 16:56:28 +0900
"Alexandre Courbot" <acourbot@...dia.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@...nel.org>
> > Signed-off-by: Zhi Wang <zhiw@...dia.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.

Powered by blists - more mailing lists