Re: [PATCH v3 2/2] drm/xe: After VF migration, repeat BO mapping in progress

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/xe: stale GGTT base cleared under lock in
  __xe_ggtt_insert_bo_at()
- [Medium] drm/xe: infinite uninterruptible retry loop in
  __xe_ggtt_insert_bo_at()
--

commit 9aaa3dde30d5f39a2d5d350e0123ac1a3e77675f
Author: Tomasz Lis <tomasz.lis@intel.com>

drm/xe: After VF migration, repeat BO mapping in progress

This commit moves the VF post-migration retry loop from LRC creation in
__xe_exec_queue_init() down into __xe_ggtt_insert_bo_at() so that any BO
being mapped into the GGTT during VF migration has its PTEs rewritten
after the GGTT base is updated.

> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 9ef2ce743770f..6c2f3078afb0c 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c

[ ... ]

> @@ -858,6 +861,9 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  		goto out;
>  	}
>  
> +retry:
> +	marker = xe_gt_sriov_vf_wait_valid_ggtt(ggtt->tile->primary_gt);
> +
>  	mutex_lock(&ggtt->lock);
>  	/*
>  	 * When inheriting the initial framebuffer, the framebuffer is
> @@ -867,20 +873,20 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  	 * remainder of GGTT, unaware that the start is reserved by WOPCM.
>  	 */
>  	if (start >= ggtt->start)
> -		start -= ggtt->start;
> +		rel_start = start - ggtt->start;
>  	else
> -		start = 0;
> +		rel_start = 0;
>  
>  	/* Should never happen, but since we handle start, fail graciously for end */
>  	if (end >= ggtt->start)
> -		end -= ggtt->start;
> +		rel_end = end - ggtt->start;
>  	else
> -		end = 0;
> +		rel_end = 0;
>  
> -	xe_tile_assert(ggtt->tile, end >= start + xe_bo_size(bo));
> +	xe_tile_assert(ggtt->tile, rel_end >= rel_start + xe_bo_size(bo));
>  
>  	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node[tile_id]->base,
> -					  xe_bo_size(bo), alignment, 0, start, end, 0);
> +					  xe_bo_size(bo), alignment, 0, rel_start, rel_end, 0);
>  	if (err) {
>  		ggtt_node_fini(bo->ggtt_node[tile_id]);
>  		bo->ggtt_node[tile_id] = NULL;
> @@ -890,6 +896,24 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  		u64 pte = ggtt->pt_ops->pte_encode_flags(bo, pat_index);
>  
>  		xe_ggtt_map_bo(ggtt, bo->ggtt_node[tile_id], bo, pte);
> +
> +		/*
> +		 * If VF migration happens during BO mapping, some PTE value writes may
> +		 * be ignored by HW. This can happen in case GGTT range changes; already
> +		 * set PTEs are moved by the VF restore, but xe_ggtt_map_bo() within VM
> +		 * may still loop at old position. To be sure, redo all PTE writes.
> +		 */
> +		if (marker != xe_vf_migration_fixups_complete_count(ggtt->tile->primary_gt)) {
> +			struct xe_ggtt_node *node = bo->ggtt_node[tile_id];
> +			/*
> +			 * Cannot skip the clear, as after releasing the ggtt lock, another
> +			 * insert operation could immediately reserve the space.
> +			 */
> +			xe_ggtt_clear(ggtt, xe_ggtt_node_addr(node), xe_ggtt_node_size(node));
> +			drm_mm_remove_node(&node->base);
> +			mutex_unlock(&ggtt->lock);
> +			goto retry;
> +		}
>  	}
>  	mutex_unlock(&ggtt->lock);

[Severity: Medium]
Does xe_ggtt_clear() here write scratch PTEs using the stale pre-migration
ggtt->start offset?

__xe_ggtt_insert_bo_at() holds ggtt->lock across xe_ggtt_map_bo() and this
retry check.  When VF migration happens during xe_ggtt_map_bo(),
vf_valid_ggtt() becomes false (for example, while GUC_INTR_SW_INT_0 is
pending or ggtt_need_fixes is set), so
xe_vf_migration_fixups_complete_count() returns -1 and enters this retry
branch.  However, the post-migration recovery worker updates ggtt->start
via xe_ggtt_shift_nodes(), which also has to take ggtt->lock:

drivers/gpu/drm/xe/xe_ggtt.c:xe_ggtt_shift_nodes() {
	guard(mutex)(&ggtt->lock);
    ...
	/* pairs with READ_ONCE in xe_ggtt_node_addr() */
	WRITE_ONCE(ggtt->start, new_start);
}

Because __xe_ggtt_insert_bo_at() has not released ggtt->lock yet,
xe_ggtt_shift_nodes() cannot have updated ggtt->start when xe_ggtt_clear()
is called.  xe_ggtt_node_addr(node) therefore computes
node->base.start + old_start instead of node->base.start + new_start.
The PTEs that PF restore copied to new_start + node->base.start are left
uncleared when drm_mm_remove_node() frees the node, and if the old and new
GGTT ranges overlap, xe_ggtt_clear() overwrites PTEs at the old address
that may now belong to another allocation.

> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> index 4412ed6eb422f..8ac91b271137d 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> @@ -1615,9 +1615,9 @@ int xe_gt_sriov_vf_wait_valid_ggtt(struct xe_gt *gt)
>  	    !xe_sriov_vf_migration_supported(gt_to_xe(gt)))
>  		return 0;
>  
> -	ret = wait_event_interruptible_timeout(gt->sriov.vf.migration.wq,
> -					       vf_valid_ggtt(gt),
> -					       HZ * 5);
> +	ret = wait_event_timeout(gt->sriov.vf.migration.wq,
> +				 vf_valid_ggtt(gt),
> +				 HZ * 5);
>  	xe_gt_WARN_ON(gt, !ret);
>  
>  	return atomic_read(&gt->sriov.vf.migration.fixups_complete_count);

[Severity: Medium]
Can __xe_ggtt_insert_bo_at() loop forever in uninterruptible sleep if
wait_event_timeout() times out while vf_valid_ggtt(gt) remains false?

On timeout (!ret), xe_gt_sriov_vf_wait_valid_ggtt() returns the
non-negative fixups_complete_count as marker, whereas
xe_vf_migration_fixups_complete_count() returns -1 whenever
!vf_valid_ggtt(gt):

drivers/gpu/drm/xe/xe_gt_sriov_vf.c:xe_vf_migration_fixups_complete_count() {
    ...
	/* should never match fixups_complete_count value */
	if (!vf_valid_ggtt(gt))
		return -1;

	return atomic_read(&gt->sriov.vf.migration.fixups_complete_count);
}

In __xe_ggtt_insert_bo_at(), marker != -1 is always true while
vf_valid_ggtt(gt) stays false, so it jumps back to retry and sleeps
uninterruptibly for another 5 seconds indefinitely while holding the BO
dma_resv lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106215700.1218318-1-tomasz.lis@intel.com?part=2