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

[Zhi Wang <zhiw@nvidia.com>]

On Fri, 31 Jul 2026 19:35:26 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> On Fri Jul 31, 2026 at 3:29 AM JST, Zhi Wang wrote:

snip

> > +/// 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;
> 
> This is only used in `read_vf_bar` and can be local to it.
> 

Yup.

> > +
> > +/// 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,
> > +}
> > +

snip

> > +    /// 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);
> 
> The special behavior on invalid index is intriguing - is this part of
> the PCI specification? Or can it return an error if `offset` is out of
> bounds?
> 

I was thinking to keep this helper infallible. Since offset
is returned by pci_find_ext_capability(), I assumed that
it had already been validated.

The spec only says the cap offset of zero in a successfully read
indicates the end of the cap. For aligning with the spec, I think we
should propagate the errors.

> If we can, I'd probably prefer that as the fallback path relies on
> `pci_ext_cap_next` to leave the `0` value untouched, which is not
> super obvious. Or if we keep the current behavior, let's at least
> document this fallback path a bit more.
> 
> > +        // 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
> 
> The `try_read32(offset)` above failing means that `offset + 4 >=
> (*self).size()`, so this will either underflow, or return a bogus
> size. Which is another argument in favor of handling the special case
> with an error if the current behavior is not warranted by the spec.
> 
> Note also that this can just be `self.size() - offset`, but it's fine
> if you want to keep the deref explicit.
> 

Sure will address it.

> > +        }
> > +    }
> > +}
> > +
> > +/// 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,
> > +}
> 
> Side-note: It would be interesting if we could end up representing
> every I/O space this way, although padding and keeping the fields
> offsets visible would make this challenging. But if we can eventually
> generalize this, then I guess we can retire the `register!` macro. :)
> 

That sounds interesting idea. :) My understanding is io projection
covers the operations of the registers but not about the fields in the
registers. Are you thinking to extend it to cover the register fields
as well?

> > +
> > +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
> > +    }
> > +}
> 
> How about making all the members public (and documenting them)? We
> won't be working with mutable values, and that way we can remove this
> entire impl block.
> 

Sure.

> > +
> > +impl ConfigSpace<'_, ExtSriovRegs> {
> 
> Since you have a typed variant defined, let this be
> 
>     impl ExtSriovCapability<'_> {
> 
> > +    /// 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> {
> > +        if bar_index >= NUM_VF_BARS {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        let low = crate::io_read!(*self, .vf_bar[try: bar_index]);
> > +        if low & bindings::PCI_BASE_ADDRESS_SPACE !=
> > bindings::PCI_BASE_ADDRESS_SPACE_MEMORY {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        let is_64bit = low &
> > bindings::PCI_BASE_ADDRESS_MEM_TYPE_MASK
> > +            == bindings::PCI_BASE_ADDRESS_MEM_TYPE_64;
> > +        let following_index =
> > bar_index.checked_add(1).ok_or(EINVAL)?;
> 
> Looks like these BAR registers should be bitfields? The `vf_bar`
> member will need to stay a regular `u32` because we don't know ahead
> of time whether a given entry is the high part of a 64-bit BAR or
> not, but once we read the value we could create a typed bitfield from
> it and avoid doing the shifting/masking magic ourselves.

Sure.