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

On Mon, 24 Aug 2026 13:38:05 +0200
"Danilo Krummrich" <[email protected]> wrote:

> On Mon Aug 24, 2026 at 10:12 AM CEST, Alexandre Courbot wrote:

snip.

> >> +/// SR-IOV register layout per PCIe spec (64 bytes starting at
> >> cap offset). +#[repr(C)]
> >> +#[derive(FromBytes, IntoBytes)]
> >> +pub struct ExtSriovRegs {
> >> +    /// Extended capability header.
> >> +    _header: u32,
> >> +    /// SR-IOV capabilities.
> >> +    pub cap: u32,
> >> +    /// SR-IOV control.
> >> +    pub ctrl: u16,  
> 
> Why is this public?
> 

This is identical to register definition layout in the PCI spec.
And it is a straight-forward idea.

Do we prefer to:

a. keep the complete register definition layout here, while only expose
the ones that allow the user to access.

or

b. only keep the allowlist of registers in a enum(offset)?

> >> +    /// SR-IOV status.
> >> +    pub status: u16,  
> 
> Why do drivers need to read this directly?
> 
> >> +    /// Initial VFs.
> >> +    pub initial_vfs: u16,
> >> +    /// Total VFs.
> >> +    pub total_vfs: u16,
> >> +    /// Number of VFs.
> >> +    pub num_vfs: u16,  
> 
> Why do we need to mess with this? This should only ever be written
> through pci_enable_sriov()?
> 
> >> +    /// Function dependency link.
> >> +    pub func_dep_link: u8,
> >> +    _reserved_0: u8,
> >> +    /// First VF offset.
> >> +    pub vf_offset: u16,
> >> +    /// VF stride.
> >> +    pub vf_stride: u16,  
> 
> Those two are read by the PCI core in pci_iov_set_numvfs() and uses
> them internally. Why do we need a driver API for those?
> 
> Why can't we use pci_iov_virtfn_devfn()?
> 

GSP VF_INFO requires the first VF offset to be
filled [1], it requires the raw value from the registers instead of the
calculated one in pci_iov_virtfn_devfn().

[1] https://lore.kernel.org/all/[email protected]/

> >> +    _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,  
> 
> Isn't this already taken care of by the PCI core? Do we need to
> expose this?
> 
> >> +    /// VF BARs (BAR0–BAR5).
> >> +    pub vf_bar: [u32; NUM_VF_BARS],  
> >
> > Now that we have an iterator method, we can make this member
> > private. I'd even say we should as making this public enables the
> > kinds of invalid accesses we built the iterator to avoid.  
> 
> Agreed.
> 
> >> +    /// VF migration state array offset.
> >> +    pub migration_state: u32,  
> 
> Do we need this? Isn't this obsolete?
> 
> >     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.  
> 
> LGTM, thanks for improving this.