Re: [PATCH v9 7/7] rust: drm/gem: Add vmap functions to shmem bindings

[Alvin Sun <alvin.sun@linux.dev>]

On 3/17/26 05:16, Lyude Paul wrote:
> One of the more obvious use cases for gem shmem objects is the ability to
> create mappings into their contents. So, let's hook this up in our rust
> bindings.
>
> Similar to how we handle SGTables, we make sure there's two different types
> of mappings: owned mappings (kernel::drm::gem::shmem::VMap) and borrowed
> mappings (kernel::drm::gem::shmem::VMapRef).
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
>
> ---
> V7:
> * Switch over to the new iosys map bindings that use the Io trait
> V8:
> * Get rid of iosys_map bindings for now, only support non-iomem types
> * s/as_shmem()/as_raw_shmem()
> V9:
> * Get rid of some outdated comments I missed
> * Add missing SIZE check to raw_vmap()
> * Add a proper unit test that ensures that we actually validate SIZE at
>    compile-time.
>    Turns out it takes only 34 lines to make a boilerplate DRM driver for a
>    kunit test :)
> * Add unit tests
> * Add some missing #[inline]s
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>   rust/kernel/drm/gem/shmem.rs | 358 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 356 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 3fab5d76c197b..5254338bad394 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
> @@ -2,7 +2,7 @@
>   
>   //! DRM GEM shmem helper objects
>   //!
> -//! C header: [`include/linux/drm/drm_gem_shmem_helper.h`](srctree/include/linux/drm/drm_gem_shmem_helper.h)
> +//! C header: [`include/linux/drm/drm_gem_shmem_helper.h`](srctree/include/drm/drm_gem_shmem_helper.h)
>   
>   // TODO:
>   // - There are a number of spots here that manually acquire/release the DMA reservation lock using
> @@ -21,6 +21,11 @@
>           from_err_ptr,
>           to_result, //
>       },
> +    io::{
> +        Io,
> +        IoCapable,
> +        IoKnownSize, //
> +    },
>       prelude::*,
>       scatterlist,
>       types::{
> @@ -29,13 +34,22 @@
>       }, //
>   };
>   use core::{
> +    ffi::c_void,
> +    mem::{
> +        self,
> +        MaybeUninit, //
> +    },
>       ops::{
>           Deref,
>           DerefMut, //
>       },
> -    ptr::NonNull,
> +    ptr::{
> +        self,
> +        NonNull, //
> +    },
>   };
>   use gem::{
> +    BaseObject,
>       BaseObjectPrivate,
>       DriverObject,
>       IntoGEMObject, //
> @@ -217,6 +231,88 @@ pub fn owned_sg_table(&self) -> Result<SGTable<T>> {
>               _owner: self.into(),
>           })
>       }
> +
> +    /// Attempt to create a vmap from the gem object, and confirm the size of said vmap.
> +    fn raw_vmap(&self, min_size: usize) -> Result<*mut c_void> {
> +        if self.size() < min_size {
> +            return Err(ENOSPC);
> +        }
> +
> +        let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
> +
> +        // SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held
> +        to_result(unsafe {
> +            // TODO: see top of file
> +            bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut());
> +            let ret = bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr());
> +            bindings::dma_resv_unlock(self.raw_dma_resv());
> +            ret
> +        })?;
> +
> +        // SAFETY: The call to drm_gem_shmem_vunmap_locked succeeded above, so we are guaranteed
> +        // that map is properly initialized.
> +        let map = unsafe { map.assume_init() };
> +
> +        // XXX: We don't currently support iomem allocations
> +        if map.is_iomem {
> +            // SAFETY:
> +            // - The vmap operation above succeeded, making it safe to call vunmap
> +            // - We checked that this is an iomem allocation, making it safe to read vaddr_iomem
> +            unsafe { self.raw_vunmap(map.__bindgen_anon_1.vaddr_iomem) };
> +
> +            Err(ENOTSUPP)
> +        } else {
> +            // SAFETY: We checked that this is not an iomem allocation, making it safe to read vaddr
> +            Ok(unsafe { map.__bindgen_anon_1.vaddr })
> +        }
> +    }
> +
> +    /// Unmap a vmap from the gem object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - The caller promises that addr came from a prior call to [`Self::raw_vmap`] on this gem
> +    ///   object.
> +    /// - The caller promises that the memory pointed to by addr will no longer be accesed through
> +    ///   this instance.
> +    unsafe fn raw_vunmap(&self, vaddr: *mut c_void) {
> +        let resv = self.raw_dma_resv();
> +        let mut map = bindings::iosys_map {
> +            is_iomem: false,
> +            __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr },
> +        };
> +
> +        // SAFETY:
> +        // - This function is safe to call with the DMA reservation lock held
> +        // - Our `ARef` is proof that the underlying gem object here is initialized and thus safe to
> +        //   dereference.
> +        unsafe {
> +            // TODO: see top of file
> +            bindings::dma_resv_lock(resv, ptr::null_mut());
> +            bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), &mut map);
> +            bindings::dma_resv_unlock(resv);
> +        }
> +    }
> +
> +    /// Creates and returns a virtual kernel memory mapping for this object.
> +    #[inline]
> +    pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, SIZE>> {
> +        Ok(VMapRef {
> +            // INVARIANT: `raw_vmap()` checks that the gem object is at least as large as `SIZE`.
> +            addr: self.raw_vmap(SIZE)?,
> +            owner: self,
> +        })
> +    }
> +
> +    /// Creates and returns an owned reference to a virtual kernel memory mapping for this object.
> +    #[inline]
> +    pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMap<T, SIZE>> {
> +        Ok(VMap {
> +            // INVARIANT: `raw_vmap()` checks that the gem object is at least as large as `SIZE`.
> +            addr: self.raw_vmap(SIZE)?,
> +            owner: self.into(),
> +        })
> +    }
>   }
>   
>   impl<T: DriverObject> Deref for Object<T> {
> @@ -268,6 +364,147 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
>       };
>   }
>   
> +macro_rules! impl_vmap_io_capable {
> +    ($impl:ident, $ty:ty $(, $lifetime:lifetime )?) => {
> +        impl<$( $lifetime ,)? D: DriverObject, const SIZE: usize> IoCapable<$ty>
> +            for $impl<$( $lifetime ,)? D, SIZE>
> +        {
> +            #[inline(always)]
> +            unsafe fn io_read(&self, address: usize) -> $ty {
> +                let ptr = address as *mut $ty;
> +
> +                // SAFETY: The safety contract of `io_read` guarantees that address is a valid
> +                // address within the bounds of `Self` of at least the size of $ty, and is properly
> +                // aligned.
> +                unsafe { ptr::read(ptr) }
> +            }
> +
> +            #[inline(always)]
> +            unsafe fn io_write(&self, value: $ty, address: usize) {
> +                let ptr = address as *mut $ty;
> +
> +                // SAFETY: The safety contract of `io_write` guarantees that address is a valid
> +                // address within the bounds of `Self` of at least the size of $ty, and is properly
> +                // aligned.
> +                unsafe { ptr::write(ptr, value) }
> +            }
> +        }
> +    };
> +}
> +
> +// Implement various traits common to both VMap types
> +macro_rules! impl_vmap_common {
> +    ($impl:ident $(, $lifetime:lifetime )?) => {
> +        impl<$( $lifetime ,)? D, const SIZE: usize> $impl<$( $lifetime ,)? D, SIZE>
> +        where
> +            D: DriverObject,
> +        {
> +            /// Borrows a reference to the object that owns this virtual mapping.
> +            #[inline(always)]
> +            pub fn owner(&self) -> &Object<D> {
> +                &self.owner
> +            }
> +        }
> +
> +        impl<$( $lifetime ,)? D, const SIZE: usize> Drop for $impl<$( $lifetime ,)? D, SIZE>
> +        where
> +            D: DriverObject,
> +        {
> +            #[inline(always)]
> +            fn drop(&mut self) {
> +                // SAFETY: Our existence is proof that this map was previously created using
> +                // self.owner
> +                unsafe { self.owner.raw_vunmap(self.addr) };
> +            }
> +        }
> +
> +        impl<$( $lifetime ,)? D, const SIZE: usize> Io for $impl<$( $lifetime ,)? D, SIZE>
> +        where
> +            D: DriverObject,
> +        {
> +            #[inline(always)]
> +            fn addr(&self) -> usize {
> +                self.addr as usize
> +            }
> +
> +            #[inline(always)]
> +            fn maxsize(&self) -> usize {
> +                self.owner.size()
> +            }
> +        }
> +
> +        impl<$( $lifetime ,)? D, const SIZE: usize> IoKnownSize for $impl<$( $lifetime ,)? D, SIZE>
> +        where
> +            D: DriverObject,
> +        {
> +            const MIN_SIZE: usize = SIZE;
> +        }
> +
> +        impl_vmap_io_capable!($impl, u8 $( , $lifetime )?);
> +        impl_vmap_io_capable!($impl, u16 $( , $lifetime )?);
> +        impl_vmap_io_capable!($impl, u32 $( , $lifetime )?);
> +        #[cfg(CONFIG_64BIT)]
> +        impl_vmap_io_capable!($impl, u64 $( , $lifetime )?);
> +    };
> +}
> +
> +/// An owned reference to a virtual mapping for a shmem-based GEM object in kernel address space.
> +///
> +/// # Invariants
> +///
> +/// - The size of `owner` is >= SIZE.
> +/// - The memory pointed to by addr remains valid at least until this object is dropped.
> +pub struct VMap<D: DriverObject, const SIZE: usize = 0> {
> +    addr: *mut c_void,
> +    owner: ARef<Object<D>>,
> +}
> +
> +impl_vmap_common!(VMap);
> +
> +impl<D: DriverObject, const SIZE: usize> Clone for VMap<D, SIZE> {
> +    #[inline]
> +    fn clone(&self) -> Self {
> +        // SAFETY: We have a successful vmap already, so this can't fail
> +        unsafe { self.owner.owned_vmap().unwrap_unchecked() }
> +    }
> +}
> +
> +impl<'a, D: DriverObject, const SIZE: usize> From<VMapRef<'a, D, SIZE>> for VMap<D, SIZE> {
> +    #[inline]
> +    fn from(value: VMapRef<'a, D, SIZE>) -> Self {
> +        let this = Self {
> +            addr: value.addr,
> +            owner: value.owner.into(),
> +        };
> +
> +        mem::forget(value);
> +        this
> +    }
> +}
> +
> +// SAFETY: addr is guaranteed to be valid and accessible for the lifetime of VMap, ensuring its
> +// safe to send across threads.
> +unsafe impl<D: DriverObject, const SIZE: usize> Send for VMap<D, SIZE> {}
> +// SAFETY: addr is guaranteed to be valid and accessible for the lifetime of VMap, ensuring its
> +// safe to send across threads.
> +unsafe impl<D: DriverObject, const SIZE: usize> Sync for VMap<D, SIZE> {}
> +
> +/// A borrowed reference to a virtual mapping for a shmem-based GEM object in kernel address space.
> +pub struct VMapRef<'a, D: DriverObject, const SIZE: usize = 0> {
> +    addr: *mut c_void,
> +    owner: &'a Object<D>,
> +}
> +
> +impl_vmap_common!(VMapRef, 'a);
> +
> +impl<'a, D: DriverObject, const SIZE: usize> Clone for VMapRef<'a, D, SIZE> {
> +    #[inline]
> +    fn clone(&self) -> Self {
> +        // SAFETY: We have a successful vmap already, so this can't fail
> +        unsafe { self.owner.vmap().unwrap_unchecked() }
> +    }
> +}
> +
>   /// An owned reference to a scatter-gather table of DMA address spans for a GEM shmem object.
>   ///
>   /// This object holds an owned reference to the underlying GEM shmem object, ensuring that the
> @@ -298,3 +535,120 @@ fn deref(&self) -> &Self::Target {
>           unsafe { self.sgt.as_ref() }
>       }
>   }
> +
> +#[kunit_tests(rust_drm_gem_shmem)]
> +mod tests {
> +    use super::*;
> +    use crate::{
> +        drm,
> +        faux,
> +        page::PAGE_SIZE, //
> +    };
> +
> +    // The bare minimum needed to create a fake drm driver for kunit
> +
> +    #[pin_data]
> +    struct KunitData {}
> +    struct KunitDriver;
> +    struct KunitFile;
> +    #[pin_data]
> +    struct KunitObject {}
> +
> +    const INFO: drm::DriverInfo = drm::DriverInfo {
> +        major: 0,
> +        minor: 0,
> +        patchlevel: 0,
> +        name: c"kunit",
> +        desc: c"Kunit",
> +    };
> +
> +    impl drm::file::DriverFile for KunitFile {
> +        type Driver = KunitDriver;
> +
> +        fn open(_dev: &drm::Device<KunitDriver>) -> Result<Pin<KBox<Self>>> {
> +            Ok(KBox::new(Self, GFP_KERNEL)?.into())
> +        }
> +    }
> +
> +    impl gem::DriverObject for KunitObject {
> +        type Driver = KunitDriver;
> +        type Args = ();
> +
> +        fn new(
> +            _dev: &drm::Device<KunitDriver>,
> +            _size: usize,
> +            _args: Self::Args,
> +        ) -> impl PinInit<Self, Error> {
> +            try_pin_init!(KunitObject {})
> +        }
> +    }
> +
> +    #[vtable]
> +    impl drm::Driver for KunitDriver {
> +        type Data = KunitData;
> +        type File = KunitFile;
> +        type Object = Object<KunitObject>;
> +
> +        const INFO: drm::DriverInfo = INFO;
> +        const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[];
> +    }
> +
> +    fn create_drm_dev() -> Result<(faux::Registration, ARef<drm::Device<KunitDriver>>)> {
> +        // Create a faux DRM device so we can test gem object creation.
> +        let data = try_pin_init!(KunitData {});
> +        let dev = faux::Registration::new(c"Kunit", None)?;
> +        let drm = drm::Device::<KunitDriver>::new(dev.as_ref(), data)?;

When applying "PATCH v5 0/4 Introduce DeviceContext", I got a 
compilation error:
error[E0049]: associated type `Object` has 0 type parameters but its 
trait declaration has 1 type parameter
    --> rust/kernel/drm/gem/shmem.rs:610:20
     |
610 |         type Object = Object<KunitObject>;
     |                    ^ found 0 type parameters
     |
    ::: rust/kernel/drm/driver.rs:114:17
     |
114 |     type Object<Ctx: drm::DeviceContext>: AllocImpl;
     |                 --- expected 1 type parameter

error[E0599]: no function or associated item named `new` found for 
struct `drm::device::Device<drm::gem::shmem::tests::KunitDriver>` in the 
current scope
    --> rust/kernel/drm/gem/shmem.rs:620:47
     |
620 |         let drm = drm::Device::<KunitDriver>::new(dev.as_ref(), 
data)?;
     |                                               ^^^ function or 
associated item not found in 
`drm::device::Device<drm::gem::shmem::tests::KunitDriver>`
     |
    ::: rust/kernel/drm/device.rs:244:1
     |
244 | pub struct Device<T: drm::Driver, C: DeviceContext = Registered> {
     | ---------------------------------------------------------------- 
function or associated item `new` not found for this struct
     |
     = help: items from traits can only be used if the trait is 
implemented and in scope
note: `drm::gem::DriverObject` defines an item `new`, perhaps you need 
to implement it
    --> rust/kernel/drm/gem/mod.rs:91:1
     |
  91 | pub trait DriverObject: Sync + Send + Sized {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

> +
> +        Ok((dev, drm))
> +    }
> +
> +    #[test]
> +    fn compile_time_vmap_sizes() -> Result {
> +        let (_dev, drm) = create_drm_dev()?;
> +
> +        // Create a gem object to test with
> +        let cfg_ = ObjectConfig::<KunitObject> {
> +            map_wc: false,
> +            parent_resv_obj: None,
> +        };
> +        let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, cfg_, ())?;
> +
> +        // Try creating a normal vmap
> +        obj.vmap::<PAGE_SIZE>()?;
> +
> +        // Try creating a vmap that's smaller then the size we specified
> +        obj.vmap::<{ PAGE_SIZE - 100 }>()?;
> +
> +        // Make sure creating a vmap that's too large fails
> +        assert!(obj.vmap::<{ PAGE_SIZE + 200 }>().is_err());
> +
> +        Ok(())
> +    }
> +
> +    #[test]
> +    fn vmap_io() -> Result {
> +        let (_dev, drm) = create_drm_dev()?;
> +
> +        // Create a gem object to test with
> +        let cfg_ = ObjectConfig::<KunitObject> {
> +            map_wc: false,
> +            parent_resv_obj: None,
> +        };
> +        let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, cfg_, ())?;
> +
> +        let vmap = obj.vmap::<PAGE_SIZE>()?;
> +
> +        vmap.write8(0xDE, 0x0);
> +        assert_eq!(vmap.read8(0x0), 0xDE);
> +        vmap.write32(0xFFFFFFFF, 0x20);
> +
> +        assert_eq!(vmap.read32(0x20), 0xFFFFFFFF);
> +
> +        assert_eq!(vmap.read8(0x20), 0xFF);
> +        assert_eq!(vmap.read8(0x21), 0xFF);
> +        assert_eq!(vmap.read8(0x22), 0xFF);
> +        assert_eq!(vmap.read8(0x23), 0xFF);
> +
> +        Ok(())
> +    }
> +}