In the NVIDIA vGPU RFC [1], the PCI configuration space access is
required in nova-core for preparing gspVFInfo when vGPU support is
enabled. This series is the following up of the discussion with Danilo
for how to introduce support of PCI configuration space access in Rust
PCI abstrations. Bascially, we are thinking of introducing another
backend for PCI configuration space access similar with Kernel::Io.

This ideas of this series are:

- Factor out a common trait IoRegion for other accessors to share the
  same compiling/runtime check like before.

- Factor the MMIO read/write macros from the define_read! and
  define_write! macros. Thus, define_{read, write}! can be used in other
  backend.

  In detail:

  * Introduce `call_mmio_read!` and `call_mmio_write!` helper macros
    to encapsulate the unsafe FFI calls.
  * Update `define_read!` and `define_write!` macros to delegate to
    the call macros.
  * Export `define_read` and `define_write` so they can be reused
    for other I/O backends (e.g. PCI config space).

- Add a helper to query configuration space size. This is mostly for
  runtime check.

- Implement the PCI configuration space access backend in PCI
  Abstractions.

  In detail:

  * `struct ConfigSpace<SIZE>` wrapping a `pdev: ARef<Device>`.
  * `IoRegion` implementation returning the device's `cfg_size`.
  * `call_config_read!` and `call_config_write!` macros bridging to
    the existing C helpers (`pci_read_config_*` /
    `pci_write_config_*`).
  * Read accessors: `read8/16/32` and `try_read8/16/32`.
  * Write accessors: `write8/16/32` and `try_write8/16/32`.

- Introduce an rust wrapper for pci_find_ext_capability(). Thus, the
  rust driver can locate the extended PCI configuration caps.

Open:

The current kernel::Io MMIO read/write doesn't return a failure, because
{read, write}{b, w, l}() are always successful. This is not true in
pci_{read, write}_config{byte, word, dword}() because a PCI device
can be disconnected from the bus. Thus a failure is returned.

- Do we still need a non-fallible version of read/write for config space?
A rust panic in accessing the PCI config space when device is
unexpectedly disconnected seems overkill.

Zhi Wang (6):
  rust: io: refactor Io<SIZE> helpers into IoRegion trait
  rust: io: factor out MMIO read/write macros
  rust: pci: add a helper to query configuration space size
  rust: pci: add config space read/write support
  rust: pci: add helper to find extended capability
  [!UPSTREAM] nova-core: test configuration routine.

 drivers/gpu/nova-core/driver.rs |   4 +
 rust/kernel/io.rs               | 132 +++++++++++++++++++++-----------
 rust/kernel/pci.rs              |  74 ++++++++++++++++++
 3 files changed, 164 insertions(+), 46 deletions(-)

-- 
2.47.3

Hi Zhi,

(Cc: Alex, Joel, John, Markus)

On Fri Oct 10, 2025 at 10:03 AM CEST, Zhi Wang wrote:
> This ideas of this series are:
>
> - Factor out a common trait IoRegion for other accessors to share the
>   same compiling/runtime check like before.

Yes, this is something we want to have in general:

Currently, we have a single I/O backend (struct Io) which is used for generic
MMIO. However, we should make Io a trait instead and require a new MMIO type to
implement the trait, where the trait methods would remain to be
{try_}{read,write}{8,16,..}().

We need the same thing for other I/O backends, such as I2C, etc.

@Markus: Most of the design aspects for the PCI configuration space below should
apply to I2C I/O accessors as well.

>   In detail:
>
>   * `struct ConfigSpace<SIZE>` wrapping a `pdev: ARef<Device>`.

