From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 347B83AB482 for ; Fri, 12 Jun 2026 19:45:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781293528; cv=none; b=Lyqsu7T+crSieWCeoq6RBk07v1dW4eqFFXTbCyhBK2nN86R5EXMp+WtNn5375JqwDtHjwxPCRDNMGNYinppxYlrzKOagm9JoPU+6QnMpLqFldAkXUxZsuTBOxhsopg24hcOeqPFMrlswjnb2Zym/GshMos6PT7RWXQcHA6aytK0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781293528; c=relaxed/simple; bh=wx4rT98uPuQnIQoeAielKUR1dVS1DVRlF9+zkU236hI=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:content-type; b=RMgKY0KtziEml+m8bk4NHrnY9bw9cNWCh54a43JkA7xtS6EKt3bnbJcjqaLqBkMUbetrkAIDrjkrtf3ZbrCpa4nfQbye3FR6GIcQ+fmsyMSUlOgQgLQEYRtTq0gbvnq+/OMJlii1xJDCZvPhDv4igXGo0WV/LdLf1OH2dl5tuEY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=iwWfm5rP; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="iwWfm5rP" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781293520; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sEXoylyRlNjC51Vo4SeCRqXXl2TvWmEvftNZc5TnX7A=; b=iwWfm5rPwWvSOqcIuc+qZEwKcQIZjF1KV4LNL3EOvMC32SykD/ghNM8q6IRGHeItS6gHkS /bGxDkDRR6rE+ISuXHLoLtngf36i8Gft6XefiPVTMK11ivMMRtL2s3y9U83F952zzXyee8 mXteuyRvIfk79bciBMnJ/H9J5u9MhKA= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-219-gBwJgp2QO9SNKVE-_hKW6A-1; Fri, 12 Jun 2026 15:45:14 -0400 X-MC-Unique: gBwJgp2QO9SNKVE-_hKW6A-1 X-Mimecast-MFC-AGG-ID: gBwJgp2QO9SNKVE-_hKW6A_1781293511 Received: from mx-prod-int-10.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-10.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.95]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 339F919560B5; Fri, 12 Jun 2026 19:45:11 +0000 (UTC) Received: from GoldenWind.redhat.com (unknown [10.22.80.21]) by mx-prod-int-10.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 157D2404; Fri, 12 Jun 2026 19:45:07 +0000 (UTC) From: Lyude Paul To: dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org Cc: Alexandre Courbot , Gary Guo , =?UTF-8?q?Christian=20K=C3=B6nig?= , driver-core@lists.linux.dev, Miguel Ojeda , Maarten Lankhorst , Alice Ryhl , Simona Vetter , linux-kernel@vger.kernel.org, Sumit Semwal , linux-media@vger.kernel.org, "Rafael J . Wysocki" , Thomas Zimmermann , Maxime Ripard , David Airlie , Benno Lossin , linaro-mm-sig@lists.linaro.org, Danilo Krummrich , Mukesh Kumar Chaurasiya , Asahi Lina , Daniel Almeida , Lyude Paul , Greg Kroah-Hartman Subject: [PATCH v21 4/4] rust: drm: gem: Introduce shmem::Object::sg_table() Date: Fri, 12 Jun 2026 15:43:36 -0400 Message-ID: <20260612194436.585385-5-lyude@redhat.com> In-Reply-To: <20260612194436.585385-1-lyude@redhat.com> References: <20260612194436.585385-1-lyude@redhat.com> Precedence: bulk X-Mailing-List: driver-core@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.6 on 10.30.177.95 X-Mimecast-MFC-PROC-ID: 4Z2XUvtMq4E-c5sCDSfgAeSHr5YVQK1I1XOaAY_dH8c_1781293511 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit content-type: text/plain; charset="US-ASCII"; x-default=true In order to do this, we need to be careful to ensure that any interface we expose for scatterlists ensures that any mappings created from one are destroyed on driver-unbind. To do this, we introduce a Devres resource into shmem::Object that we use in order to ensure that we release any SGTable mappings on driver-unbind. There's some other slightly unfortunate caveats of this: * Drivers don't have explicit control at the moment over when unmapping happens (which is exactly the same as the C side atm, so it might not be a problem). * We can't just return `SGTableMap` to the user through an Arc to attempt to fix the last caveat - because that implies the gem object would need to hold a reference count to the scatterlist mapping, which just leaves us with the same problem. Signed-off-by: Lyude Paul Reviewed-by: Alexandre Courbot Acked-by: Danilo Krummrich --- V3: * Rename OwnedSGTable to shmem::SGTable. Since the current version of the SGTable abstractions now has a `Owned` and `Borrowed` variant, I think renaming this to shmem::SGTable makes things less confusing. We do however, keep the name of owned_sg_table() as-is. V4: * Clarify safety comments for SGTable to explain why the object is thread-safe. * Rename from SGTableRef to SGTable V10: * Use Devres in order to ensure that SGTables are revocable, and are unmapped on driver-unbind. V11: * s/create_sg_table()/get_sg_table() * Get rid of extraneous `ret = ` in shmem::Object::get_sg_table() V12: * Actually move sgt_res in this patch and not the next one V13: * Use DmaResvGuard suggestion from Alexander * Use Alexander's (much better) solution for get_sg_table() * Use SetOnce instead of UnsafeCell * s/SGTableRef/SGTableMap * Fix typo in SGTableMap documentation * Create fallible constructor for SGTableMap * Don't reuse dma_resv lock for protecting Object contents, just use Mutex + SetOnce * Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to hold the dma_resv lock ourselves for anything but this function. * Check that the device we receive in the bounds for sg_table() and owned_sg_table() that said Device is in fact, the correct device. * Remove redundant docs in owned_sg_table(), just point it back to sg_table(). * Implement Deborah's suggestion to fix double-free in free_callback() * Restore original order of Object * Fix doc typo for SGTableMap V14: * Use new InitOnce container over the Mutex/SetOnce horror show we had before. * Start using LazyInit container for storing Devres for sgt unmap * Add some kunit tests for sg_table (not sure why I didn't do this before) using some of the boilerplate code leftover from the vmap bindings * Get rid of the owned SGTable variant for now, we'll add it back in a future patch if people actually need it. * Use new LazyInit container from me to get rid of the horrid Mutex> mess. * Add the best we can do for unit tests w/r/t SGTable at the moment V16: * Get rid of LazyInit, go back to SetOnce, use trick that Alice recommended that is a lot cleaner. * Fix horrid rebasing mistake V17: * Rebase * Fix missing safety comment in free_callback() (we forgot to justify why &mut is safe in `unsafe { &mut (*this).sgt_res }.reset()`) V18: * Use ManuallyDrop instead of SetOnce::reset() V19: * Explain that populate() will always return true in sg_table() * Fix outdated comment in SGTableMap * Fix invariant comment in SGTableMap rust/kernel/drm/gem/shmem.rs | 174 +++++++++++++++++++++++++++++++++-- 1 file changed, 164 insertions(+), 10 deletions(-) diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index a38c98add3d17..3ee19ef6264e7 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -11,6 +11,11 @@ use crate::{ container_of, + device::{ + self, + Bound, // + }, + devres::*, drm::{ driver, gem, @@ -19,14 +24,23 @@ DeviceContext, Registered, // }, - error::to_result, + error::{ + from_err_ptr, + to_result, // + }, io::{ Io, IoCapable, IoKnownSize, // }, prelude::*, - sync::aref::ARef, + scatterlist, + sync::{ + aref::ARef, + new_mutex, + Mutex, + SetOnce, // + }, types::{ NotThreadSafe, Opaque, // @@ -35,7 +49,10 @@ use core::{ ffi::c_void, marker::PhantomData, - mem::MaybeUninit, // + mem::{ + ManuallyDrop, + MaybeUninit, // + }, ops::{ Deref, DerefMut, // @@ -90,6 +107,11 @@ pub struct Object { obj: Opaque, /// Parent object that owns this object's DMA reservation object. parent_resv_obj: Option>>, + /// Devres object for unmapping any SGTable on driver-unbind. + sgt_res: ManuallyDrop>>>, + #[pin] + /// Lock for protecting initialization of `sgt_res`. + sgt_lock: Mutex<()>, #[pin] inner: T, _ctx: PhantomData, @@ -148,6 +170,8 @@ pub fn new( try_pin_init!(Self { obj <- Opaque::init_zeroed(), parent_resv_obj: config.parent_resv_obj.map(|p| p.into()), + sgt_res: ManuallyDrop::new(SetOnce::new()), + sgt_lock <- new_mutex!(()), inner <- T::new(dev, size, args), _ctx: PhantomData::, }), @@ -192,18 +216,26 @@ 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` - 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 drop `sgt_res` first, since doing so requires that the GEM object is still + // alive. + // SAFETY: + // - We verified above that `this` is valid. + // - We are in free_callback, guaranteeing we have exclusive access to `this` and that + // `sgt_res` will not be used after dropping it here. + unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) }; + + // 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) }; @@ -281,6 +313,46 @@ pub fn vmap(&self) -> Result> { pub fn owned_vmap(&self) -> Result> { 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, + ) -> Result<&'a scatterlist::SGTable> { + if dev.as_raw() != self.dev().as_ref().as_raw() { + return Err(EINVAL); + } + + let sgt_res = 'out: { + // Fast path: sgt_res is already initialized + if let Some(sgt_res) = self.sgt_res.as_ref() { + break 'out sgt_res; + } + + // Slow path: Grab the lock and see if we need to initialize sgt_res. + let _guard = self.sgt_lock.lock(); + + // If someone initialized it while we were waiting, we can exit early. + if let Some(sgt_res) = self.sgt_res.as_ref() { + break 'out sgt_res; + } + + // If not, finish initializing and return. `populate()` cannot return false, as + // `sgt_res` must be unpopulated, and we must hold `sgt_lock` to reach this point. + self.sgt_res + .populate(Devres::new(dev, SGTableMap::new(self))?); + + // SAFETY: We just populated sgt_res above. + unsafe { self.sgt_res.as_ref().unwrap_unchecked() } + }; + + Ok(sgt_res.access(dev)?) + } } impl Deref for Object { @@ -499,6 +571,64 @@ unsafe fn io_write(&self, value: $ty, address: usize) { #[cfg(CONFIG_64BIT)] impl_vmap_io_capable!(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_res` has an initialized and valid pointer to an +/// [`SGTable`]. +/// +/// [`SGTable`]: scatterlist::SGTable +pub struct SGTableMap { + obj: NonNull>, +} + +impl Deref for SGTableMap { + 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 Drop for SGTableMap { + 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 SGTableMap { + fn new(obj: &Object) -> impl Init { + // INVARIANT: + // - We call drm_gem_shmem_get_pages_sgt 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 Send for SGTableMap {} +// 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 Sync for SGTableMap {} + #[kunit_tests(rust_drm_gem_shmem)] mod tests { use super::*; @@ -614,4 +744,28 @@ fn vmap_io() -> Result { Ok(()) } + + // TODO: I would love to actually test the success paths of sg_table(), but that would require + // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave + // that for someone else. + + // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it + // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device + // bound to it. + #[test] + fn fail_sg_table_on_wrong_dev() -> Result { + let (_dev, drm) = create_drm_dev()?; + let wrong_dev = faux::Registration::new(c"EvilKunit", None)?; + + let obj = Object::::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?; + + assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL); + + // If sgt_res was not initialized mistakenly with the wrong device, this should still fail. + assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL); + + // TODO: Someday, we should test that creating an sg_table here still succeeds. + + Ok(()) + } } -- 2.54.0 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2D348CD98CE for ; Fri, 12 Jun 2026 19:45:23 +0000 (UTC) Received: from kara.freedesktop.org (unknown [131.252.210.166]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0600910F6B6; Fri, 12 Jun 2026 19:45:23 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="GXjh/a9P"; dkim-atps=neutral Received: from kara.freedesktop.org (localhost [127.0.0.1]) by kara.freedesktop.org (Postfix) with ESMTP id 5827A46AC8; Fri, 12 Jun 2026 19:31:20 +0000 (UTC) ARC-Seal: i=1; cv=none; a=rsa-sha256; d=lists.freedesktop.org; s=20240201; t=1781292680; b=vno9sOKadKrsCjiyGr5GP1PiyVsCUlokpnI/6ibffAr9vEjNRsassrNKxesQePOG2pRFD kq/KVDHDBmu7UemYJsb5dIzo32ojTqSqF77NV29Xd5pu9+nQVrzgiVngViJ7lIa6RASCrag 0zQgrNFORoxUKFpyqf8o8bS8bigXfftbHa6I0lEjEvpaU9mC+rwh/MF9Rm0vAdP2B0wIxOZ NAxWOLVVdZHXoa4c4tt54OQyuyLxBlNSR7vR85TOs0gXdkdF/sKwcyZd4YCbuXPlAqXWZ48 ATUWkqv3rQolecu9IB6U8AfpEJ/0HId/LWcEgfq4Lzh+EhvDcQ+E1edg3jMA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=lists.freedesktop.org; s=20240201; t=1781292680; h=from : sender : reply-to : subject : date : message-id : to : cc : mime-version : content-type : content-transfer-encoding : content-id : content-description : resent-date : resent-from : resent-sender : resent-to : resent-cc : resent-message-id : in-reply-to : references : list-id : list-help : list-unsubscribe : list-subscribe : list-post : list-owner : list-archive; bh=sEXoylyRlNjC51Vo4SeCRqXXl2TvWmEvftNZc5TnX7A=; b=zzZ0+KFIvb/6h4L9kxE0w7oJFkWnJgVcNgxreaXwnFBWa7It/B39U71/ZDYIev0MFHKh+ EBr+KP5yfyWw6HpI7SjCz6y3zuDZBUD6MJv+Vt45k0COYQXxhdIV/mj9+MW53ASYbmE0rPh 5ZBNvgMekWMROFTp84e/7tZ8KYsHd7FyZvFzmv/Htj2/14RUN1kVNeW8HlaltF8g4lgzK9u bATNGK2qWJmNZsPyrDEcz5sSKSFHZu2BHHceOkE11ZrEB6ZKLtFPFsWLafh3daaGwsLnUB6 R13mG+3nXArY4SniY4FOYM+aPOQ6p+/RzfpsiatOzR9mfYANvyOPJMwQs2qA== ARC-Authentication-Results: i=1; mail.freedesktop.org; dkim=pass header.d=redhat.com; arc=none (Message is not ARC signed); dmarc=pass (Used From Domain Record) header.from=redhat.com policy.dmarc=quarantine Authentication-Results: mail.freedesktop.org; dkim=pass header.d=redhat.com; arc=none (Message is not ARC signed); dmarc=pass (Used From Domain Record) header.from=redhat.com policy.dmarc=quarantine Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by kara.freedesktop.org (Postfix) with ESMTPS id 835AE460F9 for ; Fri, 12 Jun 2026 19:31:17 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id D74DA10F67E for ; Fri, 12 Jun 2026 19:45:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781293519; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sEXoylyRlNjC51Vo4SeCRqXXl2TvWmEvftNZc5TnX7A=; b=GXjh/a9Pt+AHJL7BEw8BMjjwmFl4PI0RbPRsY5VvDy44kVDobbdRwq6pJCKdeiGnyB1EEi 8r7yTKNWd0cAn4Urn2JeS30XrC1Et71J3Y47UoQogMjiDG4dnYbwbnEQgW1dcBb/pp0kFG Rh0qWAZUJLx167wfXAXk3z1gxBIdoQ0= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-219-gBwJgp2QO9SNKVE-_hKW6A-1; Fri, 12 Jun 2026 15:45:14 -0400 X-MC-Unique: gBwJgp2QO9SNKVE-_hKW6A-1 X-Mimecast-MFC-AGG-ID: gBwJgp2QO9SNKVE-_hKW6A_1781293511 Received: from mx-prod-int-10.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-10.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.95]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 339F919560B5; Fri, 12 Jun 2026 19:45:11 +0000 (UTC) Received: from GoldenWind.redhat.com (unknown [10.22.80.21]) by mx-prod-int-10.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 157D2404; Fri, 12 Jun 2026 19:45:07 +0000 (UTC) From: Lyude Paul To: dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org Subject: [PATCH v21 4/4] rust: drm: gem: Introduce shmem::Object::sg_table() Date: Fri, 12 Jun 2026 15:43:36 -0400 Message-ID: <20260612194436.585385-5-lyude@redhat.com> In-Reply-To: <20260612194436.585385-1-lyude@redhat.com> References: <20260612194436.585385-1-lyude@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.6 on 10.30.177.95 X-Mimecast-MFC-PROC-ID: sj37rZPErWMLOcuv6UcVbf0ViCpFO9mn5Kmb7MqG6So_1781293511 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit content-type: text/plain; charset="US-ASCII"; x-default=true Message-ID-Hash: RC3CYMHRBFV5NYOIUJ3YU7U5BZSNKP4X X-Message-ID-Hash: RC3CYMHRBFV5NYOIUJ3YU7U5BZSNKP4X X-MailFrom: lyude@redhat.com X-Mailman-Rule-Hits: member-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address CC: Alexandre Courbot , Gary Guo , =?UTF-8?q?Christian=20K=C3=B6nig?= , driver-core@lists.linux.dev, Miguel Ojeda , Maarten Lankhorst , Alice Ryhl , Simona Vetter , linux-kernel@vger.kernel.org, Sumit Semwal , linux-media@vger.kernel.org, "Rafael J . Wysocki" , Maxime Ripard , Benno Lossin , linaro-mm-sig@lists.linaro.org, Danilo Krummrich , Mukesh Kumar Chaurasiya , Asahi Lina , Daniel Almeida , Greg Kroah-Hartman X-Mailman-Version: 3.3.8 Precedence: list List-Id: Nouveau development list Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: In order to do this, we need to be careful to ensure that any interface we expose for scatterlists ensures that any mappings created from one are destroyed on driver-unbind. To do this, we introduce a Devres resource into shmem::Object that we use in order to ensure that we release any SGTable mappings on driver-unbind. There's some other slightly unfortunate caveats of this: * Drivers don't have explicit control at the moment over when unmapping happens (which is exactly the same as the C side atm, so it might not be a problem). * We can't just return `SGTableMap` to the user through an Arc to attempt to fix the last caveat - because that implies the gem object would need to hold a reference count to the scatterlist mapping, which just leaves us with the same problem. Signed-off-by: Lyude Paul Reviewed-by: Alexandre Courbot Acked-by: Danilo Krummrich --- V3: * Rename OwnedSGTable to shmem::SGTable. Since the current version of the SGTable abstractions now has a `Owned` and `Borrowed` variant, I think renaming this to shmem::SGTable makes things less confusing. We do however, keep the name of owned_sg_table() as-is. V4: * Clarify safety comments for SGTable to explain why the object is thread-safe. * Rename from SGTableRef to SGTable V10: * Use Devres in order to ensure that SGTables are revocable, and are unmapped on driver-unbind. V11: * s/create_sg_table()/get_sg_table() * Get rid of extraneous `ret = ` in shmem::Object::get_sg_table() V12: * Actually move sgt_res in this patch and not the next one V13: * Use DmaResvGuard suggestion from Alexander * Use Alexander's (much better) solution for get_sg_table() * Use SetOnce instead of UnsafeCell * s/SGTableRef/SGTableMap * Fix typo in SGTableMap documentation * Create fallible constructor for SGTableMap * Don't reuse dma_resv lock for protecting Object contents, just use Mutex + SetOnce * Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to hold the dma_resv lock ourselves for anything but this function. * Check that the device we receive in the bounds for sg_table() and owned_sg_table() that said Device is in fact, the correct device. * Remove redundant docs in owned_sg_table(), just point it back to sg_table(). * Implement Deborah's suggestion to fix double-free in free_callback() * Restore original order of Object * Fix doc typo for SGTableMap V14: * Use new InitOnce container over the Mutex/SetOnce horror show we had before. * Start using LazyInit container for storing Devres for sgt unmap * Add some kunit tests for sg_table (not sure why I didn't do this before) using some of the boilerplate code leftover from the vmap bindings * Get rid of the owned SGTable variant for now, we'll add it back in a future patch if people actually need it. * Use new LazyInit container from me to get rid of the horrid Mutex> mess. * Add the best we can do for unit tests w/r/t SGTable at the moment V16: * Get rid of LazyInit, go back to SetOnce, use trick that Alice recommended that is a lot cleaner. * Fix horrid rebasing mistake V17: * Rebase * Fix missing safety comment in free_callback() (we forgot to justify why &mut is safe in `unsafe { &mut (*this).sgt_res }.reset()`) V18: * Use ManuallyDrop instead of SetOnce::reset() V19: * Explain that populate() will always return true in sg_table() * Fix outdated comment in SGTableMap * Fix invariant comment in SGTableMap rust/kernel/drm/gem/shmem.rs | 174 +++++++++++++++++++++++++++++++++-- 1 file changed, 164 insertions(+), 10 deletions(-) diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index a38c98add3d17..3ee19ef6264e7 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -11,6 +11,11 @@ use crate::{ container_of, + device::{ + self, + Bound, // + }, + devres::*, drm::{ driver, gem, @@ -19,14 +24,23 @@ DeviceContext, Registered, // }, - error::to_result, + error::{ + from_err_ptr, + to_result, // + }, io::{ Io, IoCapable, IoKnownSize, // }, prelude::*, - sync::aref::ARef, + scatterlist, + sync::{ + aref::ARef, + new_mutex, + Mutex, + SetOnce, // + }, types::{ NotThreadSafe, Opaque, // @@ -35,7 +49,10 @@ use core::{ ffi::c_void, marker::PhantomData, - mem::MaybeUninit, // + mem::{ + ManuallyDrop, + MaybeUninit, // + }, ops::{ Deref, DerefMut, // @@ -90,6 +107,11 @@ pub struct Object { obj: Opaque, /// Parent object that owns this object's DMA reservation object. parent_resv_obj: Option>>, + /// Devres object for unmapping any SGTable on driver-unbind. + sgt_res: ManuallyDrop>>>, + #[pin] + /// Lock for protecting initialization of `sgt_res`. + sgt_lock: Mutex<()>, #[pin] inner: T, _ctx: PhantomData, @@ -148,6 +170,8 @@ pub fn new( try_pin_init!(Self { obj <- Opaque::init_zeroed(), parent_resv_obj: config.parent_resv_obj.map(|p| p.into()), + sgt_res: ManuallyDrop::new(SetOnce::new()), + sgt_lock <- new_mutex!(()), inner <- T::new(dev, size, args), _ctx: PhantomData::, }), @@ -192,18 +216,26 @@ 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` - 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 drop `sgt_res` first, since doing so requires that the GEM object is still + // alive. + // SAFETY: + // - We verified above that `this` is valid. + // - We are in free_callback, guaranteeing we have exclusive access to `this` and that + // `sgt_res` will not be used after dropping it here. + unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) }; + + // 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) }; @@ -281,6 +313,46 @@ pub fn vmap(&self) -> Result> { pub fn owned_vmap(&self) -> Result> { 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, + ) -> Result<&'a scatterlist::SGTable> { + if dev.as_raw() != self.dev().as_ref().as_raw() { + return Err(EINVAL); + } + + let sgt_res = 'out: { + // Fast path: sgt_res is already initialized + if let Some(sgt_res) = self.sgt_res.as_ref() { + break 'out sgt_res; + } + + // Slow path: Grab the lock and see if we need to initialize sgt_res. + let _guard = self.sgt_lock.lock(); + + // If someone initialized it while we were waiting, we can exit early. + if let Some(sgt_res) = self.sgt_res.as_ref() { + break 'out sgt_res; + } + + // If not, finish initializing and return. `populate()` cannot return false, as + // `sgt_res` must be unpopulated, and we must hold `sgt_lock` to reach this point. + self.sgt_res + .populate(Devres::new(dev, SGTableMap::new(self))?); + + // SAFETY: We just populated sgt_res above. + unsafe { self.sgt_res.as_ref().unwrap_unchecked() } + }; + + Ok(sgt_res.access(dev)?) + } } impl Deref for Object { @@ -499,6 +571,64 @@ unsafe fn io_write(&self, value: $ty, address: usize) { #[cfg(CONFIG_64BIT)] impl_vmap_io_capable!(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_res` has an initialized and valid pointer to an +/// [`SGTable`]. +/// +/// [`SGTable`]: scatterlist::SGTable +pub struct SGTableMap { + obj: NonNull>, +} + +impl Deref for SGTableMap { + 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 Drop for SGTableMap { + 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 SGTableMap { + fn new(obj: &Object) -> impl Init { + // INVARIANT: + // - We call drm_gem_shmem_get_pages_sgt 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 Send for SGTableMap {} +// 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 Sync for SGTableMap {} + #[kunit_tests(rust_drm_gem_shmem)] mod tests { use super::*; @@ -614,4 +744,28 @@ fn vmap_io() -> Result { Ok(()) } + + // TODO: I would love to actually test the success paths of sg_table(), but that would require + // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave + // that for someone else. + + // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it + // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device + // bound to it. + #[test] + fn fail_sg_table_on_wrong_dev() -> Result { + let (_dev, drm) = create_drm_dev()?; + let wrong_dev = faux::Registration::new(c"EvilKunit", None)?; + + let obj = Object::::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?; + + assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL); + + // If sgt_res was not initialized mistakenly with the wrong device, this should still fail. + assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL); + + // TODO: Someday, we should test that creating an sg_table here still succeeds. + + Ok(()) + } } -- 2.54.0