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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `func_dep_link` field in `ExtSriovRegs` is incorrectly defined as a `u16`, merging an 8-bit field with an adjacent reserved byte.
- [Medium] The documentation example for `find_ext_capability` demonstrates writing directly to the SR-IOV `num_vfs` capability register, which bypasses the PCI subsystem's state management.
- [Low] The `offset()` and `size()` methods are implemented exclusively for `ConfigSpace<'_, Region<0>>`, making them inaccessible on typed capability views.
--

--- Patch [1]: [PATCH v5 1/1] rust: pci: add extended capability and SR-IOV support ---
commit 5d46dcade5cf8eab7e07c33859ca991281a57115
Author: Zhi Wang <[email protected]>

rust: pci: add extended capability and SR-IOV support

This patch introduces Rust bindings for PCI extended capabilities, including
methods to locate and project these capabilities. It also defines the register
layout for the SR-IOV capability and provides a helper for decoding VF memory
BARs.

Link: https://lore.kernel.org/rust-for-linux/[email protected]/

> diff --git a/rust/kernel/pci/cap.rs b/rust/kernel/pci/cap.rs
> new file mode 100644
> index 0000000000000..004ee62bd0895
> --- /dev/null
> +++ b/rust/kernel/pci/cap.rs
> @@ -0,0 +1,251 @@
[ ... ]
> +impl ConfigSpace<'_, Region<0>> {
> +    /// Base offset of this capability in configuration space.
> +    #[inline]
> +    pub fn offset(&self) -> usize {

[Severity: Low]
If offset() and size() are implemented exclusively for ConfigSpace<'_, Region<0>>,
callers would not be able to retrieve the offset or size from typed capabilities
like ConfigSpace<'_, ExtSriovRegs>.

Is it intended to restrict offset() and size() only to Region<0>, or could this
be implemented for generic types instead?

> +        ConfigSpaceBackend::as_ptr(*self).addr()
> +    }
[ ... ]
> +    /// # Examples
> +    ///
> +    /// ```no_run
> +    /// use kernel::pci;
> +    ///
> +    /// fn probe_sriov(
> +    ///     pdev: &pci::Device<kernel::device::Bound>,
> +    /// ) -> Result<(), kernel::error::Error> {
> +    ///     let sriov = pdev
> +    ///         .config_space_extended()?
> +    ///         .find_ext_capability::<pci::ExtSriovRegs>()?;
> +    ///
> +    ///     let total_vfs = kernel::io_read!(sriov, .total_vfs);
> +    ///     let vf_offset = kernel::io_read!(sriov, .vf_offset);
> +    ///     kernel::io_write!(sriov, .num_vfs, 4u16);

[Severity: Medium]
Writing directly to the SR-IOV num_vfs capability register bypasses the PCI
subsystem's state management, such as pci_enable_sriov(), PCI core locks, and
VF pci_dev creation.

This could lead to inconsistent kernel state if a driver developer copies this
pattern.

Does this example encourage an unsafe pattern?

> +    ///     let bar0 = sriov.read_vf_bar(0)?;
> +    ///     let bar1 = sriov.read_vf_bar(bar0.next_index())?;
> +    ///
> +    ///     Ok(())
> +    /// }
> +    /// ```
[ ... ]
> +/// SR-IOV register layout per PCIe spec (64 bytes starting at cap offset).
> +#[repr(C)]
> +#[derive(FromBytes, IntoBytes)]
> +pub struct ExtSriovRegs {
> +    /// Extended capability header.
> +    pub header: u32,
> +    /// SR-IOV capabilities.
> +    pub cap: u32,
> +    /// SR-IOV control.
> +    pub ctrl: u16,
> +    /// SR-IOV status.
> +    pub status: u16,
> +    /// Initial VFs.
> +    pub initial_vfs: u16,
> +    /// Total VFs.
> +    pub total_vfs: u16,
> +    /// Number of VFs.
> +    pub num_vfs: u16,
> +    /// Function dependency link.
> +    pub func_dep_link: u16,

[Severity: Medium]
Modeling func_dep_link as a u16 merges it with the adjacent reserved byte at
offset 0x13.

This could potentially corrupt the read value if the reserved byte is non-zero
on future hardware.

According to the PCIe specification, shouldn't func_dep_link be an 8-bit field
rather than a 16-bit field?

> +    /// First VF offset.
> +    pub vf_offset: u16,
> +    /// VF stride.
> +    pub vf_stride: u16,
> +    _reserved: u16,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1