[PATCH v2 0/2] Add a Rust GPUVM abstraction

[PATCH v2 0/2] Add a Rust GPUVM abstraction

[Daniel Almeida]

Changes from v1:
- Added more people to cc.
- Used Benno's suggestion to remove the #[allow] (thanks, Benno, that is
  indeed better!)
- Added markdown as applicable in the docs/comments (please let me know
  whether I missed any)
- Changed the order between docs and attributes to match the rest of the
  code in the Rust crate.
- Did *not* remove the # Arguments section from a few functions yet, let's
  push this dicussion down the line, Miguel.
- Changed the placement of OpRemap, i.e.: it was not next to its impl block by
  mistake.
- Misc doc fixes, i.e. missing periods, missing ///, lines that would
  not render properly on the browser, etc (thanks, Miguel)
- Removed spurious clang-format change from patch 1.
- Added a return statement for find_bo/obtain_bo (thanks, Charalampos!)

Changes from v0:
  This version changes how `LockedGpuVm` operates. The previous code assumed
that the interval tree would be protected if all the associated GEM's resvs
were locked, but this is completely unrelated. In fact, this initial version
only aims to support the core VA range management feature of GPUVM, and not any
of the "convenience" functions like "exec_lock()" and friends, so this function
was removed accordingly.
  
  LockedGpuVm is now produced by a call to GpuVm::lock(). This takes a generic
guard type to signal that the driver-specific locks have been acquired in order
to protect the VMs tree, and hence its view of the address space. This approach
is somewhat similar to CondVar, but also incurs in the same pitfall: it is up
to the caller to give meaning to the guard by ensuring that it actually
protects against concurrent access to the VM. Maybe this is a good candidate to
the upcoming "Pitfall" docs section?

  Note that LockedGpuVm::obj was removed. I'm not really sure why this field
was there in the first place, but callers should indicate the object through
the sm_map() and sm_unmap() APIs instead.

  I am not sure why GpuVm::inner uses UnsafeCell, though. I did not touch this
so that we can first discuss whether UnsafeCell is really needed.

  The docs were also updated. Care was taken to reuse the C documentation as
much as possible.

  Itemized changes: 

- Rebased on top of nova-drm
  - Use `srctree` in the docs
  - Add docs as appropriate and remove #[allow(missing_docs)]
  - Remove `impl DriverGpuVa for ()`. Drivers can to that themselves, if they want.
  - Added #[inline] to getters, as Rust cannot inline across crates otherwise (unless this changed recently?)
  - Exposed `drm_gpuva_op_unmap::keep`.
  - Removed `pub(super)` from unsafe fns meant to be called from the C code. 
  - Added "# Safety" blocks to the above.
  - Removed `exec_lock_gem_object()`.
  - Removed `exec_lock()`
  - Removed `LockedGpuVm::vm_exec`. This initial version does not support `exec` calls at all.
  - By the same token, removed `LockedGpuVm::drop()`
  - Removed `LockedGpuVm::obj`. This object has to be passed explicitly.

---
Asahi Lina (2):
      rust: helpers: Add bindings/wrappers for dma_resv
      rust: drm: Add GPUVM abstraction

 rust/bindings/bindings_helper.h |   2 +
 rust/helpers/dma-resv.c         |  13 +
 rust/helpers/drm_gpuvm.c        |  29 ++
 rust/helpers/helpers.c          |   2 +
 rust/kernel/drm/gpuvm.rs        | 807 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs          |   2 +
 6 files changed, 855 insertions(+)
---
base-commit: e7de41cc4b01dd5500a0c2533c64bdb3f5360567
change-id: 20250320-gpuvm-29d3e0726f34

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

[PATCH v2 1/2] rust: helpers: Add bindings/wrappers for dma_resv

[Daniel Almeida]

From: Asahi Lina <lina@asahilina.net>

This is just for basic usage in the DRM shmem abstractions for implied
locking, not intended as a full DMA Reservation abstraction yet.

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers/dma-resv.c         | 13 +++++++++++++
 rust/helpers/helpers.c          |  1 +
 3 files changed, 15 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index e6020ba5b00237a08402fbd609c7fba27b970dd9..68d7be498a6d523797e54212d6c23ff4d8f2e92d 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -18,6 +18,7 @@
 #include <linux/blkdev.h>
 #include <linux/cred.h>
 #include <linux/device/faux.h>
+#include <linux/dma-resv.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
 #include <linux/file.h>
