Re: [PATCH v3 18/33] drm/xe: Handle defrag moves in xe_bo_move()

[sashiko-bot@kernel.org]

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 <matthew.brost@intel.com>
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/20260711025619.2540575-1-matthew.brost@intel.com?part=18