From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: rust: gpuvm: add GpuVmCore::sm_unmap()
Date: Sun, 22 Mar 2026 03:35:24 +1000
Message-ID: <review-patch5-20260320-gpuvm-rust-v5-5-76fd44f17a87@google.com>
In-Reply-To: <20260320-gpuvm-rust-v5-5-76fd44f17a87@google.com>
References: <20260320-gpuvm-rust-v5-0-76fd44f17a87@google.com> <20260320-gpuvm-rust-v5-5-76fd44f17a87@google.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

Adds unmap/remap callbacks and the `sm_unmap()` entry point.

**`SmData` missing `#[repr(C)]`:**
```rust
struct SmData<'a, 'ctx, T: DriverGpuVm> {
    gpuvm: &'a mut UniqueRefGpuVm<T>,
    user_context: &'a mut T::SmContext<'ctx>,
}
```
In patch 6, `SmMapData` is `#[repr(C)]` and contains `SmData` as its first =
field, relying on the cast `p.cast::<SmData<...>>()` being valid when `p` a=
ctually points to a `SmMapData`. This cast is only sound if `SmData` is at =
offset 0 in `SmMapData`, which `#[repr(C)]` on `SmMapData` guarantees (firs=
t field is at offset 0). So the cast is correct regardless of `SmData`'s re=
pr. However, adding `#[repr(C)]` to `SmData` would be good defensive practi=
ce.

**`OpRemap::remap()` always requires 2 `GpuVaAlloc`s:**
```rust
pub fn remap(
    self,
    va_alloc: [GpuVaAlloc<T>; 2],
    prev_data: impl PinInit<T::VaData>,
    next_data: impl PinInit<T::VaData>,
) -> (OpRemapped<'op, T>, OpRemapRet<T>)
```
This always requires 2 pre-allocated VAs even when only 1 (or 0) will be us=
ed. The unused one is returned in `OpRemapRet::unused_va`. The `prev_data`/=
`next_data` initializers are also always required even when not used =E2=80=
=94 this means the caller must construct dummy initializers. This is a usab=
ility concern; consider making them `Option<impl PinInit<T::VaData>>` or on=
ly initializing when `self.prev()`/`self.next()` is `Some`. However, since =
`PinInit` for simple types is cheap, this may be an acceptable trade-off fo=
r API simplicity.

**`lock_gpuva` returns a `MutexGuard` but uses `from_raw`:**
```rust
pub(super) fn lock_gpuva(&self) -> crate::sync::MutexGuard<'_, ()> {
    let ptr =3D unsafe { &raw mut (*self.obj().as_raw()).gpuva.lock };
    let mutex =3D unsafe { crate::sync::Mutex::from_raw(ptr) };
    mutex.lock()
}
```
Using `Mutex::from_raw` on a mutex embedded in a C struct is fine as long a=
s the mutex was initialized by the C side (which it is, as part of GEM obje=
ct init). The guard borrows `&self` which ensures the GpuVmBo stays alive w=
hile the lock is held.

---
Generated by Claude Code Patch Reviewer