diff --git a/rust/helpers/dma-resv.c b/rust/helpers/dma-resv.c
new file mode 100644
index 0000000000000000000000000000000000000000..05501cb814513b483afd0b7f220230d867863c2f
--- /dev/null
+++ b/rust/helpers/dma-resv.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-resv.h>
+
+int rust_helper_dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx)
+{
+	return dma_resv_lock(obj, ctx);
+}
+
+void rust_helper_dma_resv_unlock(struct dma_resv *obj)
+{
+	dma_resv_unlock(obj);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7a06d6bc48537248c8a3ec4243b37d8fb2b1cb26..c5e536d688bc35c7b348daa61e868c91a7bdbd23 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -14,6 +14,7 @@
 #include "build_bug.c"
 #include "cred.c"
 #include "device.c"
+#include "dma-resv.c"
 #include "drm.c"
 #include "err.c"
 #include "fs.c"

-- 
2.49.0

[PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Daniel Almeida]

From: Asahi Lina <lina@asahilina.net>

Add a GPUVM abstraction to be used by Rust GPU drivers.

GPUVM keeps track of a GPU's virtual address (VA) space and manages the
corresponding virtual mappings represented by "GPU VA" objects. It also
keeps track of the mapping's backing GEM buffers.

This initial version only support synchronous operations. In other words,
we do not support the use case where the operations are pre-built and
handed over to the driver.

We also do not support using a driver-specific lock in order to serialize
the gpuva list for a given GEM object. This has to be the GEM object resv,
which is the default behavior in C.

Similarly to the C code, locking is left to the driver. This means that the
driver should make sure that the VM's interval tree is protected against
concurrent access. In Rust, we encode this requirement by requiring a
generic Guard type in GpuVm::lock(), which is a similar approach as the one
taken by CondVar.

It is up to drivers to make sure that the guard indeed provides the
required locking. Any operations that modifies the interval tree is only
available after the VM has been locked as per above.

Signed-off-by: Asahi Lina <lina@asahilina.net>
Co-developed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/drm_gpuvm.c        |  29 ++
 rust/helpers/helpers.c          |   1 +
 rust/kernel/drm/gpuvm.rs        | 807 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs          |   2 +
 5 files changed, 840 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 68d7be498a6d523797e54212d6c23ff4d8f2e92d..68502c525fd74dcfdaf1b48f08ff0a234186f041 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_gpuvm.h>
 #include <drm/drm_ioctl.h>
 #include <kunit/test.h>
 #include <linux/auxiliary_bus.h>
diff --git a/rust/helpers/drm_gpuvm.c b/rust/helpers/drm_gpuvm.c
new file mode 100644
index 0000000000000000000000000000000000000000..7a074d2c2160ebc5f92909236c5aaecb1853e45d
--- /dev/null
+++ b/rust/helpers/drm_gpuvm.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+#include <drm/drm_gpuvm.h>
+
+#ifdef CONFIG_DRM
+#ifdef CONFIG_DRM_GPUVM
+
+struct drm_gpuvm *rust_helper_drm_gpuvm_get(struct drm_gpuvm *obj)
+{
+	return drm_gpuvm_get(obj);
+}
+
+void rust_helper_drm_gpuva_init_from_op(struct drm_gpuva *va, struct drm_gpuva_op_map *op)
+{
+	drm_gpuva_init_from_op(va, op);
+}
+
+struct drm_gpuvm_bo *rust_helper_drm_gpuvm_bo_get(struct drm_gpuvm_bo *vm_bo)
+{
+	return drm_gpuvm_bo_get(vm_bo);
+}
+
+bool rust_helper_drm_gpuvm_is_extobj(struct drm_gpuvm *gpuvm, struct drm_gem_object *obj)
+{
+	return drm_gpuvm_is_extobj(gpuvm, obj);
+}
+
+#endif /* CONFIG_DRM_GPUVM */
+#endif /* CONFIG_DRM */
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index c5e536d688bc35c7b348daa61e868c91a7bdbd23..a013cc91020ae5094a04a2e4b93993cf0b149885 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -16,6 +16,7 @@
 #include "device.c"
 #include "dma-resv.c"
 #include "drm.c"
+#include "drm_gpuvm.c"
 #include "err.c"
 #include "fs.c"
 #include "io.c"
diff --git a/rust/kernel/drm/gpuvm.rs b/rust/kernel/drm/gpuvm.rs
new file mode 100644
index 0000000000000000000000000000000000000000..7b9539554d4f4408919c4bfc2d2f5ae8879893f9
--- /dev/null
+++ b/rust/kernel/drm/gpuvm.rs
@@ -0,0 +1,807 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DRM GPU VA Manager abstractions.
+//!
+//! Only synchronous operations are supported, and the GEM's `dma_resv` is used
+//! as the GEM's gpuva lock.
+//!
+//! C header: [`include/drm/drm_gpuvm.h`](srctree/include/drm/drm_gpuvm.h)
+
+use core::cell::UnsafeCell;
+use core::marker::{PhantomData, PhantomPinned};
+use core::ops::{Deref, DerefMut, Range};
+use core::ptr::NonNull;
+
+use crate::bindings;
+use crate::drm::device;
+use crate::drm::drv;
+use crate::drm::gem::IntoGEMObject;
+use crate::error::code::ENOMEM;
+use crate::error::from_result;
+use crate::error::to_result;
+use crate::error::Result;
+use crate::init;
+use crate::init::pin_init_from_closure;
+use crate::prelude::*;
+use crate::sync::lock::Backend;
+use crate::sync::lock::Guard;
+use crate::types::ARef;
+use crate::types::AlwaysRefCounted;
+use crate::types::Opaque;
+
+// SAFETY: This type is safe to zero-initialize.
+unsafe impl init::Zeroable for bindings::drm_gpuvm_bo {}
+
+/// A convenience type for the driver's GEM object.
+type DriverObject<T> = <<T as DriverGpuVm>::Driver as drv::Driver>::Object;
+
+/// Trait that must be implemented by DRM drivers to represent a DRM [`GpuVm`],
+/// i.e. a GPU address space.
+pub trait DriverGpuVm: Sized {
+    /// The parent [`drv::Driver`] implementation for this [`DriverGpuVm`].
+    type Driver: drv::Driver;
+
+    /// The driver-specific GpuVa type, repesenting a single VA range within the
+    /// GPU.
+    type GpuVa: DriverGpuVa;
+
+    /// The driver-specific GpuVmBo type, representing the connection between a
+    /// GEM object and [`GpuVm`].
+    type GpuVmBo: DriverGpuVmBo;
+
+    /// The driver-specific context that is kept through the map, unmap and
+    /// remap steps.
+    type StepContext;
+
+    /// Implements the map step for the driver.
+    fn step_map(
+        self: &mut UpdatingGpuVm<'_, Self>,
+        op: &mut OpMap<Self>,
+        ctx: &mut Self::StepContext,
+    ) -> Result;
+
+    /// Implements the unmap step for the driver.
+    fn step_unmap(
+        self: &mut UpdatingGpuVm<'_, Self>,
+        op: &mut OpUnMap<Self>,
+        ctx: &mut Self::StepContext,
+    ) -> Result;
+
+    /// Implements the remap step for the driver.
+    fn step_remap(
+        self: &mut UpdatingGpuVm<'_, Self>,
+        op: &mut OpReMap<Self>,
+        vm_bo: &GpuVmBo<Self>,
+        ctx: &mut Self::StepContext,
+    ) -> Result;
+}
+
+/// The driver-specific context that is passed through the map, unmap and remap
+/// steps.
+struct StepContext<'a, T: DriverGpuVm> {
+    gpuvm: &'a GpuVm<T>,
+    ctx: &'a mut T::StepContext,
+}
+
+/// Trait that must be implemented by DRM drivers to represent a DRM [`GpuVa`],
+/// i.e.: a mapping in GPU address space.
+pub trait DriverGpuVa: Sized {}
+
+/// Trait that must be implemented by DRM drivers to represent a DRM
+/// [`GpuVmBo`], i.e.: a connection between a BO and a VM.
+pub trait DriverGpuVmBo: Sized {
+    /// Initializes the GpuVmBo object.
+    fn new() -> impl PinInit<Self>;
+}
+
+/// Provide a default implementation for trivial types
+impl<T: Default> DriverGpuVmBo for T {
+    fn new() -> impl PinInit<Self> {
+        // SAFETY:
+        // - `Ok(())` is always returned.
+        // - The slot is never moved.
+        unsafe {
+            pin_init_from_closure(|slot| {
+                *slot = Self::default();
+                Ok(())
+            })
+        }
+    }
+}
+
+/// A transparent wrapper over
+/// [`bindings::drm_gpuva_op_map`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_map)
+///
+/// Represents the map operation to be carried out by the driver.
+#[repr(transparent)]
+pub struct OpMap<T: DriverGpuVm>(bindings::drm_gpuva_op_map, PhantomData<T>);
+
+impl<T: DriverGpuVm> OpMap<T> {
+    /// Returns the base address of the new mapping.
+    #[inline]
+    pub fn addr(&self) -> u64 {
+        self.0.va.addr
+    }
+
+    /// Returns the range of the new mapping.
+    #[inline]
+    pub fn range(&self) -> u64 {
+        self.0.va.range
+    }
+
+    /// Returns the offset within the GEM object.
+    #[inline]
+    pub fn offset(&self) -> u64 {
+        self.0.gem.offset
+    }
+
+    /// Returns the GEM object to map.
+    #[inline]
+    pub fn object(&self) -> &<T::Driver as drv::Driver>::Object {
+        let p = <<T::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(self.0.gem.obj);
+        // SAFETY: The GEM object has an active reference for the lifetime of this op
+        unsafe { &*p }
+    }
+
+    /// A helper function to map and link a [ `GpuVa`] to a [`GpuVmBo`].
+    pub fn map_and_link_va(
+        &mut self,
+        gpuvm: &mut UpdatingGpuVm<'_, T>,
+        gpuva: Pin<KBox<GpuVa<T>>>,
+        gpuvmbo: &ARef<GpuVmBo<T>>,
+    ) -> Result<(), Pin<KBox<GpuVa<T>>>> {
+        // SAFETY: We are handing off the GpuVa ownership and it will not be moved.
+        let p = KBox::leak(unsafe { Pin::into_inner_unchecked(gpuva) });
+        // SAFETY: These C functions are called with the correct invariants
+        unsafe {
+            bindings::drm_gpuva_init_from_op(&mut p.gpuva, &mut self.0);
+            if bindings::drm_gpuva_insert(gpuvm.0.gpuvm() as *mut _, &mut p.gpuva) != 0 {
+                // EEXIST, return the GpuVa to the caller as an error
+                return Err(Pin::new_unchecked(KBox::from_raw(p)));
+            };
+            // SAFETY: This takes a new reference to the gpuvmbo.
+            bindings::drm_gpuva_link(&mut p.gpuva, &gpuvmbo.bo as *const _ as *mut _);
+        }
+        Ok(())
+    }
+}
+
+/// A transparent wrapper over
+/// [`bindings::drm_gpuva_op_remap`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_remap).
+///
+/// Represents a single remap operation generated by [`GpuVm`].
+///
+/// A remap operation is generated when an existing [`GpuVa`] mapping is split
+/// by inserting a new one or by partially unmapping existing mappings. Hence,
+/// it consists of a maximum of two maps and one unmap operation,
+///
+/// The unmap operation takes care of removing the original existing mapping.
+/// The `prev` field is used to remap the preceding part and `next` is used to
+/// remap the subsequent part.
+///
+/// If the start address of the new mapping aligns with the start address of the
+/// old mapping, `prev` will be `None`. Similarly, if the end address of the new
+/// mapping aligns with the end address of the old mapping, `next` will be
+/// `None`.
+///
+/// Note: the reason for a dedicated remap operation, rather than arbitrary
+/// unmap and map operations, is to give drivers the chance of extracting driver
+/// specific data for creating the new mappings from the unmap operations's
+/// [`GpuVa`] structure which typically is embedded in larger driver specific
+/// structures.
+#[repr(transparent)]
+pub struct OpReMap<T: DriverGpuVm>(bindings::drm_gpuva_op_remap, PhantomData<T>);
+
+/// A transparent wrapper over
+/// [`bindings::drm_gpuva_op_unmap`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_unmap).
+///
+/// Represents a single unmap operation generated by [`GpuVm`].
+#[repr(transparent)]
+pub struct OpUnMap<T: DriverGpuVm>(bindings::drm_gpuva_op_unmap, PhantomData<T>);
+
+impl<T: DriverGpuVm> OpUnMap<T> {
+    /// Returns the GPU VA to unmap.
+    #[inline]
+    pub fn va(&self) -> Option<&GpuVa<T>> {
+        if self.0.va.is_null() {
+            return None;
+        }
+        // SAFETY: Container invariant is guaranteed for ops structs created for our types.
+        let p = unsafe { crate::container_of!(self.0.va, GpuVa<T>, gpuva) as *mut GpuVa<T> };
+        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
+        Some(unsafe { &*p })
+    }
+
+    /// Indicates whether this [`GpuVa`] is physically contiguous with the
+    /// original mapping request.
+    ///
+    /// Optionally, if `keep` is set, drivers may keep the actual page table
+    /// mappings for this [`GpuVa`], adding the missing page table entries only and
+    /// subsequently updating the VM accordingly.
+    #[inline]
+    pub fn keep(&self) -> bool {
+        self.0.keep
+    }
+
+    /// A helper to unmap and unlink a [`GpuVa`] from a [`GpuVmBo`].
+    pub fn unmap_and_unlink_va(&mut self) -> Option<Pin<KBox<GpuVa<T>>>> {
+        if self.0.va.is_null() {
+            return None;
+        }
+        // SAFETY: Container invariant is guaranteed for ops structs created for our types.
+        let p = unsafe { crate::container_of!(self.0.va, GpuVa<T>, gpuva) as *mut GpuVa<T> };
+
+        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
+        unsafe {
+            bindings::drm_gpuva_unmap(&mut self.0);
+            bindings::drm_gpuva_unlink(self.0.va);
+        }
+
+        // Unlinking/unmapping relinquishes ownership of the GpuVa object,
+        // so clear the pointer
+        self.0.va = core::ptr::null_mut();
+        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
+        Some(unsafe { Pin::new_unchecked(KBox::from_raw(p)) })
+    }
+}
+
+impl<T: DriverGpuVm> OpReMap<T> {
+    /// Returns the preceding part of a split mapping, if any.
+    #[inline]
+    pub fn prev_map(&mut self) -> Option<&mut OpMap<T>> {
+        // SAFETY: The prev pointer must be valid if not-NULL per the op_remap contract
+        unsafe { (self.0.prev as *mut OpMap<T>).as_mut() }
+    }
+
+    /// Returns the subsequent part of a split mapping, if any.
+    #[inline]
+    pub fn next_map(&mut self) -> Option<&mut OpMap<T>> {
+        // SAFETY: The next pointer must be valid if not-NULL per the op_remap contract
+        unsafe { (self.0.next as *mut OpMap<T>).as_mut() }
+    }
+
+    /// Returns the unmap operation for the original existing mapping.
+    #[inline]
+    pub fn unmap(&mut self) -> &mut OpUnMap<T> {
+        // SAFETY: The unmap pointer is always valid per the op_remap contract
+        unsafe { (self.0.unmap as *mut OpUnMap<T>).as_mut().unwrap() }
+    }
+}
+
+/// A GPU VA range.
+///
+/// Drivers can use `inner` to store additional data.
+#[repr(C)]
+#[pin_data]
+pub struct GpuVa<T: DriverGpuVm> {
+    #[pin]
+    gpuva: bindings::drm_gpuva,
+    #[pin]
+    inner: T::GpuVa,
+    #[pin]
+    _p: PhantomPinned,
+}
+
+// SAFETY: This type is safe to zero-init (as far as C is concerned).
+unsafe impl init::Zeroable for bindings::drm_gpuva {}
+
+impl<T: DriverGpuVm> GpuVa<T> {
+    /// Creates a new GPU VA.
+    pub fn new<E>(inner: impl PinInit<T::GpuVa, E>) -> Result<Pin<KBox<GpuVa<T>>>>
+    where
+        Error: From<E>,
+    {
+        KBox::try_pin_init(
+            try_pin_init!(Self {
+                gpuva <- init::zeroed(),
+                inner <- inner,
+                _p: PhantomPinned
+            }),
+            GFP_KERNEL,
+        )
+    }
+
+    /// Returns the start address of the GPU VA range.
+    #[inline]
+    pub fn addr(&self) -> u64 {
+        self.gpuva.va.addr
+    }
+
+    /// Returns the range of the GPU VA.
+    #[inline]
+    pub fn range(&self) -> u64 {
+        self.gpuva.va.range
+    }
+
+    /// Returns the offset within the GEM object.
+    #[inline]
+    pub fn offset(&self) -> u64 {
+        self.gpuva.gem.offset
+    }
+}
+
+/// The connection between a GEM object and a VM.
+#[repr(C)]
+#[pin_data]
+pub struct GpuVmBo<T: DriverGpuVm> {
+    #[pin]
+    bo: bindings::drm_gpuvm_bo,
+    #[pin]
+    inner: T::GpuVmBo,
+    #[pin]
+    _p: PhantomPinned,
+}
+
+impl<T: DriverGpuVm> GpuVmBo<T> {
+    /// Return a reference to the inner driver data for this [`GpuVmBo`].
+    pub fn inner(&self) -> &T::GpuVmBo {
+        &self.inner
+    }
+}
+
+// SAFETY: DRM GpuVmBo objects are always reference counted and the get/put functions
+// satisfy the requirements.
+unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
+    fn inc_ref(&self) {
+        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
+        unsafe { bindings::drm_gpuvm_bo_get(&self.bo as *const _ as *mut _) };
+    }
+
+    unsafe fn dec_ref(mut obj: NonNull<Self>) {
+        // SAFETY: drm_gpuvm_bo_put() requires holding the gpuva lock, which is
+        // the dma_resv lock by default.
+        // The drm_gpuvm_put function satisfies the requirements for dec_ref().
+        // (We do not support custom locks yet.)
+        unsafe {
+            let resv = (*obj.as_mut().bo.obj).resv;
+            bindings::dma_resv_lock(resv, core::ptr::null_mut());
+            bindings::drm_gpuvm_bo_put(&mut obj.as_mut().bo);
+            bindings::dma_resv_unlock(resv);
+        }
+    }
+}
+
+/// The DRM GPU VA Manager.
+///
+/// It keeps track of a GPU's virtual address space by using maple tree
+/// structures.
+///
+/// Typically, an instance of [`GpuVm`] is embedded bigger, driver-specific
+/// structures.
+///
+/// Drivers can pass addresses and ranges in arbitrary units, e.g.: bytes or
+/// pages.
+///
+/// There should be one manager instance per GPU virtual address space.
+#[repr(C)]
+#[pin_data]
+pub struct GpuVm<T: DriverGpuVm> {
+    #[pin]
+    gpuvm: Opaque<bindings::drm_gpuvm>,
+    #[pin]
+    inner: UnsafeCell<T>,
+    #[pin]
+    _p: PhantomPinned,
+}
+
+/// # Safety
+///
+/// This function is only safe to be called from the GPUVM C code.
+unsafe extern "C" fn vm_free_callback<T: DriverGpuVm>(raw_gpuvm: *mut bindings::drm_gpuvm) {
+    // SAFETY: Container invariant is guaranteed for objects using our callback.
+    let p = unsafe {
+        crate::container_of!(
+            raw_gpuvm as *mut Opaque<bindings::drm_gpuvm>,
+            GpuVm<T>,
+            gpuvm
+        ) as *mut GpuVm<T>
+    };
+
+    // SAFETY: p is guaranteed to be valid for drm_gpuvm objects using this callback.
+    unsafe { drop(KBox::from_raw(p)) };
+}
+
+/// # Safety
+///
+/// This function is only safe to be called from the GPUVM C code.
+unsafe extern "C" fn vm_bo_alloc_callback<T: DriverGpuVm>() -> *mut bindings::drm_gpuvm_bo {
+    let obj: Result<Pin<KBox<GpuVmBo<T>>>> = KBox::try_pin_init(
+        try_pin_init!(GpuVmBo::<T> {
+            bo <- init::zeroed(),
+            inner <- T::GpuVmBo::new(),
+            _p: PhantomPinned
+        }),
+        GFP_KERNEL,
+    );
+
+    match obj {
+        Ok(obj) =>
+        // SAFETY: The DRM core will keep this object pinned.
+        unsafe {
+            let p = KBox::leak(Pin::into_inner_unchecked(obj));
+            &mut p.bo
+        },
+        Err(_) => core::ptr::null_mut(),
+    }
+}
+
+/// # Safety
+///
+/// This function is only safe to be called from the GPUVM C code.
+unsafe extern "C" fn vm_bo_free_callback<T: DriverGpuVm>(raw_vm_bo: *mut bindings::drm_gpuvm_bo) {
+    // SAFETY: Container invariant is guaranteed for objects using this callback.
+    let p = unsafe { crate::container_of!(raw_vm_bo, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
+
+    // SAFETY: p is guaranteed to be valid for drm_gpuvm_bo objects using this callback.
+    unsafe { drop(KBox::from_raw(p)) };
+}
+
+/// # Safety
+///
+/// This function is only safe to be called from the GPUVM C code.
+unsafe extern "C" fn step_map_callback<T: DriverGpuVm>(
+    op: *mut bindings::drm_gpuva_op,
+    _priv: *mut core::ffi::c_void,
+) -> core::ffi::c_int {
+    // SAFETY: We know this is a map op, and OpMap is a transparent wrapper.
+    let map = unsafe { &mut *((&mut (*op).__bindgen_anon_1.map) as *mut _ as *mut OpMap<T>) };
+    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
+    // guaranteed to outlive this function.
+    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
+
+    from_result(|| {
+        UpdatingGpuVm(ctx.gpuvm).step_map(map, ctx.ctx)?;
+        Ok(0)
+    })
+}
+
+/// # Safety
+///
+/// This function is only safe to be called from the GPUVM C code.
+unsafe extern "C" fn step_remap_callback<T: DriverGpuVm>(
+    op: *mut bindings::drm_gpuva_op,
+    _priv: *mut core::ffi::c_void,
+) -> core::ffi::c_int {
+    // SAFETY: We know this is a map op, and OpReMap is a transparent wrapper.
+    let remap = unsafe { &mut *((&mut (*op).__bindgen_anon_1.remap) as *mut _ as *mut OpReMap<T>) };
+    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
+    // guaranteed to outlive this function.
+    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
+
+    let p_vm_bo = remap.unmap().va().unwrap().gpuva.vm_bo;
+
+    let res = {
+        // SAFETY: vm_bo pointer must be valid and non-null by the step_remap invariants.
+        // Since we grab a ref, this reference's lifetime is until the decref.
+        let vm_bo_ref = unsafe {
+            bindings::drm_gpuvm_bo_get(p_vm_bo);
+            &*(crate::container_of!(p_vm_bo, GpuVmBo<T>, bo) as *mut GpuVmBo<T>)
+        };
+
+        from_result(|| {
+            UpdatingGpuVm(ctx.gpuvm).step_remap(remap, vm_bo_ref, ctx.ctx)?;
+            Ok(0)
+        })
+    };
+
+    // SAFETY: We incremented the refcount above, and the Rust reference we took is
+    // no longer in scope.
+    unsafe { bindings::drm_gpuvm_bo_put(p_vm_bo) };
+
+    res
+}
+
+/// # Safety
+///
+/// This function is only safe to be called from the GPUVM C code.
+unsafe extern "C" fn step_unmap_callback<T: DriverGpuVm>(
+    op: *mut bindings::drm_gpuva_op,
+    _priv: *mut core::ffi::c_void,
+) -> core::ffi::c_int {
+    // SAFETY: We know this is a map op, and OpUnMap is a transparent wrapper.
+    let unmap = unsafe { &mut *((&mut (*op).__bindgen_anon_1.unmap) as *mut _ as *mut OpUnMap<T>) };
+    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
+    // guaranteed to outlive this function.
+    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
+
+    from_result(|| {
+        UpdatingGpuVm(ctx.gpuvm).step_unmap(unmap, ctx.ctx)?;
+        Ok(0)
+    })
+}
+
+impl<T: DriverGpuVm> GpuVm<T> {
+    const OPS: bindings::drm_gpuvm_ops = bindings::drm_gpuvm_ops {
+        vm_free: Some(vm_free_callback::<T>),
+        op_alloc: None,
+        op_free: None,
+        vm_bo_alloc: Some(vm_bo_alloc_callback::<T>),
+        vm_bo_free: Some(vm_bo_free_callback::<T>),
+        vm_bo_validate: None,
+        sm_step_map: Some(step_map_callback::<T>),
+        sm_step_remap: Some(step_remap_callback::<T>),
+        sm_step_unmap: Some(step_unmap_callback::<T>),
+    };
+
+    fn gpuvm(&self) -> *const bindings::drm_gpuvm {
+        self.gpuvm.get()
+    }
+
+    /// Creates a GPUVM instance.
+    pub fn new<E>(
+        name: &'static CStr,
+        dev: &device::Device<T::Driver>,
+        r_obj: &<T::Driver as drv::Driver>::Object,
+        range: Range<u64>,
+        reserve_range: Range<u64>,
+        inner: impl PinInit<T, E>,
+    ) -> Result<ARef<GpuVm<T>>>
+    where
+        Error: From<E>,
+    {
+        let obj: Pin<KBox<Self>> = KBox::try_pin_init(
+            try_pin_init!(Self {
+                // SAFETY: drm_gpuvm_init cannot fail and always initializes the member.
+                gpuvm <- unsafe {
+                    init::pin_init_from_closure(move |slot: *mut Opaque<bindings::drm_gpuvm> | {
+                        // Zero-init required by drm_gpuvm_init.
+                        *slot = core::mem::zeroed();
+                        bindings::drm_gpuvm_init(
+                            Opaque::raw_get(slot),
+                            name.as_char_ptr(),
+                            0,
+                            dev.as_raw(),
+                            r_obj.gem_obj() as *const _ as *mut _,
+                            range.start,
+                            range.end - range.start,
+                            reserve_range.start,
+                            reserve_range.end - reserve_range.start,
+                            &Self::OPS
+                        );
+                        Ok(())
+                    })
+                },
+                // SAFETY: Just passing through to the initializer argument.
+                inner <- unsafe {
+                    init::pin_init_from_closure(move |slot: *mut UnsafeCell<T> | {
+                        inner.__pinned_init(slot as *mut _)
+                    })
+                },
+                _p: PhantomPinned
+            }),
+            GFP_KERNEL,
+        )?;
+
+        // SAFETY: We never move out of the object.
+        let vm_ref = unsafe {
+            ARef::from_raw(NonNull::new_unchecked(KBox::leak(
+                Pin::into_inner_unchecked(obj),
+            )))
+        };
+
+        Ok(vm_ref)
+    }
+
+    /// Locks the VM, protecting its interval tree against concurrent accesses.
+    ///
+    /// Callers must prove that they have exclusive access to the VM by holding
+    /// some guard type. This encodes the driver-specific locking requirements.
+    ///
+    /// It is up to the caller to ensure that the guard indeed provides the
+    /// required locking.
+    pub fn lock<U: ?Sized, B: Backend>(&self, _guard: &mut Guard<'_, U, B>) -> LockedGpuVm<'_, T> {
+        LockedGpuVm { gpuvm: self }
+    }
+
+    /// Returns true if the given object is external to the GPUVM, i.e.: if it
+    /// does not share the DMA reservation object of the GPUVM.
+    pub fn is_extobj(&self, obj: &impl IntoGEMObject) -> bool {
+        let gem = obj.gem_obj() as *const _ as *mut _;
+        // SAFETY: This is safe to call as long as the arguments are valid pointers.
+        unsafe { bindings::drm_gpuvm_is_extobj(self.gpuvm() as *mut _, gem) }
+    }
+}
+
+// SAFETY: DRM GpuVm objects are always reference counted and the get/put functions
+// satisfy the requirements.
+unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVm<T> {
+    fn inc_ref(&self) {
+        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
+        unsafe { bindings::drm_gpuvm_get(&self.gpuvm as *const _ as *mut _) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The drm_gpuvm_put function satisfies the requirements for dec_ref().
+        unsafe { bindings::drm_gpuvm_put(Opaque::raw_get(&(*obj.as_ptr()).gpuvm)) };
+    }
+}
+
+/// The object returned after a call to [`GpuVm::lock`].
+///
+/// This object has access to operations that modify the VM's interval tree.
+pub struct LockedGpuVm<'a, T: DriverGpuVm> {
+    gpuvm: &'a GpuVm<T>,
+}
+
+impl<T: DriverGpuVm> LockedGpuVm<'_, T> {
+    /// Finds the [`GpuVmBo`] object that connects `obj` to this VM.
+    ///
+    /// If found, increases the reference count of the GpuVmBo object
+    /// accordingly.
+    pub fn find_bo(&mut self, obj: &DriverObject<T>) -> Option<ARef<GpuVmBo<T>>> {
+        // SAFETY: LockedGpuVm implies the right locks are held.
+        let p = unsafe {
+            bindings::drm_gpuvm_bo_find(
+                self.gpuvm.gpuvm() as *mut _,
+                obj.gem_obj() as *const _ as *mut _,
+            )
+        };
+
+        if p.is_null() {
+            return None;
+        }
+
+        // SAFETY: All the drm_gpuvm_bo objects in this GpuVm are always
+        // allocated by us as GpuVmBo<T>.
+        let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
+        // SAFETY: We checked for NULL above, and the types ensure that
+        // this object was created by vm_bo_alloc_callback<T>.
+        Some(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
+    }
+
+    /// Obtains the [`GpuVmBo`] object that connects `obj` to this VM.
+    ///
+    /// This connection is unique, so an instane of [`GpuVmBo`] will be
+    /// allocated for `obj` once, and that instance will be returned from that
+    /// point forward.
+    pub fn obtain_bo(&mut self, obj: &DriverObject<T>) -> Result<ARef<GpuVmBo<T>>> {
+        // SAFETY: LockedGpuVm implies the right locks are held.
+        let p = unsafe {
+            bindings::drm_gpuvm_bo_obtain(
+                self.gpuvm.gpuvm() as *mut _,
+                obj.gem_obj() as *const _ as *mut _,
+            )
+        };
+
+        if p.is_null() {
+            return Err(ENOMEM);
+        }
+
+        // SAFETY: Container invariant is guaranteed for GpuVmBo objects for this GpuVm.
+        let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
+        // SAFETY: We checked for NULL above, and the types ensure that
+        // this object was created by vm_bo_alloc_callback<T>.
+        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
+    }
+
+    /// Iterates the given range of the GPU VA space.
+    ///
+    /// This utilizes [`DriverGpuVm`] to call back into the driver providing the
+    /// split and merge steps.
+    ///
+    /// A sequence of callbacks can contain map, unmap and remap operations, but
+    /// the sequence of callbacks might also be empty if no operation is
+    /// required, e.g. if the requested mapping already exists in the exact same
+    /// way.
+    ///
+    /// There can be an arbitrary amount of unmap operations, a maximum of two
+    /// remap operations and a single map operation. The latter one represents
+    /// the original map operation requested by the caller.
+    ///
+    /// # Arguments
+    ///
+    /// - `ctx`: A driver-specific context.
+    /// - `req_obj`: The GEM object to map.
+    /// - `req_addr`: The start address of the new mapping.
+    /// - `req_range`: The range of the mapping.
+    /// - `req_offset`: The offset into the GEM object.
+    pub fn sm_map(
+        &mut self,
+        ctx: &mut T::StepContext,
+        req_obj: &DriverObject<T>,
+        req_addr: u64,
+        req_range: u64,
+        req_offset: u64,
+    ) -> Result {
+        let mut ctx = StepContext {
+            ctx,
+            gpuvm: self.gpuvm,
+        };
+
+        // SAFETY: LockedGpuVm implies the right locks are held.
+        to_result(unsafe {
+            bindings::drm_gpuvm_sm_map(
+                self.gpuvm.gpuvm() as *mut _,
+                &mut ctx as *mut _ as *mut _,
+                req_addr,
+                req_range,
+                req_obj.gem_obj() as *const _ as *mut _,
+                req_offset,
+            )
+        })
+    }
+
+    /// Iterates the given range of the GPU VA space.
+    ///
+    /// This utilizes [`DriverGpuVm`] to call back into the driver providing the
+    /// operations to unmap and, if required, split existent mappings.
+    ///
+    /// A sequence of callbacks can contain unmap and remap operations,
+    /// depending on whether there are actual overlapping mappings to split.
+    ///
+    /// There can be an arbitrary amount of unmap operations and a maximum of
+    /// two remap operations.
+    ///
+    /// # Arguments
+    ///
+    /// - `ctx`: A driver-specific context.
+    /// - `req_addr`: The start address of the range to unmap.
+    /// - `req_range`: The range of the mappings to unmap.
+    pub fn sm_unmap(&mut self, ctx: &mut T::StepContext, req_addr: u64, req_range: u64) -> Result {
+        let mut ctx = StepContext {
+            ctx,
+            gpuvm: self.gpuvm,
+        };
+
+        // SAFETY: LockedGpuVm implies the right locks are held.
+        to_result(unsafe {
+            bindings::drm_gpuvm_sm_unmap(
+                self.gpuvm.gpuvm() as *mut _,
+                &mut ctx as *mut _ as *mut _,
+                req_addr,
+                req_range,
+            )
+        })
+    }
+}
+
+impl<T: DriverGpuVm> Deref for LockedGpuVm<'_, T> {
+    type Target = T;
+
+    fn deref(&self) -> &T {
+        // SAFETY: The existence of this LockedGpuVm implies the lock is held,
+        // so this is the only reference.
+        unsafe { &*self.gpuvm.inner.get() }
+    }
+}
+
+impl<T: DriverGpuVm> DerefMut for LockedGpuVm<'_, T> {
+    fn deref_mut(&mut self) -> &mut T {
+        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
+        // so this is the only reference.
+        unsafe { &mut *self.gpuvm.inner.get() }
+    }
+}
+
+/// A state representing a GPU VM that is being updated.
+pub struct UpdatingGpuVm<'a, T: DriverGpuVm>(&'a GpuVm<T>);
+
+impl<T: DriverGpuVm> UpdatingGpuVm<'_, T> {}
+
+impl<T: DriverGpuVm> Deref for UpdatingGpuVm<'_, T> {
+    type Target = T;
+
+    fn deref(&self) -> &T {
+        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
+        // so this is the only reference.
+        unsafe { &*self.0.inner.get() }
+    }
+}
+
+impl<T: DriverGpuVm> DerefMut for UpdatingGpuVm<'_, T> {
+    fn deref_mut(&mut self) -> &mut T {
+        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
+        // so this is the only reference.
+        unsafe { &mut *self.0.inner.get() }
+    }
+}
+
+// SAFETY: All our trait methods take locks.
+unsafe impl<T: DriverGpuVm> Sync for GpuVm<T> {}
+// SAFETY: All our trait methods take locks.
+unsafe impl<T: DriverGpuVm> Send for GpuVm<T> {}
+
+// SAFETY: All our trait methods take locks.
+unsafe impl<T: DriverGpuVm> Sync for GpuVmBo<T> {}
+// SAFETY: All our trait methods take locks.
+unsafe impl<T: DriverGpuVm> Send for GpuVmBo<T> {}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
index c44760a1332fa1ef875939b48e7af450f7372020..849dc1e577f15bfada11d6739dff48ac33813326 100644
--- a/rust/kernel/drm/mod.rs
+++ b/rust/kernel/drm/mod.rs
@@ -6,4 +6,6 @@
 pub mod drv;
 pub mod file;
 pub mod gem;
