I completely forgot when coming up with this type that #[derive(Default)] only works if all generics mentioned in the type implement Default (and T usually doesn't). This being said: We don't use `T` for anything besides using it for a reference type, so whether or not it implements `Default` shouldn't actually need to matter.

So, fix this by just manually implementing Default instead of deriving it.

Signed-off-by: Lyude Paul lyude@redhat.com --- rust/kernel/drm/gem/shmem.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index e1b648920d2f6..8b7de136ab1f9 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -39,7 +39,6 @@ /// /// This is used with [`Object::new()`] to control various properties that can only be set when /// initially creating a shmem-backed GEM object. -#[derive(Default)] pub struct ObjectConfig<'a, T: DriverObject> { /// Whether to set the write-combine map flag. pub map_wc: bool, @@ -50,6 +49,16 @@ pub struct ObjectConfig<'a, T: DriverObject> { pub parent_resv_obj: Option<&'a Object<T>>, }

+impl<'a, T: DriverObject> Default for ObjectConfig<'a, T> { + #[inline(always)] + fn default() -> Self { + Self { + map_wc: false, + parent_resv_obj: None, + } + } +} + /// A shmem-backed GEM object. /// /// # Invariants

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.

Similar to how we handle SGTables, we make sure there's two different types of mappings: owned mappings (kernel::drm::gem::shmem::VMap) and borrowed mappings (kernel::drm::gem::shmem::VMapRef).

Signed-off-by: Lyude Paul lyude@redhat.com

--- V7: * Switch over to the new iosys map bindings that use the Io trait V8: * Get rid of iosys_map bindings for now, only support non-iomem types * s/as_shmem()/as_raw_shmem() V9: * Get rid of some outdated comments I missed * Add missing SIZE check to raw_vmap() * Add a proper unit test that ensures that we actually validate SIZE at compile-time. Turns out it takes only 34 lines to make a boilerplate DRM driver for a kunit test :) * Add unit tests * Add some missing #[inline]s V10: * Correct issue with iomem error path We previously called raw_vunmap() if we got an iomem allocation, but raw_vunmap() was written such that it assumed all allocations were sysmem allocations. Fix this by just making raw_vunmap() accept a iosys_map. V11: * Use Alexandre's clever solution to remove the macros we were using for maintaining two different VMap types. * Change the order of items in Object<T> to ensure that sgt_res is always dropped before obj. * Fix typo in Object.raw_vmap() * s/raw_vmap()/make_vmap()/ Deduplicate code a bit more as well by using more generics here V15: * Add these patches back * We only have one VMap type now! * Use ObjectConfig::default() in unit tests since we unbroke it.

rust/kernel/drm/gem/shmem.rs | 433 +++++++++++++++++++++++++++++++++-- 1 file changed, 418 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index a116b0752e46a..494e0d0d8d0d6 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -11,18 +11,39 @@

