Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] Overloading `dst_packed_pages == 0` to mean 'not a packed destination' causes out-of-bounds cursor access and memory corruption when a packed defrag move has exactly zero changed pages. Pre-existing issues: - [Critical] During a TT-to-TT CCS defrag pass on DGFX systems, an uninitialized stack variable (`ccs_ofs`) is passed to the GPU blitter, causing memory corruption. -- --- Patch [31]: [PATCH v3 31/33] drm/xe: Blit src-natural to dst-packed for defrag-IOVA copies --- commit f45aa97c2f11618672e201ef92ad9ef7898d5e9c Author: Matthew Brost <[email protected]> drm/xe: Blit src-natural to dst-packed for defrag-IOVA copies This commit introduces a packed-destination mode in __xe_migrate_copy() for defrag moves with IOVA enabled. It adds a dst_packed_pages argument to handle asymmetric geometries where the destination only exposes changed pages. The destination PTE update size is calculated based on the changed-page count. > diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c > index e9f050eab8871..8eb5e331d5156 100644 > --- a/drivers/gpu/drm/xe/xe_migrate.c > +++ b/drivers/gpu/drm/xe/xe_migrate.c [ ... ] > @@ -1033,7 +1179,9 @@ static struct dma_fence *__xe_migrate_copy(struct > xe_migrate *m, > else > xe_res_first(src, 0, size, &src_it); > if (!dst_is_vram) > - xe_res_first_tt(dst_bo->ttm.ttm, 0, size, &dst_it); > + xe_res_first_tt(dst_bo->ttm.ttm, 0, > + pack_dst ? (u64)dst_packed_pages << PAGE_SHIFT : > + size, &dst_it); > else > xe_res_first(dst, 0, size, &dst_it); [Severity: Critical] Does overloading dst_packed_pages == 0 to mean 'not a packed destination' cause issues when a packed defrag move has exactly zero changed pages? If a defragmentation move executes under memory pressure and fails to allocate any new pages, it results in 100% borrowed pages. The IOVA mapping function legitimately maps 0 pages, so xe_migrate_copy_defrag_iova() is called with dst_packed_pages = 0. Because dst_packed_pages == 0, pack_dst becomes false (set earlier in __xe_migrate_copy). This forces the fallback to a full-size non-packed copy, and initializes dst_it requesting the full BO size against a destination IOVA that was packed to size 0. Can this walk the broken cursor and fetch garbage DMA addresses for the GPU blitter, corrupting memory? [ ... ] > @@ -1145,16 +1355,22 @@ static struct dma_fence *__xe_migrate_copy(struct > xe_migrate *m, > > if (skip_borrowed) > emit_copy_chunks(gt, bb, src_L0_ofs, dst_L0_ofs, src_L0, > - src_tt, dst_tt, start_page); > + src_tt, dst_tt, start_page, pack_dst); > else if (!flags.copy_only_ccs && !flags.defrag_ccs) > emit_copy(gt, bb, src_L0_ofs, dst_L0_ofs, src_L0, > XE_PAGE_SIZE); > > - if (needs_ccs_emit) > + if (ccs_pack) { > + if (emit_ccs_copy_chunks(gt, bb, src_L0_ofs, dst_L0_ofs, > + src_L0, src_tt, dst_tt, > + start_page)) > + flush_flags = MI_FLUSH_DW_CCS; > + } else if (needs_ccs_emit) { > flush_flags = xe_migrate_ccs_copy(m, bb, src_L0_ofs, > IS_DGFX(xe) ? > src_is_vram : src_is_pltt, > dst_L0_ofs, > IS_DGFX(xe) ? > dst_is_vram : dst_is_pltt, > src_L0, ccs_ofs, > copy_ccs); > + } [Severity: Critical] This is a pre-existing issue, but can this pass an uninitialized ccs_ofs stack variable to the GPU blitter during a TT-to-TT CCS defrag pass on DGFX systems? When __xe_migrate_copy() is called with flags.defrag_ccs = true, copy_system_ccs evaluates to false, leaving ccs_ofs uninitialized. Because !flags.defrag_copy is true, needs_ccs_emit evaluates to true and calls xe_migrate_ccs_copy(). For a TT source on DGFX, IS_DGFX(xe) ? src_is_vram : src_is_pltt evaluates to false (meaning src_is_indirect is false). xe_migrate_ccs_copy() then consumes the uninitialized ccs_ofs: drivers/gpu/drm/xe/xe_migrate.c:xe_migrate_ccs_copy() { ... } else if (copy_ccs) { if (!src_is_indirect) src_ofs = ccs_ofs; ... } Does this cause the uninitialized src_ofs to be emitted into the GPU batch buffer, resulting in memory corruption? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=31
