Claude review: gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**The UB fix itself is correct and well-motivated.** The old `write_ptr()` / `read_ptr()` methods took `&self` / `&mut self`, creating Rust references to memory that the GSP hardware can concurrently modify, violating Rust's aliasing guarantees. Using `dma_read!` / `dma_write!` avoids this.

**Bug carried over — `&` vs `%` inconsistency:**

In `advance_cpu_write_ptr` (fw.rs, new line 98):
```rust
let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;
```

But in `advance_cpu_read_ptr` (fw.rs, new line 79):
```rust
let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
```

`MSGQ_NUM_PAGES` is `0x3f` (63). `& 0x3f` is equivalent to `% 64`, not `% 63`. So these two functions use different modular arithmetic ranges: `advance_cpu_read_ptr` produces values in `[0, 62]` while `advance_cpu_write_ptr` produces values in `[0, 63]`. This was present in the original code (the diff shows the old `advance_cpu_write_ptr` also used `&`), but since this patch is a refactor touching exactly these functions, it would be a good opportunity to either:
- Fix it to `%` if the intent is modulo-63, or
- Document why `&` is intentionally different here.

Given that the invariant comments on `cpu_write_ptr` say "The returned value is between `0` and `MSGQ_NUM_PAGES`" and the `MsgqData` array is sized as `MSGQ_NUM_PAGES` elements (0 to 62), using `& 0x3f` could produce index 63 which would be out of bounds.

**Shared vs mutable reference change:**

`advance_cpu_read_ptr` and `advance_cpu_write_ptr` now take `&CoherentAllocation<GspMem>` (shared reference) instead of the original `&mut self` path which would have obtained a mutable pointer via `start_ptr_mut()`. This is fine because:
- The `dma_write!` macro handles the volatile write without needing `&mut`
- The callers on `DmaGspMem` still take `&mut self`, maintaining exclusive access at the API boundary

**Closure-based unwrap pattern:**

The `|| -> Result<u32> { Ok(dma_read!(...)) }().unwrap()` pattern is unusual but understandable — it's needed because `dma_read!` uses the `?` operator internally, requiring a `Result`-returning context. The `// PANIC` comments correctly justify why the unwrap is safe. This is acknowledged as temporary until the new `dma::Coherent` API.

**Visibility widening is appropriate** — the `pub(in crate::gsp)` scope is narrow enough and clearly documented with TODO comments for reversion.

**Minor nit:** The fence placement in `advance_cpu_read_ptr` (line 82) comes *after* computing `rptr` but *before* writing it. This matches the original. However, the comment says "Ensure read pointer is properly ordered" — it might be clearer to say it ensures prior reads of queue data are complete before the pointer update becomes visible. This is a pre-existing comment clarity issue, not introduced by this patch.

**Overall: the patch is a correct fix for the UB issue.** The `&` vs `%` inconsistency in `advance_cpu_write_ptr` should be investigated and resolved, either in this patch or as a follow-up.

---
Generated by Claude Code Patch Reviewer