From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/amdxdna: Add carveout memory support for non-IOMMU systems Date: Tue, 28 Apr 2026 14:22:37 +1000 Message-ID: In-Reply-To: <20260427170949.2666601-1-lizhi.hou@amd.com> References: <20260427170949.2666601-1-lizhi.hou@amd.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: accel/amdxdna: Add carveout memory support for non-IOMMU systems Author: Lizhi Hou Patches: 1 Reviewed: 2026-04-28T14:22:37.648917 --- This is a single patch (V2) adding carveout memory support for non-IOMMU sy= stems to the `accel/amdxdna` driver. The feature reserves physically contig= uous memory at boot time and uses it for buffer allocations when neither IO= MMU nor SVA/PASID are available =E2=80=94 a debugging/bring-up use case. The overall design is reasonable: a `drm_mm` allocator manages the carveout= region, buffers are exported as custom `dma_buf` objects, and a debugfs in= terface allows runtime configuration. The patch also refactors IOMMU/DMA ma= pping code to handle the three address modes (PASID, IOVA, physical address= ) more cleanly. However, there are several issues ranging from correctness bugs to design c= oncerns: **Key concerns:** 1. **SVA bind failure silently ignored** =E2=80=94 `amdxdna_drm_open` now d= owngrades SVA bind failures to warnings and continues, but later code paths= that assume `amdxdna_pasid_on()` returns true will silently do the wrong t= hing if the user never sets up carveout. 2. **Use-after-free risk in cleanup** =E2=80=94 `mmdrop()` is called before= `amdxdna_sva_fini()`, but SVA unbind may access the mm_struct. 3. **Resource leak in `amdxdna_get_cbuf` error path** =E2=80=94 after `dma_= buf_export` succeeds and `amdxdna_cbuf_clear` fails, `dma_buf_put` triggers= `amdxdna_cbuf_release` which frees the node, but then execution falls thro= ugh to `out:` returning an error; however, the `cbuf` struct is freed by th= e release callback so the `remove_node` label is correctly skipped. Actuall= y this path is fine, but the labels are confusingly arranged. 4. **`drm_gem_shmem_get_pages_sgt` called on imported carveout BOs** =E2=80= =94 carveout BOs are self-imported via `amdxdna_gem_prime_import`, making t= hem imported BOs. The new `amdxdna_dma_map_bo` still calls `drm_gem_shmem_g= et_pages_sgt()`, which may not work correctly on imported BOs backed by `dm= a_buf` rather than shmem pages. 5. **Security concern with debugfs carveout** =E2=80=94 allowing root to po= int the driver at arbitrary physical memory at runtime is powerful; the pat= ch lacks range validation against actual reserved memory. 6. **Missing `ssize_t` =E2=86=92 `size_t` type handling** =E2=80=94 `iommu_= map_sgtable` returns `ssize_t` but the patch adds a `size_t` variable for t= he `< 0` check. --- --- Generated by Claude Code Patch Reviewer