Re: [PATCH v3 07/10] rust: pci: add typed SR-IOV PF registration data

["Danilo Krummrich" <dakr@kernel.org>]

On Wed Sep 30, 2026 at 12:18 PM CEST, Zhi Wang wrote:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 1599b3a613d7..8782e9b06e64 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -51,6 +51,8 @@
>      Extended,
>      Normal, //
>  };
> +#[cfg(CONFIG_PCI_IOV)]
> +pub use self::iov::VfRegistration;
>  pub use self::irq::{
>      IrqType,
>      IrqTypes,
> @@ -331,6 +333,9 @@ fn probe<'bound>(
>      /// operations to gracefully tear down the device.
>      ///
>      /// Otherwise, release operations for driver resources should be performed in `Drop`.
> +    ///
> +    /// For a PF with enabled VFs, `VfRegistration` disables SR-IOV when it is dropped. This
> +    /// callback must leave resources accessed by VF drivers available until then.

I don't think we need this comment, neither this callback (which I plan to
remove anyway) nor T::Data::drop() can remove resources that can still be
accessed by VF drivers in the first place.

You could have something like Mutex<Option<_>> of course, but that would be
intentional then.

> +impl<'a, F: ForLt + 'static> VfRegistrationData<'a, F> {
> +    /// Pin-initializer for the registration data.
> +    fn new(data: impl PinInit<F::Of<'a>, Error>) -> impl PinInit<Self, Error> {

I think we can accept any error type E?

> +        try_pin_init!(Self {
> +            type_id: TypeId::of::<F>(),
> +            data <- data,
> +        })
> +    }
> +}

<snip>

