* [PATCH v8 0/1] Rust PCI capability infrastructure and SR-IOV support @ 2026-08-18 8:46 Zhi Wang 2026-08-18 8:46 ` [PATCH v8 1/1] rust: pci: add extended capability " Zhi Wang 0 siblings, 1 reply; 11+ messages in thread From: Zhi Wang @ 2026-08-18 8:46 UTC (permalink / raw) To: rust-for-linux, linux-pci, linux-kernel Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang, daniel.almeida, tamird, work This is a follow-up to v7 [15]. This patch has been used in the Boot GSP with vGPU enabled series [6]. The patch defines an ExtCapability trait that associates an extended capability ID with its register layout. The generic ConfigSpace::find_ext_capability() finder locates the capability, bounds it at the next capability or the end of extended configuration space, and projects the ConfigSpace view to the requested layout. It returns None when the capability is absent and propagates errors encountered while constructing the view. This lets the existing I/O projection and access macros operate on capability registers. ExtSriovRegs provides the SR-IOV register layout. ExtSriovCapability::vf_bars() validates the six raw VF BAR slots and returns an iterator over logical BARs. The iterator handles the different slot widths of 32-bit and 64-bit BARs internally and yields decoded ExtSriovVfBar values containing the address and width. Changes since v7: - Changed ExtCapId to a transparent newtype so unknown and future capability IDs can be represented. (Gary) - Made the extended capability header private. (Gary) - Decoded VF BARs only once instead of decoding them again from Iterator::next(). (Gary) - Documented when projection to the requested register layout can fail. (Gary) - Reworded the CAST comments for PCIe specification constants. (Danilo) - Simplified the doctest imports and use of Result, EINVAL, and io_read!. (Gary, Danilo) - Rebased onto the latest drm-rust-next. (Zhi) The conversion of status and the applicable related registers to typed register fields is not included in this revision. It depends on Gary's new I/O register projection work [16] and will be addressed in v9 after Danilo's integration branch is available. Changes since v6: - Changed ConfigSpace::find_ext_capability() to return Result<Option<...>>, using None rather than ENODEV when the capability is absent. (Gary) - Made calculate_ext_cap_size() propagate errors instead of treating a failed read as the end of capability. (Alex) - Replaced indexed VF BAR access with an iterator so callers cannot select the high DWORD of a 64-bit BAR. (Gary, Alex) - Validated all six VF BAR slots before iteration, made normal iterator exhaustion return None, and logged invalid BAR encodings before returning EINVAL. (Gary) - Decoded VF BAR low DWORDs through a typed bitfield while retaining u32 in ExtSriovRegs for entries that may be 64-bit BAR high DWORDs. (Alex) - Made ExtSriovVfBar fields public and documented, removed their trivial getters, used the ExtSriovCapability alias for its impl block, and applied the suggested local cleanups. (Alex) - Updated the doctest for the optional finder result and VF BAR iterator. (Zhi) - Added #[inline] to the ExtCapId::as_raw() abstraction method. (Sashiko) - Rebased onto the latest drm-rust-next. (Zhi) Changes since v5: - Removed the unused ConfigSpace<Region<0>> offset() and size() inherent methods; ConfigSpace already provides size through the Io trait. (Sashiko, Zhi) - Removed the doctest write to the SR-IOV NumVFs register, avoiding an example that bypasses PCI core SR-IOV state management. (Sashiko) - Corrected Function Dependency Link to an 8-bit field followed by its reserved byte, matching the PCIe SR-IOV register layout. (Sashiko) Changes since v4: - Replaced the separate is_vf_bar_64bit() and read_vf_bar64() helpers with read_vf_bar(), returning a decoded ExtSriovVfBar. (Zhi) - Moved memory BAR attribute stripping into the PCI abstraction and used named PCI attribute definitions rather than an open-coded mask. (Zhi) - Exposed the next logical BAR slot so callers can walk mixed 32-bit and 64-bit VF BAR layouts without duplicating slot arithmetic. (Zhi) - Updated the doctest and PCI exports for the decoded BAR API. (Zhi) Changes since v3: - Replaced the custom ExtCapability<T> I/O wrapper with the existing ConfigSpace view infrastructure. (Alex) - Reused ExtCapability as a trait carrying the capability ID, and made ConfigSpace::find_ext_capability() generic over register layouts. (Alex) - Removed public cast_sized() and unused find_next_ext_capability(). (Alex) - Kept capability construction in the generic finder and documented calculate_ext_cap_size(). (Alex) - Used PCI_SRIOV_NUM_BARS rather than a literal VF BAR count. (Alex, Zhi) - Added is_vf_bar_64bit() and made read_vf_bar64() reject BARs that are not 64-bit memory BARs. (Alex, Zhi) - Kept indexed VF BAR helpers because the Nova user accesses fixed BAR slots rather than iterating over them. (Alex) - Adapted the implementation and doctest to the current ConfigSpace I/O APIs. (Zhi) Changes since RFC v2: - Hardened calculate_ext_cap_size() against corrupt capability lists. (Zhi) - Added // INVARIANT: comments at all ExtCapability construction sites (make_ext_capability and cast_sized). (Zhi) - Added #[inline] to small forwarding methods (find, read_vf_bar64). (Zhi) Changes since RFC: - Rebased on io_projection branch, using Gary's Io/IoCapable traits. (Gary) - ExtCapability implements Io and delegates IoCapable to ConfigSpace instead of duplicating config read/write logic. (Gary) - Dropped the fallible I/O patch (now upstream in this tree). (Zhi) - Added Rust helper for PCI_EXT_CAP_NEXT() macro. (Zhi) - Replaced raw `as` casts with From conversions where possible. (Zhi) - Renamed SriovRegs/SriovCapability to ExtSriovRegs/ExtSriovCapability. (Zhi) [1] https://lore.kernel.org/rust-for-linux/20260409185254.3869808-1-zhiw@nvidia.com/ [2] https://lore.kernel.org/rust-for-linux/DHRTUAF52GNI.1J98TSAG1LS6Q@nvidia.com/ [3] https://lore.kernel.org/rust-for-linux/DI2SL4G5INLY.2W1IFTR081ID3@nvidia.com/ [4] https://lore.kernel.org/rust-for-linux/20260225180449.1813833-1-zhiw@nvidia.com/ [5] https://lore.kernel.org/rust-for-linux/20260323153807.1360705-1-gary@kernel.org/ [6] https://lore.kernel.org/rust-for-linux/20260313165336.935771-1-zhiw@nvidia.com/ [7] https://lore.kernel.org/rust-for-linux/20260714165827.2937960-1-zhiw@nvidia.com/ [8] https://lore.kernel.org/rust-for-linux/20260730180349.771719-1-zhiw@nvidia.com/ [9] https://sashiko.dev/#/patchset/20260730180349.771719-2-zhiw@nvidia.com?part=1 [10] https://lore.kernel.org/rust-for-linux/20260730182954.783568-1-zhiw@nvidia.com/ [11] https://lore.kernel.org/rust-for-linux/DKC4MR1ZBQMP.3L7X2C96OJLFU@garyguo.net/ [12] https://lore.kernel.org/rust-for-linux/DKCOUE9DRG5R.2VAVA78UTVKST@nvidia.com/ [13] https://lore.kernel.org/rust-for-linux/DKCRGMZJXO6X.1SWGREC78DH0W@garyguo.net/ [14] https://lore.kernel.org/linux-pci/20260730183913.B00061F000E9@smtp.kernel.org/ [15] https://lore.kernel.org/rust-for-linux/20260804161612.776752-1-zhiw@nvidia.com/ [16] https://lore.kernel.org/rust-for-linux/20260805-typed_register-v2-0-c3ca142220a0@garyguo.net/ Zhi Wang (1): rust: pci: add extended capability and SR-IOV support 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 base-commit: 4c9ba407018e8deb06dbc643112bac8f40404f95 -- 2.53.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-18 8:46 [PATCH v8 0/1] Rust PCI capability infrastructure and SR-IOV support Zhi Wang @ 2026-08-18 8:46 ` Zhi Wang 2026-08-18 8:55 ` sashiko-bot 2026-08-24 8:12 ` Alexandre Courbot 0 siblings, 2 replies; 11+ messages in thread From: Zhi Wang @ 2026-08-18 8:46 UTC (permalink / raw) To: rust-for-linux, linux-pci, linux-kernel Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, acourbot, joelagnelf, jhubbard, zhiwang, Zhi Wang, daniel.almeida, tamird, work 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/20260804161612.776752-1-zhiw@nvidia.com/ Cc: Alexandre Courbot <acourbot@nvidia.com> Cc: Gary Guo <gary@garyguo.net> Signed-off-by: Zhi Wang <zhiw@nvidia.com> --- 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, // +}; +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; + +/// 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); + + /// Creates an extended capability ID from its raw PCIe value. + #[inline] + pub const fn new(id: u16) -> Self { + Self(id) + } + + /// Returns the raw PCIe extended capability ID. + #[inline] + const fn as_raw(self) -> u16 { + self.0 + } +} + +/// A typed PCI extended capability register layout. +/// +/// Implementors describe the register layout of one extended capability. The layout must start at +/// the extended capability header, and [`Self::ID`] must identify that layout. +pub trait ExtCapability: FromBytes + IntoBytes { + /// PCI extended capability ID for this register layout. + const ID: ExtCapId; +} + +impl<'a> ConfigSpace<'a, Extended> { + /// Finds and projects an extended capability into its typed register layout. + /// + /// Returns [`None`] if the device does not implement the capability. + /// + /// Returns an error if the capability is present but its register span is too small or + /// insufficiently aligned for `C`. + /// + /// # Examples + /// + /// ```no_run + /// use kernel::{ + /// device::Bound, + /// io::io_read, + /// pci, + /// prelude::*, + /// }; + /// + /// fn probe_sriov(pdev: &pci::Device<Bound>) -> Result { + /// let Some(sriov) = pdev + /// .config_space_extended()? + /// .find_ext_capability::<pci::ExtSriovRegs>()? + /// else { + /// return Ok(()); + /// }; + /// + /// let total_vfs = io_read!(sriov, .total_vfs); + /// let vf_offset = io_read!(sriov, .vf_offset); + /// let mut vf_bars = sriov.vf_bars()?; + /// let bar0 = vf_bars.next().ok_or(EINVAL)?; + /// let bar1 = vf_bars.next().ok_or(EINVAL)?; + /// let bar2 = vf_bars.next().ok_or(EINVAL)?; + /// + /// Ok(()) + /// } + /// ``` + pub fn find_ext_capability<C: ExtCapability>(&self) -> Result<Option<ConfigSpace<'a, C>>> { + let offset = usize::from( + // SAFETY: `self.pdev` is valid by the type invariant of `ConfigSpace`. + unsafe { + bindings::pci_find_ext_capability(self.pdev.as_raw(), i32::from(C::ID.as_raw())) + }, + ); + + if offset == 0 { + return Ok(None); + } + + let size = self.calculate_ext_cap_size(offset)?; + + let base = ConfigSpaceBackend::as_ptr(*self) + .cast::<u8>() + .wrapping_add(offset); + let ptr = Region::<0>::ptr_try_from_raw_parts_mut(base, size)?; + + // SAFETY: `offset` was returned by `pci_find_ext_capability`, and + // `calculate_ext_cap_size` bounds `ptr` at the next capability or the end of the extended + // configuration space. `ptr_try_from_raw_parts_mut` verified the region layout. + let capability = unsafe { ConfigSpaceBackend::project_view(*self, ptr) }; + + capability.try_cast::<C>().map(Some) + } + + /// 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`. Returns an error if + /// the capability header is outside the extended configuration space. + fn calculate_ext_cap_size(&self, offset: usize) -> Result<usize> { + let header = self.try_read32(offset)?; + // 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; + + Ok(if next > offset { + next - offset + } else { + self.size() - offset + }) + } +} + +/// 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, + /// 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, +} + +impl ExtCapability for ExtSriovRegs { + const ID: ExtCapId = ExtCapId::SRIOV; +} + +/// A typed view of an SR-IOV extended capability. +pub type ExtSriovCapability<'a> = ConfigSpace<'a, ExtSriovRegs>; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum VfBarMemoryType { + Bits32, + Bits64, +} + +impl TryFrom<Bounded<u32, 2>> for VfBarMemoryType { + type Error = Error; + + fn try_from(value: Bounded<u32, 2>) -> Result<Self> { + match value.get() { + 0b00 => Ok(Self::Bits32), + 0b10 => Ok(Self::Bits64), + _ => Err(EINVAL), + } + } +} + +impl From<VfBarMemoryType> for Bounded<u32, 2> { + fn from(value: VfBarMemoryType) -> Self { + match value { + VfBarMemoryType::Bits32 => Self::new::<0b00>(), + VfBarMemoryType::Bits64 => Self::new::<0b10>(), + } + } +} + +crate::bitfield! { + /// Low DWORD of an SR-IOV VF BAR. + struct VfBarLow(u32) { + /// Base address bits 31:4. + 31:4 address; + /// Whether the address range is prefetchable. + 3:3 prefetchable => bool; + /// Memory BAR type. + 2:1 memory_type ?=> VfBarMemoryType; + /// Whether this is an I/O-space BAR. + 0:0 io_space => bool; + } +} + +/// A decoded VF BAR register encoding. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ExtSriovVfBar { + /// The BAR address without PCI attribute bits. + pub address: u64, + + /// Whether the BAR is 64-bit. + pub is_64bit: bool, +} + +/// Iterator over decoded VF BAR register encodings. +/// +/// A 32-bit memory BAR encoding uses one register. A 64-bit memory BAR encoding uses that register +/// for bits 31:0 and the immediately following register for bits 63:32. +/// +/// # Invariants +/// +/// - `next_bar <= bar_count <= NUM_VF_BARS`. +/// - Entries before `bar_count` contain decoded VF BARs in logical order. +struct ExtSriovVfBars { + bars: [ExtSriovVfBar; NUM_VF_BARS], + bar_count: usize, + next_bar: usize, +} + +impl ExtSriovVfBars { + fn new(slots: [u32; NUM_VF_BARS]) -> Result<Self> { + let mut bars = [ExtSriovVfBar { + address: 0, + is_64bit: false, + }; NUM_VF_BARS]; + let mut bar_count = 0; + let mut config_slot = 0; + + while config_slot < NUM_VF_BARS { + let low = VfBarLow::from(slots[config_slot]); + + if low.io_space() { + return Err(EINVAL); + } + + let is_64bit = low.memory_type()? == VfBarMemoryType::Bits64; + let low_address = u64::from(low.address()) << VfBarLow::ADDRESS_SHIFT; + + let address = if is_64bit { + if config_slot + 1 >= NUM_VF_BARS { + return Err(EINVAL); + } + + let high = slots[config_slot + 1]; + config_slot += 2; + (u64::from(high) << 32) | low_address + } else { + config_slot += 1; + low_address + }; + + bars[bar_count] = ExtSriovVfBar { address, is_64bit }; + bar_count += 1; + } + + 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>> { + let slots: [u32; NUM_VF_BARS] = + core::array::from_fn(|slot| crate::io_read!(*self, .vf_bar[panic: slot])); + + ExtSriovVfBars::new(slots).inspect_err(|_| { + dev_err!(self.pdev, "invalid VF BAR encoding in SR-IOV capability\n"); + }) + } +} -- 2.53.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-18 8:46 ` [PATCH v8 1/1] rust: pci: add extended capability " Zhi Wang @ 2026-08-18 8:55 ` sashiko-bot 2026-08-24 8:12 ` Alexandre Courbot 1 sibling, 0 replies; 11+ messages in thread From: sashiko-bot @ 2026-08-18 8:55 UTC (permalink / raw) To: Zhi Wang; +Cc: linux-pci, ojeda Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The `ExtSriovVfBars` struct is constructed without an `// INVARIANT:` comment explaining why its invariants are satisfied. -- --- Patch [1]: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support --- commit e0446bc8a9435cb9072472f040f7215e21d10ce1 Author: Zhi Wang <zhiw@nvidia.com> rust: pci: add extended capability and SR-IOV support This commit adds Rust PCI abstractions for locating and accessing PCIe extended capabilities. It provides specific support for the SR-IOV extended capability and a decoded VF BAR iterator. Link: https://lore.kernel.org/rust-for-linux/20260804161612.776752-1-zhiw@nvidia.com/ > diff --git a/rust/kernel/pci/cap.rs b/rust/kernel/pci/cap.rs > new file mode 100644 > index 0000000000000..ddb3fd73e1955 > --- /dev/null > +++ b/rust/kernel/pci/cap.rs [ ... ] > +struct ExtSriovVfBars { > + bars: [ExtSriovVfBar; NUM_VF_BARS], > + bar_count: usize, > + next_bar: usize, > +} > + > +impl ExtSriovVfBars { > + fn new(slots: [u32; NUM_VF_BARS]) -> Result<Self> { [ ... ] > + bars[bar_count] = ExtSriovVfBar { address, is_64bit }; > + bar_count += 1; > + } > + > + Ok(Self { > + bars, > + bar_count, > + next_bar: 0, > + }) [Severity: Low] This isn't a bug, but the Rust subsystem guidelines require an // INVARIANT: comment when constructing a struct that has documented invariants. ExtSriovVfBars documents its invariants above, but is constructed here without explaining how they are satisfied. Could an // INVARIANT: comment be added here? > + } > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/20260818084633.1673214-2-zhiw@nvidia.com?part=1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-18 8:46 ` [PATCH v8 1/1] rust: pci: add extended capability " Zhi Wang 2026-08-18 8:55 ` sashiko-bot @ 2026-08-24 8:12 ` Alexandre Courbot 2026-08-24 10:48 ` Gary Guo 2026-08-24 11:38 ` Danilo Krummrich 1 sibling, 2 replies; 11+ messages in thread From: Alexandre Courbot @ 2026-08-24 8:12 UTC (permalink / raw) To: Zhi Wang Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work 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/20260804161612.776752-1-zhiw@nvidia.com/ > Cc: Alexandre Courbot <acourbot@nvidia.com> > Cc: Gary Guo <gary@garyguo.net> > Signed-off-by: Zhi Wang <zhiw@nvidia.com> > --- > 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 }>()); > + > + /// 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. > + 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`. :) > +} > + > +/// A typed PCI extended capability register layout. > +/// > +/// Implementors describe the register layout of one extended capability. The layout must start at > +/// the extended capability header, and [`Self::ID`] must identify that layout. > +pub trait ExtCapability: FromBytes + IntoBytes { > + /// PCI extended capability ID for this register layout. > + const ID: ExtCapId; > +} > + > +impl<'a> ConfigSpace<'a, Extended> { > + /// Finds and projects an extended capability into its typed register layout. > + /// > + /// Returns [`None`] if the device does not implement the capability. > + /// > + /// Returns an error if the capability is present but its register span is too small or > + /// insufficiently aligned for `C`. > + /// > + /// # Examples > + /// > + /// ```no_run > + /// use kernel::{ > + /// device::Bound, > + /// io::io_read, > + /// pci, > + /// prelude::*, > + /// }; > + /// > + /// fn probe_sriov(pdev: &pci::Device<Bound>) -> Result { > + /// let Some(sriov) = pdev > + /// .config_space_extended()? > + /// .find_ext_capability::<pci::ExtSriovRegs>()? > + /// else { > + /// return Ok(()); > + /// }; > + /// > + /// let total_vfs = io_read!(sriov, .total_vfs); > + /// let vf_offset = io_read!(sriov, .vf_offset); > + /// let mut vf_bars = sriov.vf_bars()?; > + /// let bar0 = vf_bars.next().ok_or(EINVAL)?; > + /// let bar1 = vf_bars.next().ok_or(EINVAL)?; > + /// let bar2 = vf_bars.next().ok_or(EINVAL)?; > + /// > + /// Ok(()) > + /// } > + /// ``` > + pub fn find_ext_capability<C: ExtCapability>(&self) -> Result<Option<ConfigSpace<'a, C>>> { > + let offset = usize::from( > + // SAFETY: `self.pdev` is valid by the type invariant of `ConfigSpace`. > + unsafe { > + bindings::pci_find_ext_capability(self.pdev.as_raw(), i32::from(C::ID.as_raw())) > + }, > + ); > + > + if offset == 0 { > + return Ok(None); > + } > + > + let size = self.calculate_ext_cap_size(offset)?; > + > + let base = ConfigSpaceBackend::as_ptr(*self) > + .cast::<u8>() > + .wrapping_add(offset); > + let ptr = Region::<0>::ptr_try_from_raw_parts_mut(base, size)?; > + > + // SAFETY: `offset` was returned by `pci_find_ext_capability`, and > + // `calculate_ext_cap_size` bounds `ptr` at the next capability or the end of the extended > + // configuration space. `ptr_try_from_raw_parts_mut` verified the region layout. > + let capability = unsafe { ConfigSpaceBackend::project_view(*self, ptr) }; > + > + capability.try_cast::<C>().map(Some) > + } > + > + /// 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`. Returns an error if > + /// the capability header is outside the extended configuration space. > + fn calculate_ext_cap_size(&self, offset: usize) -> Result<usize> { > + let header = self.try_read32(offset)?; > + // 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; This `CAST` as well can be removed: let next = casts::u32_as_usize(unsafe { bindings::pci_ext_cap_next(header) }); > + > + Ok(if next > offset { > + next - offset > + } else { > + self.size() - offset > + }) > + } > +} > + > +/// 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, > + /// 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], 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. > + /// VF migration state array offset. > + pub migration_state: u32, > +} > + > +impl ExtCapability for ExtSriovRegs { > + const ID: ExtCapId = ExtCapId::SRIOV; > +} > + > +/// A typed view of an SR-IOV extended capability. > +pub type ExtSriovCapability<'a> = ConfigSpace<'a, ExtSriovRegs>; > + > +#[derive(Debug, Clone, Copy, PartialEq, Eq)] > +enum VfBarMemoryType { > + Bits32, > + Bits64, > +} > + > +impl TryFrom<Bounded<u32, 2>> for VfBarMemoryType { > + type Error = Error; > + > + fn try_from(value: Bounded<u32, 2>) -> Result<Self> { > + match value.get() { > + 0b00 => Ok(Self::Bits32), > + 0b10 => Ok(Self::Bits64), > + _ => Err(EINVAL), > + } > + } > +} > + > +impl From<VfBarMemoryType> for Bounded<u32, 2> { > + fn from(value: VfBarMemoryType) -> Self { > + match value { > + VfBarMemoryType::Bits32 => Self::new::<0b00>(), > + VfBarMemoryType::Bits64 => Self::new::<0b10>(), > + } > + } > +} > + > +crate::bitfield! { > + /// Low DWORD of an SR-IOV VF BAR. > + struct VfBarLow(u32) { > + /// Base address bits 31:4. > + 31:4 address; > + /// Whether the address range is prefetchable. > + 3:3 prefetchable => bool; > + /// Memory BAR type. > + 2:1 memory_type ?=> VfBarMemoryType; > + /// Whether this is an I/O-space BAR. > + 0:0 io_space => bool; > + } > +} > + > +/// A decoded VF BAR register encoding. > +#[derive(Debug, Clone, Copy, PartialEq, Eq)] > +pub struct ExtSriovVfBar { > + /// The BAR address without PCI attribute bits. > + pub address: u64, > + > + /// Whether the BAR is 64-bit. > + pub is_64bit: bool, > +} > + > +/// Iterator over decoded VF BAR register encodings. > +/// > +/// A 32-bit memory BAR encoding uses one register. A 64-bit memory BAR encoding uses that register > +/// for bits 31:0 and the immediately following register for bits 63:32. > +/// > +/// # Invariants > +/// > +/// - `next_bar <= bar_count <= NUM_VF_BARS`. > +/// - Entries before `bar_count` contain decoded VF BARs in logical order. > +struct ExtSriovVfBars { > + bars: [ExtSriovVfBar; NUM_VF_BARS], > + bar_count: usize, > + next_bar: usize, > +} > + > +impl ExtSriovVfBars { > + fn new(slots: [u32; NUM_VF_BARS]) -> Result<Self> { > + let mut bars = [ExtSriovVfBar { > + address: 0, > + is_64bit: false, > + }; NUM_VF_BARS]; > + let mut bar_count = 0; > + let mut config_slot = 0; > + > + while config_slot < NUM_VF_BARS { > + let low = VfBarLow::from(slots[config_slot]); > + > + if low.io_space() { > + return Err(EINVAL); > + } A comment would be appreciated for those not familiar with the PCI spec. :) > + > + let is_64bit = low.memory_type()? == VfBarMemoryType::Bits64; > + let low_address = u64::from(low.address()) << VfBarLow::ADDRESS_SHIFT; > + > + let address = if is_64bit { > + if config_slot + 1 >= NUM_VF_BARS { > + return Err(EINVAL); > + } > + > + let high = slots[config_slot + 1]; > + config_slot += 2; > + (u64::from(high) << 32) | low_address > + } else { > + config_slot += 1; > + low_address > + }; > + > + bars[bar_count] = ExtSriovVfBar { address, is_64bit }; > + bar_count += 1; > + } This looks a lot like C code - double counters in particular are error-prone. We can make this a bit more idiomatic. I sense you didn't use iterators because 64-bit entries take two slots, but you can use this trick: // Store a mutable iterator. let mut slots = slots.into_iter(); // Note the early conversion to `VfBarLow` while let Some(low) = slots.next().map(VfBarLow::from) { ... let bar = match low.memory_type()? { VfBarMemoryType::Bits64 => ExtSriovVfBar { // We read the second slot here. address: (u64::from(slots.next().ok_or(EINVAL)?) << 32) | low_address, is_64bit: true, }, VfBarMemoryType::Bits32 => ExtSriovVfBar { address: low_address, is_64bit: false, }, }; ... } ... but we can go a bit further, please read along. > + > + 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-24 8:12 ` Alexandre Courbot @ 2026-08-24 10:48 ` Gary Guo 2026-08-24 11:14 ` Alexandre Courbot 2026-08-24 11:38 ` Danilo Krummrich 1 sibling, 1 reply; 11+ messages in thread From: Gary Guo @ 2026-08-24 10:48 UTC (permalink / raw) To: Alexandre Courbot, Zhi Wang Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work 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/20260804161612.776752-1-zhiw@nvidia.com/ >> Cc: Alexandre Courbot <acourbot@nvidia.com> >> Cc: Gary Guo <gary@garyguo.net> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-24 10:48 ` Gary Guo @ 2026-08-24 11:14 ` Alexandre Courbot 2026-08-24 11:59 ` Gary Guo 0 siblings, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2026-08-24 11:14 UTC (permalink / raw) To: Gary Guo Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work On Mon Aug 24, 2026 at 7:48 PM JST, Gary Guo wrote: > 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/20260804161612.776752-1-zhiw@nvidia.com/ >>> Cc: Alexandre Courbot <acourbot@nvidia.com> >>> Cc: Gary Guo <gary@garyguo.net> >>> Signed-off-by: Zhi Wang <zhiw@nvidia.com> >>> --- >>> 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`. Looks are subjective (or so I like to tell myself), so let's focus on what isn't. What is not subjective is that you don't lose a single bit as long as you use these functions, and can do your conversions without a `CAST` comment. These helpers were merged precisely for this use-case; if you want to remove them and use `as` instead, that's of course open but also a larger discussion than this patch. > >>> [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. Yeah no strong opinion on this. The use of a closure makes me a bit unsure that the optimizer will catch the hint at every level of optimization. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-24 11:14 ` Alexandre Courbot @ 2026-08-24 11:59 ` Gary Guo 2026-08-24 15:21 ` Alexandre Courbot 0 siblings, 1 reply; 11+ messages in thread From: Gary Guo @ 2026-08-24 11:59 UTC (permalink / raw) To: Alexandre Courbot, Gary Guo Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work On Mon Aug 24, 2026 at 12:14 PM BST, Alexandre Courbot wrote: > On Mon Aug 24, 2026 at 7:48 PM JST, Gary Guo wrote: >> 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/20260804161612.776752-1-zhiw@nvidia.com/ >>>> Cc: Alexandre Courbot <acourbot@nvidia.com> >>>> Cc: Gary Guo <gary@garyguo.net> >>>> Signed-off-by: Zhi Wang <zhiw@nvidia.com> >>>> --- >>>> 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`. > > Looks are subjective (or so I like to tell myself), so let's focus on > what isn't. > > What is not subjective is that you don't lose a single bit as long as > you use these functions, and can do your conversions without a `CAST` > comment. Aesthetic and ergnomics is a big part of code, and is what make people like Rust. In a world where less code is produced by human, I think it's especially important that code remains easily human readable. You can probably already tell that I have very strong opinion about this. > > These helpers were merged precisely for this use-case; if you want to > remove them and use `as` instead, that's of course open but also a > larger discussion than this patch. I was under impression that the series just provides safe cast from lower width integer to higher width ones (mostly for u32 -> usize and usize -> u64). If I see the above snippet, I'd nacked it. This is not what const generics is designed for and I rather view this as abuse of language feature. Best, Gary ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-24 11:59 ` Gary Guo @ 2026-08-24 15:21 ` Alexandre Courbot 2026-08-24 15:46 ` Gary Guo 0 siblings, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2026-08-24 15:21 UTC (permalink / raw) To: Gary Guo Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work On Mon Aug 24, 2026 at 8:59 PM JST, Gary Guo wrote: > On Mon Aug 24, 2026 at 12:14 PM BST, Alexandre Courbot wrote: >> On Mon Aug 24, 2026 at 7:48 PM JST, Gary Guo wrote: >>> 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/20260804161612.776752-1-zhiw@nvidia.com/ >>>>> Cc: Alexandre Courbot <acourbot@nvidia.com> >>>>> Cc: Gary Guo <gary@garyguo.net> >>>>> Signed-off-by: Zhi Wang <zhiw@nvidia.com> >>>>> --- >>>>> 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`. >> >> Looks are subjective (or so I like to tell myself), so let's focus on >> what isn't. >> >> What is not subjective is that you don't lose a single bit as long as >> you use these functions, and can do your conversions without a `CAST` >> comment. > > Aesthetic and ergnomics is a big part of code, and is what make people like > Rust. In a world where less code is produced by human, I think it's especially > important that code remains easily human readable. > > You can probably already tell that I have very strong opinion about this. So do I, and the fact we both have opinions on the matter is irrelevant. "It looks horrible" is not an argument. Safety and correctness are the very reason for using Rust, not that it looks better or is more ergonomic. It often does, it sometimes doesn't (see the hoops we have to jump through to cast a pointer for instance), and when it does, that is usually to improve correctness, not to compromise it. The reason we go through these lengths is to remove issues and footguns at build-time, and this is exactly what these helpers do because `as` is one of the footguns. They are just a temporary band-aid and won't be there forever anyway: once we have const `TryFrom`, we can make a bonfire with them, and I'll be happy to light the match myself. Meanwhile, we can very well make things more sightly through the use of a macro. `casts::u32_into_u16!(...)` would definitely look better if we overlook the exclamation mark. > >> >> These helpers were merged precisely for this use-case; if you want to >> remove them and use `as` instead, that's of course open but also a >> larger discussion than this patch. > > I was under impression that the series just provides safe cast from lower width > integer to higher width ones (mostly for u32 -> usize and usize -> u64). If I > see the above snippet, I'd nacked it. > > This is not what const generics is designed for and I rather view this as abuse > of language feature. I cannot say I am versed enough to know what the deep intent of a language feature is. But if you are suggesting that I used such a feature in a way that goes beyond its original intent, then I can only take it as the highest form of compliment around here. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-24 15:21 ` Alexandre Courbot @ 2026-08-24 15:46 ` Gary Guo 2026-08-24 16:25 ` Gary Guo 0 siblings, 1 reply; 11+ messages in thread From: Gary Guo @ 2026-08-24 15:46 UTC (permalink / raw) To: Alexandre Courbot, Gary Guo Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work On Mon Aug 24, 2026 at 4:21 PM BST, Alexandre Courbot wrote: > On Mon Aug 24, 2026 at 8:59 PM JST, Gary Guo wrote: >> On Mon Aug 24, 2026 at 12:14 PM BST, Alexandre Courbot wrote: >>> On Mon Aug 24, 2026 at 7:48 PM JST, Gary Guo wrote: >>>> 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/20260804161612.776752-1-zhiw@nvidia.com/ >>>>>> Cc: Alexandre Courbot <acourbot@nvidia.com> >>>>>> Cc: Gary Guo <gary@garyguo.net> >>>>>> Signed-off-by: Zhi Wang <zhiw@nvidia.com> >>>>>> --- >>>>>> 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`. >>> >>> Looks are subjective (or so I like to tell myself), so let's focus on >>> what isn't. >>> >>> What is not subjective is that you don't lose a single bit as long as >>> you use these functions, and can do your conversions without a `CAST` >>> comment. >> >> Aesthetic and ergnomics is a big part of code, and is what make people like >> Rust. In a world where less code is produced by human, I think it's especially >> important that code remains easily human readable. >> >> You can probably already tell that I have very strong opinion about this. > > So do I, and the fact we both have opinions on the matter is irrelevant. > "It looks horrible" is not an argument. Ergnomics is a perfectly valid argument and often a key factor in design decisions. Otherwise we'd make all functions carry extra argument indicating the context they're in and would require sleeping functions to carry such token types. We rejected that approach because it'll infect all functions and make Rust code look horrible. Everything is a trade-off. Whether to perform extra validation vs better ergnomics is a genuine thing that needs deliberation. A outright dismissal of the argument is itself a non-constructive argument. > > Safety and correctness are the very reason for using Rust, not that it > looks better or is more ergonomic. It often does, it sometimes doesn't > (see the hoops we have to jump through to cast a pointer for instance), > and when it does, that is usually to improve correctness, not to > compromise it. The reason we go through these lengths is to remove > issues and footguns at build-time, and this is exactly what these > helpers do because `as` is one of the footguns. There's nothing unsafe or incorrect in using `as`. Your suggested code is no more correct than Zhi's current version. You just added some extra check to some constant that will never change, and we know will not produce error. My argument is that that makes the syntax horrible and it's not a trade-off worth making. When we create new abstractions, we try to make thing has minimal overhead. Otherwise we'd be using a GC language and that'd solve memory safety issues. Your abstraction is not zero-cost -- it sacrifices compilation-time, binary size (for debug info) and it adds a significant cognitive overhead for writing code. > > They are just a temporary band-aid and won't be there forever anyway: > once we have const `TryFrom`, we can make a bonfire with them, and I'll > be happy to light the match myself. Well, I'm happy converting the code once we have `const TryFrom`, and until then, I'd rather keep `as`. Best, Gary ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-24 15:46 ` Gary Guo @ 2026-08-24 16:25 ` Gary Guo 0 siblings, 0 replies; 11+ messages in thread From: Gary Guo @ 2026-08-24 16:25 UTC (permalink / raw) To: Gary Guo, Alexandre Courbot Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work On Mon Aug 24, 2026 at 4:46 PM BST, Gary Guo wrote: > On Mon Aug 24, 2026 at 4:21 PM BST, Alexandre Courbot wrote: >> On Mon Aug 24, 2026 at 8:59 PM JST, Gary Guo wrote: >>> >>> Aesthetic and ergnomics is a big part of code, and is what make people like >>> Rust. In a world where less code is produced by human, I think it's especially >>> important that code remains easily human readable. >>> >>> You can probably already tell that I have very strong opinion about this. >> >> So do I, and the fact we both have opinions on the matter is irrelevant. >> "It looks horrible" is not an argument. > > Ergnomics is a perfectly valid argument and often a key factor in design > decisions. Otherwise we'd make all functions carry extra argument indicating the > context they're in and would require sleeping functions to carry such token > types. We rejected that approach because it'll infect all functions and make > Rust code look horrible. > > Everything is a trade-off. Whether to perform extra validation vs better > ergnomics is a genuine thing that needs deliberation. A outright dismissal of > the argument is itself a non-constructive argument. > >> >> Safety and correctness are the very reason for using Rust, not that it >> looks better or is more ergonomic. It often does, it sometimes doesn't >> (see the hoops we have to jump through to cast a pointer for instance), >> and when it does, that is usually to improve correctness, not to >> compromise it. The reason we go through these lengths is to remove >> issues and footguns at build-time, and this is exactly what these >> helpers do because `as` is one of the footguns. > > There's nothing unsafe or incorrect in using `as`. Your suggested code is no > more correct than Zhi's current version. You just added some extra check to some > constant that will never change, and we know will not produce error. My argument > is that that makes the syntax horrible and it's not a trade-off worth making. > > When we create new abstractions, we try to make thing has minimal overhead. > Otherwise we'd be using a GC language and that'd solve memory safety issues. > Your abstraction is not zero-cost -- it sacrifices compilation-time, binary size > (for debug info) and it adds a significant cognitive overhead for writing code. The code here is const eval, so debug info doesn't apply. And for the specific cases of functions containing only a single cast, as all instructions are going to be optimized out debug info should (hopefully) not be left too. However, metadata sizes would still be larger because rustc record all inlined functions within its MIR. Anyway, this is really more an auxiliary point, my main focus is on the cognitive complexity. Best, Gary ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/1] rust: pci: add extended capability and SR-IOV support 2026-08-24 8:12 ` Alexandre Courbot 2026-08-24 10:48 ` Gary Guo @ 2026-08-24 11:38 ` Danilo Krummrich 1 sibling, 0 replies; 11+ messages in thread From: Danilo Krummrich @ 2026-08-24 11:38 UTC (permalink / raw) To: Alexandre Courbot Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, boqun, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, cjia, smitra, ankita, aniketa, kwankhede, targupta, kjaju, alkumar, joelagnelf, jhubbard, zhiwang, daniel.almeida, tamird, work 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-08-24 16:25 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-18 8:46 [PATCH v8 0/1] Rust PCI capability infrastructure and SR-IOV support Zhi Wang 2026-08-18 8:46 ` [PATCH v8 1/1] rust: pci: add extended capability " Zhi Wang 2026-08-18 8:55 ` sashiko-bot 2026-08-24 8:12 ` Alexandre Courbot 2026-08-24 10:48 ` Gary Guo 2026-08-24 11:14 ` Alexandre Courbot 2026-08-24 11:59 ` Gary Guo 2026-08-24 15:21 ` Alexandre Courbot 2026-08-24 15:46 ` Gary Guo 2026-08-24 16:25 ` Gary Guo 2026-08-24 11:38 ` Danilo Krummrich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.