[Linaro-mm-sig] Re: [PATCH v12 4/5] rust: drm: gem: Introduce shmem::SGTable

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