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 heldto_result(unsafe {// TODO: see top of filebindings::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 allocationsif 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_iomemunsafe { self.raw_vunmap(map) };Err(ENOTSUPP)} else {// SAFETY: We checked that this is not an iomem allocation, making it safe to read vaddrOk(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 filebindings::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.
whereD: 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>whereD: 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>whereD: 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>whereD: 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.