Re: [PATCH v5 8/9] rust: pci: add SR-IOV enable and disable callbacks

["Alexandre Courbot" <acourbot@nvidia.com>]

On Sat Oct 31, 2026 at 2:12 AM JST, Simon Song wrote:
> From: Peter Colberg <peter@colberg.org>
>
> Allow Rust PCI drivers to configure SR-IOV through sriov_numvfs.
> Split the C configure callback into sriov_enable() and sriov_disable(),
> passing pinned driver data and a token for the requested operation.
> Require drivers to implement both callbacks together.
>
> Return the enabled guard's VF count on success and disarm its rollback.
> On a disable request, verify that the driver has removed all VFs before
> reporting success. VfRegistration handles VF removal during PF teardown;
> these callbacks only handle explicit configuration requests.
>
> The PCI core invokes sriov_configure only through the PF-only
> sriov_numvfs sysfs attribute, so no extra PF role check is needed.
>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Peter Colberg <peter@colberg.org>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> Signed-off-by: Simon Song <xinmengs@nvidia.com>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

A few comments/questions below.

> ---
>  rust/kernel/pci.rs     | 91 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/pci/iov.rs | 51 +++++++++++++++++++----
>  2 files changed, 135 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 3a768477014..5de784373ec 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -96,12 +96,22 @@ unsafe fn register(
>          name: &'static CStr,
>          module: &'static ThisModule,
>      ) -> Result {
> +        #[cfg(CONFIG_PCI_IOV)]
> +        build_assert!(

This can be a `const_assert!` IIUC.

> +            T::HAS_SRIOV_ENABLE == T::HAS_SRIOV_DISABLE,
> +            "PCI drivers must implement both sriov_enable and sriov_disable"
> +        );
> +
>          // SAFETY: It's safe to set the fields of `struct pci_driver` on initialization.
>          unsafe {
>              (*pdrv.get()).name = name.as_char_ptr();
>              (*pdrv.get()).probe = Some(Self::probe_callback);
>              (*pdrv.get()).remove = Some(Self::remove_callback);
>              (*pdrv.get()).id_table = T::ID_TABLE.as_ptr();
> +            #[cfg(CONFIG_PCI_IOV)]
> +            if T::HAS_SRIOV_ENABLE {
> +                (*pdrv.get()).sriov_configure = Some(Self::sriov_configure_callback);
> +            }
>          }
>  
>          // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
> @@ -350,6 +360,87 @@ fn probe<'bound>(
>      fn unbind<'bound>(dev: &'bound Device<device::Core<'_>>, this: Pin<&Self::Data<'bound>>) {
>          let _ = (dev, this);
>      }
> +
> +    /// Enables Single Root I/O Virtualization (SR-IOV) for a PF.
> +    ///
> +    /// Called when userspace writes a nonzero VF count to `sriov_numvfs`. The token carries the
> +    /// requested count and permission to enable VFs on this PF. Return the guard obtained from
> +    /// [`SriovEnable::enable()`] after completing setup; dropping it rolls back the enable.
> +    ///
> +    /// The PF must own a [`VfRegistration`] before enabling VFs. That registration disables
> +    /// SR-IOV when the driver data is destroyed, while resources needed by VF drivers remain live.
> +    /// Implement this callback and [`Self::sriov_disable()`] together, or implement neither.
> +    ///
> +    /// See [PCI Express I/O Virtualization].
> +    ///
> +    /// [PCI Express I/O Virtualization]: https://docs.kernel.org/PCI/pci-iov-howto.html
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::{
> +    /// #     device::Core,
> +    /// #     pci,
> +    /// #     prelude::*, //
> +    /// # };
> +    /// # struct Data;
> +    /// fn sriov_enable<'a>(
> +    ///     _dev: &'a pci::Device<Core<'_>>,
> +    ///     _data: Pin<&Data>,
> +    ///     token: pci::SriovEnable<'a>,
> +    /// ) -> Result<pci::SriovEnabled<'a>> {
> +    ///     let num_vfs = token.num_vfs();
> +    ///     let enabled = token.enable(num_vfs)?;
> +    ///     // Complete any additional setup before returning the guard.
> +    ///     Ok(enabled)
> +    /// }
> +    /// ```
> +    #[cfg(CONFIG_PCI_IOV)]
> +    fn sriov_enable<'a>(
> +        dev: &'a Device<device::Core<'_>>,
> +        data: Pin<&Self::Data<'a>>,
> +        token: SriovEnable<'a>,
> +    ) -> Result<SriovEnabled<'a>> {
> +        let _ = (dev, data, token);
> +        build_error!(crate::error::VTABLE_DEFAULT_ERROR)
> +    }

IIUC you can use the new-ish `#[optional]` attribute instead of writing
a default method.

