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

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Feb 20, 2026 at 9:16 AM CET, Alice Ryhl wrote:
>> > +    /// 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 think we should make them public once actually needed.

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

Yeah, but my experience is that drivers can get very creative in figuring out
how to abuse APIs. I think it's best to keep the surface for this as small as
possible.

>> > +/// 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>>.

GpuVmBoAlloc<T> dereferences to GpuVmBo<T>, so currently it is not.

>> 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.

We seem to agree then: What I want is that from a driver perspective there is
only GpuVmBo<T> (which is the registered thing) and GpuVmBoAlloc<T> which is the
pre-allocated thing, i.e.  no separate GpuVmBoRegistered<T> type.