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

On Mon Aug 24, 2026 at 9:12 AM BST, Alexandre Courbot wrote:
> On Tue Aug 18, 2026 at 5:46 PM JST, 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 iterator that
>> reads and validates all six VF BAR register slots up front, yields decoded
>> BAR addresses and widths in logical order, and keeps the raw
>> configuration-space slot advancement internal. Since PCI_EXT_CAP_NEXT() is
>> a function-like macro, expose it through a Rust helper.
>>
>> Link: https://lore.kernel.org/rust-for-linux/[email protected]/
>> Cc: Alexandre Courbot <[email protected]>
>> Cc: Gary Guo <[email protected]>
>> Signed-off-by: Zhi Wang <[email protected]>
>> ---
>>  rust/helpers/pci.c     |   5 +
>>  rust/kernel/pci.rs     |   8 +
>>  rust/kernel/pci/cap.rs | 329 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 342 insertions(+)
>>  create mode 100644 rust/kernel/pci/cap.rs
>>
>> diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
>> index 4ebf256dff23..b946b14d79e4 100644
>> --- a/rust/helpers/pci.c
>> +++ b/rust/helpers/pci.c
>> @@ -24,6 +24,11 @@ __rust_helper bool rust_helper_dev_is_pci(const struct device *dev)
>>  	return dev_is_pci(dev);
>>  }
>>  
>> +__rust_helper u32 rust_helper_pci_ext_cap_next(u32 header)
>> +{
>> +	return PCI_EXT_CAP_NEXT(header);
>> +}
>> +
>>  #ifndef CONFIG_PCI_IOV
>>  __rust_helper unsigned int
>>  rust_helper_pci_sriov_get_totalvfs(struct pci_dev *pdev)
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index 9f19ccd5905c..008c2770a3f3 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -32,10 +32,18 @@
>>      },
>>  };
>>  
>> +mod cap;
>>  mod id;
>>  mod io;
>>  mod irq;
>>  
>> +pub use self::cap::{
>> +    ExtCapId,
>> +    ExtCapability,
>> +    ExtSriovCapability,
>> +    ExtSriovRegs,
>> +    ExtSriovVfBar, //
>> +};
>>  pub use self::id::{
>>      Class,
>>      ClassMask,
>> diff --git a/rust/kernel/pci/cap.rs b/rust/kernel/pci/cap.rs
>> new file mode 100644
>> index 000000000000..ddb3fd73e195
>> --- /dev/null
>> +++ b/rust/kernel/pci/cap.rs
>> @@ -0,0 +1,329 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! PCI extended capability support.
>> +
>> +use super::{
>> +    io::ConfigSpaceBackend,
>> +    ConfigSpace,
>> +    Extended, //
>> +};
>
> Let's merge this block with the one below, i.e. using `crate::pci`?
>
>> +use crate::{
>> +    bindings,
>> +    io::{
>> +        Io,
>> +        IoBackend,
>> +        Region, //
>> +    },
>> +    num::Bounded,
>> +    prelude::*,
>> +};
>> +
>> +/// Number of VF BAR register slots in an SR-IOV capability.
>> +// CAST: `PCI_SRIOV_NUM_BARS` is the PCIe-specified number of VF BAR register slots and fits in
>> +// `usize`.
>> +const NUM_VF_BARS: usize = bindings::PCI_SRIOV_NUM_BARS as usize;
>
> The infallible casts module is now available in `master`. If you import
> `crate::num::casts` you can now turn this into
>
>     const NUM_VF_BARS: usize = casts::u32_as_usize(bindings::PCI_SRIOV_NUM_BARS);
>
> and remove the `CAST` comment.
>
>> +
>> +/// PCI extended capability IDs.
>> +#[repr(transparent)]
>> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> +pub struct ExtCapId(u16);
>> +
>> +impl ExtCapId {
>> +    /// Single Root I/O Virtualization.
>> +    // CAST: PCI extended capability IDs are 16-bit values defined by the PCIe specification.
>> +    pub const SRIOV: Self = Self(bindings::PCI_EXT_CAP_ID_SRIOV as u16);
>
> Same here, the `CAST` comment can be removed if you turn this line into
>
>     pub const SRIOV: Self = Self(casts::u32_into_u16::<{ bindings::PCI_EXT_CAP_ID_SRIOV }>());

This looks horrible. I'd prefer `as`.

>> [snip]
>>
>> +
>> +        Ok(Self {
>> +            bars,
>> +            bar_count,
>> +            next_bar: 0,
>> +        })
>> +    }
>> +}
>> +
>> +impl Iterator for ExtSriovVfBars {
>> +    type Item = ExtSriovVfBar;
>> +
>> +    fn next(&mut self) -> Option<Self::Item> {
>> +        if self.next_bar >= self.bar_count {
>> +            return None;
>> +        }
>> +
>> +        let bar = self.bars[self.next_bar];
>> +        self.next_bar += 1;
>> +        Some(bar)
>> +    }
>> +}
>> +
>> +impl ExtSriovCapability<'_> {
>> +    /// Returns an iterator over decoded VF BAR register encodings.
>> +    ///
>> +    /// All six raw VF BAR register slots are read and decoded up front. A 32-bit encoding yields
>> +    /// one entry; a 64-bit encoding combines two slots into one entry.
>> +    ///
>> +    /// A zero-valued low DWORD is yielded as a 32-bit BAR at address zero; this method does not
>> +    /// probe whether a BAR is implemented.
>> +    ///
>> +    /// Returns [`EINVAL`] and logs an error if a BAR low DWORD does not encode a 32-bit or 64-bit
>> +    /// memory BAR, or if a 64-bit encoding has no upper DWORD.
>> +    pub fn vf_bars(&self) -> Result<impl Iterator<Item = ExtSriovVfBar>> {
>
> Since `ExtSriovVfBars` is private and we are returning an `impl`, I
> think we can get rid of it altogether. Combining with my suggestion from
> above, here is an alternative version of this method:
>
>     pub fn vf_bars(&self) -> Result<impl Iterator<Item = ExtSriovVfBar>> {
>         let slots: [u32; NUM_VF_BARS] =
>             core::array::from_fn(|slot| crate::io_read!(*self, .vf_bar[panic: slot]));
>         let mut slots = slots.into_iter();
>         let mut bars = [None; NUM_VF_BARS];
>         let mut count = 0;
>
>         while let Some(low) = slots.next().map(VfBarLow::from) {
>             if low.io_space() {
>                 return Err(EINVAL);
>             }
>
>             let low_address = u64::from(low.address()) << VfBarLow::ADDRESS_SHIFT;
>             let bar = match low.memory_type()? {
>                 VfBarMemoryType::Bits64 => ExtSriovVfBar {
>                     address: (u64::from(slots.next().ok_or(EINVAL)?) << 32) | low_address,
>                     is_64bit: true,
>                 },
>                 VfBarMemoryType::Bits32 => ExtSriovVfBar {
>                     address: low_address,
>                     is_64bit: false,
>                 },
>             };
>
>             bars[count] = Some(bar);
>             count += 1;
>         }
>
>         Ok(bars.into_iter().flatten())
>     }
>
> With this you don't need `ExtSriovVfBars` at all, which removes a bit
> (almost 50 LoCs!) of code.
>
>> +        let slots: [u32; NUM_VF_BARS] =
>> +            core::array::from_fn(|slot| crate::io_read!(*self, .vf_bar[panic: slot]));
>
> Can't this be `build:`? `vf_bar` is sized by `NUM_VF_BARS`, and so is
> the result, so I'd assume the optimizer can infer this. Not that `panic:`
> is problematic here but I wonder whether you chose this because you hit
> an issue. To reiterate, I'm fine with `panic:` here, as long as the
> alternative has been considered.

`slot` is technically variable and you'd rely on optimization pass that is not
const folding. I think `panic: ` is better.

Best,
Gary