use crate::{ container_of, + device::{ + self, + Bound, // + }, + devres::*, drm::{ - device, driver, gem, - private::Sealed, // + private::Sealed, + Device, // + }, + error::{ + from_err_ptr, + to_result, // + }, + io::{ + Io, + IoCapable, + IoKnownSize, // }, - error::to_result, prelude::*, - sync::aref::ARef, + scatterlist, + sync::{ + aref::ARef, + new_lazy_init, + LazyInit, + LazyInitError, // + }, types::Opaque, // }; use core::{ + ffi::c_void, + mem::MaybeUninit, // ops::{ Deref, DerefMut, // @@ -33,6 +54,7 @@ }, }; use gem::{ + BaseObject, BaseObjectPrivate, DriverObject, IntoGEMObject, // @@ -75,6 +97,9 @@ pub struct Object<T: DriverObject> { obj: Opaquebindings::drm_gem_shmem_object, /// Parent object that owns this object's DMA reservation object. parent_resv_obj: Option<ARef<Object<T>>>, + /// Devres object for unmapping any SGTable on driver-unbind. + #[pin] + sgt_res: LazyInit<Devres<SGTableMap<T>>>, #[pin] inner: T, } @@ -118,7 +143,7 @@ fn as_raw_shmem(&self) -> *mut bindings::drm_gem_shmem_object { /// /// Additional config options can be specified using `config`. pub fn new( - dev: &device::Device<T::Driver>, + dev: &Device<T::Driver>, size: usize, config: ObjectConfig<'_, T>, args: T::Args, @@ -127,6 +152,7 @@ pub fn new( try_pin_init!(Self { obj <- Opaque::init_zeroed(), parent_resv_obj: config.parent_resv_obj.map(|p| p.into()), + sgt_res <- new_lazy_init!(), inner <- T::new(dev, size, args), }), GFP_KERNEL, @@ -160,9 +186,9 @@ pub fn new( }

/// Returns the `Device` that owns this GEM object. - pub fn dev(&self) -> &device::Device<T::Driver> { + pub fn dev(&self) -> &Device<T::Driver> { // SAFETY: `dev` will have been initialized in `Self::new()` by `drm_gem_shmem_init()`. - unsafe { device::Device::from_raw((*self.as_raw()).dev) } + unsafe { Device::from_raw((*self.as_raw()).dev) } }

extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { @@ -170,22 +196,123 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { // - DRM always passes a valid gem object here // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that // `obj` is contained within a drm_gem_shmem_object - let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) }; - - // SAFETY: - // - We're in free_callback - so this function is safe to call. - // - We won't be using the gem resources on `this` after this call. - unsafe { bindings::drm_gem_shmem_release(this) }; + let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };

// SAFETY: // - We verified above that `obj` is valid, which makes `this` valid // - This function is set in AllocOps, so we know that `this` is contained within a // `Object<T>` - let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut(); + let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut(); + + // We need to reset `sgt_res` first, since doing so requires that the GEM object is still + // alive. + // SAFETY: + // - We verified above that `this` is valid. + // - `&mut (*this)` does not implement Unpin. + unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset(); + + // SAFETY: + // - We're in free_callback - so this function is safe to call. + // - We won't be using the gem resources on `this` after this call. + unsafe { bindings::drm_gem_shmem_release(base) };

// SAFETY: We're recovering the Kbox<> we created in gem_create_object() let _ = unsafe { KBox::from_raw(this) }; } + + /// Attempt to create a vmap from the gem object, and confirm the size of said vmap. + fn make_vmap<'a, R, const SIZE: usize>(&'a self) -> Result<VMap<T, R, SIZE>> + where + R: Deref<Target = Self> + From<&'a Self>, + { + // INVARIANT: We check here that the gem object is at least as large as `SIZE`. + if self.size() < SIZE { + return Err(ENOSPC); + } + + let mut map: MaybeUninitbindings::iosys_map = MaybeUninit::uninit(); + let guard = DmaResvGuard::new(self); + + // SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held + to_result(unsafe { + bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr()) + })?; + + // Drop the guard explicitly here, since we may need to call raw_vunmap() (which re-acquires + // the lock). + drop(guard); + + // SAFETY: The call to drm_gem_shmem_vmap_locked succeeded above, so we are guaranteed 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 { + Ok(VMap { + // SAFETY: We checked that this is not an iomem allocation, making it safe to read + // vaddr + addr: unsafe { map.__bindgen_anon_1.vaddr }, + owner: self.into(), + }) + } + } + + /// 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 + // 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, 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, SIZE>> { + self.make_vmap() + } + + /// Creates (if necessary) and returns an immutable reference to a scatter-gather table of DMA + /// pages for this object. + /// + /// This will pin the object in memory. It is expected that `dev` should be a pointer to the + /// same [`device::Device`] which `self` belongs to, otherwise this function will return + /// `Err(EINVAL)`. + pub fn sg_table<'a>( + &'a self, + dev: &'a device::Device<Bound>, + ) -> Result<&'a scatterlist::SGTable> { + if dev.as_raw() != self.dev().as_ref().as_raw() { + return Err(EINVAL); + } + + match self.sgt_res.init(Devres::new(dev, SGTableMap::new(self))) { + Ok(ret) | Err(LazyInitError::AlreadyInit(ret)) => Ok(ret.access(dev)?), + Err(LazyInitError::DuringInit(e)) => Err(e), + } + } }

impl<T: DriverObject> Deref for Object<T> { @@ -246,7 +373,6 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {

impl<'a, T: DriverObject> DmaResvGuard<'a, T> { #[inline(always)] - #[expect(unused)] fn new(obj: &'a Object<T>) -> Self { // SAFETY: This lock is initialized throughout the lifetime of `object`. unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) }; @@ -262,3 +388,280 @@ 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, const SIZE: usize> IoCapable<$ty> for $impl<D, R, SIZE> + where + D: DriverObject, + R: Deref<Target = Object<D>>, + { + #[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) } + } + } + }; +} + +/// 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. +pub struct VMap<D, R, const SIZE: usize = 0> +where + D: DriverObject, + R: Deref<Target = Object<D>>, +{ + addr: *mut c_void, + owner: R, +} + +/// An alias type for a reference to a shmem-based GEM object's VMap. +pub type VMapRef<'a, D, const SIZE: usize = 0> = VMap<D, &'a Object<D>, SIZE>; + +/// An alias type for an owned reference to a shmem-based GEM object's VMap. +pub type VMapOwned<D, const SIZE: usize = 0> = VMap<D, ARef<Object<D>>, SIZE>; + +impl<D, R, const SIZE: usize> VMap<D, R, SIZE> +where + D: DriverObject, + R: Deref<Target = Object<D>>, +{ + /// Borrows a reference to the object that owns this virtual mapping. + #[inline(always)] + pub fn owner(&self) -> &Object<D> { + &self.owner + } +} + +impl<D, R, const SIZE: usize> Drop for VMap<D, R, SIZE> +where + D: DriverObject, + R: Deref<Target = Object<D>>, +{ + #[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<D, R, const SIZE: usize> Io for VMap<D, R, SIZE> +where + D: DriverObject, + R: Deref<Target = Object<D>>, +{ + #[inline(always)] + fn addr(&self) -> usize { + self.addr as usize + } + + #[inline(always)] + fn maxsize(&self) -> usize { + self.owner.size() + } +} + +impl<D, R, const SIZE: usize> IoKnownSize for VMap<D, R, SIZE> +where + D: DriverObject, + R: Deref<Target = Object<D>>, +{ + const MIN_SIZE: usize = SIZE; +} + +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); + +/// A reference to a GEM object that is known to have a mapped [`SGTable`]. +/// +/// This is used by the Rust bindings with [`Devres`] in order to ensure that mappings for SGTables +/// on GEM shmem objects are revoked on driver-unbind. +/// +/// # Invariants +/// +/// - `self.obj` always points to a valid GEM object. +/// - This object is proof that `self.obj.owner.sgt` has an initialized and valid +/// [`scatterlist::SGTable`]. +pub struct SGTableMap<T: DriverObject> { + obj: NonNull<Object<T>>, +} + +impl<T: DriverObject> Deref for SGTableMap<T> { + type Target = scatterlist::SGTable; + + fn deref(&self) -> &Self::Target { + // SAFETY: + // - The NonNull is guaranteed to be valid via our type invariants. + // - The sgt field is guaranteed to be initialized and valid via our type invariants. + unsafe { scatterlist::SGTable::from_raw((*self.obj.as_ref().as_raw_shmem()).sgt) } + } +} + +impl<T: DriverObject> Drop for SGTableMap<T> { + fn drop(&mut self) { + // SAFETY: `obj` is always valid via our type invariants + let obj = unsafe { self.obj.as_ref() }; + let _lock = DmaResvGuard::new(obj); + + // SAFETY: We acquired the lock needed for calling this function above + unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) }; + } +} + +impl<T: DriverObject> SGTableMap<T> { + fn new(obj: &Object<T>) -> impl Init<Self, Error> { + // INVARIANT: + // - We call drm_gem_shmem_get_pages_sgt_locked below and check whether or not it + // succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is + // initialized. + // SAFETY: + // - `obj` is fully initialized, making this function safe to call. + from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?; + + Ok(Self { obj: obj.into() }) + } +} + +// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object +// it points to is guaranteed to be thread-safe. +unsafe impl<T: DriverObject> Send for SGTableMap<T> {} +// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object +// it points to is guaranteed to be thread-safe. +unsafe impl<T: DriverObject> Sync for SGTableMap<T> {} + +#[kunit_tests(rust_drm_gem_shmem)] +mod tests { + use super::*; + use crate::{ + drm, + faux, + page::PAGE_SIZE, // + }; + + // The bare minimum needed to create a fake drm driver for kunit + + #[pin_data] + struct KunitData {} + struct KunitDriver; + struct KunitFile; + #[pin_data] + struct KunitObject {} + + const INFO: drm::DriverInfo = drm::DriverInfo { + major: 0, + minor: 0, + patchlevel: 0, + name: c"kunit", + desc: c"Kunit", + }; + + impl drm::file::DriverFile for KunitFile { + type Driver = KunitDriver; + + fn open(_dev: &drm::Device<KunitDriver>) -> Result<Pin<KBox<Self>>> { + Ok(KBox::new(Self, GFP_KERNEL)?.into()) + } + } + + impl gem::DriverObject for KunitObject { + type Driver = KunitDriver; + type Args = (); + + fn new( + _dev: &drm::Device<KunitDriver>, + _size: usize, + _args: Self::Args, + ) -> impl PinInit<Self, Error> { + try_pin_init!(KunitObject {}) + } + } + + #[vtable] + impl drm::Driver for KunitDriver { + type Data = KunitData; + type File = KunitFile; + type Object = Object<KunitObject>; + + const INFO: drm::DriverInfo = INFO; + const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[]; + } + + fn create_drm_dev() -> Result<(faux::Registration, ARef<drm::Device<KunitDriver>>)> { + // Create a faux DRM device so we can test gem object creation. + let data = try_pin_init!(KunitData {}); + let dev = faux::Registration::new(c"Kunit", None)?; + let drm = drm::Device::<KunitDriver>::new(dev.as_ref(), data)?; + + Ok((dev, drm)) + } + + #[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 }>()?; + + // 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); + + 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(()) + } +}

One of the complications of trying to use the shmem helpers to create a scatterlist for shmem objects is that we need to be able to provide a guarantee that the driver cannot be unbound for the lifetime of the scatterlist.

The easiest way of handling this seems to be just hooking up an unmap operation to devres the first time we create a scatterlist, which allows us to still take advantage of gem shmem facilities without breaking that guarantee. To allow for this, we extract __drm_gem_shmem_free_sgt_locked() - which allows a caller (e.g. the rust bindings) to manually unmap the sgt for a gem object as needed.

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Alexandre Courbot acourbot@nvidia.com

--- V10: * Fix incorrect function name in documentation for __drm_gem_shmem_release_sgt_locked()

drivers/gpu/drm/drm_gem_shmem_helper.c | 32 +++++++++++++++++++++----- include/drm/drm_gem_shmem_helper.h | 1 + 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 545933c7f7121..c989459eb2159 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -158,6 +158,30 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t } EXPORT_SYMBOL_GPL(drm_gem_shmem_create);

+/** + * __drm_gem_shmem_release_sgt_locked - Unpin and DMA unmap pages, and release the + * cached scatter/gather table for an shmem GEM object. + * @shmem: shmem GEM object + * + * If the passed shmem object has an active scatter/gather table for driver + * usage, this function will unmap it and release the memory associated with it. + * It is the responsibility of the caller to ensure it holds the dma_resv_lock + * for this object. + * + * Drivers should not need to call this function themselves, it is mainly + * intended for usage in the Rust shmem bindings. + */ +void __drm_gem_shmem_free_sgt_locked(struct drm_gem_shmem_object *shmem) +{ + dma_resv_assert_held(shmem->base.resv); + + dma_unmap_sgtable(shmem->base.dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); + sg_free_table(shmem->sgt); + kfree(shmem->sgt); + shmem->sgt = NULL; +} +EXPORT_SYMBOL_GPL(__drm_gem_shmem_free_sgt_locked); + /** * drm_gem_shmem_release - Release resources associated with a shmem GEM object. * @shmem: shmem GEM object @@ -176,12 +200,8 @@ void drm_gem_shmem_release(struct drm_gem_shmem_object *shmem)

drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));

- if (shmem->sgt) { - dma_unmap_sgtable(obj->dev->dev, shmem->sgt, - DMA_BIDIRECTIONAL, 0); - sg_free_table(shmem->sgt); - kfree(shmem->sgt); - } + if (shmem->sgt) + __drm_gem_shmem_free_sgt_locked(shmem); if (shmem->pages) drm_gem_shmem_put_pages_locked(shmem);

diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index 5ccdae21b94a9..b2c23af628e1a 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -111,6 +111,7 @@ int drm_gem_shmem_init(struct drm_device *dev, struct drm_gem_shmem_object *shme struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size); void drm_gem_shmem_release(struct drm_gem_shmem_object *shmem); void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem); +void __drm_gem_shmem_free_sgt_locked(struct drm_gem_shmem_object *shmem);

void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem); int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem);