Re: [PATCH v6 1/1] rust: pci: add extended capability and SR-IOV support

[Gary Guo]

On Fri Jul 31, 2026 at 10:57 AM BST, Alexandre Courbot wrote:
> On Fri Jul 31, 2026 at 3:45 AM JST, Gary Guo wrote:
>> On Thu Jul 30, 2026 at 7:29 PM BST, Zhi Wang wrote:
>>> Rust PCI drivers have no typed interface for locating and accessing PCIe
>>> extended capabilities.
>>>
>>> The SR-IOV extended capability describes VF topology and VF BARs. Expose
>>> this information through the Rust PCI abstraction so drivers can use the
>>> existing typed configuration-space accessors instead of raw bindings.
>>>
>>> Define ExtCapability to associate a capability ID with a register layout,
>>> and add ConfigSpace::find_ext_capability() to locate and project that
>>> layout. Bound the view at the next capability or the end of extended
>>> configuration space. Add ExtSriovRegs and a decoded VF BAR helper that
>>> returns the address, width, and next configuration-space slot, using
>>> PCI_SRIOV_NUM_BARS for the number of BAR slots. Since PCI_EXT_CAP_NEXT()
>>> is a function-like macro, expose it through a Rust helper.
>>>
>>> Link: https://lore.kernel.org/rust-for-linux/20260730180349.771719-1-zhiw@xxxxxxxxxx/
>>> Cc: Alexandre Courbot <acourbot@xxxxxxxxxx>
>>> Cc: Gary Guo <gary@xxxxxxxxxxx>
>>> Signed-off-by: Zhi Wang <zhiw@xxxxxxxxxx>
>>> ---
>>>  rust/helpers/pci.c     |   5 +
>>>  rust/kernel/pci.rs     |   8 ++
>>>  rust/kernel/pci/cap.rs | 236 +++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 249 insertions(+)
>>>  create mode 100644 rust/kernel/pci/cap.rs
>>>
>>> diff --git a/rust/kernel/pci/cap.rs b/rust/kernel/pci/cap.rs
>>> new file mode 100644
>>> index 000000000000..08c044bedb70
>>> --- /dev/null
>>> +++ b/rust/kernel/pci/cap.rs
>>> @@ -0,0 +1,236 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! PCI extended capability support.
>>> +
>>> +use super::{
>>> +    io::ConfigSpaceBackend,
>>> +    ConfigSpace,
>>> +    Extended, //
>>> +};
>>> +use crate::{
>>> +    bindings,
>>> +    io::{
>>> +        Io,
>>> +        IoBackend,
>>> +        Region, //
>>> +    },
>>> +    prelude::*,
>>> +};
>>> +
>>> +/// Number of VF BAR register slots in an SR-IOV capability.
>>> +// CAST: `PCI_SRIOV_NUM_BARS` is 6, which fits in `usize`.
>>> +const NUM_VF_BARS: usize = bindings::PCI_SRIOV_NUM_BARS as usize;
>>> +
>>> +/// Attribute bits encoded in the low DWORD of a memory BAR.
>>> +const VF_MEMORY_BAR_ATTRIBUTE_BITS: u32 = bindings::PCI_BASE_ADDRESS_SPACE
>>> +    | bindings::PCI_BASE_ADDRESS_MEM_TYPE_MASK
>>> +    | bindings::PCI_BASE_ADDRESS_MEM_PREFETCH;
>>> +
>>> +/// PCI extended capability IDs.
>>> +#[repr(u16)]
>>> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
>>> +pub enum ExtCapId {
>>> +    /// Single Root I/O Virtualization.
>>> +    // CAST: `PCI_EXT_CAP_ID_SRIOV` is `0x10`, which fits in `u16`.
>>> +    Sriov = bindings::PCI_EXT_CAP_ID_SRIOV as u16,
>>> +}
>>> +
>>> +impl ExtCapId {
>>> +    fn as_raw(self) -> u16 {
>>> +        self as u16
>>> +    }
>>> +}
>>> +
>>> +/// A typed PCI extended capability register layout.
>>> +///
>>> +/// Implementors describe the register layout of one extended capability. The layout must start at
>>> +/// the extended capability header, and [`Self::ID`] must identify that layout.
>>> +pub trait ExtCapability: FromBytes + IntoBytes {
>>> +    /// PCI extended capability ID for this register layout.
>>> +    const ID: ExtCapId;
>>> +}
>>> +
>>> +impl<'a> ConfigSpace<'a, Extended> {
>>> +    /// Finds and projects an extended capability into its typed register layout.
>>> +    ///
>>> +    /// # Examples
>>> +    ///
>>> +    /// ```no_run
>>> +    /// use kernel::pci;
>>> +    ///
>>> +    /// fn probe_sriov(
>>> +    ///     pdev: &pci::Device<kernel::device::Bound>,
>>> +    /// ) -> Result<(), kernel::error::Error> {
>>> +    ///     let sriov = pdev
>>> +    ///         .config_space_extended()?
>>> +    ///         .find_ext_capability::<pci::ExtSriovRegs>()?;
>>> +    ///
>>> +    ///     let total_vfs = kernel::io_read!(sriov, .total_vfs);
>>> +    ///     let vf_offset = kernel::io_read!(sriov, .vf_offset);
>>> +    ///     let bar0 = sriov.read_vf_bar(0)?;
>>> +    ///     let bar1 = sriov.read_vf_bar(bar0.next_index())?;
>>> +    ///
>>> +    ///     Ok(())
>>> +    /// }
>>> +    /// ```
>>> +    pub fn find_ext_capability<C: ExtCapability>(&self) -> Result<ConfigSpace<'a, C>> {
>>> +        let offset = usize::from(
>>> +            // SAFETY: `self.pdev` is valid by the type invariant of `ConfigSpace`.
>>> +            unsafe {
>>> +                bindings::pci_find_ext_capability(self.pdev.as_raw(), i32::from(C::ID.as_raw()))
>>> +            },
>>> +        );
>>> +
>>> +        if offset == 0 {
>>> +            return Err(ENODEV);
>>> +        }
>>> +
>>> +        let size = self.calculate_ext_cap_size(offset);
>>> +
>>> +        let base = ConfigSpaceBackend::as_ptr(*self)
>>> +            .cast::<u8>()
>>> +            .wrapping_add(offset);
>>> +        let ptr = Region::<0>::ptr_try_from_raw_parts_mut(base, size)?;
>>
>> The signature should be 
>>
>>     Result<Option<...>>
>>
>> where the result is usually handled via `?` and `None` needs to be handled
>> explicitly, rather than matching on ENODEV.
>>
>>> +
>>> +        // SAFETY: `offset` was returned by `pci_find_ext_capability`, and
>>> +        // `calculate_ext_cap_size` bounds `ptr` at the next capability or the end of the extended
>>> +        // configuration space. `ptr_try_from_raw_parts_mut` verified the region layout.
>>> +        let capability = unsafe { ConfigSpaceBackend::project_view(*self, ptr) };
>>> +
>>> +        capability.try_cast::<C>()
>>> +    }
>>> +
>>> +    /// Calculates the size of the extended capability at `offset`.
>>> +    ///
>>> +    /// The capability extends to the next extended capability, or to the end of the extended
>>> +    /// configuration space if it is the last one. `offset` must be a DWORD-aligned offset within
>>> +    /// the extended configuration space returned by `pci_find_ext_capability`. If its header
>>> +    /// cannot be read, the capability is treated as the last one.
>>> +    fn calculate_ext_cap_size(&self, offset: usize) -> usize {
>>> +        let header = self.try_read32(offset).unwrap_or(0);
>>> +        // SAFETY: Pure bit manipulation, no preconditions.
>>> +        // CAST: The next-cap pointer is a 12-bit field (max 0xFFC), always fits in `usize`.
>>> +        let next = unsafe { bindings::pci_ext_cap_next(header) } as usize;
>>> +
>>> +        if next > offset {
>>> +            next - offset
>>> +        } else {
>>> +            (*self).size() - offset
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +/// SR-IOV register layout per PCIe spec (64 bytes starting at cap offset).
>>> +#[repr(C)]
>>> +#[derive(FromBytes, IntoBytes)]
>>> +pub struct ExtSriovRegs {
>>> +    /// Extended capability header.
>>> +    pub header: u32,
>>> +    /// SR-IOV capabilities.
>>> +    pub cap: u32,
>>> +    /// SR-IOV control.
>>> +    pub ctrl: u16,
>>> +    /// SR-IOV status.
>>> +    pub status: u16,
>>> +    /// Initial VFs.
>>> +    pub initial_vfs: u16,
>>> +    /// Total VFs.
>>> +    pub total_vfs: u16,
>>> +    /// Number of VFs.
>>> +    pub num_vfs: u16,
>>> +    /// Function dependency link.
>>> +    pub func_dep_link: u8,
>>> +    _reserved_0: u8,
>>> +    /// First VF offset.
>>> +    pub vf_offset: u16,
>>> +    /// VF stride.
>>> +    pub vf_stride: u16,
>>> +    _reserved_1: u16,
>>> +    /// VF device ID.
>>> +    pub vf_device_id: u16,
>>> +    /// Supported page sizes.
>>> +    pub supported_page_sizes: u32,
>>> +    /// System page size.
>>> +    pub system_page_size: u32,
>>> +    /// VF BARs (BAR0–BAR5).
>>> +    pub vf_bar: [u32; NUM_VF_BARS],
>>> +    /// VF migration state array offset.
>>> +    pub migration_state: u32,
>>> +}
>>> +
>>> +impl ExtCapability for ExtSriovRegs {
>>> +    const ID: ExtCapId = ExtCapId::Sriov;
>>> +}
>>> +
>>> +/// A typed view of an SR-IOV extended capability.
>>> +pub type ExtSriovCapability<'a> = ConfigSpace<'a, ExtSriovRegs>;
>>> +
>>> +/// A decoded VF memory BAR.
>>> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
>>> +pub struct ExtSriovVfBar {
>>> +    address: u64,
>>> +    is_64bit: bool,
>>> +    next_index: usize,
>>> +}
>>> +
>>> +impl ExtSriovVfBar {
>>> +    /// Returns the BAR address without PCI attribute bits.
>>> +    #[inline]
>>> +    pub fn address(&self) -> u64 {
>>> +        self.address
>>> +    }
>>> +
>>> +    /// Returns whether the BAR is 64-bit.
>>> +    #[inline]
>>> +    pub fn is_64bit(&self) -> bool {
>>> +        self.is_64bit
>>> +    }
>>> +
>>> +    /// Returns the configuration-space slot index of the next logical BAR.
>>> +    #[inline]
>>> +    pub fn next_index(&self) -> usize {
>>> +        self.next_index
>>> +    }
>>> +}
>>> +
>>> +impl ConfigSpace<'_, ExtSriovRegs> {
>>> +    /// Reads and decodes the VF memory BAR at configuration-space slot `bar_index`.
>>> +    #[inline]
>>> +    pub fn read_vf_bar(&self, bar_index: usize) -> Result<ExtSriovVfBar> {
>>
>> Do you expect people to pass in a random index instead of 0 or next_index? If
>> not, this should be an iterator. Otherwise it'd be possible to index into high
>> part of 64-bit address.
>
> I made a similar suggestion on patch 3 [1], and I think Zhi kept the
> index for ergonomic reasons. But given that not all indices within range
> are valid, an iterator indeed sounds safer.
>
> [1] https://lore.kernel.org/all/DHRTUAF52GNI.1J98TSAG1LS6Q@xxxxxxxxxx/
>
>>
>>> +        if bar_index >= NUM_VF_BARS {
>>> +            return Err(EINVAL);
>>> +        }
>>> +
>>> +        let low = crate::io_read!(*self, .vf_bar[try: bar_index]);
>>
>> Given the bound checking above I'd use `panic: ` here.
>
> Or better, one could just remove the `if bar_index >= ...` block, and
> keep the `try:`. That way it will be used for what is was designed for,
> the bounds checking will be done against the actual size of the array
> and not a constant that also happens to be used as the array size, and a
> dedicated error code will be returned instead of the ubiquitous
> `EINVAL`.

`projection::OutOfBound` converts to `ERANGE` so it'll be different. Also, when
this is converted to iterators, for the first case you'd probably want to just
return `None` and for the second case log an error that the device is having out
of spec capability.

Best,
Gary