[PATCH v4 1/2] rust: drm: gpuvm: require Send + Sync for the driver's associated data

[PATCH v4 1/2] rust: drm: gpuvm: require Send + Sync for the driver's associated data

[Sami Tolvanen]

DriverGpuVm permitted !Send/!Sync associated data on an abstraction whose
handles are shared and dropped across threads: obtain() runs from many
threads and the VA API performs deferred cross-thread drops. That is
unsound.

Require Send + Sync on the trait and its associated data so the GpuVm and
UniqueRefGpuVm handle impls need no per-impl bounds.

Fixes: 82b78182eacf ("rust: drm: add base GPUVM immediate mode abstraction")
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 rust/kernel/drm/gpuvm/mod.rs | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/rust/kernel/drm/gpuvm/mod.rs b/rust/kernel/drm/gpuvm/mod.rs
index a625fcd9b5f2..70cf11346cee 100644
--- a/rust/kernel/drm/gpuvm/mod.rs
+++ b/rust/kernel/drm/gpuvm/mod.rs
@@ -72,10 +72,12 @@ pub struct GpuVm<T: DriverGpuVm> {
     data: UnsafeCell<T>,
 }
 
-// SAFETY: The GPUVM api does not assume that it is tied to a specific thread. The destructor will
-// drop the `data` field, which is okay because it is guaranteed `Send` by the `DriverGpuVm` trait.
+// SAFETY: It is safe to send a `GpuVm<T>` to another thread: all data reachable through it
+// (`T`, `T::VmBoData`, and the GEM `T::Object`) is `Send` by the `DriverGpuVm` bounds.
 unsafe impl<T: DriverGpuVm> Send for GpuVm<T> {}
-// SAFETY: The GPUVM api is designed to allow &self methods to be called in parallel.
+// SAFETY: It is safe to share a `&GpuVm<T>` between threads: `&self` methods only alias data
+// that is `Sync` by the `DriverGpuVm` bounds, and any thread may drop that data, or upgrade the
+// reference and ultimately drop `T`, which the same bounds make `Send`.
 unsafe impl<T: DriverGpuVm> Sync for GpuVm<T> {}
 
 // SAFETY: By type invariants, the allocation is managed by the refcount in `self.vm`.
@@ -250,18 +252,22 @@ fn raw_resv(&self) -> *mut bindings::dma_resv {
 }
 
 /// The manager for a GPUVM.
-pub trait DriverGpuVm: Sized + Send {
+pub trait DriverGpuVm: Sized + Send + Sync {
     /// Parent `Driver` for this object.
     type Driver: drm::Driver;
 
     /// The kind of GEM object stored in this GPUVM.
-    type Object: drm::driver::AllocImpl<Driver = Self::Driver>;
+    type Object: drm::driver::AllocImpl<Driver = Self::Driver> + Send + Sync;
 
     /// Data stored with each [`struct drm_gpuva`](struct@GpuVa).
-    type VaData;
+    ///
+    /// Only `Send` is required: the data has a single owner at all times, moving
+    /// between threads by value (handed back as a [`GpuVaRemoved`]) but never
+    /// accessed by two threads concurrently.
+    type VaData: Send;
 
     /// Data stored with each [`struct drm_gpuvm_bo`](struct@GpuVmBo).
-    type VmBoData;
+    type VmBoData: Send + Sync;
 
     /// The private data passed to callbacks.
     type SmContext<'ctx>;
@@ -296,12 +302,10 @@ fn sm_step_remap<'op, 'ctx>(
 /// # Invariants
 ///
 /// Each `GpuVm` instance has at most one `UniqueRefGpuVm` reference.
+// `Send`/`Sync` derive from `ARef<GpuVm<T>>`; the trait bounds make them correct for the unique
+// handle's `&mut T` access.
 pub struct UniqueRefGpuVm<T: DriverGpuVm>(ARef<GpuVm<T>>);
 
-// SAFETY: The GPUVM api is designed to allow &self methods to be called in parallel, and
-// concurrent access to `data` is safe due to the `T: Sync` requirement.
-unsafe impl<T: DriverGpuVm + Sync> Sync for UniqueRefGpuVm<T> {}
-
 impl<T: DriverGpuVm> UniqueRefGpuVm<T> {
     /// Access the data owned by this `UniqueRefGpuVm` immutably.
     #[inline]

-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v4 2/2] rust: drm: gpuvm: implement Send and Sync for GpuVaAlloc and GpuVmBo

[Sami Tolvanen]

Moving a GpuVaAlloc or GpuVmBo between threads currently forces drivers
to write their own unsafe Send and Sync impls. Provide the markers in
the abstraction instead.

GpuVaAlloc wraps only uninitialised memory and exposes none of it.
GpuVmBo hands out the driver data and GEM object by shared reference and
drops them in its deferred put; the DriverGpuVm trait already guarantees
both are Send + Sync, so both impls are unconditional.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 rust/kernel/drm/gpuvm/va.rs    | 8 ++++++++
 rust/kernel/drm/gpuvm/vm_bo.rs | 9 +++++++++
 2 files changed, 17 insertions(+)

diff --git a/rust/kernel/drm/gpuvm/va.rs b/rust/kernel/drm/gpuvm/va.rs
index 0b09fe44ab39..b108ec7aa1bc 100644
--- a/rust/kernel/drm/gpuvm/va.rs
+++ b/rust/kernel/drm/gpuvm/va.rs
@@ -104,6 +104,14 @@ pub fn vm_bo(&self) -> &GpuVmBo<T> {
 /// The memory is zeroed.
 pub struct GpuVaAlloc<T: DriverGpuVm>(KBox<MaybeUninit<GpuVa<T>>>);
 
+// SAFETY: A `GpuVaAlloc` is an owned, uninitialised allocation with no live `T::VaData` and no
+// thread-bound state.
+unsafe impl<T: DriverGpuVm> Send for GpuVaAlloc<T> {}
+
+// SAFETY: A `GpuVaAlloc` has no `&self` method that reaches its contents, so a shared
+// `&GpuVaAlloc` cannot access the allocation.
+unsafe impl<T: DriverGpuVm> Sync for GpuVaAlloc<T> {}
+
 impl<T: DriverGpuVm> GpuVaAlloc<T> {
     /// Pre-allocate a [`GpuVa`] object.
     pub fn new(flags: AllocFlags) -> Result<GpuVaAlloc<T>, AllocError> {
diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs
index c064ac63897b..a30f838c11b8 100644
--- a/rust/kernel/drm/gpuvm/vm_bo.rs
+++ b/rust/kernel/drm/gpuvm/vm_bo.rs
@@ -19,6 +19,15 @@ pub struct GpuVmBo<T: DriverGpuVm> {
     data: T::VmBoData,
 }
 
+// SAFETY: It is safe to send a `GpuVmBo<T>` to another thread: dropping it there drops
+// `T::VmBoData` and the GEM `T::Object`, both `Send` by the `DriverGpuVm` bounds.
+unsafe impl<T: DriverGpuVm> Send for GpuVmBo<T> {}
+
+// SAFETY: It is safe to share a `&GpuVmBo<T>` between threads: it effectively shares
+// `&T::VmBoData` and the GEM `&T::Object` (both `Sync`), and any thread may upgrade to an
+// `ARef` and ultimately drop them (both `Send`), per the `DriverGpuVm` bounds.
+unsafe impl<T: DriverGpuVm> Sync for GpuVmBo<T> {}
+
 // SAFETY: By type invariants, the allocation is managed by the refcount in `self.inner`.
 unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
     fn inc_ref(&self) {

-- 
2.54.0.1136.gdb2ca164c4-goog