Re: [PATCH v2 1/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pci: forcefully disabling SR-IOV on driver unbind crashes assigned guest VMs
- [Medium] rust: pci: missing C wrapper causes compilation failure on !CONFIG_PCI_IOV
- [Low] rust: pci: missing inline attribute on enable_sriov and disable_sriov
--

commit 3eb63d8034c8f8f4a7b4af84ac17b76c3927a676
Author: Peter Colberg <pcolberg@redhat.com>

rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability

This commit adds Rust methods to enable and disable the SR-IOV capability
for a PCI device by wrapping pci_enable_sriov() and pci_disable_sriov().
It also modifies the PCI remove callback to synchronously disable SR-IOV
before unbinding the Physical Function (PF) driver.

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 3ec897709e890..e6dac919f02d6 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -133,6 +133,10 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
>          // INVARIANT: `pdev` is valid for the duration of `remove_callback()`.
>          let pdev = unsafe { &*pdev.cast::<Device<device::CoreInternal<'_>>>() };
>  
> +        // Keep PF data installed until all VF remove callbacks have completed.
> +        #[cfg(CONFIG_PCI_IOV)]
> +        pdev.disable_sriov();

[Severity: High]
Does this code safely handle the case where VFs are currently assigned to a
guest VM?

In the C PCI subsystem, standard drivers check pci_vfs_assigned(pdev) before
disabling SR-IOV during removal. If pdev.disable_sriov() is called
unconditionally here, won't it forcefully destroy the VF devices without
checking their assignment state?

This could forcefully remove the hardware from running guest VMs, leading to
fatal guest crashes or Advanced Error Reporting (AER) events.

[ ... ]

> @@ -472,6 +476,38 @@ pub fn set_master(&self) {
>          // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
>          unsafe { bindings::pci_set_master(self.as_raw()) };
>      }
> +
> +    /// Enable the Single Root I/O Virtualization (SR-IOV) capability for this device,
> +    /// where `nr_virtfn` is number of Virtual Functions (VF) to enable.
> +    #[cfg(CONFIG_PCI_IOV)]
> +    pub fn enable_sriov(&self, nr_virtfn: i32) -> Result {

[Severity: Medium]
Will this regression cause compilation failures for drivers when CONFIG_PCI_IOV
is disabled?

Since #[cfg(CONFIG_PCI_IOV)] entirely removes the enable_sriov() method when
the config is unset, drivers calling it without explicit #[cfg] gating will
fail to compile.

The C API provides static inline fallbacks that safely return -ENODEV or
do nothing when the config is disabled.

Could we expose these functions via rust/helpers/pci.c so bindgen can
generate bindings for the fallbacks, rather than completely removing the
method here?

[Severity: Low]
This isn't a bug, but should this method include an #[inline] attribute?

Since this is a small abstraction function forwarding directly to a C binding
call, adding #[inline] helps avoid minor performance overhead from missed
optimizations.

> +        // SAFETY:
> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
[ ... ]
> +        // The Core device context inherits from the Bound device context,
> +        // which guarantees that the PF device is bound to a driver.
> +        to_result(unsafe { bindings::pci_enable_sriov(self.as_raw(), nr_virtfn) })
> +    }
> +
> +    /// Disable the Single Root I/O Virtualization (SR-IOV) capability for this device.
> +    #[cfg(CONFIG_PCI_IOV)]
> +    pub fn disable_sriov(&self) {

[Severity: Medium]
Can this code also break builds on !CONFIG_PCI_IOV configurations?

Similar to enable_sriov(), hiding this method behind #[cfg(CONFIG_PCI_IOV)]
can break drivers that call disable_sriov() without their own cfg gating.

[Severity: Low]
This isn't a bug, but could we add an #[inline] attribute here as well for
the same reason as enable_sriov()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924190556.1620886-1-zhiw@nvidia.com?part=1