> +
> +    /// Disables all VFs of a PF in response to a userspace request.
> +    ///
> +    /// Called when userspace writes zero to `sriov_numvfs`. Call [`SriovDisable::disable()`]
> +    /// while resources used by VF drivers are still available. Returning an error before calling
> +    /// it leaves the VFs enabled. Returning success requires that all VFs have been disabled.
> +    ///
> +    /// This callback is not invoked during PF unbind; [`VfRegistration`] owns that teardown.
> +    /// Implement this callback and [`Self::sriov_enable()`] together, or implement neither.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::{
> +    /// #     device::Core,
> +    /// #     pci,
> +    /// #     prelude::*, //
> +    /// # };
> +    /// # struct Data;
> +    /// fn sriov_disable<'a>(
> +    ///     _dev: &'a pci::Device<Core<'_>>,
> +    ///     _data: Pin<&Data>,
> +    ///     token: pci::SriovDisable<'a>,
> +    /// ) -> Result {
> +    ///     token.disable();
> +    ///     Ok(())
> +    /// }
> +    /// ```
> +    #[cfg(CONFIG_PCI_IOV)]
> +    fn sriov_disable<'a>(
> +        dev: &'a Device<device::Core<'_>>,
> +        data: Pin<&Self::Data<'a>>,
> +        token: SriovDisable<'a>,
> +    ) -> Result {
> +        let _ = (dev, data, token);
> +        build_error!(crate::error::VTABLE_DEFAULT_ERROR)
> +    }

Same here.

>  }
>  
>  /// The PCI device representation.
> diff --git a/rust/kernel/pci/iov.rs b/rust/kernel/pci/iov.rs
> index 197b0c2bb0f..2af9f4c20b8 100644
> --- a/rust/kernel/pci/iov.rs
> +++ b/rust/kernel/pci/iov.rs
> @@ -2,12 +2,19 @@
>  
>  //! Abstractions for PCI Single Root I/O Virtualization (SR-IOV) drivers.
>  
> -use super::Device;
> +use super::{
> +    Adapter,
> +    Device,
> +    Driver, //
> +};
>  
>  use crate::{
>      bindings,
>      device,
> -    error::to_result,
> +    error::{
> +        from_result,
> +        to_result, //
> +    },
>      prelude::*,
>      types::{
>          CovariantForLt,
> @@ -65,7 +72,7 @@ pub(crate) fn disable_sriov(&self) {
>      }
>  }
>  
> -/// Permission to enable VFs during a driver's `sriov_enable()` callback.
> +/// Permission to enable VFs during a driver's [`Driver::sriov_enable()`] callback.
>  ///
>  /// The PCI adapter creates this token for the PF being configured. It cannot be cloned or sent
>  /// to another thread, and its lifetime is restricted to the callback. Enabling VFs consumes it.
> @@ -95,7 +102,7 @@ pub fn num_vfs(&self) -> u16 {
>      ///
>      /// VF drivers can probe before this method returns, so the PF resources they access must
>      /// already be initialized. The returned guard disables the VFs if subsequent setup fails.
> -    /// Return it from `sriov_enable()` to leave the VFs enabled on callback success.
> +    /// Return it from [`Driver::sriov_enable()`] to leave the VFs enabled on callback success.
>      pub fn enable(self, num_vfs: u16) -> Result<SriovEnabled<'a>> {
>          if num_vfs == 0 || num_vfs > self.num_vfs {

Do we want to make `num_vfs` a `NonZero<u16>`? (and for
`SriovEnable::num_vfs()` as well)

>              return Err(EINVAL);
> @@ -115,7 +122,7 @@ pub fn enable(self, num_vfs: u16) -> Result<SriovEnabled<'a>> {
>      }
>  }
>  
> -/// Enabled VFs awaiting successful completion of `sriov_enable()`.
> +/// Enabled VFs awaiting successful completion of [`Driver::sriov_enable()`].
>  pub struct SriovEnabled<'a> {
>      pdev: &'a Device<device::CoreInternal<'a>>,
>      num_vfs: u16,
> @@ -123,7 +130,6 @@ pub struct SriovEnabled<'a> {
>  
>  impl SriovEnabled<'_> {
>      #[inline]
> -    #[expect(dead_code)]
>      fn disarm(self) -> u16 {
>          ManuallyDrop::new(self).num_vfs
>      }
> @@ -135,7 +141,7 @@ fn drop(&mut self) {
>      }
>  }
>  
> -/// Permission to disable VFs during a driver's `sriov_disable()` callback.
> +/// Permission to disable VFs during a driver's [`Driver::sriov_disable()`] callback.
>  ///
>  /// The PCI adapter creates this token for the PF being configured. It cannot be cloned or sent
>  /// to another thread, and its lifetime is restricted to the callback. Dropping the token does
> @@ -154,6 +160,37 @@ pub fn disable(self) {
>      }
>  }
>  
> +impl<T: Driver> Adapter<T> {
> +    pub(super) extern "C" fn sriov_configure_callback(
> +        pdev: *mut bindings::pci_dev,
> +        nr_virtfn: c_int,
> +    ) -> c_int {
> +        // SAFETY: The PCI core calls this through the PF-only `sriov_numvfs` sysfs attribute,
> +        // with a valid `struct pci_dev` and while holding the device lock.
> +        //
> +        // INVARIANT: `pdev` is valid for the duration of `sriov_configure_callback()`.
> +        let pdev = unsafe { &*pdev.cast::<Device<device::CoreInternal<'_>>>() };
> +
> +        // SAFETY: `sriov_configure` is called only after a successful probe and before unbind, so
> +        // the stored pointer has type `T::Data<'_>` and remains valid throughout this callback.
> +        let data = unsafe { pdev.as_ref().drvdata_borrow::<T::Data<'_>>() };
> +
> +        from_result(|| {
> +            if nr_virtfn == 0 {
> +                T::sriov_disable(pdev, data, SriovDisable { pdev })?;
> +                if pdev.num_vf() != 0 {

Can you add a comment justifying this guard? IIUC it is here in
case `sriov_disable` lied to us and returned `Ok` without calling
`SriovDisable::disable`.