Re: [PATCH v3 6/6] rust: gpuvm: add GpuVmCore::sm_map()

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Alice,

> On 21 Jan 2026, at 08:31, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> Finally also add the operation for creating new mappings. Mapping
> operations need extra data in the context since they involve a vm_bo
> coming from the outside.
> 
> Co-developed-by: Asahi Lina <lina+kernel@asahilina.net>
> Signed-off-by: Asahi Lina <lina+kernel@asahilina.net>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/drm/gpuvm/mod.rs    |   9 ++-
> rust/kernel/drm/gpuvm/sm_ops.rs | 154 ++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 157 insertions(+), 6 deletions(-)
> 
> diff --git a/rust/kernel/drm/gpuvm/mod.rs b/rust/kernel/drm/gpuvm/mod.rs
> index 165a25666ccc3d62e59b73483d4eedff044423e9..557c0d629eec912a97fc4ef18495d5bf0807db0a 100644
> --- a/rust/kernel/drm/gpuvm/mod.rs
> +++ b/rust/kernel/drm/gpuvm/mod.rs
> @@ -93,7 +93,7 @@ const fn vtable() -> &'static bindings::drm_gpuvm_ops {
>             vm_bo_alloc: GpuVmBo::<T>::ALLOC_FN,
>             vm_bo_free: GpuVmBo::<T>::FREE_FN,
>             vm_bo_validate: None,
> -            sm_step_map: None,
> +            sm_step_map: Some(Self::sm_step_map),
>             sm_step_unmap: Some(Self::sm_step_unmap),
>             sm_step_remap: Some(Self::sm_step_remap),
>         }
> @@ -248,6 +248,13 @@ pub trait DriverGpuVm: Sized {
>     /// The private data passed to callbacks.
>     type SmContext<'ctx>;
> 
> +    /// Indicates that a new mapping should be created.
> +    fn sm_step_map<'op, 'ctx>(
> +        &mut self,
> +        op: OpMap<'op, Self>,
> +        context: &mut Self::SmContext<'ctx>,
> +    ) -> Result<OpMapped<'op, Self>, Error>;
> +
>     /// Indicates that an existing mapping should be removed.
>     fn sm_step_unmap<'op, 'ctx>(
>         &mut self,
> diff --git a/rust/kernel/drm/gpuvm/sm_ops.rs b/rust/kernel/drm/gpuvm/sm_ops.rs
> index 3c29d10d63f0b0a1976c714a86d486948ba81a15..5f3c5d3918147a6962e5658443c343835baa10b8 100644
> --- a/rust/kernel/drm/gpuvm/sm_ops.rs
> +++ b/rust/kernel/drm/gpuvm/sm_ops.rs
> @@ -8,6 +8,100 @@ struct SmData<'a, 'ctx, T: DriverGpuVm> {
>     user_context: &'a mut T::SmContext<'ctx>,
> }
> 
> +#[repr(C)]
> +struct SmMapData<'a, 'ctx, T: DriverGpuVm> {
> +    sm_data: SmData<'a, 'ctx, T>,
> +    vm_bo: GpuVmBoResident<T>,
> +}
> +
> +/// The argument for [`GpuVmCore::sm_map`].
> +pub struct OpMapRequest<'a, 'ctx, T: DriverGpuVm> {
> +    /// Address in GPU virtual address space.
> +    pub addr: u64,
> +    /// Length of mapping to create.
> +    pub range: u64,
> +    /// Offset in GEM object.
> +    pub offset: u64,

I’d rename this gem_offset. A bit vague/confusing otherwise.

> +    /// The GEM object to map.
> +    pub vm_bo: GpuVmBoResident<T>,
> +    /// The user-provided context type.
> +    pub context: &'a mut T::SmContext<'ctx>,
> +}
> +
> +impl<'a, 'ctx, T: DriverGpuVm> OpMapRequest<'a, 'ctx, T> {
> +    fn raw_request(&self) -> bindings::drm_gpuvm_map_req {
> +        bindings::drm_gpuvm_map_req {
> +            map: bindings::drm_gpuva_op_map {
> +                va: bindings::drm_gpuva_op_map__bindgen_ty_1 {
> +                    addr: self.addr,
> +                    range: self.range,
> +                },
> +                gem: bindings::drm_gpuva_op_map__bindgen_ty_2 {
> +                    offset: self.offset,
> +                    obj: self.vm_bo.obj().as_raw(),
> +                },
> +            },
> +        }
> +    }
> +}
> +
> +/// Represents an `sm_step_map` operation that has not yet been completed.

> +pub struct OpMap<'op, T: DriverGpuVm> {
> +    op: &'op bindings::drm_gpuva_op_map,
> +    // Since these abstractions are designed for immediate mode, the VM BO needs to be
> +    // pre-allocated, so we always have it available when we reach this point.
> +    vm_bo: &'op GpuVmBo<T>,
> +    _invariant: PhantomData<*mut &'op mut T>,
> +}
> +
> +impl<'op, T: DriverGpuVm> OpMap<'op, T> {
> +    /// The base address of the new mapping.
> +    pub fn addr(&self) -> u64 {
> +        self.op.va.addr
> +    }
> +
> +    /// The length of the new mapping.
> +    pub fn length(&self) -> u64 {
> +        self.op.va.range
> +    }
> +
> +    /// The offset within the [`drm_gem_object`](crate::gem::Object).
> +    pub fn gem_offset(&self) -> u64 {
> +        self.op.gem.offset
> +    }
> +
> +    /// The [`drm_gem_object`](crate::gem::Object) to map.
> +    pub fn obj(&self) -> &T::Object {
> +        // SAFETY: The `obj` pointer is guaranteed to be valid.
> +        unsafe { <T::Object as IntoGEMObject>::from_raw(self.op.gem.obj) }
> +    }
> +
> +    /// The [`GpuVmBo`] that the new VA will be associated with.
> +    pub fn vm_bo(&self) -> &GpuVmBo<T> {
> +        self.vm_bo
> +    }
> +
> +    /// Use the pre-allocated VA to carry out this map operation.
> +    pub fn insert(self, va: GpuVaAlloc<T>, va_data: impl PinInit<T::VaData>) -> OpMapped<'op, T> {
> +        let va = va.prepare(va_data);
> +        // SAFETY: By the type invariants we may access the interval tree.
> +        unsafe { bindings::drm_gpuva_map(self.vm_bo.gpuvm().as_raw(), va, self.op) };
> +
> +        let _gpuva_guard = self.vm_bo().lock_gpuva();
> +        // SAFETY: The va is prepared for insertion, and we hold the GEM lock.
> +        unsafe { bindings::drm_gpuva_link(va, self.vm_bo.as_raw()) };
> +
> +        OpMapped {
> +            _invariant: self._invariant,
> +        }
> +    }
> +}
> +
> +/// Represents a completed [`OpMap`] operation.
> +pub struct OpMapped<'op, T> {
> +    _invariant: PhantomData<*mut &'op mut T>,
> +}
> +
> /// Represents an `sm_step_unmap` operation that has not yet been completed.
> pub struct OpUnmap<'op, T: DriverGpuVm> {
>     op: &'op bindings::drm_gpuva_op_unmap,
> @@ -205,6 +299,30 @@ pub struct OpRemapped<'op, T> {
> }
> 
> impl<T: DriverGpuVm> GpuVmCore<T> {
> +    /// Create a mapping, removing or remapping anything that overlaps.
> +    ///
> +    /// Internally calls the [`DriverGpuVm`] callbacks similar to [`Self::sm_unmap`], except that
> +    /// the [`DriverGpuVm::sm_step_map`] is called once to create the requested mapping.
> +    #[inline]
> +    pub fn sm_map(&mut self, req: OpMapRequest<'_, '_, T>) -> Result {
> +        let gpuvm = self.as_raw();
> +        let raw_req = req.raw_request();
> +        let mut p = SmMapData {
> +            sm_data: SmData {
> +                gpuvm: self,
> +                user_context: req.context,
> +            },
> +            vm_bo: req.vm_bo,
> +        };
> +        // SAFETY:
> +        // * raw_request() creates a valid request.
> +        // * The private data is valid to be interpreted as both SmData and SmMapData since the
> +        //   first field of SmMapData is SmData.
> +        to_result(unsafe {
> +            bindings::drm_gpuvm_sm_map(gpuvm, (&raw mut p).cast(), &raw const raw_req)
> +        })
> +    }
> +
>     /// Remove any mappings in the given region.
>     ///
>     /// Internally calls [`DriverGpuVm::sm_step_unmap`] for ranges entirely contained within the
> @@ -218,19 +336,45 @@ pub fn sm_unmap(&mut self, addr: u64, length: u64, context: &mut T::SmContext<'_
>         };
>         // SAFETY:
>         // * raw_request() creates a valid request.
> -        // * The private data is valid to be interpreted as SmData.
> +        // * The private data is a valid SmData.
>         to_result(unsafe { bindings::drm_gpuvm_sm_unmap(gpuvm, (&raw mut p).cast(), addr, length) })
>     }
> }
> 
> impl<T: DriverGpuVm> GpuVm<T> {
>     /// # Safety
> -    /// Must be called from `sm_unmap` with a pointer to `SmData`.
> +    /// Must be called from `sm_map` with a pointer to `SmMapData`.
> +    pub(super) unsafe extern "C" fn sm_step_map(
> +        op: *mut bindings::drm_gpuva_op,
> +        p: *mut c_void,
> +    ) -> c_int {
> +        // SAFETY: If we reach `sm_step_map` then we were called from `sm_map` which always passes
> +        // an `SmMapData` as private data.
> +        let p = unsafe { &mut *p.cast::<SmMapData<'_, '_, T>>() };
> +        let op = OpMap {
> +            // SAFETY: sm_step_map is called with a map operation.
> +            op: unsafe { &(*op).__bindgen_anon_1.map },
> +            vm_bo: &p.vm_bo,
> +            _invariant: PhantomData,
> +        };
> +        match p
> +            .sm_data
> +            .gpuvm
> +            .data()
> +            .sm_step_map(op, p.sm_data.user_context)
> +        {
> +            Ok(OpMapped { .. }) => 0,
> +            Err(err) => err.to_errno(),
> +        }
> +    }
> +
> +    /// # Safety
> +    /// Must be called from `sm_map` or `sm_unmap` with a pointer to `SmMapData` or `SmData`.
>     pub(super) unsafe extern "C" fn sm_step_unmap(
>         op: *mut bindings::drm_gpuva_op,
>         p: *mut c_void,
>     ) -> c_int {
> -        // SAFETY: The caller provides a pointer to `SmData`.
> +        // SAFETY: The caller provides a pointer that can be treated as `SmData`.
>         let p = unsafe { &mut *p.cast::<SmData<'_, '_, T>>() };
>         let op = OpUnmap {
>             // SAFETY: sm_step_unmap is called with an unmap operation.
> @@ -244,12 +388,12 @@ impl<T: DriverGpuVm> GpuVm<T> {
>     }
> 
>     /// # Safety
> -    /// Must be called from `sm_unmap` with a pointer to `SmData`.
> +    /// Must be called from `sm_map` or `sm_unmap` with a pointer to `SmMapData` or `SmData`.
>     pub(super) unsafe extern "C" fn sm_step_remap(
>         op: *mut bindings::drm_gpuva_op,
>         p: *mut c_void,
>     ) -> c_int {
> -        // SAFETY: The caller provides a pointer to `SmData`.
> +        // SAFETY: The caller provides a pointer that can be treated as `SmData`.
>         let p = unsafe { &mut *p.cast::<SmData<'_, '_, T>>() };
>         let op = OpRemap {
>             // SAFETY: sm_step_remap is called with a remap operation.
> 
> -- 
> 2.52.0.457.g6b5491de43-goog
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>