There are two cases:

  (1) I/O backends that embedd a dedicated device resource. For instance, a
      pci::Bar embedds an iomapped pointer that must be wrapped with Devres to
      ensure it can't outlive the driver being bound to its corresponding
      device.

      In this case we have a method pci::Device<Bound>::iomap_region(), which
      returns a Devres<pci::Bar>.

  (2) I/O backends that don't need to embedd a dedicated device resource because
      the resource is already attached to the device itself. This is the case
      with the PCI configuration space; drivers don't need to create their own
      mapping, but can access it directly through the device.

      For this case we want a method pci::Device<Bound>::config_space() that
      returns a ConfigSpace<'a>, where 'a is the lifetime of the
      &'a Device<Bound> given to config_space().

      This ensures that the ConfigSpace structure still serves as I/O backend
      for the types generated by the register!() macro, but at the same time
      can't outlife the scope of the bound device.

      (Drivers shouldn't be able to write the PCI configuration space of a
      device they're not bound to.)

Besides that, we should also use the register!() macro to create the common
configuration space register types in the PCI core for driver to use.

Of course, there is no need to (re-)implement the following one, but it's a
good example:

	register!(PCI_CONFIG_ID @ 0x0 {
	    31:16   device_id ?=> pci::DeviceId, "Device ID";
	    15:0    vendor_id ?=> pci::VendorId, "Vendor ID";
	});

	// Assume we have a `&pci::Device<Bound>`, e.g. from probe().
	let pdev: &pci::Device<Bound> = ...;

	// Grab the configuration space I/O backend; lives as long as `pdev`.
	let config_space = pdev.config_space();

	// Read the first standard register of the configuration space header.
	let id = PCI_CONFIG_ID::read(config_space);

	// `id.vendor()` returns a `pci::Vendor`. Since it implements `Display`
	// the `dev_info()` call prints the actual vendor string.
	dev_info!(pdev.as_ref(), "VendorId={}\n", id.vendor());

> Open:
>
> The current kernel::Io MMIO read/write doesn't return a failure, because
> {read, write}{b, w, l}() are always successful. This is not true in
> pci_{read, write}_config{byte, word, dword}() because a PCI device
> can be disconnected from the bus. Thus a failure is returned.

This is in fact also true for the PCI configuration space. The PCI configuration
space has a minimum size that is known at compile time. All registers within
this minimum size can be access in an infallible way with the non try_*()
methods.

The main idea behind the fallible and infallible accessors is that you can
assert a minimum expected size of an I/O backend (e.g. a PCI bar). I.e. drivers
know their minimum requirements of the size of the I/O region. If the I/O
backend can fulfill the request we can be sure about the minimum size and hence
accesses with offsets that are known at compile time can be infallible (because
we know the minimum accepted size of the I/O backend at compile time as well).

Accesses with offsets that are not known at compile time still remain fallible
of course. That's why we have both, e.g. write32() and try_write32().

On Mon, 13 Oct 2025 17:39:41 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> Hi Zhi,
> 
> (Cc: Alex, Joel, John, Markus)
> 
> On Fri Oct 10, 2025 at 10:03 AM CEST, Zhi Wang wrote:
> > This ideas of this series are:
> >
> > - Factor out a common trait IoRegion for other accessors to share
> > the same compiling/runtime check like before.  
> 
> Yes, this is something we want to have in general:
> 
> Currently, we have a single I/O backend (struct Io) which is used for
> generic MMIO. However, we should make Io a trait instead and require
> a new MMIO type to implement the trait, where the trait methods would
> remain to be {try_}{read,write}{8,16,..}().
> 

I was considering the same when writing this series. The concern is
mostly about having to change the drivers' MMIO code to adapt to the
re-factor.

IMHO, if we are seeing the necessity of this re-factor, we should do it
before it got more usage. This could be the part 1 of the next spin.

and adding pci::Device<Bound>::config_space() could be part 2 and
register! marco could be part 3.

ditto

> 
> > The current kernel::Io MMIO read/write doesn't return a failure,
> > because {read, write}{b, w, l}() are always successful. This is not
> > true in pci_{read, write}_config{byte, word, dword}() because a PCI
> > device can be disconnected from the bus. Thus a failure is
> > returned.  
> 
> This is in fact also true for the PCI configuration space. The PCI
> configuration space has a minimum size that is known at compile time.
> All registers within this minimum size can be access in an infallible
> way with the non try_*() methods.
> 
> The main idea behind the fallible and infallible accessors is that
> you can assert a minimum expected size of an I/O backend (e.g. a PCI
> bar). I.e. drivers know their minimum requirements of the size of the
> I/O region. If the I/O backend can fulfill the request we can be sure
> about the minimum size and hence accesses with offsets that are known
> at compile time can be infallible (because we know the minimum
> accepted size of the I/O backend at compile time as well).
> 

For PCI configuration space. Standard configuration space should be
readable and to access the extended configuration space, the MCFG
should be enabled beforehand and the enabling is system-wide.

I think the size of standard configuration space falls in "falliable
accessors", and the extended configuration space falls in "infalliable"
parts

But for the "infallible" part in PCI configuration space, the device
can be disconnected from the PCI bus. E.g. unresponsive device. In that
case, the current PCI core will mark the device as "disconnected" before
they causes more problems and any access to the configuration space
will fail with an error code. This can also happen on access to
"infalliable" part.

How should we handle this case in "infallible" accessors of PCI
configuration space? Returning Result<> seems doesn't fit the concept
of "infallible", but causing a rust panic seems overkill...

Z.

On Mon Oct 13, 2025 at 8:25 PM CEST, Zhi Wang wrote:
> I was considering the same when writing this series. The concern is
> mostly about having to change the drivers' MMIO code to adapt to the
> re-factor.

For this you need to adjust the register macro to take something that
dereferences to `T: Io` instead of something that dereferences to `Io`.

This change should be trivial.

> IMHO, if we are seeing the necessity of this re-factor, we should do it
> before it got more usage. This could be the part 1 of the next spin.

Yes, you can do it in a separete series. But I'd also be fine if you do both in
a single one. The required code changes shouldn't be crazy.

> and adding pci::Device<Bound>::config_space() could be part 2 and
> register! marco could be part 3.

Part 3 has to happen with part 1 anyways, otherwise it breaks compilation.

> I think the size of standard configuration space falls in "falliable
> accessors", and the extended configuration space falls in "infalliable"
> parts

Both can be infallible. The standard configuration space size is constant, hence
all accesses to the standard configuration space with constant offsets can be
infallible.

For the extended space it depends what a driver can assert, just like for any
MMIO space.

However, you seem to talk about whether a physical device is still present.

> But for the "infallible" part in PCI configuration space, the device
> can be disconnected from the PCI bus. E.g. unresponsive device. In that
> case, the current PCI core will mark the device as "disconnected" before
> they causes more problems and any access to the configuration space
> will fail with an error code. This can also happen on access to
> "infalliable" part.
>
> How should we handle this case in "infallible" accessors of PCI
> configuration space? Returning Result<> seems doesn't fit the concept
> of "infallible", but causing a rust panic seems overkill...

Panics are for the "the machine is unrecoverably dead" case, this clearly isn't
one of them. :)

I think we should do the same as with "normal" MMIO and just return the value,
i.e. all bits set (PCI_ERROR_RESPONSE).

The window between physical unplug and the driver core unbinds the driver should
be pretty small and drivers have to be able to deal with garbage values read
from registers anyways.

If we really want to handle it, you can only implement the try_*() methods and
for the non-try_*() methods throw a compile time error, but I don't see a reason
for that.