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

[Zhi Wang]

On Wed, 30 Sep 2026 15:59:27 +0200
"Danilo Krummrich" <dakr@xxxxxxxxxx> wrote:

> 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.
> 

I see. 

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

Got it. Will proceed in the next re-spin.

> > != 0 {
> > +                // 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?
> 

Nice idea. Thanks for the suggestion.

> > +
> > +                // 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 =

<snip>

> > +    ///
> > +    /// 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.

Got it. I will add a helper for the PF driver to access 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
>