Re: [RFC PATCH 0/2] rust: pci: add config space accessors (and a small in-tree user)

[Charalampos Mitrodimas <charmitro@posteo.net>]

Zijing Zhang <zijing.zhang@ry.rs> writes:

> This RFC proposes adding basic PCI config space accessors to
> `rust/kernel/pci`.
> It also includes a tiny update to the existing Rust PCI driver sample to
> exercise the new API.
>
> Motivation
> ----------
> Rust PCI drivers occasionally need to access PCI config space (e.g. for
> capability discovery, SR-IOV queries, or MSI-related setup).
>
> Having a small, reviewed API in `kernel::pci` avoids each Rust driver
> growing its own ad-hoc wrappers and error handling around config space.
>
> This RFC is also motivated by the "PCI MISC APIs" TODO item in
> `Documentation/gpu/nova/core/todo.rst`: config space accessors as a first
> step, with capability/MSI/SR-IOV helpers as follow-ups.
>
> Proposed API
> ------------
> Add the following methods to `pci::Device`:
>
>   - read_config_u8/u16/u32(offset: u16) -> Result<T>
>   - write_config_u8/u16/u32(offset: u16, val: T) -> Result
>
> Notes
> -----
> This is intentionally a thin wrapper: it exposes a safe interface and
> translates errors into `Result`, but it does not try to add policy or extra
> validation.
>
>   - No additional range/alignment checks are performed in Rust. If an
>     argument needs validation beyond what the C PCI accessors already do,
>     it should likely be addressed in the PCI core instead of in a Rust-only
>     wrapper.
>   - The underlying C helpers may return positive PCIBIOS status codes.
>     These are mapped to the corresponding `-errno` values for Rust callers
>     (same mapping as `pcibios_err_to_errno()` in `include/linux/pci.h`).
>
> `pcibios_err_to_errno` mapping helper
> -------------------------------------
> The mapping logic is kept as a private helper in the `kernel::pci` module
> rather than inside `Device`: it is not tied to any particular device
> instance and may be reused by future PCI helpers.
>
> Also, the C `pcibios_err_to_errno()` is a `static inline`, so Rust cannot
> call it directly without adding an exported wrapper.
>
> In-tree user
> ------------
> The `samples/rust/rust_driver_pci` sample is updated to read vendor/device
> IDs from config space (0x00/0x02) and print them during probe using
> `dev_dbg!`.
>
> Note: in the current Rust support, `dev_dbg!` does not hook into dynamic
> debug and is compiled out unless Rust debug assertions are enabled.
>
> For local testing, enable `CONFIG_RUST_DEBUG_ASSERTIONS=y` if you want
> to see the `dev_dbg!` line.
>
> Questions for reviewers
> -----------------------
> 1) Does using `u16` for the config-space offset and returning `Result<T>`
>    look OK?

IMO using u16 looks good to me, since it covers the full PCIe extended
config space while making negative offsets unrepresentable at the type
level.

> 2) Is mapping PCIBIOS status codes to `-errno` acceptable for Rust callers,
>    or would you prefer we add a small exported C wrapper so Rust can reuse
>    the existing `pcibios_err_to_errno()` helper directly?

Personally I would prefer a small exported C wrapper. Although the local
re-implementation works, I believe it is an established pattern in the
rust helpers (i.e. to wrap `static inline` helpers), so the Rust side
automatically stays in sync with any changes done in the C side, no
logic duplication etc.

>
> Testing
> -------
> Build:
>   - x86_64 defconfig-based kernel with Rust enabled.
>   - Out-of-tree build directory (i.e. `make O=...`).
>   - Options enabled for this test:
>       * CONFIG_SAMPLES_RUST=y
>       * CONFIG_SAMPLE_RUST_DRIVER_PCI=y (built-in)
>       * CONFIG_RUST_DEBUG_ASSERTIONS=y
>
> Runtime:
>   - Booted the resulting bzImage under QEMU x86_64 with:
>       * `-device virtio-serial-pci`
>       * `-device pci-testdev`
>   - Kernel command line included:
>       * `loglevel=8`
>   - Observed:
>       * `pci-testdev data-match count: 1`
>       * `Probe Rust PCI driver sample (... cfg: 0x1b36:0x0005).`

Tested-by: Charalampos Mitrodimas <charmitro@posteo.net>

  [    0.176714] rust_driver_pci 0000:00:02.0: Probe Rust PCI driver sample (PCI ID: REDHAT, 0x5; cfg: 0x1b36:0x0005).
  [    0.176727] rust_driver_pci 0000:00:02.0: enabling device (0000 -> 0002)
  [    0.177100] rust_driver_pci 0000:00:02.0: pci-testdev data-match count: 1

Cheers,
C. Mitrodimas

>
> Zijing Zhang (2):
>   rust: pci: add config space accessors
>   samples: rust: pci: exercise config space accessors
>
>  rust/kernel/pci.rs              | 86 +++++++++++++++++++++++++++++++++
>  samples/rust/rust_driver_pci.rs |  8 ++-
>  2 files changed, 92 insertions(+), 2 deletions(-)