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: mm: Add TLB flush support
Date: Tue, 28 Apr 2026 15:32:00 +1000
Message-ID: <review-patch7-20260425211454.174696-8-joelagnelf@nvidia.com>
In-Reply-To: <20260425211454.174696-8-joelagnelf@nvidia.com>
References: <20260425211454.174696-1-joelagnelf@nvidia.com> <20260425211454.174696-8-joelagnelf@nvidia.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 `Tlb` with `flush()` that writes PDB address and triggers invalidation=
 with polling.

**Issue =E2=80=94 `TlbAckMode::None` hardcoded for all flushes:**

```rust
bar.write_reg(
    regs::NV_TLB_FLUSH_CTRL::zeroed()
        .with_all_va(true)
        .with_ack(TlbAckMode::None)
        .with_trigger(true),
);
```

The comment on `TlbAckMode::Globally` (line 62) explicitly states it is "st=
rictly required during unmap or permission tightening, because the backing =
memory may be reassigned after the flush returns and a stale TLB entry coul=
d let the GPU access freed memory." Yet `flush()` always uses `None`. Curre=
ntly the only caller is `install_mappings()` (new mappings) and `invalidate=
_ptes()` (unmapping), where the unmap path **needs** `Globally`. Consider m=
aking `TlbAckMode` a parameter to `flush()`, or at minimum adding separate =
`flush_for_map()` / `flush_for_unmap()` methods.

**Minor =E2=80=94 `Delta::ZERO` for inter-poll delay:**

```rust
read_poll_timeout(
    || Ok(bar.read(regs::NV_TLB_FLUSH_CTRL)),
    |ctrl: &regs::NV_TLB_FLUSH_CTRL| !ctrl.trigger(),
    Delta::ZERO,
    Delta::from_secs(2),
)?;
```

`Delta::ZERO` as the sleep between polls means tight busy-looping on the MM=
IO register. A small delay (10-100us) would reduce PCIe bus contention with=
out meaningfully affecting latency.

---
Generated by Claude Code Patch Reviewer