Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Pipelined defrag move detaches VF CCS metadata before the GPU copy completes, risking CCS metadata corruption on VF migration. - [Medium] Using `ctx->defrag` to identify a defrag move incorrectly applies defrag-specific VF CCS attach/detach logic to unrelated victim BOs evicted during a defrag operation. Pre-existing issues: - [Critical] Returning an error from `xe_bo_move()` after successfully committing the move causes TTM to free the new resource, leaving `bo->resource` as a dangling pointer. -- --- Patch [18]: [PATCH v3 18/33] drm/xe: Handle defrag moves in xe_bo_move() --- commit 22e925edd18b0faf0ff581a79813091d539553b8 Author: Matthew Brost <[email protected]> Subject: drm/xe: Handle defrag moves in xe_bo_move() This commit modifies xe_bo_move() to handle defragmentation moves, allowing TTM to reallocate a BO's backing at an optimal order while copying existing contents. It introduces a fast path for defrag moves via GPU relocation, pipelines the teardown asynchronously, and extends VF CCS attach/detach logic to cover defrag transitions. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 02395968d613b..bdcc9bed5d36b 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -1315,13 +1331,13 @@ static int xe_bo_move(struct ttm_buffer_object > *ttm_bo, bool evict, > * BBs from BO as it is no longer needed. > */ > if (IS_VF_CCS_READY(xe) && old_mem_type == XE_PL_TT && > - new_mem->mem_type == XE_PL_SYSTEM) > + (new_mem->mem_type == XE_PL_SYSTEM || ctx->defrag)) > xe_sriov_vf_ccs_detach_bo(bo); [Severity: High] Does this code synchronously detach VF CCS metadata before the GPU copy completes? Since a defrag move is pipelined asynchronously (evict is false, so ttm_bo_move_accel_cleanup() delegates to a ghost object without waiting for the fence), xe_bo_move() immediately calls xe_sriov_vf_ccs_detach_bo(bo). This synchronously clears the GuC CCS save/restore batch buffers for the old TT. If a VF migration triggers while the async GPU copy job is still in flight, the GuC won't save the old TT's CCS state because its batch buffers were detached. When the copy job resumes, can it copy uninitialized or garbage data from the old hardware CCS region into the new TT, leading to CCS metadata corruption? > if (IS_VF_CCS_READY(xe) && > ((move_lacks_source && new_mem->mem_type == XE_PL_TT) || > - (old_mem_type == XE_PL_SYSTEM && new_mem->mem_type == XE_PL_TT)) && > - handle_system_ccs) > + ((old_mem_type == XE_PL_SYSTEM || ctx->defrag) && > + new_mem->mem_type == XE_PL_TT)) && handle_system_ccs) > ret = xe_sriov_vf_ccs_attach_bo(bo); [Severity: Medium] Does checking ctx->defrag instead of ttm_bo->defrag_old_tt misclassify victim BOs? TTM passes the original ttm_operation_ctx down to all evictions triggered by an allocation, which means ctx->defrag is true for any victim BO evicted during a defrag pass. If unrelated BOs (such as those evicted from STOLEN to TT) have VF CCS logic applied to them because ctx->defrag is true, will this modify their hardware CCS tracking unexpectedly and break standard eviction flows? [Severity: Critical] This is a pre-existing issue, but returning an error from xe_bo_move() after successfully committing the move causes TTM to free the new resource, leaving bo->resource as a dangling pointer. In xe_bo_move(), the move is successfully committed via ttm_bo_move_accel_cleanup() which assigns the new resource to bo->resource. If a subsequent operation like xe_sriov_vf_ccs_attach_bo(bo) fails (e.g. returns -ENOMEM), xe_bo_move() propagates this error back to TTM. TTM's error recovery in ttm_bo_validate() then unconditionally calls ttm_resource_free(bo, &res) for the new allocation. Since bo->resource already points to this new allocation, does this free the memory while leaving bo->resource as a dangling pointer, creating a use-after-free risk? > > out: -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
