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

On Mon Aug 24, 2026 at 10:12 AM CEST, Alexandre Courbot wrote:
>> 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`?

I think I prefer it as is. Eventually PCI will be its own crate, so it makes
sense.

Although, ultimately I'd probably want this to be

	use crate::{
	    io::ConfigSpaceBackend,
	    ...
	};

but having it as a separate block already seems fine.

>> +use crate::{
>> +    bindings,
>> +    io::{
>> +        Io,
>> +        IoBackend,
>> +        Region, //
>> +    },
>> +    num::Bounded,
>> +    prelude::*,
>> +};

[...]

>> +/// 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 }>());

(From the other thread.)

I agree that the variant without casts::u32_into_u16() reads better, but I think
this will impove at some point to

	casts::u32_into_u16(bindings::PCI_EXT_CAP_ID_SRIOV)

or just bindings::PCI_EXT_CAP_ID_SRIOV.into(), so it's not a huge concern and it
does the job.

>> +
>> +    /// Creates an extended capability ID from its raw PCIe value.
>> +    #[inline]
>> +    pub const fn new(id: u16) -> Self {
>
> For symmetry with `as_raw`, should this be `from_raw`? The other PCI
> types (e.g. Class and Vendor) also use this naming pattern.

I think new() is never used, so let's just drop it?

>> +        Self(id)
>> +    }
>> +
>> +    /// Returns the raw PCIe extended capability ID.
>> +    #[inline]
>> +    const fn as_raw(self) -> u16 {
>> +        self.0
>> +    }
>
> ... and for symmetry as well, let's make this `pub`. :)

If we don't need it outside this module, let's keep it private.

>> +/// 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?

>> +    /// 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()?

>> +    _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.