Re: [PATCH v18 2/4] rust: drm: gem: shmem: Add vmap functions

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

On Fri Jun 5, 2026 at 4:24 AM JST, 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.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

A few final nits below.

<...>
> +    /// 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 _guard = DmaResvGuard::new(self);
> +
> +        // 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

We aren't necessarily backed by a `ARef` anymore, but maybe we can refer
to the `Safety` comment instead.

> +        //   dereference.
> +        unsafe { bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), &mut map) };
> +    }
> +
> +    /// Creates and returns a virtual kernel memory mapping for this object.
> +    #[inline]
> +    pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {
> +        self.make_vmap()
> +    }
> +
> +    /// 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<VMapOwned<T, C, SIZE>> {
> +        self.make_vmap()
> +    }
>  }
>  
>  impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> {
> @@ -257,7 +339,6 @@ impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> {
>  
>  impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> {
>      #[inline(always)]
> -    #[expect(unused)]
>      fn new(obj: &'a Object<T, C>) -> Self {
>          // SAFETY: This lock is initialized throughout the lifetime of `object`.
>          unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
> @@ -273,3 +354,232 @@ fn drop(&mut self) {
>          unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
>      }
>  }
> +
> +macro_rules! impl_vmap_io_capable {
> +    ($impl:ident, $ty:ty) => {
> +        impl<D, R, C, const SIZE: usize> IoCapable<$ty> for $impl<D, R, C, SIZE>
> +        where
> +            D: DriverObject,
> +            C: DeviceContext,
> +            R: Deref<Target = Object<D, C>>,
> +        {
> +            #[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) }
> +            }
> +        }
> +    };
> +}

I would maybe move the macro definition right before its use, since it
is very local and the code reads more naturally if `VMap` is introduced
before imho.

> +
> +/// A reference to a virtual mapping for an 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.

nit: `addr`.

(also noticed a few other missing `` quotes in the patch)

<...>
> +impl_vmap_io_capable!(VMap, u8);
> +impl_vmap_io_capable!(VMap, u16);
> +impl_vmap_io_capable!(VMap, u32);
> +#[cfg(CONFIG_64BIT)]
> +impl_vmap_io_capable!(VMap, u64);

Having to specify `VMap` seems a bit redundant. Since the macro is only
usable on `VMap` due to its constraints, and even has it in its name, I
guess you can just hardcode it.

> +
> +#[kunit_tests(rust_drm_gem_shmem)]
<...>
> +    #[test]
> +    fn compile_time_vmap_sizes() -> Result {
> +        let (_dev, drm) = create_drm_dev()?;
> +
> +        let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
> +
> +        // 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 }>()?;

For these two, maybe also check that `maxsize()` and `owner()` have the
expected value?

`owned_vmap` also doesn't appear to be tested, although I am not sure
whether that would bring much more coverage, so please take this as just
a sidenote.

> +
> +        // 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()?;
> +
> +        let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
> +
> +        let vmap = obj.vmap::<PAGE_SIZE>()?;
> +
> +        vmap.write8(0xDE, 0x0);
> +        assert_eq!(vmap.read8(0x0), 0xDE);
> +        vmap.write32(0xFFFFFFFF, 0x20);

Let's maybe use a more varied pattern (e.g. `0xFEDCBA98`) so the
ordering is also properly tested by the tests below.

> +
> +        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(())
> +    }
> +}
> -- 
> 2.54.0