Message-ID: <20260205135547.2ffc0df4@inno-thin-ubuntu>
Date: Thu, 5 Feb 2026 13:57:14 +0200
From: Zhi Wang <zhiw@...dia.com>
To: Gary Guo <gary@...yguo.net>
CC: <rust-for-linux@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <dakr@...nel.org>, <aliceryhl@...gle.com>,
	<bhelgaas@...gle.com>, <kwilczynski@...nel.org>, <ojeda@...nel.org>,
	<alex.gaynor@...il.com>, <boqun.feng@...il.com>, <bjorn3_gh@...tonmail.com>,
	<lossin@...nel.org>, <a.hindborg@...nel.org>, <tmgross@...ch.edu>,
	<markus.probst@...teo.de>, <helgaas@...nel.org>, <cjia@...dia.com>,
	<smitra@...dia.com>, <ankita@...dia.com>, <aniketa@...dia.com>,
	<kwankhede@...dia.com>, <targupta@...dia.com>, <acourbot@...dia.com>,
	<joelagnelf@...dia.com>, <jhubbard@...dia.com>, <zhiwang@...nel.org>,
	<daniel.almeida@...labora.com>
Subject: Re: [RFC 2/2] pci: Add PCI capability infrastructure and SR-IOV
 capability support

On Tue, 27 Jan 2026 15:36:29 +0000
"Gary Guo" <gary@...yguo.net> wrote:

snip

> > +/// Marker for normal (legacy) PCI capabilities.
> > +impl CapabilityKind for Normal {
> > +    type IdType = u8;
> > +    const START_OFFSET: usize = bindings::PCI_CAPABILITY_LIST as
> > usize; +}
> > +
> > +/// Marker for extended PCI capabilities.
> > +impl CapabilityKind for Extended {
> > +    type IdType = u16;
> > +    const START_OFFSET: usize = bindings::PCI_CFG_SPACE_SIZE as
> > usize; +}
> > +
> > +/// A PCI capability.
> > +///
> > +/// This type represents a discovered PCI capability and provides
> > safe access +/// to its registers. All I/O operations are relative
> > to the capability's +/// base offset in configuration space.
> > +pub struct Capability<'a, S: ConfigSpaceKind, K: CapabilityKind> {
> > +    config_space: &'a ConfigSpace<'a, S>,
> > +    offset: usize,
> > +    id: K::IdType,
> > +    size: usize,
> > +}
> 
> Some thoughts about this: given that the IDs are different for PCI
> capability and PCI extended capabilities, it feels that we shouldn't
> overlap them into the same type.
> 

Make sense. Sorry for the late reply as I was evaluating what would be
the impact of IoCapable trait refinement to this patch.

> The `offset` and `size` feels like it can be something more generic,
> something like
> 
>     /// A subview into I/O.
>     pub struct IoView<T> {
>         io: T,
>         offset: usize,
>         size: usize,
>     }
> 
>     impl<T: IO> IoView<T> {
>         // ....
>     }
> 
> This is just like a slice, but act on arbitrary IO.
> 
> The capability enumeration can just be something like
> 
>     fn capabilities(&self) -> impl Iterator<Item = (CapabilityId,
> IoView<&Self>)>
> 
> and another method for capabilities_ext.
> 

I actually had a version of that, but dropped it. The C side already
handles finding capabilities by ID, so a Rust iterator seemed redundant
for the common case. Unless we need to handle multiple capabilities
with the same ID (which is rare). IMO, it might be better we add it when
a rust driver really need this.

> If you want to add typed capabilities, an option is to have
> 
>     enum Capability<'a> {
>         SpecificParsedCapability,
>         Other(CapabilityId, IoView<&ConfigSpace<...>>),
>     }
> 

But given that the kernel's C API favors looking up by ID, usage of an
enum might be slightly awkward here (e.g., calling find_capability(ID)
and still having to unwrap an enum)?