+#[cfg(CONFIG_DRM_GPUVM = "y")]
+pub mod gpuvm;
 pub mod ioctl;

-- 
2.49.0

Re: [PATCH v2 1/2] rust: helpers: Add bindings/wrappers for dma_resv

[Alyssa Rosenzweig]

This needs your Signed-off-by too as you are the submitter

Le Tue, Apr 22, 2025 at 01:41:52PM -0300, Daniel Almeida a écrit :
> From: Asahi Lina <lina@asahilina.net>
> 
> This is just for basic usage in the DRM shmem abstractions for implied
> locking, not intended as a full DMA Reservation abstraction yet.
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers/dma-resv.c         | 13 +++++++++++++
>  rust/helpers/helpers.c          |  1 +
>  3 files changed, 15 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index e6020ba5b00237a08402fbd609c7fba27b970dd9..68d7be498a6d523797e54212d6c23ff4d8f2e92d 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -18,6 +18,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/cred.h>
>  #include <linux/device/faux.h>
> +#include <linux/dma-resv.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
>  #include <linux/file.h>
> diff --git a/rust/helpers/dma-resv.c b/rust/helpers/dma-resv.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..05501cb814513b483afd0b7f220230d867863c2f
> --- /dev/null
> +++ b/rust/helpers/dma-resv.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/dma-resv.h>
> +
> +int rust_helper_dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx)
> +{
> +	return dma_resv_lock(obj, ctx);
> +}
> +
> +void rust_helper_dma_resv_unlock(struct dma_resv *obj)
> +{
> +	dma_resv_unlock(obj);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7a06d6bc48537248c8a3ec4243b37d8fb2b1cb26..c5e536d688bc35c7b348daa61e868c91a7bdbd23 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -14,6 +14,7 @@
>  #include "build_bug.c"
>  #include "cred.c"
>  #include "device.c"
> +#include "dma-resv.c"
>  #include "drm.c"
>  #include "err.c"
>  #include "fs.c"
> 
> -- 
> 2.49.0
> 

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Danilo Krummrich]

(Not a full review, but some things that took my attention from a brief look.)

On Tue, Apr 22, 2025 at 01:41:53PM -0300, Daniel Almeida wrote:
> diff --git a/rust/helpers/drm_gpuvm.c b/rust/helpers/drm_gpuvm.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..7a074d2c2160ebc5f92909236c5aaecb1853e45d
> --- /dev/null
> +++ b/rust/helpers/drm_gpuvm.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +
> +#include <drm/drm_gpuvm.h>
> +
> +#ifdef CONFIG_DRM
> +#ifdef CONFIG_DRM_GPUVM

Why do we need those?

> +/// A transparent wrapper over
> +/// [`bindings::drm_gpuva_op_map`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_map)
> +///
> +/// Represents the map operation to be carried out by the driver.
> +#[repr(transparent)]
> +pub struct OpMap<T: DriverGpuVm>(bindings::drm_gpuva_op_map, PhantomData<T>);

struct drm_gpuva_op_map must be wrapped in an Opaque. Same for all other ops.

