Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Out-of-bounds array access due to incorrect page shift in start_page calculation - [High] Hardware assertion failure and command stream corruption due to using PAGE_SIZE as pitch --
--- Patch [19]: [PATCH v2 19/33] drm/xe: Skip self-copies for borrowed pages on defrag moves --- commit 0cb7423f91b7764bc47ff13572694c591c1f58e2 Author: Matthew Brost <[email protected]> drm/xe: Skip self-copies for borrowed pages on defrag moves A defrag move may harvest already beneficial-order chunks from the old tt, so the freshly reallocated backing borrows (references) some of the same physical pages. > diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c > index 956fd75454fa0..450ca8bf4f3b6 100644 > --- a/drivers/gpu/drm/xe/xe_migrate.c > +++ b/drivers/gpu/drm/xe/xe_migrate.c [ ... ] > @@ -835,6 +835,63 @@ static void emit_copy(struct xe_gt *gt, struct xe_bb *bb, > emit_xy_fast_copy(gt, bb, src_ofs, dst_ofs, size, pitch); > } > > +/* > + * emit_copy_chunks() - Emit the data blits for one defrag L0 chunk, skipping [ ... ] > + /* Coalesce a run of pages that actually need copying. */ > + while (i + run < npages && > + src_tt->pages[start_page + i + run] != > + dst_tt->pages[start_page + i + run]) > + run++; > + > + if (bb) > + emit_copy(gt, bb, src_L0_ofs + (u64)i * PAGE_SIZE, > + dst_L0_ofs + (u64)i * PAGE_SIZE, > + run * PAGE_SIZE, PAGE_SIZE); [Severity: High] Can the use of PAGE_SIZE here cause a hardware assertion failure and command stream corruption on systems with 64KB pages? The PAGE_SIZE is passed as the pitch parameter to emit_copy(), which flows into emit_xy_fast_copy() where there is a hardware limit check: xe_gt_assert(gt, pitch <= U16_MAX); On 64KB page systems, PAGE_SIZE is 65536, which exceeds U16_MAX (65535). This will unconditionally trigger the driver assertion. Furthermore, because the pitch is bitwise OR'ed into the BLT command instruction, won't this oversized pitch overflow its bit field and corrupt adjacent fields in the GPU command? Should this use XE_PAGE_SIZE (4096) instead, similar to the fallback path in normal emit_copy() usage? [ ... ] > @@ -985,6 +1052,8 @@ static struct dma_fence *__xe_migrate_copy(struct > xe_migrate *m, > u64 ccs_ofs, ccs_size; > u32 ccs_pt; > u32 pte_flags; > + pgoff_t start_page = (xe_bo_size(src_bo) - size) >> > XE_PTE_SHIFT; > + u32 ndata_blits = 1; > > bool usm = xe->info.has_usm; [Severity: High] Is there a risk of an out-of-bounds array access here on systems with a page size larger than 4KB (like 64KB on ARM64 or PPC64)? By shifting with XE_PTE_SHIFT (12), the index is computed in terms of 4KB GPU pages. However, this index is later passed to emit_copy_chunks() to index the src_tt->pages and dst_tt->pages arrays: if (src_tt->pages[start_page + i] == dst_tt->pages[start_page + i]) { Since the ttm_tt page arrays are sized based on the system's PAGE_SHIFT, wouldn't an index calculated with XE_PTE_SHIFT grow much faster than the array bounds on a 64KB page system, leading to a memory out-of-bounds read? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=19
