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

Alexandre Courbot
2026-04-11T13:32:51Z

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.