> +
> +impl<T: DriverGpuVm> OpMap<T> {
> +    /// Returns the base address of the new mapping.
> +    #[inline]
> +    pub fn addr(&self) -> u64 {
> +        self.0.va.addr
> +    }
> +
> +    /// Returns the range of the new mapping.
> +    #[inline]
> +    pub fn range(&self) -> u64 {
> +        self.0.va.range
> +    }
> +
> +    /// Returns the offset within the GEM object.
> +    #[inline]
> +    pub fn offset(&self) -> u64 {
> +        self.0.gem.offset
> +    }
> +
> +    /// Returns the GEM object to map.
> +    #[inline]
> +    pub fn object(&self) -> &<T::Driver as drv::Driver>::Object {

You can use drm::Driver instead, which reads much better.

> +        let p = <<T::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(self.0.gem.obj);
> +        // SAFETY: The GEM object has an active reference for the lifetime of this op
> +        unsafe { &*p }
> +    }
> +
> +    /// A helper function to map and link a [ `GpuVa`] to a [`GpuVmBo`].
> +    pub fn map_and_link_va(

Why are these operations combined? Drivers may need to call them separately,
since they have different locking requirements.

> +        &mut self,
> +        gpuvm: &mut UpdatingGpuVm<'_, T>,
> +        gpuva: Pin<KBox<GpuVa<T>>>,
> +        gpuvmbo: &ARef<GpuVmBo<T>>,
> +    ) -> Result<(), Pin<KBox<GpuVa<T>>>> {
> +        // SAFETY: We are handing off the GpuVa ownership and it will not be moved.
> +        let p = KBox::leak(unsafe { Pin::into_inner_unchecked(gpuva) });
> +        // SAFETY: These C functions are called with the correct invariants
> +        unsafe {
> +            bindings::drm_gpuva_init_from_op(&mut p.gpuva, &mut self.0);
> +            if bindings::drm_gpuva_insert(gpuvm.0.gpuvm() as *mut _, &mut p.gpuva) != 0 {
> +                // EEXIST, return the GpuVa to the caller as an error
> +                return Err(Pin::new_unchecked(KBox::from_raw(p)));
> +            };

How do you ensure that the VM lock is held when there's a list of operations?

> +            // SAFETY: This takes a new reference to the gpuvmbo.
> +            bindings::drm_gpuva_link(&mut p.gpuva, &gpuvmbo.bo as *const _ as *mut _);

How do you ensure that the dma_resv lock is held?

> +        }
> +        Ok(())
> +    }
> +}
> +
> +/// A transparent wrapper over
> +/// [`bindings::drm_gpuva_op_remap`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_remap).
> +///
> +/// Represents a single remap operation generated by [`GpuVm`].
> +///
> +/// A remap operation is generated when an existing [`GpuVa`] mapping is split
> +/// by inserting a new one or by partially unmapping existing mappings. Hence,
> +/// it consists of a maximum of two maps and one unmap operation,
> +///
> +/// The unmap operation takes care of removing the original existing mapping.
> +/// The `prev` field is used to remap the preceding part and `next` is used to
> +/// remap the subsequent part.
> +///
> +/// If the start address of the new mapping aligns with the start address of the
> +/// old mapping, `prev` will be `None`. Similarly, if the end address of the new
> +/// mapping aligns with the end address of the old mapping, `next` will be
> +/// `None`.
> +///
> +/// Note: the reason for a dedicated remap operation, rather than arbitrary
> +/// unmap and map operations, is to give drivers the chance of extracting driver
> +/// specific data for creating the new mappings from the unmap operations's
> +/// [`GpuVa`] structure which typically is embedded in larger driver specific
> +/// structures.
> +#[repr(transparent)]
> +pub struct OpReMap<T: DriverGpuVm>(bindings::drm_gpuva_op_remap, PhantomData<T>);
> +
> +/// A transparent wrapper over
> +/// [`bindings::drm_gpuva_op_unmap`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_unmap).
> +///
> +/// Represents a single unmap operation generated by [`GpuVm`].
> +#[repr(transparent)]
> +pub struct OpUnMap<T: DriverGpuVm>(bindings::drm_gpuva_op_unmap, PhantomData<T>);
> +
> +impl<T: DriverGpuVm> OpUnMap<T> {
> +    /// Returns the GPU VA to unmap.
> +    #[inline]
> +    pub fn va(&self) -> Option<&GpuVa<T>> {
> +        if self.0.va.is_null() {
> +            return None;
> +        }
> +        // SAFETY: Container invariant is guaranteed for ops structs created for our types.
> +        let p = unsafe { crate::container_of!(self.0.va, GpuVa<T>, gpuva) as *mut GpuVa<T> };
> +        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
> +        Some(unsafe { &*p })
> +    }
> +
> +    /// Indicates whether this [`GpuVa`] is physically contiguous with the
> +    /// original mapping request.
> +    ///
> +    /// Optionally, if `keep` is set, drivers may keep the actual page table
> +    /// mappings for this [`GpuVa`], adding the missing page table entries only and
> +    /// subsequently updating the VM accordingly.
> +    #[inline]
> +    pub fn keep(&self) -> bool {
> +        self.0.keep
> +    }
> +
> +    /// A helper to unmap and unlink a [`GpuVa`] from a [`GpuVmBo`].

A drm_gpuva_unmap() removes the VA from the internal interval tree, but
drm_gpuva_unlink() removes the VA from the VM_BO's GPUVA list.

> +    pub fn unmap_and_unlink_va(&mut self) -> Option<Pin<KBox<GpuVa<T>>>> {
> +        if self.0.va.is_null() {
> +            return None;
> +        }
> +        // SAFETY: Container invariant is guaranteed for ops structs created for our types.
> +        let p = unsafe { crate::container_of!(self.0.va, GpuVa<T>, gpuva) as *mut GpuVa<T> };
> +
> +        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
> +        unsafe {
> +            bindings::drm_gpuva_unmap(&mut self.0);
> +            bindings::drm_gpuva_unlink(self.0.va);
> +        }

Same questions as in map_and_link_va().

> +
> +        // Unlinking/unmapping relinquishes ownership of the GpuVa object,
> +        // so clear the pointer
> +        self.0.va = core::ptr::null_mut();
> +        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
> +        Some(unsafe { Pin::new_unchecked(KBox::from_raw(p)) })
> +    }
> +}
> +
> +impl<T: DriverGpuVm> OpReMap<T> {
> +    /// Returns the preceding part of a split mapping, if any.
> +    #[inline]
> +    pub fn prev_map(&mut self) -> Option<&mut OpMap<T>> {
> +        // SAFETY: The prev pointer must be valid if not-NULL per the op_remap contract
> +        unsafe { (self.0.prev as *mut OpMap<T>).as_mut() }
> +    }
> +
> +    /// Returns the subsequent part of a split mapping, if any.
> +    #[inline]
> +    pub fn next_map(&mut self) -> Option<&mut OpMap<T>> {
> +        // SAFETY: The next pointer must be valid if not-NULL per the op_remap contract
> +        unsafe { (self.0.next as *mut OpMap<T>).as_mut() }
> +    }
> +
> +    /// Returns the unmap operation for the original existing mapping.
> +    #[inline]
> +    pub fn unmap(&mut self) -> &mut OpUnMap<T> {
> +        // SAFETY: The unmap pointer is always valid per the op_remap contract
> +        unsafe { (self.0.unmap as *mut OpUnMap<T>).as_mut().unwrap() }
> +    }
> +}
> +
> +/// A GPU VA range.
> +///
> +/// Drivers can use `inner` to store additional data.
> +#[repr(C)]
> +#[pin_data]
> +pub struct GpuVa<T: DriverGpuVm> {
> +    #[pin]
> +    gpuva: bindings::drm_gpuva,

Should be wrapped in an Opaque.

> +    #[pin]
> +    inner: T::GpuVa,
> +    #[pin]
> +    _p: PhantomPinned,
> +}
> +
> +// SAFETY: This type is safe to zero-init (as far as C is concerned).
> +unsafe impl init::Zeroable for bindings::drm_gpuva {}
> +
> +impl<T: DriverGpuVm> GpuVa<T> {
> +    /// Creates a new GPU VA.
> +    pub fn new<E>(inner: impl PinInit<T::GpuVa, E>) -> Result<Pin<KBox<GpuVa<T>>>>
> +    where
> +        Error: From<E>,
> +    {
> +        KBox::try_pin_init(
> +            try_pin_init!(Self {
> +                gpuva <- init::zeroed(),
> +                inner <- inner,
> +                _p: PhantomPinned
> +            }),
> +            GFP_KERNEL,
> +        )
> +    }
> +
> +    /// Returns the start address of the GPU VA range.
> +    #[inline]
> +    pub fn addr(&self) -> u64 {
> +        self.gpuva.va.addr
> +    }
> +
> +    /// Returns the range of the GPU VA.
> +    #[inline]
> +    pub fn range(&self) -> u64 {
> +        self.gpuva.va.range
> +    }
> +
> +    /// Returns the offset within the GEM object.
> +    #[inline]
> +    pub fn offset(&self) -> u64 {
> +        self.gpuva.gem.offset
> +    }
> +}
> +
> +/// The connection between a GEM object and a VM.
> +#[repr(C)]
> +#[pin_data]
> +pub struct GpuVmBo<T: DriverGpuVm> {
> +    #[pin]
> +    bo: bindings::drm_gpuvm_bo,

Should be wrapped in an Opaque.

> +    #[pin]
> +    inner: T::GpuVmBo,
> +    #[pin]
> +    _p: PhantomPinned,
> +}
> +
> +impl<T: DriverGpuVm> GpuVmBo<T> {
> +    /// Return a reference to the inner driver data for this [`GpuVmBo`].
> +    pub fn inner(&self) -> &T::GpuVmBo {
> +        &self.inner
> +    }
> +}
> +
> +// SAFETY: DRM GpuVmBo objects are always reference counted and the get/put functions
> +// satisfy the requirements.
> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
> +    fn inc_ref(&self) {
> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
> +        unsafe { bindings::drm_gpuvm_bo_get(&self.bo as *const _ as *mut _) };
> +    }
> +
> +    unsafe fn dec_ref(mut obj: NonNull<Self>) {
> +        // SAFETY: drm_gpuvm_bo_put() requires holding the gpuva lock, which is
> +        // the dma_resv lock by default.
> +        // The drm_gpuvm_put function satisfies the requirements for dec_ref().
> +        // (We do not support custom locks yet.)
> +        unsafe {
> +            let resv = (*obj.as_mut().bo.obj).resv;
> +            bindings::dma_resv_lock(resv, core::ptr::null_mut());
> +            bindings::drm_gpuvm_bo_put(&mut obj.as_mut().bo);
> +            bindings::dma_resv_unlock(resv);

What if the resv_lock is held already? Please also make sure to put multiple
unsafe calls each in a separate unsafe block.

> +        }
> +    }
> +}
> +
> +/// The DRM GPU VA Manager.
> +///
> +/// It keeps track of a GPU's virtual address space by using maple tree
> +/// structures.
> +///
> +/// Typically, an instance of [`GpuVm`] is embedded bigger, driver-specific
> +/// structures.
> +///
> +/// Drivers can pass addresses and ranges in arbitrary units, e.g.: bytes or
> +/// pages.
> +///
> +/// There should be one manager instance per GPU virtual address space.
> +#[repr(C)]
> +#[pin_data]
> +pub struct GpuVm<T: DriverGpuVm> {
> +    #[pin]
> +    gpuvm: Opaque<bindings::drm_gpuvm>,
> +    #[pin]
> +    inner: UnsafeCell<T>,

This looks a bit odd, why does this need UnsafeCell?

> +    #[pin]
> +    _p: PhantomPinned,
> +}
> +
> +/// # Safety
> +///
> +/// This function is only safe to be called from the GPUVM C code.
> +unsafe extern "C" fn vm_free_callback<T: DriverGpuVm>(raw_gpuvm: *mut bindings::drm_gpuvm) {
> +    // SAFETY: Container invariant is guaranteed for objects using our callback.
> +    let p = unsafe {
> +        crate::container_of!(
> +            raw_gpuvm as *mut Opaque<bindings::drm_gpuvm>,
> +            GpuVm<T>,
> +            gpuvm
> +        ) as *mut GpuVm<T>
> +    };
> +
> +    // SAFETY: p is guaranteed to be valid for drm_gpuvm objects using this callback.
> +    unsafe { drop(KBox::from_raw(p)) };
> +}
> +
> +/// # Safety
> +///
> +/// This function is only safe to be called from the GPUVM C code.
> +unsafe extern "C" fn vm_bo_alloc_callback<T: DriverGpuVm>() -> *mut bindings::drm_gpuvm_bo {
> +    let obj: Result<Pin<KBox<GpuVmBo<T>>>> = KBox::try_pin_init(
> +        try_pin_init!(GpuVmBo::<T> {
> +            bo <- init::zeroed(),
> +            inner <- T::GpuVmBo::new(),
> +            _p: PhantomPinned
> +        }),
> +        GFP_KERNEL,
> +    );
> +
> +    match obj {
> +        Ok(obj) =>
> +        // SAFETY: The DRM core will keep this object pinned.
> +        unsafe {
> +            let p = KBox::leak(Pin::into_inner_unchecked(obj));
> +            &mut p.bo
> +        },
> +        Err(_) => core::ptr::null_mut(),
> +    }
> +}
> +
> +/// # Safety
> +///
> +/// This function is only safe to be called from the GPUVM C code.
> +unsafe extern "C" fn vm_bo_free_callback<T: DriverGpuVm>(raw_vm_bo: *mut bindings::drm_gpuvm_bo) {
> +    // SAFETY: Container invariant is guaranteed for objects using this callback.
> +    let p = unsafe { crate::container_of!(raw_vm_bo, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
> +
> +    // SAFETY: p is guaranteed to be valid for drm_gpuvm_bo objects using this callback.
> +    unsafe { drop(KBox::from_raw(p)) };
> +}
> +
> +/// # Safety
> +///
> +/// This function is only safe to be called from the GPUVM C code.
> +unsafe extern "C" fn step_map_callback<T: DriverGpuVm>(
> +    op: *mut bindings::drm_gpuva_op,
> +    _priv: *mut core::ffi::c_void,
> +) -> core::ffi::c_int {
> +    // SAFETY: We know this is a map op, and OpMap is a transparent wrapper.
> +    let map = unsafe { &mut *((&mut (*op).__bindgen_anon_1.map) as *mut _ as *mut OpMap<T>) };

Please don't create (mutable) references to FFI objects, this happens in various
places.

> +    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
> +    // guaranteed to outlive this function.
> +    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
> +
> +    from_result(|| {
> +        UpdatingGpuVm(ctx.gpuvm).step_map(map, ctx.ctx)?;
> +        Ok(0)
> +    })
> +}
> +
> +/// # Safety
> +///
> +/// This function is only safe to be called from the GPUVM C code.
> +unsafe extern "C" fn step_remap_callback<T: DriverGpuVm>(
> +    op: *mut bindings::drm_gpuva_op,
> +    _priv: *mut core::ffi::c_void,
> +) -> core::ffi::c_int {
> +    // SAFETY: We know this is a map op, and OpReMap is a transparent wrapper.
> +    let remap = unsafe { &mut *((&mut (*op).__bindgen_anon_1.remap) as *mut _ as *mut OpReMap<T>) };
> +    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
> +    // guaranteed to outlive this function.
> +    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
> +
> +    let p_vm_bo = remap.unmap().va().unwrap().gpuva.vm_bo;
> +
> +    let res = {
> +        // SAFETY: vm_bo pointer must be valid and non-null by the step_remap invariants.
> +        // Since we grab a ref, this reference's lifetime is until the decref.
> +        let vm_bo_ref = unsafe {
> +            bindings::drm_gpuvm_bo_get(p_vm_bo);
> +            &*(crate::container_of!(p_vm_bo, GpuVmBo<T>, bo) as *mut GpuVmBo<T>)
> +        };
> +
> +        from_result(|| {
> +            UpdatingGpuVm(ctx.gpuvm).step_remap(remap, vm_bo_ref, ctx.ctx)?;
> +            Ok(0)
> +        })
> +    };
> +
> +    // SAFETY: We incremented the refcount above, and the Rust reference we took is
> +    // no longer in scope.
> +    unsafe { bindings::drm_gpuvm_bo_put(p_vm_bo) };
> +
> +    res
> +}
> +
> +/// # Safety
> +///
> +/// This function is only safe to be called from the GPUVM C code.
> +unsafe extern "C" fn step_unmap_callback<T: DriverGpuVm>(
> +    op: *mut bindings::drm_gpuva_op,
> +    _priv: *mut core::ffi::c_void,
> +) -> core::ffi::c_int {
> +    // SAFETY: We know this is a map op, and OpUnMap is a transparent wrapper.
> +    let unmap = unsafe { &mut *((&mut (*op).__bindgen_anon_1.unmap) as *mut _ as *mut OpUnMap<T>) };
> +    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
> +    // guaranteed to outlive this function.
> +    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
> +
> +    from_result(|| {
> +        UpdatingGpuVm(ctx.gpuvm).step_unmap(unmap, ctx.ctx)?;
> +        Ok(0)
> +    })
> +}
> +
> +impl<T: DriverGpuVm> GpuVm<T> {
> +    const OPS: bindings::drm_gpuvm_ops = bindings::drm_gpuvm_ops {
> +        vm_free: Some(vm_free_callback::<T>),
> +        op_alloc: None,
> +        op_free: None,
> +        vm_bo_alloc: Some(vm_bo_alloc_callback::<T>),
> +        vm_bo_free: Some(vm_bo_free_callback::<T>),
> +        vm_bo_validate: None,
> +        sm_step_map: Some(step_map_callback::<T>),
> +        sm_step_remap: Some(step_remap_callback::<T>),
> +        sm_step_unmap: Some(step_unmap_callback::<T>),
> +    };
> +
> +    fn gpuvm(&self) -> *const bindings::drm_gpuvm {
> +        self.gpuvm.get()
> +    }
> +
> +    /// Creates a GPUVM instance.
> +    pub fn new<E>(
> +        name: &'static CStr,
> +        dev: &device::Device<T::Driver>,

You can use drm::Device instead.

> +        r_obj: &<T::Driver as drv::Driver>::Object,
> +        range: Range<u64>,
> +        reserve_range: Range<u64>,
> +        inner: impl PinInit<T, E>,
> +    ) -> Result<ARef<GpuVm<T>>>
> +    where
> +        Error: From<E>,
> +    {
> +        let obj: Pin<KBox<Self>> = KBox::try_pin_init(
> +            try_pin_init!(Self {
> +                // SAFETY: drm_gpuvm_init cannot fail and always initializes the member.
> +                gpuvm <- unsafe {
> +                    init::pin_init_from_closure(move |slot: *mut Opaque<bindings::drm_gpuvm> | {
> +                        // Zero-init required by drm_gpuvm_init.
> +                        *slot = core::mem::zeroed();
> +                        bindings::drm_gpuvm_init(
> +                            Opaque::raw_get(slot),
> +                            name.as_char_ptr(),
> +                            0,
> +                            dev.as_raw(),
> +                            r_obj.gem_obj() as *const _ as *mut _,
> +                            range.start,
> +                            range.end - range.start,
> +                            reserve_range.start,
> +                            reserve_range.end - reserve_range.start,
> +                            &Self::OPS
> +                        );
> +                        Ok(())
> +                    })
> +                },
> +                // SAFETY: Just passing through to the initializer argument.
> +                inner <- unsafe {
> +                    init::pin_init_from_closure(move |slot: *mut UnsafeCell<T> | {
> +                        inner.__pinned_init(slot as *mut _)
> +                    })
> +                },
> +                _p: PhantomPinned
> +            }),
> +            GFP_KERNEL,
> +        )?;
> +
> +        // SAFETY: We never move out of the object.
> +        let vm_ref = unsafe {
> +            ARef::from_raw(NonNull::new_unchecked(KBox::leak(
> +                Pin::into_inner_unchecked(obj),
> +            )))
> +        };
> +
> +        Ok(vm_ref)
> +    }
> +
> +    /// Locks the VM, protecting its interval tree against concurrent accesses.
> +    ///
> +    /// Callers must prove that they have exclusive access to the VM by holding
> +    /// some guard type. This encodes the driver-specific locking requirements.
> +    ///
> +    /// It is up to the caller to ensure that the guard indeed provides the
> +    /// required locking.
> +    pub fn lock<U: ?Sized, B: Backend>(&self, _guard: &mut Guard<'_, U, B>) -> LockedGpuVm<'_, T> {
> +        LockedGpuVm { gpuvm: self }
> +    }
> +
> +    /// Returns true if the given object is external to the GPUVM, i.e.: if it
> +    /// does not share the DMA reservation object of the GPUVM.
> +    pub fn is_extobj(&self, obj: &impl IntoGEMObject) -> bool {
> +        let gem = obj.gem_obj() as *const _ as *mut _;
> +        // SAFETY: This is safe to call as long as the arguments are valid pointers.
> +        unsafe { bindings::drm_gpuvm_is_extobj(self.gpuvm() as *mut _, gem) }
> +    }
> +}
> +
> +// SAFETY: DRM GpuVm objects are always reference counted and the get/put functions
> +// satisfy the requirements.
> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVm<T> {
> +    fn inc_ref(&self) {
> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
> +        unsafe { bindings::drm_gpuvm_get(&self.gpuvm as *const _ as *mut _) };
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The drm_gpuvm_put function satisfies the requirements for dec_ref().
> +        unsafe { bindings::drm_gpuvm_put(Opaque::raw_get(&(*obj.as_ptr()).gpuvm)) };
> +    }
> +}
> +
> +/// The object returned after a call to [`GpuVm::lock`].
> +///
> +/// This object has access to operations that modify the VM's interval tree.
> +pub struct LockedGpuVm<'a, T: DriverGpuVm> {
> +    gpuvm: &'a GpuVm<T>,
> +}
> +
> +impl<T: DriverGpuVm> LockedGpuVm<'_, T> {
> +    /// Finds the [`GpuVmBo`] object that connects `obj` to this VM.
> +    ///
> +    /// If found, increases the reference count of the GpuVmBo object
> +    /// accordingly.
> +    pub fn find_bo(&mut self, obj: &DriverObject<T>) -> Option<ARef<GpuVmBo<T>>> {
> +        // SAFETY: LockedGpuVm implies the right locks are held.
> +        let p = unsafe {
> +            bindings::drm_gpuvm_bo_find(
> +                self.gpuvm.gpuvm() as *mut _,
> +                obj.gem_obj() as *const _ as *mut _,
> +            )
> +        };
> +
> +        if p.is_null() {
> +            return None;
> +        }
> +
> +        // SAFETY: All the drm_gpuvm_bo objects in this GpuVm are always
> +        // allocated by us as GpuVmBo<T>.
> +        let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
> +        // SAFETY: We checked for NULL above, and the types ensure that
> +        // this object was created by vm_bo_alloc_callback<T>.
> +        Some(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
> +    }
> +
> +    /// Obtains the [`GpuVmBo`] object that connects `obj` to this VM.
> +    ///
> +    /// This connection is unique, so an instane of [`GpuVmBo`] will be
> +    /// allocated for `obj` once, and that instance will be returned from that
> +    /// point forward.
> +    pub fn obtain_bo(&mut self, obj: &DriverObject<T>) -> Result<ARef<GpuVmBo<T>>> {
> +        // SAFETY: LockedGpuVm implies the right locks are held.

No, this must be locked by the dma-resv or the GEM gpuva lock, not by the
GPUVM lock that protects the interval tree.

> +        let p = unsafe {
> +            bindings::drm_gpuvm_bo_obtain(
> +                self.gpuvm.gpuvm() as *mut _,
> +                obj.gem_obj() as *const _ as *mut _,
> +            )
> +        };
> +
> +        if p.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY: Container invariant is guaranteed for GpuVmBo objects for this GpuVm.
> +        let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
> +        // SAFETY: We checked for NULL above, and the types ensure that
> +        // this object was created by vm_bo_alloc_callback<T>.
> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
> +    }
> +
> +    /// Iterates the given range of the GPU VA space.
> +    ///
> +    /// This utilizes [`DriverGpuVm`] to call back into the driver providing the
> +    /// split and merge steps.
> +    ///
> +    /// A sequence of callbacks can contain map, unmap and remap operations, but
> +    /// the sequence of callbacks might also be empty if no operation is
> +    /// required, e.g. if the requested mapping already exists in the exact same
> +    /// way.
> +    ///
> +    /// There can be an arbitrary amount of unmap operations, a maximum of two
> +    /// remap operations and a single map operation. The latter one represents
> +    /// the original map operation requested by the caller.
> +    ///
> +    /// # Arguments
> +    ///
> +    /// - `ctx`: A driver-specific context.
> +    /// - `req_obj`: The GEM object to map.
> +    /// - `req_addr`: The start address of the new mapping.
> +    /// - `req_range`: The range of the mapping.
> +    /// - `req_offset`: The offset into the GEM object.
> +    pub fn sm_map(
> +        &mut self,
> +        ctx: &mut T::StepContext,
> +        req_obj: &DriverObject<T>,
> +        req_addr: u64,
> +        req_range: u64,
> +        req_offset: u64,
> +    ) -> Result {
> +        let mut ctx = StepContext {
> +            ctx,
> +            gpuvm: self.gpuvm,
> +        };
> +
> +        // SAFETY: LockedGpuVm implies the right locks are held.
> +        to_result(unsafe {
> +            bindings::drm_gpuvm_sm_map(
> +                self.gpuvm.gpuvm() as *mut _,
> +                &mut ctx as *mut _ as *mut _,
> +                req_addr,
> +                req_range,
> +                req_obj.gem_obj() as *const _ as *mut _,
> +                req_offset,
> +            )
> +        })
> +    }
> +
> +    /// Iterates the given range of the GPU VA space.
> +    ///
> +    /// This utilizes [`DriverGpuVm`] to call back into the driver providing the
> +    /// operations to unmap and, if required, split existent mappings.
> +    ///
> +    /// A sequence of callbacks can contain unmap and remap operations,
> +    /// depending on whether there are actual overlapping mappings to split.
> +    ///
> +    /// There can be an arbitrary amount of unmap operations and a maximum of
> +    /// two remap operations.
> +    ///
> +    /// # Arguments
> +    ///
> +    /// - `ctx`: A driver-specific context.
> +    /// - `req_addr`: The start address of the range to unmap.
> +    /// - `req_range`: The range of the mappings to unmap.
> +    pub fn sm_unmap(&mut self, ctx: &mut T::StepContext, req_addr: u64, req_range: u64) -> Result {
> +        let mut ctx = StepContext {
> +            ctx,
> +            gpuvm: self.gpuvm,
> +        };
> +
> +        // SAFETY: LockedGpuVm implies the right locks are held.
> +        to_result(unsafe {
> +            bindings::drm_gpuvm_sm_unmap(
> +                self.gpuvm.gpuvm() as *mut _,
> +                &mut ctx as *mut _ as *mut _,
> +                req_addr,
> +                req_range,
> +            )
> +        })
> +    }
> +}
> +
> +impl<T: DriverGpuVm> Deref for LockedGpuVm<'_, T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &T {
> +        // SAFETY: The existence of this LockedGpuVm implies the lock is held,
> +        // so this is the only reference.
> +        unsafe { &*self.gpuvm.inner.get() }
> +    }
> +}
> +
> +impl<T: DriverGpuVm> DerefMut for LockedGpuVm<'_, T> {
> +    fn deref_mut(&mut self) -> &mut T {
> +        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
> +        // so this is the only reference.
> +        unsafe { &mut *self.gpuvm.inner.get() }
> +    }
> +}
> +
> +/// A state representing a GPU VM that is being updated.
> +pub struct UpdatingGpuVm<'a, T: DriverGpuVm>(&'a GpuVm<T>);

