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

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