Re: [PATCH v12 4/5] rust: drm: gem: Introduce shmem::SGTable

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

On Fri Apr 24, 2026 at 12:01 AM JST, Alexandre Courbot wrote:
> Hi Lyude,
>
> On Wed Apr 22, 2026 at 8:52 AM JST, Lyude Paul wrote:
>> In order to do this, we need to be careful to ensure that any interface we
>> expose for scatterlists ensures that any mappings created from one are
>> destroyed on driver-unbind. To do this, we introduce a Devres resource into
>> shmem::Object that we use in order to ensure that we release any SGTable
>> mappings on driver-unbind. We store this in an UnsafeCell and protect
>> access to it using the dma_resv lock that we already have from the shmem
>> gem object, which is the same lock that currently protects
>> drm_gem_object_shmem->sgt.
>>
>> We also provide two different methods for acquiring an sg table:
>> self.sg_table(), and self.owned_sg_table(). The first function is for
>> short-term uses of mapped SGTables, the second is for callers that need to
>> hold onto the mapped SGTable for an extended period of time. The second
>> variant uses Devres of course, whereas the first simply relies on rust's
>> borrow checker to prevent driver-unbind when using the mapped SGTable.
>>
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>>
>> ---
>> V3:
>> * Rename OwnedSGTable to shmem::SGTable. Since the current version of the
>>   SGTable abstractions now has a `Owned` and `Borrowed` variant, I think
>>   renaming this to shmem::SGTable makes things less confusing.
>>   We do however, keep the name of owned_sg_table() as-is.
>> V4:
>> * Clarify safety comments for SGTable to explain why the object is
>>   thread-safe.
>> * Rename from SGTableRef to SGTable
>> V10:
>> * Use Devres in order to ensure that SGTables are revocable, and are
>>   unmapped on driver-unbind.
>> V11:
>> * s/create_sg_table()/get_sg_table()
>> * Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
>> V12:
>> * Actually move sgt_res in this patch and not the next one
>>
>>  rust/kernel/drm/gem/shmem.rs | 192 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 190 insertions(+), 2 deletions(-)
>>
>> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
>> index 11749c36e8695..a477312c8a09b 100644
>> --- a/rust/kernel/drm/gem/shmem.rs
>> +++ b/rust/kernel/drm/gem/shmem.rs
>> @@ -11,25 +11,38 @@
>>  
>>  use crate::{
>>      container_of,
>> +    device::{
>> +        self,
>> +        Bound, //
>> +    },
>> +    devres::*,
>>      drm::{
>>          driver,
>>          gem,
>>          private::Sealed,
>>          Device, //
>>      },
>> -    error::to_result,
>> +    error::{
>> +        from_err_ptr,
>> +        to_result, //
>> +    },
>>      prelude::*,
>> +    scatterlist,
>>      types::{
>>          ARef,
>
> This fails on master:
>
>     error[E0432]: unresolved import `crate::sync::ARef`
>       --> ../rust/kernel/drm/gem/shmem.rs:36:5
>       |
>     36 |     sync::ARef,
>       |     ^^^^^^^^^^ no `ARef` in `sync`
>
> Importing `sync::aref::ARef` seems to be the correct way now.
>
>>          Opaque, //
>>      }, //
>>  };
>>  use core::{
>> +    cell::UnsafeCell,
>>      ops::{
>>          Deref,
>>          DerefMut, //
>>      },
>> -    ptr::NonNull,
>> +    ptr::{
>> +        self,
>> +        NonNull, //
>> +    },
>>  };
>>  use gem::{
>>      BaseObjectPrivate,
>> @@ -61,6 +74,11 @@ pub struct ObjectConfig<'a, T: DriverObject> {
>>  #[repr(C)]
>>  #[pin_data]
>>  pub struct Object<T: DriverObject> {
>> +    /// Devres object for unmapping any SGTable on driver-unbind.
>> +    ///
>> +    /// This is protected by the object's dma_resv lock. It needs to be before `obj` to ensure that
>> +    /// it is destroyed before `obj` on `Drop`.
>> +    sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>,
>
> I didn't like this `UnsafeCell<Option>` since the last time, but only figured how to replace it now:
>
>     sgt_res: SetOnce<Devres<SGTableMap<T>>>,
>
> It's actually designed for that! And lets you remove at least one unsafe
> statement, while simplifying `get_sg_table` quite a bit. With the other
> suggestions I have below, here is my version of `get_sg_table` for
> reference:
>
>     fn get_sg_table<'a>(
>         &'a self,
>         dev: &'a device::Device<Bound>,
>     ) -> Result<&'a Devres<SGTableMap<T>>> {
>         let _dma_resv = DmaResvGuard::new(self);
>
>         if let Some(devres) = self.sgt_res.as_ref() {
>             Ok(devres)
>         } else {
>             // Only called for the side-effect of populating the GEM SG table.
>             // SAFETY: We grabbed the lock required for calling this function above.
>             from_err_ptr(unsafe {
>                 bindings::drm_gem_shmem_get_pages_sgt_locked(self.as_raw_shmem())
>             })?;
>
>             // INVARIANT:
>             // - We called drm_gem_shmem_get_pages_sgt_locked above and checked that it
>             //   succeeded, fulfilling the invariant of `SGTableMap` that the object's `sgt` field
>             //   is initialized.
>             // - We store this Devres in the object itself and don't move it, ensuring that the
>             //   object it points to remains valid for the lifetime of the `SGTableMap`.
>             let devres =
>                 Devres::new(dev, init!(SGTableMap { obj: self.into() })).inspect_err(|_| {
>                     // We can't make sure that the pages for this object are unmapped on
>                     // driver-unbind, so we need to release the sgt
>                     // SAFETY:
>                     // - We grabbed the lock required for calling this function above
>                     // - We checked above that get_pages_sgt_locked() was successful
>                     unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) }
>                 })?;
>
>             self.sgt_res.populate(devres);
>
>             // PANIC: `populate` has just succeeded, guaranteeing that `sgt_res` is populated.
>             Ok(self.sgt_res.as_ref().unwrap())
>         }
>     }
>
> And if only we could populate the `SetOnce` with a `impl Init<T, E>`,
> then we could even remove the DMA reservation acquisition on the fast
> path, because `SetOnce` comes with its own locking and the DMA lock here
> is used outside of its intended scope. I'll try to push the necessary
> work for `SetOnce` and maybe we can do that as a follow-up patch.
>
>>      #[pin]
>>      obj: Opaque<bindings::drm_gem_shmem_object>,
>>      /// Parent object that owns this object's DMA reservation object.
>> @@ -117,6 +135,7 @@ pub fn new(
>>              try_pin_init!(Self {
>>                  obj <- Opaque::init_zeroed(),
>>                  parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
>> +                sgt_res: UnsafeCell::new(None),
>>                  inner <- T::new(dev, size, args),
>>              }),
>>              GFP_KERNEL,
>> @@ -176,6 +195,100 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
>>          // SAFETY: We're recovering the Kbox<> we created in gem_create_object()
>>          let _ = unsafe { KBox::from_raw(this) };
>>      }
>> +
>> +    // If necessary, create an SGTable for the gem object and register a Devres for it to ensure
>> +    // that it is unmapped on driver unbind.
>> +    fn get_sg_table<'a>(
>> +        &'a self,
>> +        dev: &'a device::Device<Bound>,
>> +    ) -> Result<&'a Devres<SGTableMap<T>>> {
>> +        let sgt_res_ptr = self.sgt_res.get();
>> +
>> +        // SAFETY: This lock is initialized throughout the lifetime of the gem object
>> +        unsafe { bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut()) };
>
> There are 4 sites where we acquire and release the DMA resv lock, each
> of which require unsafe blocks and carrying the risk that we forget
> releasing the lock in the end. For this method in particular we need to
> jump through hoops a bit and store the return value into a temporary
> variable so we can unlock the DMA reservation.
>
> Let's do ourselves a favor and implement a small, private guard type:
>
>     struct DmaResvGuard<'a, T: DriverObject>(&'a Object<T>);
>
>     impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
>         fn new(object: &'a Object<T>) -> Self {
>             // SAFETY: This lock is initialized throughout the lifetime of `object`
>             unsafe { bindings::dma_resv_lock(object.raw_dma_resv(), ptr::null_mut()) };
>
>             Self(object)
>         }
>     }
>
>     impl<'a, T> Drop for DmaResvGuard<'a, T>
>     where
>         T: DriverObject,
>     {
>         fn drop(&mut self) {
>             // SAFETY: We are releasing the lock grabbed during the creation of this object.
>             unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
>         }
>     }
>
> There here you would just do
>
>     let _dma_resv = DmaResvGuard::new(self);
>
> and write the rest of the method without without having to worry about
> not returning early. It also let's you improve the flow of the code a
> bit, and requires less unsafe blocks overall.
>
> I am not sure how much of the TODO at the beginning of the file this
> solves, but it should also make it easier to switch to something that
> acquires a reference to a Wwmutex.
>
>> +
>> +        // SAFETY: We just grabbed the lock required for reading this data above.
>> +        let sgt_res = unsafe { (*sgt_res_ptr).as_ref() };
>> +
>> +        let ret = if let Some(sgt_res) = sgt_res {
>> +            // We already have a Devres object for this sg table, return it
>> +            Ok(sgt_res)
>> +        } else {
>> +            // SAFETY: We grabbed the lock required for calling this function above */
>> +            let sgt = from_err_ptr(unsafe {
>> +                bindings::drm_gem_shmem_get_pages_sgt_locked(self.as_raw_shmem())
>> +            });
>> +
>> +            if let Err(e) = sgt {
>> +                Err(e)
>> +            } else {
>> +                // INVARIANT:
>> +                // - We called drm_gem_shmem_get_pages_sgt_locked above and checked that it
>> +                //   succeeded, fulfilling the invariant of SGTableRef that the object's `sgt` field
>
> s/SGTableRef/SGTableMap? (several like this through the patch).
>
>> +                //   is initialized.
>> +                // - We store this Devres in the object itself and don't move it, ensuring that the
>> +                //   object it points to remains valid for the lifetime of the SGTableRef.
>> +                let devres = Devres::new(dev, init!(SGTableMap { obj: self.into() }));
>> +                match devres {
>> +                    Ok(devres) => {
>> +                        // SAFETY: We acquired the lock protecting this data above, making it safe
>> +                        // to write into here
>> +                        unsafe { (*sgt_res_ptr) = Some(devres) };
>> +
>> +                        // SAFETY: We just write Some() into *sgt_res_ptr above
>> +                        Ok(unsafe { (&*sgt_res_ptr).as_ref().unwrap_unchecked() })
>> +                    }
>> +                    Err(e) => {
>> +                        // We can't make sure that the pages for this object are unmapped on
>> +                        // driver-unbind, so we need to release the sgt
>> +                        // SAFETY:
>> +                        // - We grabbed the lock required for calling this function above
>> +                        // - We checked above that get_pages_sgt_locked() was successful
>> +                        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) };
>> +
>> +                        Err(e)
>> +                    }
>> +                }
>> +            }
>> +        };
>> +
>> +        // SAFETY: We're releasing the lock that we grabbed above.
>> +        unsafe { bindings::dma_resv_unlock(self.raw_dma_resv()) };
>> +
>> +        ret
>> +    }
>> +
>> +    /// Creates (if necessary) and returns an immutable reference to a scatter-gather table of DMA
>> +    /// pages for this object.
>> +    ///
>> +    /// This will pin the object in memory.
>> +    #[inline]
>> +    pub fn sg_table<'a>(
>> +        &'a self,
>> +        dev: &'a device::Device<Bound>,
>> +    ) -> Result<&'a scatterlist::SGTable> {
>> +        let sgt = self.get_sg_table(dev)?;
>> +
>> +        Ok(sgt.access(dev)?.deref())
>> +    }
>> +
>> +    /// Creates (if necessary) and returns an owned reference to a scatter-gather table of DMA pages
>> +    /// for this object.
>> +    ///
>> +    /// This is the same as [`sg_table`](Self::sg_table), except that it instead returns an
>> +    /// [`shmem::SGTable`] which holds a reference to the associated gem object, instead of a
>> +    /// reference to an [`scatterlist::SGTable`].
>> +    ///
>> +    /// This will pin the object in memory.
>> +    ///
>> +    /// [`shmem::SGTable`]: SGTable
>> +    pub fn owned_sg_table(&self, dev: &device::Device<Bound>) -> Result<SGTable<T>> {
>> +        self.get_sg_table(dev)?;
>> +
>> +        // INVARIANT: We just ensured above that `self.sgt_res` is initialized with
>> +        // `Some(Devres<SGTableMap<T>>)`.
>> +        Ok(SGTable(self.into()))
>> +    }
>>  }
>>  
>>  impl<T: DriverObject> Deref for Object<T> {
>> @@ -226,3 +339,78 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
>>          dumb_map_offset: None,
>>      };
>>  }
>> +
>> +/// A reference to a GEM object that is known to have a mapped [`SGTable`].
>> +///
>> +/// This is used by the Rust bindings with [`Devres`] in order to ensure that mappings for SGTables
>> +/// on GEM shmem objects are revoked on driver-unbind.
>> +///
>> +/// # Invariants
>> +///
>> +/// - `self.obj` always points to a valid GEM object.
>> +/// - This object is proof that `self.0.owner.sgt` has an initialized and valid SGTable.
>
> The comment mentions `self.0` and "a valid SGTable", which don't seem to
> apply to this type.
>
>> +pub struct SGTableMap<T: DriverObject> {
>> +    obj: NonNull<Object<T>>,
>> +}
>> +
>> +impl<T: DriverObject> Deref for SGTableMap<T> {
>> +    type Target = scatterlist::SGTable;
>> +
>> +    fn deref(&self) -> &Self::Target {
>> +        // SAFETY:
>> +        // - The NonNull is guaranteed to be valid via our type invariants.
>> +        // - The sgt field is guaranteed to be initialized and valid via our type invariants.
>> +        unsafe { scatterlist::SGTable::from_raw((*self.obj.as_ref().as_raw_shmem()).sgt) }
>> +    }
>> +}
>> +
>> +impl<T: DriverObject> Drop for SGTableMap<T> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: `obj` is always valid via our type invariants
>> +        let obj = unsafe { self.obj.as_ref() };
>> +
>> +        // SAFETY: The dma_resv for GEM objects is initialized throughout its lifetime
>> +        unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
>> +
>> +        // SAFETY: We acquired the lock needed for calling this function above
>> +        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
>
> For symmetry, I wanted to suggest moving the call to
> `drm_gem_shmem_get_pages_sgt_locked` to a (fallible) constructor of
> `SGTableMap`.
>
> It's not only for cosmetic and code proximity reasons - once you bind
> the call to `drm_gem_shmem_get_pages_sgt_locked` to the successful
> creation of the `SGTableMap` object, you can remove the custom error
> path of `get_sg_table` that called `__drm_gem_shmem_free_sgt_locked` if
> `Devres::new` failed since the destructor of `SGTableMap` will now take
> care of this for us. This simplifies the last complex bit of
> `get_sg_table`.
>
> The problem is that it would also normally require `SGTableMap::new` to
> acquire the DMA reservation lock in order to call
> `drm_gem_shmem_get_pages_sgt_locked`, but we already have it in
> `get_sg_table`.
>
> If we stopped using the DMA reservation lock as a lock for population
> `sgt_res` and instead switched to a regular Mutex, we could then move
> the DMA reservation acquisition to the constructor, attain symmetry, and
> simplify `get_sg_table` to the point where it becomes trivial. That use
> is also fragile as the `SGTableMap` destructor acquires it, so we must
> be very careful to never drop it in `get_sg_table`.
>
> I think that would be a good tradeoff for the time until we make
> `SetOnce` capable of being populated using an `impl Init`.

Another benefit (I guess) of using a dedicated lock is that you could
also call `drm_gem_shmem_get_pages_sgt`, removing the need for patch 3.