What does it mean to update a GPUVM instance? How is it different compared to a
LockedGpuVm?

> +
> +impl<T: DriverGpuVm> UpdatingGpuVm<'_, T> {}
> +
> +impl<T: DriverGpuVm> Deref for UpdatingGpuVm<'_, T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &T {
> +        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
> +        // so this is the only reference.
> +        unsafe { &*self.0.inner.get() }
> +    }
> +}
> +
> +impl<T: DriverGpuVm> DerefMut for UpdatingGpuVm<'_, T> {
> +    fn deref_mut(&mut self) -> &mut T {
> +        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
> +        // so this is the only reference.
> +        unsafe { &mut *self.0.inner.get() }
> +    }
> +}
> +
> +// SAFETY: All our trait methods take locks.
> +unsafe impl<T: DriverGpuVm> Sync for GpuVm<T> {}
> +// SAFETY: All our trait methods take locks.
> +unsafe impl<T: DriverGpuVm> Send for GpuVm<T> {}
> +
> +// SAFETY: All our trait methods take locks.
> +unsafe impl<T: DriverGpuVm> Sync for GpuVmBo<T> {}
> +// SAFETY: All our trait methods take locks.
> +unsafe impl<T: DriverGpuVm> Send for GpuVmBo<T> {}
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> index c44760a1332fa1ef875939b48e7af450f7372020..849dc1e577f15bfada11d6739dff48ac33813326 100644
> --- a/rust/kernel/drm/mod.rs
> +++ b/rust/kernel/drm/mod.rs
> @@ -6,4 +6,6 @@
>  pub mod drv;
>  pub mod file;
>  pub mod gem;
> +#[cfg(CONFIG_DRM_GPUVM = "y")]
> +pub mod gpuvm;
>  pub mod ioctl;
> 
> -- 
> 2.49.0
> 

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Daniel Almeida]

Hi Danilo,

FYI, most of this patch still retains the original code from the Asahi project,

> On 22 Apr 2025, at 18:16, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> (Not a full review, but some things that took my attention from a brief look.)
> 
> On Tue, Apr 22, 2025 at 01:41:53PM -0300, Daniel Almeida wrote:
>> diff --git a/rust/helpers/drm_gpuvm.c b/rust/helpers/drm_gpuvm.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..7a074d2c2160ebc5f92909236c5aaecb1853e45d
>> --- /dev/null
>> +++ b/rust/helpers/drm_gpuvm.c
>> @@ -0,0 +1,29 @@
>> +// SPDX-License-Identifier: GPL-2.0 or MIT
>> +
>> +#include <drm/drm_gpuvm.h>
>> +
>> +#ifdef CONFIG_DRM
>> +#ifdef CONFIG_DRM_GPUVM
> 
> Why do we need those?

Good question. I see that drm_gpuvm.o (which has the definitions for most of
the functions used by this file) depends on CONFIG_DRM_GPUVM, so maybe
that’s why.

OTOH, now that you have brought this up, I see that no files in helpers.c have
guards in them.

> 
>> +/// A transparent wrapper over
>> +/// [`bindings::drm_gpuva_op_map`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_map)
>> +///
>> +/// Represents the map operation to be carried out by the driver.
>> +#[repr(transparent)]
>> +pub struct OpMap<T: DriverGpuVm>(bindings::drm_gpuva_op_map, PhantomData<T>);
> 
> struct drm_gpuva_op_map must be wrapped in an Opaque. Same for all other ops.

Right, that’s true.

