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

Peter Colberg posted 8 patches 2 months, 3 weeks ago
There is a newer version of this series
[PATCH 3/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability
Posted by Peter Colberg 2 months, 3 weeks ago
Add methods to enable and disable the Single Root I/O Virtualization
(SR-IOV) capability for a PCI device. The wrapped C methods take care
of validating whether the device is a Physical Function (PF), whether
SR-IOV is currently disabled (or enabled), and whether the number of
requested VFs does not exceed the total number of supported VFs.

Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Peter Colberg <pcolberg@redhat.com>
---
 rust/kernel/pci.rs | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 814990d386708fe2ac652ccaa674c10a6cf390cb..556a01ed9bc3b1300a3340a3d2383e08ceacbfe5 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -454,6 +454,36 @@ 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 {
+        // SAFETY:
+        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
+        //
+        // `pci_enable_sriov()` checks that the enable operation is valid:
+        // - the device is a Physical Function (PF),
+        // - SR-IOV is currently disabled, and
+        // - `nr_virtfn` does not exceed the total number of supported VFs.
+        let ret = unsafe { bindings::pci_enable_sriov(self.as_raw(), nr_virtfn) };
+        if ret != 0 {
+            return Err(crate::error::Error::from_errno(ret));
+        }
+        Ok(())
+    }
+
+    /// Disable the Single Root I/O Virtualization (SR-IOV) capability for this device.
+    #[cfg(CONFIG_PCI_IOV)]
+    pub fn disable_sriov(&self) {
+        // SAFETY:
+        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
+        //
+        // `pci_disable_sriov()` checks that the disable operation is valid:
+        // - the device is a Physical Function (PF), and
+        // - SR-IOV is currently enabled.
+        unsafe { bindings::pci_disable_sriov(self.as_raw()) };
+    }
 }
 
 // SAFETY: `pci::Device` is a transparent wrapper of `struct pci_dev`.

