Re: [PATCH v4 3/6] rust: gpuvm: add GpuVm::obtain()

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Feb 19, 2026 at 08:22:14PM +0100, Danilo Krummrich wrote:
> On Fri Jan 30, 2026 at 3:24 PM CET, Alice Ryhl wrote:
> > +/// Represents that a given GEM object has at least one mapping on this [`GpuVm`] instance.
> > +///
> > +/// Does not assume that GEM lock is held.
> > +#[repr(C)]
> > +#[pin_data]
> > +pub struct GpuVmBo<T: DriverGpuVm> {
> > +    #[pin]
> > +    inner: Opaque<bindings::drm_gpuvm_bo>,
> > +    #[pin]
> > +    data: T::VmBoData,
> > +}
> > +
> > +impl<T: DriverGpuVm> GpuVmBo<T> {
> > +    pub(super) const ALLOC_FN: Option<unsafe extern "C" fn() -> *mut bindings::drm_gpuvm_bo> = {
> > +        use core::alloc::Layout;
> > +        let base = Layout::new::<bindings::drm_gpuvm_bo>();
> > +        let rust = Layout::new::<Self>();
> > +        assert!(base.size() <= rust.size());
> > +        if base.size() != rust.size() || base.align() != rust.align() {
> > +            Some(Self::vm_bo_alloc)
> > +        } else {
> > +            // This causes GPUVM to allocate a `GpuVmBo<T>` with `kzalloc(sizeof(drm_gpuvm_bo))`.
> > +            None
> 
> So, if T::VmBoData is a ZST *and* needs drop, we may end up allocating on the C
> side and freeing on the Rust side.
> 
> I assume this is intentional and there is nothing wrong with it, but without a
> comment it might be a bit subtle.

Yeah it's subtle but correct.

> Another subtlety is that vm_bo_free() and vm_bo_alloc() assume that inner is
> always the first member. I'd probably add a brief comment why this even has to
> be the case, i.e. vm_bo_alloc() does not return *mut c_void, but *mut
> bindings::drm_gpuvm_bo.

I will add comments.

> > +    /// Access this [`GpuVmBo`] from a raw pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// For the duration of `'a`, the pointer must reference a valid `drm_gpuvm_bo` associated with
> > +    /// a [`GpuVm<T>`].
> > +    #[inline]
> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_gpuvm_bo) -> &'a Self {
> 
> I think this a good candidate for crate private, as we don't want drivers to use
> this, but still use it in other DRM core modules.
> 
> > +        // SAFETY: `drm_gpuvm_bo` is first field and `repr(C)`.
> > +        unsafe { &*ptr.cast() }
> > +    }
> > +
> > +    /// Returns a raw pointer to underlying C value.
> > +    #[inline]
> > +    pub fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
> 
> Less important, but probably also only needed in core DRM code.

For cases like these two, I do think one can run into cases where you
want them to be public. E.g. the vma confusion bugfix uses a raw pointer
for now:
https://lore.kernel.org/all/20260218-binder-vma-check-v2-1-60f9d695a990@google.com/

I'm generally not so worried about methods like these being public
because they can't be used without unsafe.

> > +    /// Look up whether there is an existing [`GpuVmBo`] for this gem object.
> > +    #[inline]
> > +    pub(super) fn obtain(self) -> GpuVmBoRegistered<T> {
> > +        let me = ManuallyDrop::new(self);
> > +        // SAFETY: Valid `drm_gpuvm_bo` not already in the lists.
> > +        let ptr = unsafe { bindings::drm_gpuvm_bo_obtain_prealloc(me.as_raw()) };
> > +
> > +        // Add the vm_bo to the extobj list if it's an external object, and if the vm_bo does not
> > +        // already exist. (If we are using an existing vm_bo, it's already in the extobj list.)
> > +        if ptr::eq(ptr, me.as_raw()) && me.gpuvm().is_extobj(me.obj()) {
> > +            let resv_lock = me.gpuvm().raw_resv();
> > +            // SAFETY: The GPUVM is still alive, so its resv lock is too.
> > +            unsafe { bindings::dma_resv_lock(resv_lock, ptr::null_mut()) };
> 
> Maybe add a TODO to replace this with a proper lock guard once available?

Ok.

> > +/// A [`GpuVmBo`] object in the GEM list.
> > +///
> > +/// # Invariants
> > +///
> > +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is present in the gem list.
> > +pub struct GpuVmBoRegistered<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);
> 
> I know that I proposed to rename this from GpuVmBoResident to GpuVmBoRegistered
> in a drive-by comment on v3.
> 
> But now that I have a closer look, I think it would be nice to just have GpuVmBo
> being the registered one and GpuVmBoAlloc being the pre-allocated one.
> 
> As it is currently, I think it is bad to ever present a &GpuVmBo to a driver
> because it implies that we don't know whether it is a pre-allocated one or a
> "normal", registered one. But we do always know.

Actually, I think GpuVmBo is already the registered one.
GpuVmBoRegistered is just ARef<GpuVmBo<T>>.

> For instance, in patch 6 we give out &'op GpuVmBo<T>, but it actually carries
> the invariant of being registered.
> 
> Of course, we could fix this by giving out a &'op GpuVmBoRegistered<T> instead,
> but it would be nice to not have drivers be in touch with a type that can be one
> or the other.
> 
> I know that the current GpuVmBo<T> also serves the purpose of storing common
> code. Maybe we can make it private, call it GpuVmBoInner<T> and have inline
> forwarding methods for GpuVmBo<T> and GpuVmBoAlloc<T>. This is slightly more
> overhead in this implementation due to the forwarding methods, but less
> ambiguity for drivers, which I think is more important.

I think we should keep the current state that GpuVmBo is registered, and
only GpuVmBoAlloc is not. That is most useful.

Alice