> +impl<'a, F: ForLt + 'static> VfRegistration<'a, F>
> +where
> +    for<'b> F::Of<'b>: Send + Sync,
> +{
> +    /// Create a new VF registration.
> +    ///
> +    /// Returns a pin-initializer so the registration can be embedded directly
> +    /// in the PF driver's bus device private data.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The returned registration must be embedded in the driver's bus device private data.

I think this is the only requirement we need.

> +    /// The initializer must run as part of the PF driver's probe.
> +    /// The driver's `unbind` callback and the enclosing data's destructor must keep resources
> +    /// accessed by VF drivers available until this registration has disabled SR-IOV.

This is not a safety requirement; it would require unsafe code to do this.

> +    pub unsafe fn new(
> +        pdev: &'a Device<device::Core<'_>>,
> +        data: impl PinInit<F::Of<'a>, Error> + 'a,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        let pdev: &'a Device<device::Bound> = pdev;
> +        try_pin_init!(Self {
> +            _: {
> +                if pdev.is_virtfn() {
> +                    return Err(ENODEV);
> +                }
> +            },
> +            pdev,
> +            inner <- VfRegistrationData::new(data),
> +            _pin: PhantomPinned,
> +            _: {
> +                // Check after initialization in case it registered another object for this PF.
> +                // SAFETY: The caller runs this initializer during PF probing, before VF
> +                // configuration callbacks can run.
> +                if unsafe { bindings::pci_num_vf(pdev.as_raw()) } != 0 {

Don't we have a safe pci::Device::num_vf() method already?

> +                    return Err(EBUSY);
> +                }
> +
> +                // SAFETY: This initializer runs during PF probing, and no VFs are enabled.
> +                if !unsafe { (*pdev.as_raw()).vf_registration_data_rust }.is_null() {
> +                    return Err(EBUSY);
> +                }

Those two checks can go into the first _: block, no?

> +
> +                // SAFETY: The data is initialized and pinned, the slot is unoccupied, and
> +                // no VF can access it yet. No fallible work follows publication.
> +                unsafe {
> +                    (*pdev.as_raw()).vf_registration_data_rust =
> +                        core::ptr::from_ref(inner.as_ref().get_ref()).cast_mut().cast();
> +                }
> +            },
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<F: ForLt + 'static> PinnedDrop for VfRegistration<'_, F> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: `pci_disable_sriov()` is safe to call on any `pci_dev`; it
> +        // is a no-op if the device has no VFs enabled. When VFs are enabled,
> +        // this blocks until all VF `remove()` callbacks complete.
> +        unsafe { bindings::pci_disable_sriov(self.pdev.as_raw()) };
> +
> +        // SAFETY: The device is valid and all VF remove callbacks have completed, so no VF
> +        // can access the registration pointer. The data remains alive until this returns.
> +        unsafe { (*self.pdev.as_raw()).vf_registration_data_rust = core::ptr::null_mut() };
> +    }
> +}
> +
> +// SAFETY: The inner data is `Send` (enforced by the bound), and `&Device` is `Send + Sync`.
> +unsafe impl<F: ForLt> Send for VfRegistration<'_, F> where for<'a> F::Of<'a>: Send {}
> +
> +// SAFETY: The inner data is `Send + Sync`. `VfRegistration` doesn't expose mutable access;
> +// VF drivers only read the data through an immutable pinned reference.
> +unsafe impl<F: ForLt> Sync for VfRegistration<'_, F> where for<'a> F::Of<'a>: Send + Sync {}
> +
> +impl Device<device::Bound> {
> +    /// Internal helper: reads the `vf_registration_data_rust` pointer from the
> +    /// PF, checks the `TypeId`, and returns a pinned reference.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The data lifetime must be hidden behind a higher-ranked closure independently of the
> +    /// reference lifetime, or `F` must be covariant in its encoded lifetime.
> +    unsafe fn vf_registration_data_pinned<F: ForLt + 'static>(&self) -> Result<Pin<&F::Of<'_>>> {
> +        if !self.is_virtfn() {
> +            return Err(ENODEV);
> +        }

We don't want to exclude PF drivers to access this. Have a look at
drivers/gpu/nova-core/api.rs, it makes sense for the PF driver to provide a
helper API around this.

> +        // SAFETY: This bound VF uses the `physfn` union field. PCI retains its PF until
> +        // VF removal completes, and the PF keeps the registration installed until then.
> +        let ptr = unsafe {
> +            let pf = (*self.as_raw()).__bindgen_anon_1.physfn;
> +            (*pf).vf_registration_data_rust
> +        };
> +
> +        if ptr.is_null() {
> +            return Err(ENOENT);

Maybe ENODEV?

> +        }
> +
> +        // SAFETY: The registration keeps its data installed until VF removal completes,
> +        // including when probe initialization rolls back.
> +        // `ptr` points to a `VfRegistrationData` whose first field is a `TypeId`.
> +        let type_id = unsafe { ptr.cast::<TypeId>().read() };
> +        if type_id != TypeId::of::<F>() {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: TypeId check confirms the stored type matches `F`. The data
> +        // is pinned inside the PF's driver data struct. Lifetime shortening
> +        // from the PF's binding scope to `'_` is layout-compatible.
> +        let data_ptr = unsafe {
> +            let vfrd = ptr.cast::<VfRegistrationData<'_, F>>();
> +            &raw const (*vfrd).data
> +        };
> +
> +        // SAFETY: `data` is structurally pinned inside `VfRegistrationData`.
> +        Ok(unsafe { Pin::new_unchecked(&*data_ptr) })
> +    }
> +
> +    /// Access the VF registration data through a closure with an HRTB lifetime.
> +    ///
> +    /// `F` is the [`ForLt`](trait@ForLt) encoding of the data type. Returns
> +    /// [`ENODEV`] if this is not a VF, [`ENOENT`] if no data was registered,
> +    /// or [`EINVAL`] if `F` does not match the type registered by the PF.
> +    ///
> +    /// The reference is borrowed from this VF, while the registration data's lifetime remains
> +    /// abstract so the closure cannot store shorter-lived references in invariant data.
> +    pub fn vf_registration_data_with<'this, F: ForLt + 'static, R>(
> +        &'this self,
> +        f: impl for<'a> FnOnce(Pin<&'this F::Of<'a>>) -> R,
> +    ) -> Result<R> {
> +        // SAFETY: The closure is higher-ranked over the data lifetime, independently of `'this`.
> +        // It cannot insert shorter-lived references, and the outer borrow cannot outlive this VF.
> +        let pinned = unsafe { self.vf_registration_data_pinned::<F>()? };
> +        Ok(f(pinned))
> +    }
> +
> +    /// Returns a pinned reference to the VF registration data.
> +    ///
> +    /// Available only when `F` implements [`CovariantForLt`](trait@crate::types::CovariantForLt),
> +    /// guaranteeing that shortening the PF data lifetime is sound.
> +    ///
> +    /// For non-covariant types, use [`Self::vf_registration_data_with()`].
> +    ///
> +    /// It returns the same errors as [`Self::vf_registration_data_with()`].
> +    pub fn vf_registration_data<F: CovariantForLt + 'static>(&self) -> Result<Pin<&F::Of<'_>>> {
> +        // SAFETY: `CovariantForLt` permits shortening the encoded lifetime to this borrow.
> +        unsafe { self.vf_registration_data_pinned::<F>() }
> +    }
> +}
> -- 
> 2.53.0