> 
>> +
>> +impl<T: DriverGpuVm> OpMap<T> {
>> +    /// Returns the base address of the new mapping.
>> +    #[inline]
>> +    pub fn addr(&self) -> u64 {
>> +        self.0.va.addr
>> +    }
>> +
>> +    /// Returns the range of the new mapping.
>> +    #[inline]
>> +    pub fn range(&self) -> u64 {
>> +        self.0.va.range
>> +    }
>> +
>> +    /// Returns the offset within the GEM object.
>> +    #[inline]
>> +    pub fn offset(&self) -> u64 {
>> +        self.0.gem.offset
>> +    }
>> +
>> +    /// Returns the GEM object to map.
>> +    #[inline]
>> +    pub fn object(&self) -> &<T::Driver as drv::Driver>::Object {
> 
> You can use drm::Driver instead, which reads much better.
> 
>> +        let p = <<T::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(self.0.gem.obj);
>> +        // SAFETY: The GEM object has an active reference for the lifetime of this op
>> +        unsafe { &*p }
>> +    }
>> +
>> +    /// A helper function to map and link a [ `GpuVa`] to a [`GpuVmBo`].
>> +    pub fn map_and_link_va(
> 
> Why are these operations combined? Drivers may need to call them separately,
> since they have different locking requirements.

I guess that was the only use-case in AGX. I will split them up.

> 
>> +        &mut self,
>> +        gpuvm: &mut UpdatingGpuVm<'_, T>,
>> +        gpuva: Pin<KBox<GpuVa<T>>>,
>> +        gpuvmbo: &ARef<GpuVmBo<T>>,
>> +    ) -> Result<(), Pin<KBox<GpuVa<T>>>> {
>> +        // SAFETY: We are handing off the GpuVa ownership and it will not be moved.
>> +        let p = KBox::leak(unsafe { Pin::into_inner_unchecked(gpuva) });
>> +        // SAFETY: These C functions are called with the correct invariants
>> +        unsafe {
>> +            bindings::drm_gpuva_init_from_op(&mut p.gpuva, &mut self.0);
>> +            if bindings::drm_gpuva_insert(gpuvm.0.gpuvm() as *mut _, &mut p.gpuva) != 0 {
>> +                // EEXIST, return the GpuVa to the caller as an error
>> +                return Err(Pin::new_unchecked(KBox::from_raw(p)));
>> +            };
> 
> How do you ensure that the VM lock is held when there's a list of operations?

Are you saying that this will not work for async binds, or that it’s already broken in sync binds too?

Perhaps we can get rid of this `UpdatingGpuVm` type and pass `LockedGpuVm` instead?

> 
>> +            // SAFETY: This takes a new reference to the gpuvmbo.
>> +            bindings::drm_gpuva_link(&mut p.gpuva, &gpuvmbo.bo as *const _ as *mut _);
> 
> How do you ensure that the dma_resv lock is held?

Right, I totally missed that.

> 
>> +        }
>> +        Ok(())
>> +    }
>> +}
>> +
>> +/// A transparent wrapper over
>> +/// [`bindings::drm_gpuva_op_remap`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_remap).
>> +///
>> +/// Represents a single remap operation generated by [`GpuVm`].
>> +///
>> +/// A remap operation is generated when an existing [`GpuVa`] mapping is split
>> +/// by inserting a new one or by partially unmapping existing mappings. Hence,
>> +/// it consists of a maximum of two maps and one unmap operation,
>> +///
>> +/// The unmap operation takes care of removing the original existing mapping.
>> +/// The `prev` field is used to remap the preceding part and `next` is used to
>> +/// remap the subsequent part.
>> +///
>> +/// If the start address of the new mapping aligns with the start address of the
>> +/// old mapping, `prev` will be `None`. Similarly, if the end address of the new
>> +/// mapping aligns with the end address of the old mapping, `next` will be
>> +/// `None`.
>> +///
>> +/// Note: the reason for a dedicated remap operation, rather than arbitrary
>> +/// unmap and map operations, is to give drivers the chance of extracting driver
>> +/// specific data for creating the new mappings from the unmap operations's
>> +/// [`GpuVa`] structure which typically is embedded in larger driver specific
>> +/// structures.
>> +#[repr(transparent)]
>> +pub struct OpReMap<T: DriverGpuVm>(bindings::drm_gpuva_op_remap, PhantomData<T>);
>> +
>> +/// A transparent wrapper over
>> +/// [`bindings::drm_gpuva_op_unmap`](https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_unmap).
>> +///
>> +/// Represents a single unmap operation generated by [`GpuVm`].
>> +#[repr(transparent)]
>> +pub struct OpUnMap<T: DriverGpuVm>(bindings::drm_gpuva_op_unmap, PhantomData<T>);
>> +
>> +impl<T: DriverGpuVm> OpUnMap<T> {
>> +    /// Returns the GPU VA to unmap.
>> +    #[inline]
>> +    pub fn va(&self) -> Option<&GpuVa<T>> {
>> +        if self.0.va.is_null() {
>> +            return None;
>> +        }
>> +        // SAFETY: Container invariant is guaranteed for ops structs created for our types.
>> +        let p = unsafe { crate::container_of!(self.0.va, GpuVa<T>, gpuva) as *mut GpuVa<T> };
>> +        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
>> +        Some(unsafe { &*p })
>> +    }
>> +
>> +    /// Indicates whether this [`GpuVa`] is physically contiguous with the
>> +    /// original mapping request.
>> +    ///
>> +    /// Optionally, if `keep` is set, drivers may keep the actual page table
>> +    /// mappings for this [`GpuVa`], adding the missing page table entries only and
>> +    /// subsequently updating the VM accordingly.
>> +    #[inline]
>> +    pub fn keep(&self) -> bool {
>> +        self.0.keep
>> +    }
>> +
>> +    /// A helper to unmap and unlink a [`GpuVa`] from a [`GpuVmBo`].
> 
> A drm_gpuva_unmap() removes the VA from the internal interval tree, but
> drm_gpuva_unlink() removes the VA from the VM_BO's GPUVA list.
> 
>> +    pub fn unmap_and_unlink_va(&mut self) -> Option<Pin<KBox<GpuVa<T>>>> {
>> +        if self.0.va.is_null() {
>> +            return None;
>> +        }
>> +        // SAFETY: Container invariant is guaranteed for ops structs created for our types.
>> +        let p = unsafe { crate::container_of!(self.0.va, GpuVa<T>, gpuva) as *mut GpuVa<T> };
>> +
>> +        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
>> +        unsafe {
>> +            bindings::drm_gpuva_unmap(&mut self.0);
>> +            bindings::drm_gpuva_unlink(self.0.va);
>> +        }
> 
> Same questions as in map_and_link_va().
> 
>> +
>> +        // Unlinking/unmapping relinquishes ownership of the GpuVa object,
>> +        // so clear the pointer
>> +        self.0.va = core::ptr::null_mut();
>> +        // SAFETY: The GpuVa object reference is valid per the op_unmap contract
>> +        Some(unsafe { Pin::new_unchecked(KBox::from_raw(p)) })
>> +    }
>> +}
>> +
>> +impl<T: DriverGpuVm> OpReMap<T> {
>> +    /// Returns the preceding part of a split mapping, if any.
>> +    #[inline]
>> +    pub fn prev_map(&mut self) -> Option<&mut OpMap<T>> {
>> +        // SAFETY: The prev pointer must be valid if not-NULL per the op_remap contract
>> +        unsafe { (self.0.prev as *mut OpMap<T>).as_mut() }
>> +    }
>> +
>> +    /// Returns the subsequent part of a split mapping, if any.
>> +    #[inline]
>> +    pub fn next_map(&mut self) -> Option<&mut OpMap<T>> {
>> +        // SAFETY: The next pointer must be valid if not-NULL per the op_remap contract
>> +        unsafe { (self.0.next as *mut OpMap<T>).as_mut() }
>> +    }
>> +
>> +    /// Returns the unmap operation for the original existing mapping.
>> +    #[inline]
>> +    pub fn unmap(&mut self) -> &mut OpUnMap<T> {
>> +        // SAFETY: The unmap pointer is always valid per the op_remap contract
>> +        unsafe { (self.0.unmap as *mut OpUnMap<T>).as_mut().unwrap() }
>> +    }
>> +}
>> +
>> +/// A GPU VA range.
>> +///
>> +/// Drivers can use `inner` to store additional data.
>> +#[repr(C)]
>> +#[pin_data]
>> +pub struct GpuVa<T: DriverGpuVm> {
>> +    #[pin]
>> +    gpuva: bindings::drm_gpuva,
> 
> Should be wrapped in an Opaque.
> 
>> +    #[pin]
>> +    inner: T::GpuVa,
>> +    #[pin]
>> +    _p: PhantomPinned,
>> +}
>> +
>> +// SAFETY: This type is safe to zero-init (as far as C is concerned).
>> +unsafe impl init::Zeroable for bindings::drm_gpuva {}
>> +
>> +impl<T: DriverGpuVm> GpuVa<T> {
>> +    /// Creates a new GPU VA.
>> +    pub fn new<E>(inner: impl PinInit<T::GpuVa, E>) -> Result<Pin<KBox<GpuVa<T>>>>
>> +    where
>> +        Error: From<E>,
>> +    {
>> +        KBox::try_pin_init(
>> +            try_pin_init!(Self {
>> +                gpuva <- init::zeroed(),
>> +                inner <- inner,
>> +                _p: PhantomPinned
>> +            }),
>> +            GFP_KERNEL,
>> +        )
>> +    }
>> +
>> +    /// Returns the start address of the GPU VA range.
>> +    #[inline]
>> +    pub fn addr(&self) -> u64 {
>> +        self.gpuva.va.addr
>> +    }
>> +
>> +    /// Returns the range of the GPU VA.
>> +    #[inline]
>> +    pub fn range(&self) -> u64 {
>> +        self.gpuva.va.range
>> +    }
>> +
>> +    /// Returns the offset within the GEM object.
>> +    #[inline]
>> +    pub fn offset(&self) -> u64 {
>> +        self.gpuva.gem.offset
>> +    }
>> +}
>> +
>> +/// The connection between a GEM object and a VM.
>> +#[repr(C)]
>> +#[pin_data]
>> +pub struct GpuVmBo<T: DriverGpuVm> {
>> +    #[pin]
>> +    bo: bindings::drm_gpuvm_bo,
> 
> Should be wrapped in an Opaque.
> 
>> +    #[pin]
>> +    inner: T::GpuVmBo,
>> +    #[pin]
>> +    _p: PhantomPinned,
>> +}
>> +
>> +impl<T: DriverGpuVm> GpuVmBo<T> {
>> +    /// Return a reference to the inner driver data for this [`GpuVmBo`].
>> +    pub fn inner(&self) -> &T::GpuVmBo {
>> +        &self.inner
>> +    }
>> +}
>> +
>> +// SAFETY: DRM GpuVmBo objects are always reference counted and the get/put functions
>> +// satisfy the requirements.
>> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
>> +    fn inc_ref(&self) {
>> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
>> +        unsafe { bindings::drm_gpuvm_bo_get(&self.bo as *const _ as *mut _) };
>> +    }
>> +
>> +    unsafe fn dec_ref(mut obj: NonNull<Self>) {
>> +        // SAFETY: drm_gpuvm_bo_put() requires holding the gpuva lock, which is
>> +        // the dma_resv lock by default.
>> +        // The drm_gpuvm_put function satisfies the requirements for dec_ref().
>> +        // (We do not support custom locks yet.)
>> +        unsafe {
>> +            let resv = (*obj.as_mut().bo.obj).resv;
>> +            bindings::dma_resv_lock(resv, core::ptr::null_mut());
>> +            bindings::drm_gpuvm_bo_put(&mut obj.as_mut().bo);
>> +            bindings::dma_resv_unlock(resv);
> 
> What if the resv_lock is held already? Please also make sure to put multiple
> unsafe calls each in a separate unsafe block.

By whom?

See my comment about “[..] a new type for GEM objects which are known to be locked"
below.

> 
>> +        }
>> +    }
>> +}
>> +
>> +/// The DRM GPU VA Manager.
>> +///
>> +/// It keeps track of a GPU's virtual address space by using maple tree
>> +/// structures.
>> +///
>> +/// Typically, an instance of [`GpuVm`] is embedded bigger, driver-specific
>> +/// structures.
>> +///
>> +/// Drivers can pass addresses and ranges in arbitrary units, e.g.: bytes or
>> +/// pages.
>> +///
>> +/// There should be one manager instance per GPU virtual address space.
>> +#[repr(C)]
>> +#[pin_data]
>> +pub struct GpuVm<T: DriverGpuVm> {
>> +    #[pin]
>> +    gpuvm: Opaque<bindings::drm_gpuvm>,
>> +    #[pin]
>> +    inner: UnsafeCell<T>,
> 
> This looks a bit odd, why does this need UnsafeCell?

No idea, perhaps a plain `T` will do.

> 
>> +    #[pin]
>> +    _p: PhantomPinned,
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function is only safe to be called from the GPUVM C code.
>> +unsafe extern "C" fn vm_free_callback<T: DriverGpuVm>(raw_gpuvm: *mut bindings::drm_gpuvm) {
>> +    // SAFETY: Container invariant is guaranteed for objects using our callback.
>> +    let p = unsafe {
>> +        crate::container_of!(
>> +            raw_gpuvm as *mut Opaque<bindings::drm_gpuvm>,
>> +            GpuVm<T>,
>> +            gpuvm
>> +        ) as *mut GpuVm<T>
>> +    };
>> +
>> +    // SAFETY: p is guaranteed to be valid for drm_gpuvm objects using this callback.
>> +    unsafe { drop(KBox::from_raw(p)) };
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function is only safe to be called from the GPUVM C code.
>> +unsafe extern "C" fn vm_bo_alloc_callback<T: DriverGpuVm>() -> *mut bindings::drm_gpuvm_bo {
>> +    let obj: Result<Pin<KBox<GpuVmBo<T>>>> = KBox::try_pin_init(
>> +        try_pin_init!(GpuVmBo::<T> {
>> +            bo <- init::zeroed(),
>> +            inner <- T::GpuVmBo::new(),
>> +            _p: PhantomPinned
>> +        }),
>> +        GFP_KERNEL,
>> +    );
>> +
>> +    match obj {
>> +        Ok(obj) =>
>> +        // SAFETY: The DRM core will keep this object pinned.
>> +        unsafe {
>> +            let p = KBox::leak(Pin::into_inner_unchecked(obj));
>> +            &mut p.bo
>> +        },
>> +        Err(_) => core::ptr::null_mut(),
>> +    }
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function is only safe to be called from the GPUVM C code.
>> +unsafe extern "C" fn vm_bo_free_callback<T: DriverGpuVm>(raw_vm_bo: *mut bindings::drm_gpuvm_bo) {
>> +    // SAFETY: Container invariant is guaranteed for objects using this callback.
>> +    let p = unsafe { crate::container_of!(raw_vm_bo, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
>> +
>> +    // SAFETY: p is guaranteed to be valid for drm_gpuvm_bo objects using this callback.
>> +    unsafe { drop(KBox::from_raw(p)) };
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function is only safe to be called from the GPUVM C code.
>> +unsafe extern "C" fn step_map_callback<T: DriverGpuVm>(
>> +    op: *mut bindings::drm_gpuva_op,
>> +    _priv: *mut core::ffi::c_void,
>> +) -> core::ffi::c_int {
>> +    // SAFETY: We know this is a map op, and OpMap is a transparent wrapper.
>> +    let map = unsafe { &mut *((&mut (*op).__bindgen_anon_1.map) as *mut _ as *mut OpMap<T>) };
> 
> Please don't create (mutable) references to FFI objects, this happens in various
> places.

Sure

> 
>> +    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
>> +    // guaranteed to outlive this function.
>> +    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
>> +
>> +    from_result(|| {
>> +        UpdatingGpuVm(ctx.gpuvm).step_map(map, ctx.ctx)?;
>> +        Ok(0)
>> +    })
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function is only safe to be called from the GPUVM C code.
>> +unsafe extern "C" fn step_remap_callback<T: DriverGpuVm>(
>> +    op: *mut bindings::drm_gpuva_op,
>> +    _priv: *mut core::ffi::c_void,
>> +) -> core::ffi::c_int {
>> +    // SAFETY: We know this is a map op, and OpReMap is a transparent wrapper.
>> +    let remap = unsafe { &mut *((&mut (*op).__bindgen_anon_1.remap) as *mut _ as *mut OpReMap<T>) };
>> +    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
>> +    // guaranteed to outlive this function.
>> +    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
>> +
>> +    let p_vm_bo = remap.unmap().va().unwrap().gpuva.vm_bo;
>> +
>> +    let res = {
>> +        // SAFETY: vm_bo pointer must be valid and non-null by the step_remap invariants.
>> +        // Since we grab a ref, this reference's lifetime is until the decref.
>> +        let vm_bo_ref = unsafe {
>> +            bindings::drm_gpuvm_bo_get(p_vm_bo);
>> +            &*(crate::container_of!(p_vm_bo, GpuVmBo<T>, bo) as *mut GpuVmBo<T>)
>> +        };
>> +
>> +        from_result(|| {
>> +            UpdatingGpuVm(ctx.gpuvm).step_remap(remap, vm_bo_ref, ctx.ctx)?;
>> +            Ok(0)
>> +        })
>> +    };
>> +
>> +    // SAFETY: We incremented the refcount above, and the Rust reference we took is
>> +    // no longer in scope.
>> +    unsafe { bindings::drm_gpuvm_bo_put(p_vm_bo) };
>> +
>> +    res
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function is only safe to be called from the GPUVM C code.
>> +unsafe extern "C" fn step_unmap_callback<T: DriverGpuVm>(
>> +    op: *mut bindings::drm_gpuva_op,
>> +    _priv: *mut core::ffi::c_void,
>> +) -> core::ffi::c_int {
>> +    // SAFETY: We know this is a map op, and OpUnMap is a transparent wrapper.
>> +    let unmap = unsafe { &mut *((&mut (*op).__bindgen_anon_1.unmap) as *mut _ as *mut OpUnMap<T>) };
>> +    // SAFETY: This is a pointer to a StepContext created inline in sm_map(), which is
>> +    // guaranteed to outlive this function.
>> +    let ctx = unsafe { &mut *(_priv as *mut StepContext<'_, T>) };
>> +
>> +    from_result(|| {
>> +        UpdatingGpuVm(ctx.gpuvm).step_unmap(unmap, ctx.ctx)?;
>> +        Ok(0)
>> +    })
>> +}
>> +
>> +impl<T: DriverGpuVm> GpuVm<T> {
>> +    const OPS: bindings::drm_gpuvm_ops = bindings::drm_gpuvm_ops {
>> +        vm_free: Some(vm_free_callback::<T>),
>> +        op_alloc: None,
>> +        op_free: None,
>> +        vm_bo_alloc: Some(vm_bo_alloc_callback::<T>),
>> +        vm_bo_free: Some(vm_bo_free_callback::<T>),
>> +        vm_bo_validate: None,
>> +        sm_step_map: Some(step_map_callback::<T>),
>> +        sm_step_remap: Some(step_remap_callback::<T>),
>> +        sm_step_unmap: Some(step_unmap_callback::<T>),
>> +    };
>> +
>> +    fn gpuvm(&self) -> *const bindings::drm_gpuvm {
>> +        self.gpuvm.get()
>> +    }
>> +
>> +    /// Creates a GPUVM instance.
>> +    pub fn new<E>(
>> +        name: &'static CStr,
>> +        dev: &device::Device<T::Driver>,
> 
> You can use drm::Device instead.
> 
>> +        r_obj: &<T::Driver as drv::Driver>::Object,
>> +        range: Range<u64>,
>> +        reserve_range: Range<u64>,
>> +        inner: impl PinInit<T, E>,
>> +    ) -> Result<ARef<GpuVm<T>>>
>> +    where
>> +        Error: From<E>,
>> +    {
>> +        let obj: Pin<KBox<Self>> = KBox::try_pin_init(
>> +            try_pin_init!(Self {
>> +                // SAFETY: drm_gpuvm_init cannot fail and always initializes the member.
>> +                gpuvm <- unsafe {
>> +                    init::pin_init_from_closure(move |slot: *mut Opaque<bindings::drm_gpuvm> | {
>> +                        // Zero-init required by drm_gpuvm_init.
>> +                        *slot = core::mem::zeroed();
>> +                        bindings::drm_gpuvm_init(
>> +                            Opaque::raw_get(slot),
>> +                            name.as_char_ptr(),
>> +                            0,
>> +                            dev.as_raw(),
>> +                            r_obj.gem_obj() as *const _ as *mut _,
>> +                            range.start,
>> +                            range.end - range.start,
>> +                            reserve_range.start,
>> +                            reserve_range.end - reserve_range.start,
>> +                            &Self::OPS
>> +                        );
>> +                        Ok(())
>> +                    })
>> +                },
>> +                // SAFETY: Just passing through to the initializer argument.
>> +                inner <- unsafe {
>> +                    init::pin_init_from_closure(move |slot: *mut UnsafeCell<T> | {
>> +                        inner.__pinned_init(slot as *mut _)
>> +                    })
>> +                },
>> +                _p: PhantomPinned
>> +            }),
>> +            GFP_KERNEL,
>> +        )?;
>> +
>> +        // SAFETY: We never move out of the object.
>> +        let vm_ref = unsafe {
>> +            ARef::from_raw(NonNull::new_unchecked(KBox::leak(
>> +                Pin::into_inner_unchecked(obj),
>> +            )))
>> +        };
>> +
>> +        Ok(vm_ref)
>> +    }
>> +
>> +    /// Locks the VM, protecting its interval tree against concurrent accesses.
>> +    ///
>> +    /// Callers must prove that they have exclusive access to the VM by holding
>> +    /// some guard type. This encodes the driver-specific locking requirements.
>> +    ///
>> +    /// It is up to the caller to ensure that the guard indeed provides the
>> +    /// required locking.
>> +    pub fn lock<U: ?Sized, B: Backend>(&self, _guard: &mut Guard<'_, U, B>) -> LockedGpuVm<'_, T> {
>> +        LockedGpuVm { gpuvm: self }
>> +    }
>> +
>> +    /// Returns true if the given object is external to the GPUVM, i.e.: if it
>> +    /// does not share the DMA reservation object of the GPUVM.
>> +    pub fn is_extobj(&self, obj: &impl IntoGEMObject) -> bool {
>> +        let gem = obj.gem_obj() as *const _ as *mut _;
>> +        // SAFETY: This is safe to call as long as the arguments are valid pointers.
>> +        unsafe { bindings::drm_gpuvm_is_extobj(self.gpuvm() as *mut _, gem) }
>> +    }
>> +}
>> +
>> +// SAFETY: DRM GpuVm objects are always reference counted and the get/put functions
>> +// satisfy the requirements.
>> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVm<T> {
>> +    fn inc_ref(&self) {
>> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
>> +        unsafe { bindings::drm_gpuvm_get(&self.gpuvm as *const _ as *mut _) };
>> +    }
>> +
>> +    unsafe fn dec_ref(obj: NonNull<Self>) {
>> +        // SAFETY: The drm_gpuvm_put function satisfies the requirements for dec_ref().
>> +        unsafe { bindings::drm_gpuvm_put(Opaque::raw_get(&(*obj.as_ptr()).gpuvm)) };
>> +    }
>> +}
>> +
>> +/// The object returned after a call to [`GpuVm::lock`].
>> +///
>> +/// This object has access to operations that modify the VM's interval tree.
>> +pub struct LockedGpuVm<'a, T: DriverGpuVm> {
>> +    gpuvm: &'a GpuVm<T>,
>> +}
>> +
>> +impl<T: DriverGpuVm> LockedGpuVm<'_, T> {
>> +    /// Finds the [`GpuVmBo`] object that connects `obj` to this VM.
>> +    ///
>> +    /// If found, increases the reference count of the GpuVmBo object
>> +    /// accordingly.
>> +    pub fn find_bo(&mut self, obj: &DriverObject<T>) -> Option<ARef<GpuVmBo<T>>> {
>> +        // SAFETY: LockedGpuVm implies the right locks are held.
>> +        let p = unsafe {
>> +            bindings::drm_gpuvm_bo_find(
>> +                self.gpuvm.gpuvm() as *mut _,
>> +                obj.gem_obj() as *const _ as *mut _,
>> +            )
>> +        };
>> +
>> +        if p.is_null() {
>> +            return None;
>> +        }
>> +
>> +        // SAFETY: All the drm_gpuvm_bo objects in this GpuVm are always
>> +        // allocated by us as GpuVmBo<T>.
>> +        let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
>> +        // SAFETY: We checked for NULL above, and the types ensure that
>> +        // this object was created by vm_bo_alloc_callback<T>.
>> +        Some(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
>> +    }
>> +
>> +    /// Obtains the [`GpuVmBo`] object that connects `obj` to this VM.
>> +    ///
>> +    /// This connection is unique, so an instane of [`GpuVmBo`] will be
>> +    /// allocated for `obj` once, and that instance will be returned from that
>> +    /// point forward.
>> +    pub fn obtain_bo(&mut self, obj: &DriverObject<T>) -> Result<ARef<GpuVmBo<T>>> {
>> +        // SAFETY: LockedGpuVm implies the right locks are held.
> 
> No, this must be locked by the dma-resv or the GEM gpuva lock, not by the
> GPUVM lock that protects the interval tree.

By “GEM gpuva lock” you’re referring to the custom lock which we
currently do not support, right?

This series currently rely on manual calls to dma_resv_{lock,unlock}, I wonder
if we should ditch that in favor of something written in Rust directly. This
would let us introduce a new type for GEM objects which are known to have
`resv` locked. WDYT?

> 
>> +        let p = unsafe {
>> +            bindings::drm_gpuvm_bo_obtain(
>> +                self.gpuvm.gpuvm() as *mut _,
>> +                obj.gem_obj() as *const _ as *mut _,
>> +            )
>> +        };
>> +
>> +        if p.is_null() {
>> +            return Err(ENOMEM);
>> +        }
>> +
>> +        // SAFETY: Container invariant is guaranteed for GpuVmBo objects for this GpuVm.
>> +        let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
>> +        // SAFETY: We checked for NULL above, and the types ensure that
>> +        // this object was created by vm_bo_alloc_callback<T>.
>> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
>> +    }
>> +
>> +    /// Iterates the given range of the GPU VA space.
>> +    ///
>> +    /// This utilizes [`DriverGpuVm`] to call back into the driver providing the
>> +    /// split and merge steps.
>> +    ///
>> +    /// A sequence of callbacks can contain map, unmap and remap operations, but
>> +    /// the sequence of callbacks might also be empty if no operation is
>> +    /// required, e.g. if the requested mapping already exists in the exact same
>> +    /// way.
>> +    ///
>> +    /// There can be an arbitrary amount of unmap operations, a maximum of two
>> +    /// remap operations and a single map operation. The latter one represents
>> +    /// the original map operation requested by the caller.
>> +    ///
>> +    /// # Arguments
>> +    ///
>> +    /// - `ctx`: A driver-specific context.
>> +    /// - `req_obj`: The GEM object to map.
>> +    /// - `req_addr`: The start address of the new mapping.
>> +    /// - `req_range`: The range of the mapping.
>> +    /// - `req_offset`: The offset into the GEM object.
>> +    pub fn sm_map(
>> +        &mut self,
>> +        ctx: &mut T::StepContext,
>> +        req_obj: &DriverObject<T>,
>> +        req_addr: u64,
>> +        req_range: u64,
>> +        req_offset: u64,
>> +    ) -> Result {
>> +        let mut ctx = StepContext {
>> +            ctx,
>> +            gpuvm: self.gpuvm,
>> +        };
>> +
>> +        // SAFETY: LockedGpuVm implies the right locks are held.
>> +        to_result(unsafe {
>> +            bindings::drm_gpuvm_sm_map(
>> +                self.gpuvm.gpuvm() as *mut _,
>> +                &mut ctx as *mut _ as *mut _,
>> +                req_addr,
>> +                req_range,
>> +                req_obj.gem_obj() as *const _ as *mut _,
>> +                req_offset,
>> +            )
>> +        })
>> +    }
>> +
>> +    /// Iterates the given range of the GPU VA space.
>> +    ///
>> +    /// This utilizes [`DriverGpuVm`] to call back into the driver providing the
>> +    /// operations to unmap and, if required, split existent mappings.
>> +    ///
>> +    /// A sequence of callbacks can contain unmap and remap operations,
>> +    /// depending on whether there are actual overlapping mappings to split.
>> +    ///
>> +    /// There can be an arbitrary amount of unmap operations and a maximum of
>> +    /// two remap operations.
>> +    ///
>> +    /// # Arguments
>> +    ///
>> +    /// - `ctx`: A driver-specific context.
>> +    /// - `req_addr`: The start address of the range to unmap.
>> +    /// - `req_range`: The range of the mappings to unmap.
>> +    pub fn sm_unmap(&mut self, ctx: &mut T::StepContext, req_addr: u64, req_range: u64) -> Result {
>> +        let mut ctx = StepContext {
>> +            ctx,
>> +            gpuvm: self.gpuvm,
>> +        };
>> +
>> +        // SAFETY: LockedGpuVm implies the right locks are held.
>> +        to_result(unsafe {
>> +            bindings::drm_gpuvm_sm_unmap(
>> +                self.gpuvm.gpuvm() as *mut _,
>> +                &mut ctx as *mut _ as *mut _,
>> +                req_addr,
>> +                req_range,
>> +            )
>> +        })
>> +    }
>> +}
>> +
>> +impl<T: DriverGpuVm> Deref for LockedGpuVm<'_, T> {
>> +    type Target = T;
>> +
>> +    fn deref(&self) -> &T {
>> +        // SAFETY: The existence of this LockedGpuVm implies the lock is held,
>> +        // so this is the only reference.
>> +        unsafe { &*self.gpuvm.inner.get() }
>> +    }
>> +}
>> +
>> +impl<T: DriverGpuVm> DerefMut for LockedGpuVm<'_, T> {
>> +    fn deref_mut(&mut self) -> &mut T {
>> +        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
>> +        // so this is the only reference.
>> +        unsafe { &mut *self.gpuvm.inner.get() }
>> +    }
>> +}
>> +
>> +/// A state representing a GPU VM that is being updated.
>> +pub struct UpdatingGpuVm<'a, T: DriverGpuVm>(&'a GpuVm<T>);
> 
> What does it mean to update a GPUVM instance? How is it different compared to a
> LockedGpuVm?

Perhaps this type can go away entirely in favor of LockedGpuVm.

> 
>> +
>> +impl<T: DriverGpuVm> UpdatingGpuVm<'_, T> {}
>> +
>> +impl<T: DriverGpuVm> Deref for UpdatingGpuVm<'_, T> {
>> +    type Target = T;
>> +
>> +    fn deref(&self) -> &T {
>> +        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
>> +        // so this is the only reference.
>> +        unsafe { &*self.0.inner.get() }
>> +    }
>> +}
>> +
>> +impl<T: DriverGpuVm> DerefMut for UpdatingGpuVm<'_, T> {
>> +    fn deref_mut(&mut self) -> &mut T {
>> +        // SAFETY: The existence of this UpdatingGpuVm implies the lock is held,
>> +        // so this is the only reference.
>> +        unsafe { &mut *self.0.inner.get() }
>> +    }
>> +}
>> +
>> +// SAFETY: All our trait methods take locks.
>> +unsafe impl<T: DriverGpuVm> Sync for GpuVm<T> {}
>> +// SAFETY: All our trait methods take locks.
>> +unsafe impl<T: DriverGpuVm> Send for GpuVm<T> {}
>> +
>> +// SAFETY: All our trait methods take locks.
>> +unsafe impl<T: DriverGpuVm> Sync for GpuVmBo<T> {}
>> +// SAFETY: All our trait methods take locks.
>> +unsafe impl<T: DriverGpuVm> Send for GpuVmBo<T> {}
>> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
>> index c44760a1332fa1ef875939b48e7af450f7372020..849dc1e577f15bfada11d6739dff48ac33813326 100644
>> --- a/rust/kernel/drm/mod.rs
>> +++ b/rust/kernel/drm/mod.rs
>> @@ -6,4 +6,6 @@
>> pub mod drv;
>> pub mod file;
>> pub mod gem;
>> +#[cfg(CONFIG_DRM_GPUVM = "y")]
>> +pub mod gpuvm;
>> pub mod ioctl;
>> 
>> -- 
>> 2.49.0

— Daniel

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Danilo Krummrich]

On Wed, Apr 23, 2025 at 10:29:01AM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> FYI, most of this patch still retains the original code from the Asahi project,
> 
> > On 22 Apr 2025, at 18:16, Danilo Krummrich <dakr@kernel.org> wrote:
> > 
> > (Not a full review, but some things that took my attention from a brief look.)
> > 
> > On Tue, Apr 22, 2025 at 01:41:53PM -0300, Daniel Almeida wrote:
> 
> > 
> >> +        &mut self,
> >> +        gpuvm: &mut UpdatingGpuVm<'_, T>,
> >> +        gpuva: Pin<KBox<GpuVa<T>>>,
> >> +        gpuvmbo: &ARef<GpuVmBo<T>>,
> >> +    ) -> Result<(), Pin<KBox<GpuVa<T>>>> {
> >> +        // SAFETY: We are handing off the GpuVa ownership and it will not be moved.
> >> +        let p = KBox::leak(unsafe { Pin::into_inner_unchecked(gpuva) });
> >> +        // SAFETY: These C functions are called with the correct invariants
> >> +        unsafe {
> >> +            bindings::drm_gpuva_init_from_op(&mut p.gpuva, &mut self.0);
> >> +            if bindings::drm_gpuva_insert(gpuvm.0.gpuvm() as *mut _, &mut p.gpuva) != 0 {
> >> +                // EEXIST, return the GpuVa to the caller as an error
> >> +                return Err(Pin::new_unchecked(KBox::from_raw(p)));
> >> +            };
> > 
> > How do you ensure that the VM lock is held when there's a list of operations?
> 
> Are you saying that this will not work for async binds, or that it’s already broken in sync binds too?

I think here it's fine since sm_map() is implemented for LockedGpuVm, and that
seems to be the only place where you hand out a reference to an OpMap. But this
falls apart once we give out sets of operations, e.g. as list.

Even though, your patch doesn't do that yet, that's where we're heading to
eventually. Hence, the initial design must consider those things, otherwise
we only end up with something upstream that needs to be rewritten.

> 
> Perhaps we can get rid of this `UpdatingGpuVm` type and pass `LockedGpuVm` instead?

At a first glance I didn't get what's the difference between the two anyways.
But I don't think it's really related to the problem above, is it?

> 
> > 
> >> +            // SAFETY: This takes a new reference to the gpuvmbo.
> >> +            bindings::drm_gpuva_link(&mut p.gpuva, &gpuvmbo.bo as *const _ as *mut _);
> > 
> > How do you ensure that the dma_resv lock is held?
> 
> Right, I totally missed that.
> 
> > 
> >> +        }
> >> +        Ok(())
> >> +    }
> >> +}

<snip>

> >> +// SAFETY: DRM GpuVmBo objects are always reference counted and the get/put functions
> >> +// satisfy the requirements.
> >> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
> >> +    fn inc_ref(&self) {
> >> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
> >> +        unsafe { bindings::drm_gpuvm_bo_get(&self.bo as *const _ as *mut _) };
> >> +    }
> >> +
> >> +    unsafe fn dec_ref(mut obj: NonNull<Self>) {
> >> +        // SAFETY: drm_gpuvm_bo_put() requires holding the gpuva lock, which is
> >> +        // the dma_resv lock by default.
> >> +        // The drm_gpuvm_put function satisfies the requirements for dec_ref().
> >> +        // (We do not support custom locks yet.)
> >> +        unsafe {
> >> +            let resv = (*obj.as_mut().bo.obj).resv;
> >> +            bindings::dma_resv_lock(resv, core::ptr::null_mut());
> >> +            bindings::drm_gpuvm_bo_put(&mut obj.as_mut().bo);
> >> +            bindings::dma_resv_unlock(resv);
> > 
> > What if the resv_lock is held already? Please also make sure to put multiple
> > unsafe calls each in a separate unsafe block.
> 
> By whom?

The lock might be held already by the driver or by TTM when things are called
from TTM callbacks.

This is why GPUVM never takes locks by itself, but asserts that the correct lock
is held.

I think we really want to get proof by the driver by providing lock guard
references.

> See my comment about “[..] a new type for GEM objects which are known to be locked"
> below.

<snip>

> >> +
> >> +    /// Obtains the [`GpuVmBo`] object that connects `obj` to this VM.
> >> +    ///
> >> +    /// This connection is unique, so an instane of [`GpuVmBo`] will be
> >> +    /// allocated for `obj` once, and that instance will be returned from that
> >> +    /// point forward.
> >> +    pub fn obtain_bo(&mut self, obj: &DriverObject<T>) -> Result<ARef<GpuVmBo<T>>> {
> >> +        // SAFETY: LockedGpuVm implies the right locks are held.
> > 
> > No, this must be locked by the dma-resv or the GEM gpuva lock, not by the
> > GPUVM lock that protects the interval tree.
> 
> By “GEM gpuva lock” you’re referring to the custom lock which we
> currently do not support, right?

Yes.

> This series currently rely on manual calls to dma_resv_{lock,unlock}, I wonder
> if we should ditch that in favor of something written in Rust directly. This
> would let us introduce a new type for GEM objects which are known to have
> `resv` locked. WDYT?

Not all functions that require the dma-resv lock to be held are called with a
GEM object parameter, it could also be a struct drm_gpuvm_bo, struct drm_gpuva
or struct drm_gpuvm, since they all carry GEM object pointers.

For reference, you can look for "_held" in drivers/gpu/drm/drm_gpuvm.c.

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Daniel Almeida]

> 
> The lock might be held already by the driver or by TTM when things are called
> from TTM callbacks.
> 
> This is why GPUVM never takes locks by itself, but asserts that the correct lock
> is held.
> 
> I think we really want to get proof by the driver by providing lock guard
> references.
> 
>> See my comment about “[..] a new type for GEM objects which are known to be locked"
>> below.
> 
> <snip>
> 
>>>> +
>>>> +    /// Obtains the [`GpuVmBo`] object that connects `obj` to this VM.
>>>> +    ///
>>>> +    /// This connection is unique, so an instane of [`GpuVmBo`] will be
>>>> +    /// allocated for `obj` once, and that instance will be returned from that
>>>> +    /// point forward.
>>>> +    pub fn obtain_bo(&mut self, obj: &DriverObject<T>) -> Result<ARef<GpuVmBo<T>>> {
>>>> +        // SAFETY: LockedGpuVm implies the right locks are held.
>>> 
>>> No, this must be locked by the dma-resv or the GEM gpuva lock, not by the
>>> GPUVM lock that protects the interval tree.
>> 
>> By “GEM gpuva lock” you’re referring to the custom lock which we
>> currently do not support, right?
> 
> Yes.
> 
>> This series currently rely on manual calls to dma_resv_{lock,unlock}, I wonder
>> if we should ditch that in favor of something written in Rust directly. This
>> would let us introduce a new type for GEM objects which are known to have
>> `resv` locked. WDYT?
> 
> Not all functions that require the dma-resv lock to be held are called with a
> GEM object parameter, it could also be a struct drm_gpuvm_bo, struct drm_gpuva
> or struct drm_gpuvm, since they all carry GEM object pointers.
> 
> For reference, you can look for "_held" in drivers/gpu/drm/drm_gpuvm.c.
> 

Looking at Lyude’s (excellent) KMS series, one thing that comes to mind is
using Lock::from_raw() on the dma-resv lock. This will build a rust Mutex that
we can then assert to be locked (or fail with an Error otherwise).

See [0] for the specific patch whose idea I want to copy.

Can I get a +1 on this idea before pursuing it?

-- Daniel

[0] https://lore.kernel.org/rust-for-linux/20250305230406.567126-10-lyude@redhat.com/

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Daniel Almeida]

Hi Danilo,

> The lock might be held already by the driver or by TTM when things are called
> from TTM callbacks.
> 
> This is why GPUVM never takes locks by itself, but asserts that the correct lock
> is held.
> 
> I think we really want to get proof by the driver by providing lock guard
> references.

Can’t we add a lock to our Rust GpuVmBo type?

This is already supported by the C code, since it asks driver to either provide
a custom lock _or_ use the bo's resv. So what I am suggesting here is the
former, except that said lock would be transparently managed by our Rust GPUVM
abstraction.

By using our own lock, we forbid drivers from introducing races.

Another option is to also require a(nother) Guard when mutating the BO's VA
list, but I find this a bit cumbersome for a couple of reasons:

a) It's proving a bit difficult to provide said Guard for the interval tree
itself,

b) This will appear in all functions where the lock should be taken, which
pollutes the API quite a bit.

c) Having "either a custom lock or the resv lock" sounds a bit confusing.
Handling this transparently in Rust makes the API easier to use (and harder to
misuse)

— Daniel

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Daniel Almeida]