-- 
2.51.1
Re: [PATCH 3/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability
Posted by Joel Fernandes 2 months ago
On Wed, Nov 19, 2025 at 05:19:07PM -0500, Peter Colberg wrote:
> Add methods to enable and disable the Single Root I/O Virtualization
> (SR-IOV) capability for a PCI device. The wrapped C methods take care
> of validating whether the device is a Physical Function (PF), whether
> SR-IOV is currently disabled (or enabled), and whether the number of
> requested VFs does not exceed the total number of supported VFs.
> 
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Peter Colberg <pcolberg@redhat.com>
> ---
>  rust/kernel/pci.rs | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 814990d386708fe2ac652ccaa674c10a6cf390cb..556a01ed9bc3b1300a3340a3d2383e08ceacbfe5 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -454,6 +454,36 @@ 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 {
> +        // SAFETY:
> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        //
> +        // `pci_enable_sriov()` checks that the enable operation is valid:
> +        // - the device is a Physical Function (PF),
> +        // - SR-IOV is currently disabled, and
> +        // - `nr_virtfn` does not exceed the total number of supported VFs.
> +        let ret = unsafe { bindings::pci_enable_sriov(self.as_raw(), nr_virtfn) };
> +        if ret != 0 {
> +            return Err(crate::error::Error::from_errno(ret));
> +        }
> +        Ok(())

nit: Why not use `to_result()` here?

thanks,

 - Joel



> +    }
> +
> +    /// Disable the Single Root I/O Virtualization (SR-IOV) capability for this device.
> +    #[cfg(CONFIG_PCI_IOV)]
> +    pub fn disable_sriov(&self) {
> +        // SAFETY:
> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        //
> +        // `pci_disable_sriov()` checks that the disable operation is valid:
> +        // - the device is a Physical Function (PF), and
> +        // - SR-IOV is currently enabled.
> +        unsafe { bindings::pci_disable_sriov(self.as_raw()) };
> +    }
>  }
>  
>  // SAFETY: `pci::Device` is a transparent wrapper of `struct pci_dev`.
> 
> -- 
> 2.51.1
>
Re: [PATCH 3/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability
Posted by Peter Colberg 2 months ago
On Sun, Dec 07, 2025 at 01:33:55AM -0500, Joel Fernandes wrote:
> On Wed, Nov 19, 2025 at 05:19:07PM -0500, Peter Colberg wrote:
> > Add methods to enable and disable the Single Root I/O Virtualization
> > (SR-IOV) capability for a PCI device. The wrapped C methods take care
> > of validating whether the device is a Physical Function (PF), whether
> > SR-IOV is currently disabled (or enabled), and whether the number of
> > requested VFs does not exceed the total number of supported VFs.
> > 
> > Suggested-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Peter Colberg <pcolberg@redhat.com>
> > ---
> >  rust/kernel/pci.rs | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 814990d386708fe2ac652ccaa674c10a6cf390cb..556a01ed9bc3b1300a3340a3d2383e08ceacbfe5 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -454,6 +454,36 @@ 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 {
> > +        // SAFETY:
> > +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> > +        //
> > +        // `pci_enable_sriov()` checks that the enable operation is valid:
> > +        // - the device is a Physical Function (PF),
> > +        // - SR-IOV is currently disabled, and
> > +        // - `nr_virtfn` does not exceed the total number of supported VFs.
> > +        let ret = unsafe { bindings::pci_enable_sriov(self.as_raw(), nr_virtfn) };
> > +        if ret != 0 {
> > +            return Err(crate::error::Error::from_errno(ret));
> > +        }
> > +        Ok(())
> 
> nit: Why not use `to_result()` here?

Thanks! I queued `to_result()` for v2, from an unrelated review [1].

Peter

[1] https://lore.kernel.org/rust-for-linux/b8234f181d35b21a3319b95a54b21bdba11b8001.camel@redhat.com/

> 
> thanks,
> 
>  - Joel
> 
> 
> 
> > +    }
> > +
> > +    /// Disable the Single Root I/O Virtualization (SR-IOV) capability for this device.
> > +    #[cfg(CONFIG_PCI_IOV)]
> > +    pub fn disable_sriov(&self) {
> > +        // SAFETY:
> > +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> > +        //
> > +        // `pci_disable_sriov()` checks that the disable operation is valid:
> > +        // - the device is a Physical Function (PF), and
> > +        // - SR-IOV is currently enabled.
> > +        unsafe { bindings::pci_disable_sriov(self.as_raw()) };
> > +    }
> >  }
> >  
> >  // SAFETY: `pci::Device` is a transparent wrapper of `struct pci_dev`.
> > 
> > -- 
> > 2.51.1
> > 
>
Re: [PATCH 3/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability
Posted by Jason Gunthorpe 2 months, 2 weeks ago
On Wed, Nov 19, 2025 at 05:19:07PM -0500, Peter Colberg wrote:
> Add methods to enable and disable the Single Root I/O Virtualization
> (SR-IOV) capability for a PCI device. The wrapped C methods take care
> of validating whether the device is a Physical Function (PF), whether
> SR-IOV is currently disabled (or enabled), and whether the number of
> requested VFs does not exceed the total number of supported VFs.
> 
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Peter Colberg <pcolberg@redhat.com>
> ---
>  rust/kernel/pci.rs | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 814990d386708fe2ac652ccaa674c10a6cf390cb..556a01ed9bc3b1300a3340a3d2383e08ceacbfe5 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -454,6 +454,36 @@ 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 {
> +        // SAFETY:
> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        //
> +        // `pci_enable_sriov()` checks that the enable operation is valid:
> +        // - the device is a Physical Function (PF),
> +        // - SR-IOV is currently disabled, and
> +        // - `nr_virtfn` does not exceed the total number of supported VFs.
> +        let ret = unsafe { bindings::pci_enable_sriov(self.as_raw(), nr_virtfn) };
> +        if ret != 0 {
> +            return Err(crate::error::Error::from_errno(ret));
> +        }
> +        Ok(())
> +    }
> +
> +    /// Disable the Single Root I/O Virtualization (SR-IOV) capability for this device.
> +    #[cfg(CONFIG_PCI_IOV)]
> +    pub fn disable_sriov(&self) {
> +        // SAFETY:
> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        //
> +        // `pci_disable_sriov()` checks that the disable operation is valid:
> +        // - the device is a Physical Function (PF), and
> +        // - SR-IOV is currently enabled.
> +        unsafe { bindings::pci_disable_sriov(self.as_raw()) };
> +    }

Both these functions should only be called on bound devices - the
safety statement should call it out, does the code require it?

Also per my other email SRIOV should be disabled before a driver can
be unbound, this patch should take care of it to not introduce an
dangerous enable_sriov().

Jason
Re: [PATCH 3/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability
Posted by Peter Colberg 1 month, 2 weeks ago
On Fri, Nov 21, 2025 at 07:28:33PM -0400, Jason Gunthorpe wrote:
> On Wed, Nov 19, 2025 at 05:19:07PM -0500, Peter Colberg wrote:
> > Add methods to enable and disable the Single Root I/O Virtualization
> > (SR-IOV) capability for a PCI device. The wrapped C methods take care
> > of validating whether the device is a Physical Function (PF), whether
> > SR-IOV is currently disabled (or enabled), and whether the number of
> > requested VFs does not exceed the total number of supported VFs.
> > 
> > Suggested-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Peter Colberg <pcolberg@redhat.com>
> > ---
> >  rust/kernel/pci.rs | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 814990d386708fe2ac652ccaa674c10a6cf390cb..556a01ed9bc3b1300a3340a3d2383e08ceacbfe5 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -454,6 +454,36 @@ 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 {
> > +        // SAFETY:
> > +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> > +        //
> > +        // `pci_enable_sriov()` checks that the enable operation is valid:
> > +        // - the device is a Physical Function (PF),
> > +        // - SR-IOV is currently disabled, and
> > +        // - `nr_virtfn` does not exceed the total number of supported VFs.
> > +        let ret = unsafe { bindings::pci_enable_sriov(self.as_raw(), nr_virtfn) };
> > +        if ret != 0 {
> > +            return Err(crate::error::Error::from_errno(ret));
> > +        }
> > +        Ok(())
> > +    }
> > +
> > +    /// Disable the Single Root I/O Virtualization (SR-IOV) capability for this device.
> > +    #[cfg(CONFIG_PCI_IOV)]
> > +    pub fn disable_sriov(&self) {
> > +        // SAFETY:
> > +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> > +        //
> > +        // `pci_disable_sriov()` checks that the disable operation is valid:
> > +        // - the device is a Physical Function (PF), and
> > +        // - SR-IOV is currently enabled.
> > +        unsafe { bindings::pci_disable_sriov(self.as_raw()) };
> > +    }
> 
> Both these functions should only be called on bound devices - the
> safety statement should call it out, does the code require it?

Yes, these functions are in the Core device context that inherits from
the Bound device context, which guarantees that the PF device is bound
to a driver. I have added a note to the SAFETY comments.

> 
> Also per my other email SRIOV should be disabled before a driver can
> be unbound, this patch should take care of it to not introduce an
> dangerous enable_sriov().

Thanks for your review. This has been addressed in v2, which disables
SR-IOV before remove() when a PF driver opts in using a new flag
sriov_disable_on_remove, which is set by default for a Rust driver.

Further, enable_sriov() is prevented during remove() using a new
flag inhibit_enable in the pci_sriov structure that is set before
and cleared after the PF driver is unbound from the device.

Thanks,
Peter

> 
> Jason
>
Re: [PATCH 3/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability
Posted by Jason Gunthorpe 1 month ago
On Tue, Dec 23, 2025 at 02:17:12PM -0500, Peter Colberg wrote:
> Further, enable_sriov() is prevented during remove() using a new
> flag inhibit_enable in the pci_sriov structure that is set before
> and cleared after the PF driver is unbound from the device.

Doesn't this need something concurrent safe like a revocable?

Jason
Re: [PATCH 3/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability
Posted by Peter Colberg 3 days, 21 hours ago
Hi Jason,

On Mon, Jan 05, 2026 at 09:22:36PM -0400, Jason Gunthorpe wrote:
> On Tue, Dec 23, 2025 at 02:17:12PM -0500, Peter Colberg wrote:
> > Further, enable_sriov() is prevented during remove() using a new
> > flag inhibit_enable in the pci_sriov structure that is set before
> > and cleared after the PF driver is unbound from the device.
> 
> Doesn't this need something concurrent safe like a revocable?

Thank you for the follow-up and apologies for the delay. I have now
posted a v2 series that takes a different approach from what was
described earlier, after discussing this further with Danilo.

In the v2 patch [1], when the new PCI driver flag managed_sriov is
set, which is always the case for Rust, SR-IOV is disabled twice if
needed: once before the unbind() callback for a well-behaved driver,
and a second time after unbind() for a broken (but nevertheless using
safe APIs only) driver that re-enables SR-IOV during unbind().

Together with commit a995fe1a3aa7 ("rust: driver: drop device private
data post unbind") which ensures that the device private data for the
PF device is still alive until after the function pci_iov_remove() is
called and forcibly re-disables SR_IOV if needed, this upholds the
safety guarantee which pci::Device::physfn() depends on.

As an alternative for preventing SR-IOV management during unbind(), I
briefly considered adding a new context, e.g., CoreInternal => Core =>
CoreUnbind => Bound => Normal, where {enable,disable}_sriov() are
implemented by Core but not CoreUnbind, but discarded that solution
since it seems too specific to this case to warrant the complexity.

Thanks,
Peter

[1] https://lore.kernel.org/rust-for-linux/20260205-rust-pci-sriov-v2-1-ef9400c7767b@redhat.com/

> 
> Jason
>