Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Overloading `dst_packed_pages` as a boolean flag causes GPU IOMMU faults when defragmentation yields 0 changed pages. Pre-existing issues: - [High] Mismatch between XE_PTE_SHIFT and PAGE_SHIFT causes out-of-bounds array indexing and assertion failures on architectures with 64K pages. -- --- Patch [31]: [PATCH v2 31/33] drm/xe: Blit src-natural to dst-packed for defrag-IOVA copies --- commit b1d39bbd2548d9ad82a4ac12f4936dc748d93736 Author: Matthew Brost <[email protected]> Subject: drm/xe: Blit src-natural to dst-packed for defrag-IOVA copies This commit adds a packed-destination mode to __xe_migrate_copy for defrag moves with IOVA enabled. It allows reading from the old tt at natural offsets and writing to the new tt at packed contiguous offsets. A new xe_migrate_copy_defrag_iova entry point is exposed for this functionality. > diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c > index 0a3a1c5ca1938..35fd0253fef30 100644 > --- a/drivers/gpu/drm/xe/xe_migrate.c > +++ b/drivers/gpu/drm/xe/xe_migrate.c [ ... ] > @@ -883,9 +949,71 @@ static u32 emit_copy_chunks(struct xe_gt *gt, struct > xe_bb *bb, > > if (bb) > emit_copy(gt, bb, src_L0_ofs + (u64)i * PAGE_SIZE, > - dst_L0_ofs + (u64)i * PAGE_SIZE, > + dst_L0_ofs + > + (u64)(pack_dst ? pack : i) * PAGE_SIZE, > run * PAGE_SIZE, PAGE_SIZE); [Severity: High] This isn't a bug introduced by this patch, but does passing PAGE_SIZE as the pitch to emit_copy overflow the 16-bit pitch limit on systems with 64KB CPU pages? The xe_gt_assert condition in emit_xy_fast_copy expects the pitch to be <= U16_MAX, which could cause a crash on architectures like ARM64 when defragmenting BOs. > nblits++; > + pack += run; > + i += run; > + } [ ... ] > @@ -1005,6 +1134,23 @@ static struct dma_fence *__xe_migrate_copy(struct > xe_migrate *m, > */ > struct ttm_tt *dst_tt = dst_bo->ttm.ttm; > bool skip_borrowed = flags.defrag_copy && src_tt && dst_tt; > + /* > + * Defrag-IOVA copy: the destination tt only maps the changed > + * (non-borrowed) pages, packed contiguously into the first > + * @dst_packed_pages pages of its IOVA. The source is read from the old > + * tt at natural offsets and each changed run is blitted into the packed > + * destination prefix. > + */ > + bool pack_dst = dst_packed_pages != 0; [Severity: High] Can evaluating pack_dst this way lead to GPU IOMMU faults when a defragmentation move yields 0 changed pages? If a BO defragmentation successfully borrows all pages, dst_packed_pages will be 0. This makes pack_dst false, which disables the packed CCS logic. During the CCS pass, the driver might then fall back to a full BO CCS copy via xe_migrate_ccs_copy(), assuming the destination is fully mapped. However, because the destination IOVA mapping only covers the 0 changed pages, the GPU would write CCS data to unmapped IOVA addresses. > + /* > + * Packed defrag CCS pass: migrate the flat-CCS aux state of the changed > + * pages only, from the source at its natural offset to the destination > + * packed prefix. Borrowed pages share the physical page (and thus its > + * aux) on both sides, so they are skipped exactly like the data pass. > + */ > + bool ccs_pack = flags.defrag_ccs && pack_dst && src_tt && dst_tt; [ ... ] > @@ -1054,29 +1202,86 @@ static struct dma_fence *__xe_migrate_copy(struct > xe_migrate *m, > u32 pte_flags; > pgoff_t start_page = (xe_bo_size(src_bo) - size) >> > XE_PTE_SHIFT; [Severity: High] This isn't a bug introduced by this patch, but could the start_page calculation cause out-of-bounds array indexing on architectures with 64KB pages? The index start_page is computed using XE_PTE_SHIFT (which is 4KB), but it is later used to index ttm_tt->pages which is sized based on PAGE_SHIFT. On a 64KB page system, the calculated index would be 16x larger than the array bounds, potentially leading to memory corruption during defragmentation. > u32 ndata_blits = 1; > + u32 nchg = 0; > + u64 dst_size; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=31