Hi Danilo,

[…]

> 
>> +
>> +impl<T: DriverGpuVm> OpMap<T> {
>> +    /// Returns the base address of the new mapping.
>> +    #[inline]
>> +    pub fn addr(&self) -> u64 {
>> +        self.0.va.addr
>> +    }
>> +
>> +    /// Returns the range of the new mapping.
>> +    #[inline]
>> +    pub fn range(&self) -> u64 {
>> +        self.0.va.range
>> +    }
>> +
>> +    /// Returns the offset within the GEM object.
>> +    #[inline]
>> +    pub fn offset(&self) -> u64 {
>> +        self.0.gem.offset
>> +    }
>> +
>> +    /// Returns the GEM object to map.
>> +    #[inline]
>> +    pub fn object(&self) -> &<T::Driver as drv::Driver>::Object {
> 
> You can use drm::Driver instead, which reads much better.

Can you expand a bit on this?


— Daniel

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Daniel Almeida]

Danilo,


> <snip>
> 
>>>> +// SAFETY: DRM GpuVmBo objects are always reference counted and the get/put functions
>>>> +// satisfy the requirements.
>>>> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
>>>> +    fn inc_ref(&self) {
>>>> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
>>>> +        unsafe { bindings::drm_gpuvm_bo_get(&self.bo as *const _ as *mut _) };
>>>> +    }
>>>> +
>>>> +    unsafe fn dec_ref(mut obj: NonNull<Self>) {
>>>> +        // SAFETY: drm_gpuvm_bo_put() requires holding the gpuva lock, which is
>>>> +        // the dma_resv lock by default.
>>>> +        // The drm_gpuvm_put function satisfies the requirements for dec_ref().
>>>> +        // (We do not support custom locks yet.)
>>>> +        unsafe {
>>>> +            let resv = (*obj.as_mut().bo.obj).resv;
>>>> +            bindings::dma_resv_lock(resv, core::ptr::null_mut());
>>>> +            bindings::drm_gpuvm_bo_put(&mut obj.as_mut().bo);
>>>> +            bindings::dma_resv_unlock(resv);
>>> 
>>> What if the resv_lock is held already? Please also make sure to put multiple
>>> unsafe calls each in a separate unsafe block.
>> 
>> By whom?
> 
> The lock might be held already by the driver or by TTM when things are called
> from TTM callbacks.
> 
> This is why GPUVM never takes locks by itself, but asserts that the correct lock
> is held.
> 
> I think we really want to get proof by the driver by providing lock guard
> references.
> 

There doesn’t seem to be a solution that fits all the boxes here.

As you said, at this point the current status of the resv is unknown. If we
simply assume that it is not taken, we run into the problem you pointed out:
i.e.: recursive locking where ttm or some other layer has the lock already.

Alternatively, if we assume that the resv must be locked in dec_ref(), then we
may build a lock::Guard from it and assert that it is held, but in any case
it's very confusing to expect the reservation to be locked on a dec_ref() call.

The fact that dec_ref() is placed automatically on drop will massively
complicate the call sites:

We will have to ensure that the resv is locked at all times where we interface
with a GpuVmBo, because each of these points could possibly be the last active
ref. If we don't, then we've introduced a race where the list is modified but
no lock is taken, which will be a pretty easy mistake to make. This seems to
also be the case in C, which we should try to improve upon.

My suggestion is to introduce a separate GPU-VA lock here:

/// A base GEM object.
#[repr(C)]
#[pin_data]
pub struct Object<T: DriverObject> {
    obj: bindings::drm_gem_object,
    // The DRM core ensures the Device exists as long as its objects exist, so we don't need to
    // manage the reference count here.
    dev: *const bindings::drm_device,
    #[pin]
    inner: T,
    #[pin]
    _p: PhantomPinned,
    // Add a GPU-VA lock here <--------
}

And only support custom locks in Rust, to the detriment of the optimization
where the resv is used and to the detriment of any perf improvements that
reusing the reservation might bring to the table.

Notice that this would sidestep this entire discussion: nobody else would be
aware of this new lock so we could safely lock it in dec_ref(). We would also
be transparently managing the locking on behalf of drivers in all the other
calls where the VA list is accessed, which is another plus as I said above.

I understand that most C drivers do not need an extra lock, but it's getting
hard to emulate this behavior in Rust.

Also, the fact that they don't need an extra lock does not invalidate the fact
that it would be simply safer to have this extra lock anyways. In other words,
it is still completely possible to use GPUVM without locking anything and IMHO
we shouldn't bring this over if we can help it.

— Daniel

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Boris Brezillon]

Hi,

On Fri, 13 Jun 2025 13:42:59 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> Danilo,
> 
> 
> > <snip>
> >   
> >>>> +// SAFETY: DRM GpuVmBo objects are always reference counted and the get/put functions
> >>>> +// satisfy the requirements.
> >>>> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
> >>>> +    fn inc_ref(&self) {
> >>>> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
> >>>> +        unsafe { bindings::drm_gpuvm_bo_get(&self.bo as *const _ as *mut _) };
> >>>> +    }
> >>>> +
> >>>> +    unsafe fn dec_ref(mut obj: NonNull<Self>) {
> >>>> +        // SAFETY: drm_gpuvm_bo_put() requires holding the gpuva lock, which is
> >>>> +        // the dma_resv lock by default.
> >>>> +        // The drm_gpuvm_put function satisfies the requirements for dec_ref().
> >>>> +        // (We do not support custom locks yet.)
> >>>> +        unsafe {
> >>>> +            let resv = (*obj.as_mut().bo.obj).resv;
> >>>> +            bindings::dma_resv_lock(resv, core::ptr::null_mut());
> >>>> +            bindings::drm_gpuvm_bo_put(&mut obj.as_mut().bo);
> >>>> +            bindings::dma_resv_unlock(resv);  
> >>> 
> >>> What if the resv_lock is held already? Please also make sure to put multiple
> >>> unsafe calls each in a separate unsafe block.  
> >> 
> >> By whom?  
> > 
> > The lock might be held already by the driver or by TTM when things are called
> > from TTM callbacks.
> > 
> > This is why GPUVM never takes locks by itself, but asserts that the correct lock
> > is held.
> > 
> > I think we really want to get proof by the driver by providing lock guard
> > references.
> >   
> 
> There doesn’t seem to be a solution that fits all the boxes here.
> 
> As you said, at this point the current status of the resv is unknown. If we
> simply assume that it is not taken, we run into the problem you pointed out:
> i.e.: recursive locking where ttm or some other layer has the lock already.
> 
> Alternatively, if we assume that the resv must be locked in dec_ref(), then we
> may build a lock::Guard from it and assert that it is held, but in any case
> it's very confusing to expect the reservation to be locked on a dec_ref() call.
> 
> The fact that dec_ref() is placed automatically on drop will massively
> complicate the call sites:

I'm digressing, but there's an aspect I found very annoying in the C
version of the API: the fact that we have to take a BO ref, then lock,
then release the vm_bo [1], because otherwise the vm_bo might be the
last owner of a BO ref leading to a UAF on the lock itself. This to me,
denotes a lifetime issue that I think would be good to address in the
rust version of the API.

It's not exactly the same problem, but I think it comes from the same
root issue: lax ownership definition. By that I mean it's not clear
who's the owner and who's the owned. gem_object::gpuva::list has
weak refs on the vm_bos contained in this list, which kinda makes sense
because vm_bos themselves have a ref on the gem_object, and if we were
to make this weak ref a strong ref we'd never free any of these two
objects. The lock is also part of the BO (either the BO resv lock, or a
custom lock), and since it's the very same lock we use to insert/remove
vm_bos, that's problematic.

If we were making the gpuvm_bo_list a separate object that's originally
created by the BO, and then let the GPUVM layer manipulate only this
list, it could work. Of course that means the resv lock/driver custom
lock should come from this object too, and I'm not too sure that's an
option when dma_buf imports are involved.

> 
> We will have to ensure that the resv is locked at all times where we interface
> with a GpuVmBo, because each of these points could possibly be the last active
> ref. If we don't, then we've introduced a race where the list is modified but
> no lock is taken, which will be a pretty easy mistake to make. This seems to
> also be the case in C, which we should try to improve upon.

Yep, with auto-unref thrown into the mix you have to be very careful on
which paths might release the last vm_bo ref, and make sure an extra
ref is taken on the BO, and the resv/custom lock is held when that
happens.

> 
> My suggestion is to introduce a separate GPU-VA lock here:
> 
> /// A base GEM object.
> #[repr(C)]
> #[pin_data]
> pub struct Object<T: DriverObject> {
>     obj: bindings::drm_gem_object,
>     // The DRM core ensures the Device exists as long as its objects exist, so we don't need to
>     // manage the reference count here.
>     dev: *const bindings::drm_device,
>     #[pin]
>     inner: T,
>     #[pin]
>     _p: PhantomPinned,
>     // Add a GPU-VA lock here <--------
> }
> 
> And only support custom locks in Rust, to the detriment of the optimization
> where the resv is used and to the detriment of any perf improvements that
> reusing the reservation might bring to the table.

Yes, if it was only about perf optimizations, then I'd like to see
numbers that prove taking an extra lock that's always going to be free
in a path where you already took the BO resv lock actually makes a
difference, and honestly, I doubt it. But my fear is that it's not so
much about avoiding an extra lock to be taken, and more about making
sure this list insertion/deletion doesn't race with other paths that
are assuming that taking the resv lock is enough to guarantee exclusive
access to this vm_bo list (I mean places outside gpuvm, in the drivers
directly). I guess the is fixable.

> 
> Notice that this would sidestep this entire discussion: nobody else would be
> aware of this new lock so we could safely lock it in dec_ref(). We would also
> be transparently managing the locking on behalf of drivers in all the other
> calls where the VA list is accessed, which is another plus as I said above.

If the lock is part of the gem_object, it's not solving the problem I
described above, because you might be taking a lock that disappears if
you don't take a BO ref before taking the lock. In the end, that's
still a risky business.

> 
> I understand that most C drivers do not need an extra lock, but it's getting
> hard to emulate this behavior in Rust.
> 
> Also, the fact that they don't need an extra lock does not invalidate the fact
> that it would be simply safer to have this extra lock anyways. In other words,
> it is still completely possible to use GPUVM without locking anything and IMHO
> we shouldn't bring this over if we can help it.

Overall, I do agree with Daniel here. We'd rather think about how to
make the C API more user-friendly by clearly defining
ownership/lifetime before we try to add rust bindings on top.
Deciding where the lock comes from is part of the discussion, but IMHO,
that's not the only thing we need to sort out.

I hope that me chiming in didn't make the situation worse :-/, and I'd
be fine if someone convince me that what I complain about here is
actually not a problem in rust :-).

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v6.15.2/source/drivers/gpu/drm/panthor/panthor_mmu.c#L1090

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Daniel Almeida]

Hi Boris,

> On 19 Jun 2025, at 08:57, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> Hi,
> 
> On Fri, 13 Jun 2025 13:42:59 -0300
> Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
>> Danilo,
>> 
>> 
>>> <snip>
>>> 
>>>>>> +// SAFETY: DRM GpuVmBo objects are always reference counted and the get/put functions
>>>>>> +// satisfy the requirements.
>>>>>> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
>>>>>> +    fn inc_ref(&self) {
>>>>>> +        // SAFETY: The drm_gpuvm_get function satisfies the requirements for inc_ref().
>>>>>> +        unsafe { bindings::drm_gpuvm_bo_get(&self.bo as *const _ as *mut _) };
>>>>>> +    }
>>>>>> +
>>>>>> +    unsafe fn dec_ref(mut obj: NonNull<Self>) {
>>>>>> +        // SAFETY: drm_gpuvm_bo_put() requires holding the gpuva lock, which is
>>>>>> +        // the dma_resv lock by default.
>>>>>> +        // The drm_gpuvm_put function satisfies the requirements for dec_ref().
>>>>>> +        // (We do not support custom locks yet.)
>>>>>> +        unsafe {
>>>>>> +            let resv = (*obj.as_mut().bo.obj).resv;
>>>>>> +            bindings::dma_resv_lock(resv, core::ptr::null_mut());
>>>>>> +            bindings::drm_gpuvm_bo_put(&mut obj.as_mut().bo);
>>>>>> +            bindings::dma_resv_unlock(resv);  
>>>>> 
>>>>> What if the resv_lock is held already? Please also make sure to put multiple
>>>>> unsafe calls each in a separate unsafe block.  
>>>> 
>>>> By whom?  
>>> 
>>> The lock might be held already by the driver or by TTM when things are called
>>> from TTM callbacks.
>>> 
>>> This is why GPUVM never takes locks by itself, but asserts that the correct lock
>>> is held.
>>> 
>>> I think we really want to get proof by the driver by providing lock guard
>>> references.
>>> 
>> 
>> There doesn’t seem to be a solution that fits all the boxes here.
>> 
>> As you said, at this point the current status of the resv is unknown. If we
>> simply assume that it is not taken, we run into the problem you pointed out:
>> i.e.: recursive locking where ttm or some other layer has the lock already.
>> 
>> Alternatively, if we assume that the resv must be locked in dec_ref(), then we
>> may build a lock::Guard from it and assert that it is held, but in any case
>> it's very confusing to expect the reservation to be locked on a dec_ref() call.
>> 
>> The fact that dec_ref() is placed automatically on drop will massively
>> complicate the call sites:
> 
> I'm digressing, but there's an aspect I found very annoying in the C
> version of the API: the fact that we have to take a BO ref, then lock,
> then release the vm_bo [1], because otherwise the vm_bo might be the
> last owner of a BO ref leading to a UAF on the lock itself. This to me,
> denotes a lifetime issue that I think would be good to address in the
> rust version of the API.
> 
> It's not exactly the same problem, but I think it comes from the same
> root issue: lax ownership definition. By that I mean it's not clear
> who's the owner and who's the owned. gem_object::gpuva::list has
> weak refs on the vm_bos contained in this list, which kinda makes sense
> because vm_bos themselves have a ref on the gem_object, and if we were
> to make this weak ref a strong ref we'd never free any of these two
> objects. The lock is also part of the BO (either the BO resv lock, or a
> custom lock), and since it's the very same lock we use to insert/remove
> vm_bos, that's problematic.
> 
> If we were making the gpuvm_bo_list a separate object that's originally
> created by the BO, and then let the GPUVM layer manipulate only this
> list, it could work. Of course that means the resv lock/driver custom
> lock should come from this object too, and I'm not too sure that's an
> option when dma_buf imports are involved.

This would require changes to the C side, because the C helpers expect the gpuva
list to be inside drm_gem_object, and these helpers are used in the Rust
bindings.

Don't get me wrong, IMHO we can definitely pursue this if needed, or if it's an
improvement. I am merely stating the fact that it's a more elaborate solution
that what I had originally proposed so that we are all aware.

> 
>> 
>> We will have to ensure that the resv is locked at all times where we interface
>> with a GpuVmBo, because each of these points could possibly be the last active
>> ref. If we don't, then we've introduced a race where the list is modified but
>> no lock is taken, which will be a pretty easy mistake to make. This seems to
>> also be the case in C, which we should try to improve upon.
> 
> Yep, with auto-unref thrown into the mix you have to be very careful on
> which paths might release the last vm_bo ref, and make sure an extra
> ref is taken on the BO, and the resv/custom lock is held when that
> happens.
> 
>> 
>> My suggestion is to introduce a separate GPU-VA lock here:
>> 
>> /// A base GEM object.
>> #[repr(C)]
>> #[pin_data]
>> pub struct Object<T: DriverObject> {
>>    obj: bindings::drm_gem_object,
>>    // The DRM core ensures the Device exists as long as its objects exist, so we don't need to
>>    // manage the reference count here.
>>    dev: *const bindings::drm_device,
>>    #[pin]
>>    inner: T,
>>    #[pin]
>>    _p: PhantomPinned,
>>    // Add a GPU-VA lock here <--------
>> }
>> 
>> And only support custom locks in Rust, to the detriment of the optimization
>> where the resv is used and to the detriment of any perf improvements that
>> reusing the reservation might bring to the table.
> 
> Yes, if it was only about perf optimizations, then I'd like to see
> numbers that prove taking an extra lock that's always going to be free
> in a path where you already took the BO resv lock actually makes a
> difference, and honestly, I doubt it. But my fear is that it's not so
> much about avoiding an extra lock to be taken, and more about making
> sure this list insertion/deletion doesn't race with other paths that
> are assuming that taking the resv lock is enough to guarantee exclusive
> access to this vm_bo list (I mean places outside gpuvm, in the drivers
> directly). I guess the is fixable.
> 
>> 
>> Notice that this would sidestep this entire discussion: nobody else would be
>> aware of this new lock so we could safely lock it in dec_ref(). We would also
>> be transparently managing the locking on behalf of drivers in all the other
>> calls where the VA list is accessed, which is another plus as I said above.
> 
> If the lock is part of the gem_object, it's not solving the problem I
> described above, because you might be taking a lock that disappears if
> you don't take a BO ref before taking the lock. In the end, that's
> still a risky business.

I must confess I was not aware of the issue you're discussing. We can add an
instance of gem::ObjectRef in gpuvm::GpuVmBo if you think this will solve it.
This will make sure that the vmbo owns the gem object (or rather that it has a
+1 on the refcount) for its lifetime. That seems to be the semantics you're
looking for?

I thought about adding this lock to the Rust wrapper for the vmbo, but then
we'd run into the issue of shared BOs, where each would be connected to a
different vmbo and would thus take a different lock when attempting to modify
the same underlying gpuva list.

Just to be clear, by shared BO here I specifically mean a BO that's connected
to more than one VM, which is possible, IIUC.

— Daniel

Re: [PATCH v2 2/2] rust: drm: Add GPUVM abstraction

[Danilo Krummrich]

On Thu, Jun 19, 2025 at 01:57:09PM +0200, Boris Brezillon wrote:
> I'm digressing, but there's an aspect I found very annoying in the C
> version of the API: the fact that we have to take a BO ref, then lock,
> then release the vm_bo [1], because otherwise the vm_bo might be the
> last owner of a BO ref leading to a UAF on the lock itself. This to me,
> denotes a lifetime issue that I think would be good to address in the
> rust version of the API.
> 
> It's not exactly the same problem, but I think it comes from the same
> root issue: lax ownership definition. By that I mean it's not clear
> who's the owner and who's the owned. gem_object::gpuva::list has
> weak refs on the vm_bos contained in this list, which kinda makes sense
> because vm_bos themselves have a ref on the gem_object, and if we were
> to make this weak ref a strong ref we'd never free any of these two
> objects. The lock is also part of the BO (either the BO resv lock, or a
> custom lock), and since it's the very same lock we use to insert/remove
> vm_bos, that's problematic.

I think the ownership is actually well defined. A struct drm_gpuvm_bo holds a
reference of its DRM GEM object.

When the drm_gpuvm_bo is freed it needs to remove itself from the GEM objects
drm_gpuvm_bo list. This also guarantees that all list entries of the GEM object
are always valid and can be safely used as long as the list lock is held.

The reference count dance you describe does not come from the ownership model
(which, again, I think is fine), but it comes from the design decision to let
drivers take all required locks externally rather than locking things
internally. Or in other words typically GPUVM functions behave like a *_locked()
variant.

The main reason for that is that GPUVM had to be designed with a certain degree
of flexibility in order to be usable for different driver use-cases. For this
reason, the external locking approach was used to avoid drivers running into
lock inversion problems.

For instance, when we use a separate lock to protect the GEM objects list of
drm_gpuvm_bo objects, we can either iterate the list of objects from a scope
where the GEM objects dma_resv lock is already held (e.g. TTM callbacks), which
is a common use-case, or execute operations that have to grab a dma_resv lock
while iterating the GEM objects list of drm_gpuvm_bo objects.

Meanwhile, I think we have some experience in how drivers typically have to use
those APIs. And I think that in this case - as long as we keep the external
locking scheme for the dma_resv lock - we should be fine to make the lock that
protects the GEM objects drm_gpuvm_bo list an internal lock in the Rust
abstractions. For this, we can rename the existing, corresponding C functions
into *_locked() variants and provide functions with internal locking for both C
drivers and the Rust abstractions.

- Danilo