> Thoughts?
> 
> Best,
> Gary
> 
> 
> > +
> > +impl<'a, S: ConfigSpaceKind, K: CapabilityKind> Capability<'a, S,
> > K> {
> > +    /// Creates a new capability handle.
> > +    fn new(
> > +        config_space: &'a ConfigSpace<'a, S>,
> > +        offset: usize,
> > +        id: K::IdType,
> > +        size: usize,
> > +    ) -> Self {
> > +        Self {
> > +            config_space,
> > +            offset,
> > +            id,
> > +            size,
> > +        }
> > +    }
> > +
> > +    /// Returns the offset of this capability in configuration
> > space.
> > +    pub fn offset(&self) -> usize {
> > +        self.offset
> > +    }
> > +
> > +    /// Returns the capability ID.
> > +    pub fn id(&self) -> K::IdType {
> > +        self.id
> > +    }
> > +
> > +    /// Returns the size of this capability in bytes.
> > +    pub fn size(&self) -> usize {
> > +        self.size
> > +    }
> > +}
> > +
> > +// Implement IoCapable for Capability
> > +impl<'a, S: ConfigSpaceKind, K: CapabilityKind> IoCapable<u8> for
> > Capability<'a, S, K> {} +impl<'a, S: ConfigSpaceKind, K:
> > CapabilityKind> IoCapable<u16> for Capability<'a, S, K> {}
> > CapabilityKind> +impl<'a, S: ConfigSpaceKind, K: CapabilityKind>
> > CapabilityKind> IoCapable<u32> for Capability<'a, S, K> {}
> > +
> > +// Implement Io trait for Capability using fallible methods
> > (runtime checks) +impl<'a, S: ConfigSpaceKind, K: CapabilityKind>
> > Io for Capability<'a, S, K> {
> > +    const MIN_SIZE: usize = S::SIZE; // Use config space size for
> > fallible bounds checking +
> > +    #[inline]
> > +    fn addr(&self) -> usize {
> > +        0  // Offsets are relative to capability base, not absolute
> > +    }
> > +
> > +    #[inline]
> > +    fn maxsize(&self) -> usize {
> > +        self.size()
> > +    }
> > +
> > +    // Only implement fallible methods (no IoKnownSize, so
> > infallible methods not available)
> > +    define_read!(fallible, try_read8, call_cap_read(try_read8) ->
> > u8);
> > +    define_read!(fallible, try_read16, call_cap_read(try_read16)
> > -> u16);
> > +    define_read!(fallible, try_read32, call_cap_read(try_read32)
> > -> u32); +
> > +    define_write!(fallible, try_write8, call_cap_write(try_write8)
> > <- u8);
> > +    define_write!(fallible, try_write16,
> > call_cap_write(try_write16) <- u16);
> > +    define_write!(fallible, try_write32,
> > call_cap_write(try_write32) <- u32); +}
> > +
> > +impl<'a> ConfigSpace<'a, Extended> {
> > +    /// Finds a specific extended capability by ID using the
> > kernel's `pci_find_ext_capability`.
> > +    pub fn find_ext_capability(&self, id: u16) ->
> > Option<Capability<'_, Extended, Extended>> {
> > +        // SAFETY: pdev is valid by ConfigSpace invariants
> > +        let offset = unsafe {
> > bindings::pci_find_ext_capability(self.pdev.as_raw(), id as i32) };
> > +
> > +        if offset == 0 {
> > +            return None;
> > +        }
> > +
> > +        let size = self.calculate_ext_cap_size(offset as usize);
> > +        Some(Capability::new(self, offset as usize, id, size))
> > +    }
> > +
> > +    fn calculate_ext_cap_size(&self, offset: usize) -> usize {
> > +        // Extended capability header: [31:20] = next capability
> > offset
> > +        // Use 0xffc mask (not 0xfff) to match kernel's
> > PCI_EXT_CAP_NEXT macro
> > +        let header = self.try_read32(offset).unwrap_or(0);
> > +        let next_ptr = ((header >> 20) & 0xffc) as usize;
> > +
> > +        if next_ptr == 0 {
> > +            // Last capability, size goes to end of config space
> > +            self.pdev.cfg_size().into_raw() - offset
> > +        } else {
> > +            // Size is distance to next capability
> > +            next_ptr - offset
> > +        }
> > +    }
> > +
> > +    /// Finds the next occurrence of a specific extended
> > capability starting from a given position.
> > +    pub fn find_next_ext_capability(
> > +        &self,
> > +        start_pos: u16,
> > +        id: u16,
> > +    ) -> Option<Capability<'_, Extended, Extended>> {
> > +        // SAFETY: pdev is valid by ConfigSpace invariants
> > +        let offset = unsafe {
> > +
> > bindings::pci_find_next_ext_capability(self.pdev.as_raw(),
> > start_pos, id as i32)
> > +        };
> > +
> > +        if offset == 0 {
> > +            return None;
> > +        }
> > +
> > +        // Calculate real capability size
> > +        let size = self.calculate_ext_cap_size(offset as usize);
> > +        Some(Capability::new(self, offset as usize, id, size))
> > +    }
> > +}
> > +
> > +/// SR-IOV register offsets (relative to the capability base).
> > +mod sriov_offsets {
> > +    use crate::bindings;
> > +
> > +    /// First VF Offset register offset
> > +    pub(super) const VF_OFFSET: usize =
> > bindings::PCI_SRIOV_VF_OFFSET as usize;
> > +    /// VF BAR0 register offset (first of 6 VF BARs)
> > +    pub(super) const VF_BAR0: usize = bindings::PCI_SRIOV_BAR as
> > usize; +}
> > +
> > +/// SR-IOV capability structure.
> > +///
> > +/// This structure provides typed access to the SR-IOV extended
> > capability +/// registers using the PCI configuration space backend.
> > +pub struct SriovCapability<'a> {
> > +    cap: Capability<'a, Extended, Extended>,
> > +}
> > +
> > +impl<'a> SriovCapability<'a> {
> > +    /// Creates a new SR-IOV capability from an extended
> > capability.
> > +    pub fn new(cap: Capability<'a, Extended, Extended>) ->
> > Result<Self> {
> > +        if cap.id() != ExtCapabilityId::SRIOV as u16 {
> > +            return Err(EINVAL);
> > +        }
> > +        Ok(Self { cap })
> > +    }
> > +
> > +    /// Tries to find and create an SR-IOV capability from a
> > config space.
> > +    pub fn from_config_space(config_space: &'a ConfigSpace<'a,
> > Extended>) -> Result<Self> {
> > +        let cap = config_space
> > +            .find_ext_capability(ExtCapabilityId::SRIOV as u16)
> > +            .ok_or(ENODEV)?;
> > +        Self::new(cap)
> > +    }
> > +
> > +    /// Returns the offset of this capability in configuration
> > space.
> > +    pub fn offset(&self) -> usize {
> > +        self.cap.offset()
> > +    }
> > +
> > +    /// Reads the First VF Offset register.
> > +    pub fn read_vf_offset(&self) -> Result<u16> {
> > +        self.cap.try_read16(sriov_offsets::VF_OFFSET)
> > +    }
> > +
> > +    /// Reads a VF BAR register (32-bit).
> > +    /// Returns the 32-bit value of the specified VF BAR register.
> > +    pub fn read_vf_bar(&self, bar_index: usize) -> Result<u32> {
> > +        if bar_index >= 6 {
> > +            return Err(EINVAL);
> > +        }
> > +        self.cap.try_read32(sriov_offsets::VF_BAR0 + bar_index * 4)
> > +    }
> > +
> > +    /// Reads a 64-bit VF BAR register.
> > +    /// Returns the 64-bit address combining BAR[n] (low) and
> > BAR[n+1] (high).
> > +    pub fn read_vf_bar64(&self, bar_index: usize) -> Result<u64> {
> > +        if bar_index >= 5 {
> > +            return Err(EINVAL);
> > +        }
> > +        let low = self.read_vf_bar(bar_index)?;
> > +        let high = self.read_vf_bar(bar_index + 1)?;
> > +        Ok((u64::from(high) << 32) | u64::from(low))
> > +    }
> > +}
> 

Powered by blists - more mailing lists