linux-kernel - Re: [RFC 0/6] rust: pci: add config space read/write support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DDHB2T3G9BUA.18YWV70J82Z01@kernel.org>
Date: Mon, 13 Oct 2025 17:39:41 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Zhi Wang" <zhiw@...dia.com>
Cc: <rust-for-linux@...r.kernel.org>, <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>, <aliceryhl@...gle.com>,
 <tmgross@...ch.edu>, <linux-pci@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <cjia@...dia.com>, <smitra@...dia.com>,
 <ankita@...dia.com>, <aniketa@...dia.com>, <kwankhede@...dia.com>,
 <targupta@...dia.com>, <zhiwang@...nel.org>, <acourbot@...dia.com>,
 <joelagnelf@...dia.com>, <jhubbard@...dia.com>, <markus.probst@...teo.de>
Subject: Re: [RFC 0/6] rust: pci: add config space read/write support

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().