Re: [PATCH v10 5/5] rust: drm: gem: Add vmap functions to shmem bindings

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

Hi Lyude,

On Thu Apr 9, 2026 at 9:12 AM JST, Lyude Paul wrote:
<snip>
> @@ -288,6 +299,84 @@ pub fn owned_sg_table(&self, dev: &device::Device<Bound>) -> Result<SGTable<T>>
>          // `Some(Devres<SGTableMap<T>>)`.
>          Ok(SGTable(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

Looks like a typo: the call above is `drm_gem_shmem_vmap_locked`.

> +        // 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, guaranteeing that `map` points to a valid
> +            //   memory mapping.
> +            // - We checked that this is an iomem allocation, making it safe to read vaddr_iomem
> +            unsafe { self.raw_vunmap(map) };
> +
> +            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 `map` is a valid vmap on this gem object.
> +    /// - The caller promises that the memory pointed to by map will no longer be accesed through
> +    ///   this instance.
> +    unsafe fn raw_vunmap(&self, mut map: bindings::iosys_map) {
> +        let resv = self.raw_dma_resv();
> +
> +        // 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> {
> @@ -386,6 +475,154 @@ unsafe impl<T: DriverObject> Send for SGTableMap<T> {}
>  // it points to is guaranteed to be thread-safe.
>  unsafe impl<T: DriverObject> Sync for SGTableMap<T> {}
>  
> +macro_rules! impl_vmap_io_capable {
> +    ($impl:ident, $ty:ty $(, $lifetime:lifetime )?) => {

How about taking a list of types as argument, so you don't need to
invoke the macro once per supported primitive?

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

The comment says "Implement various traits" but this block is not a
trait implementation.

> +        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.
> +                // - Since we are in Drop, we are guaranteed that no one will access the memory
> +                //   through this mapping after calling this.
> +                unsafe {
> +                    self.owner.raw_vunmap(bindings::iosys_map {
> +                        is_iomem: false,
> +                        __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: 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);

I believe you can considerably reduce the use of macros and consolidate
the code around a single type if you define `VMap` this way:

    pub struct VMap<D, R, const SIZE: usize = 0>
    where
        D: DriverObject,
        R: Deref<Target = Object<D>>,
    {
        addr: *mut c_void,
        owner: R,
    }

Then `R` can either be `&'a Object<D>` or `ARef<Object<D>>` and you
don't need `impl_vmap_common!` anymore (`impl_vmap_io_capable!` is
probably still useful though).

The extra generic makes the type a bit more complex, but you can also
fold it into something similar to what you currently have by defining
type aliases:

    pub type VMapRef<'a, D, const SIZE: usize = 0> = VMap<D, &'a Object<D>, SIZE>;
    pub type VMapOwned<D, const SIZE: usize = 0> = VMap<D, ARef<Object<D>>, SIZE>;

... although I don't think that is necessary as callers will never need
to specify these generics anyway.