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: gpu: nova-core: fix stack overflow in GSP memory allocation
Date: Sat, 14 Feb 2026 07:16:12 +1000
Message-ID: <review-patch1-20260213-drm-rust-next-v2-1-aa094f78721a@proton.me>
In-Reply-To: <20260213-drm-rust-next-v2-1-aa094f78721a@proton.me>
References: <20260213-drm-rust-next-v2-1-aa094f78721a@proton.me>
 <20260213-drm-rust-next-v2-1-aa094f78721a@proton.me>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**LogBuffer changes (gsp.rs)**

The `LogBuffer::new` changes look correct. The original code:

> -        let ptes =3D PteArray::<NUM_PAGES>::new(obj.0.dma_handle())?;
> -        // SAFETY: `obj` has just been created and we are its sole user.
> -        unsafe {
> -            // Copy the self-mapping PTE at the expected location.
> -            obj.0
> -                .as_slice_mut(size_of::<u64>(), size_of_val(&ptes))?
> -                .copy_from_slice(ptes.as_bytes())
> -        };

is replaced with:

> +        let start_addr =3D obj.0.dma_handle();
> +
> +        // SAFETY: `obj` has just been created and we are its sole user.
> +        let pte_region =3D unsafe {
> +            obj.0
> +                .as_slice_mut(size_of::<u64>(), NUM_PAGES * size_of::<u64>=
())?
> +        };
> +
> +        // As in [`DmaGspMem`], this is a  one by one GSP Page write to th=
e memory
> +        // to avoid stack overflow when allocating the whole array at once.
> +        for (i, chunk) in pte_region.chunks_exact_mut(size_of::<u64>()).en=
umerate() {
> +            let pte_value =3D start_addr
> +                .checked_add(num::usize_as_u64(i) << GSP_PAGE_SHIFT)
> +                .ok_or(EOVERFLOW)?;
> +
> +            chunk.copy_from_slice(&pte_value.to_ne_bytes());
> +        }

Since the `CoherentAllocation` is `CoherentAllocation<u8>`, the `as_slice_mut=
` offset and count parameters are in bytes. The original code passed `size_of=
::<u64>()` (8) as the offset and `size_of_val(&ptes)` as the count =E2=80=94 =
which was `NUM_PAGES * 8`. The new code passes `size_of::<u64>()` (8) as offs=
et and `NUM_PAGES * size_of::<u64>()` as count, which is equivalent. The `chu=
nks_exact_mut(size_of::<u64>())` then iterates over 8-byte chunks, and `NUM_P=
AGES * 8 / 8 =3D NUM_PAGES` chunks, so all PTEs are written. This looks corre=
ct.

Minor nit: the comment has a double space ("this is a  one by one").

**Cmdq changes (gsp/cmdq.rs)**

> +        const NUM_PAGES: usize =3D GSP_PAGE_SIZE / size_of::<u64>();

This computes 4096 / 8 =3D 512, matching the original `PteArray<{ GSP_PAGE_SI=
ZE / size_of::<u64>() }>` generic parameter. However, this `NUM_PAGES` name i=
s potentially misleading =E2=80=94 this isn't the number of GSP pages in the =
`GspMem` allocation, it's the number of PTE *entries* that fit in one GSP pag=
e. The existing code used this same expression as the array size, so this isn=
't a new issue, but the name could cause confusion with the `Cmdq::NUM_PTES` =
constant which equals `size_of::<GspMem>() >> GSP_PAGE_SHIFT` (the actual num=
ber of pages in the GspMem structure). That said, this is a naming preference=
 not a bug.

> +        let item =3D gsp_mem.item_from_index(0)?;
> +        for i in 0..NUM_PAGES {
> +            let pte_value =3D gsp_mem
> +                .dma_handle()
> +                .checked_add(num::usize_as_u64(i) << GSP_PAGE_SHIFT)
> +                .ok_or(EOVERFLOW)?;
> +
> +            // SAFETY: `item_from_index` ensures that `item` is always a v=
alid pointer and can be
> +            // dereferenced. The compiler also further validates the expre=
ssion on whether `field`
> +            // is a member of `item` when expanded by the macro.
> +            //
> +            // Further, this is dma_write! macro expanded and modified to =
allow for individual
> +            // page write.
> +            unsafe {
> +                let ptr_field =3D core::ptr::addr_of_mut!((*item).ptes[i]);
> +                gsp_mem.field_write(ptr_field, pte_value);
> +            }
> +        }

The safety comment says this is the `dma_write!` macro "expanded and modified=
," which is an accurate description of what's happening. The `field_write` me=
thod performs a volatile write of each `u64` PTE value. The original `dma_wri=
te!` macro for the whole `PteArray` would have written the entire struct via =
a single volatile write =E2=80=94 but since the GSP isn't running at this poi=
nt during initialization (the command queue hasn't been used yet), element-by=
-element volatile writes should be functionally equivalent.

The struct field type was also changed:

> -    ptes: PteArray<{ GSP_PAGE_SIZE / size_of::<u64>() }>,
> +    ptes: [u64; GSP_PAGE_SIZE / size_of::<u64>()],

This is correct since `PteArray` was just a `#[repr(C)]` newtype around `[u64=
; N]`, so the layout is identical and the `GspMem` struct layout is preserved=
. The `AsBytes`/`FromBytes` impls for `GspMem` are manual `unsafe impl`s so t=
hey don't depend on `PteArray` implementing those traits.

One thing worth verifying: the `PteArray` type is also imported in `cmdq.rs`:

> -        PteArray,

This import is removed, consistent with `PteArray` being deleted entirely fro=
m `gsp.rs`. The unused `DmaAddress` import is also correctly removed from `gs=
p.rs` since `dma_handle()` returns a `DmaAddress` but it's now used via metho=
d call rather than being named explicitly.

The `use core::iter::Iterator` addition at the top of `gsp.rs`:

> +use core::iter::Iterator;

This appears unnecessary =E2=80=94 `Iterator` is in the prelude and the patch=
 doesn't use it explicitly as a trait bound anywhere in `gsp.rs`. The `chunks=
_exact_mut` method is on `[T]`, not on `Iterator`. Was this left over from an=
 earlier version of the patch?

Overall this is a straightforward and correct fix. The only items worth addre=
ssing are the unnecessary `use core::iter::Iterator` import and the double-sp=
ace typo in the comment.

---
Generated by Claude Code Patch Reviewer