Re: [PATCH v4 16/25] drm/xe: Add CPU bind layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] TLB invalidation on mirrored (shared) tiles uses uninitialized `start` and `last` addresses, failing to invalidate the correctly updated address range.
- [Critical] Shallow copying of the page reclaim list (PRL) for mirrored tiles leads to a Use-After-Free and double-free of the PRL entries during TLB invalidation.
- [Critical] The arguments `tile_mask` and `usage` are swapped when calling `unbind_op_commit()` in the `DRM_GPUVA_OP_UNMAP` case, leading to un-cleared VMAs and corrupted dma-resv state.
- [High] Missing NULL check for the result of `drmm_kzalloc()` in `xe_cpu_bind_init()`.
- [High] `op_prepare` mistakenly uses `XE_VMA_OPS_FLAG_WAIT_VM_KERNEL` instead of `XE_VMA_OPS_FLAG_WAIT_VM_BOOKKEEP` for REMAP operations, breaking dma-resv wait semantics.
--

commit 87e03e5ddd64ecb931de7c6186e72459d6401535
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/xe: Add CPU bind layer

This commit introduces a dedicated CPU bind layer for the xe driver, moving
CPU bind handling out of the migrate layer since it's decoupled from
hardware. It also updates the page table layer to generate a single bind job
when pages are mirrored across tiles.

Link: https://patch.msgid.link/20260228013501.106680-17-matthew.brost@intel.com

> diff --git a/drivers/gpu/drm/xe/xe_cpu_bind.c b/drivers/gpu/drm/xe/xe_cpu_bind.c
> new file mode 100644
> index 0000000000000..ed79e229151b4
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cpu_bind.c
> @@ -0,0 +1,295 @@
[ ... ]
> +int xe_cpu_bind_init(struct xe_device *xe)
> +{
> +	struct xe_cpu_bind *cpu_bind =
> +		drmm_kzalloc(&xe->drm, sizeof(*cpu_bind), GFP_KERNEL);
> +	struct xe_exec_queue *q;
> +
> +	q = xe_exec_queue_create_bind(xe, xe_device_get_root_tile(xe), NULL,
> +				      EXEC_QUEUE_FLAG_KERNEL |
> +				      EXEC_QUEUE_FLAG_MIGRATE, 0);
> +	if (IS_ERR(q))
> +		return PTR_ERR(q);
> +
> +	cpu_bind->xe = xe;

[Severity: High]
Does this code dereference cpu_bind without checking if drmm_kzalloc()
succeeded? If the allocation fails under memory pressure and returns NULL,
this assignment will cause a NULL pointer dereference kernel panic during
device probe.

> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 7c7cd7a0365b7..821edeaecaed9 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
[ ... ]
> @@ -2395,7 +2388,7 @@ static int op_prepare(struct xe_vm *vm,
>  
>  		err = bind_op_prepare(vm, tile, pt_update_ops, op->map.vma,
>  				      op->map.invalidate_on_bind);
> -		pt_update_ops->wait_vm_kernel = true;
> +		vops->flags |= XE_VMA_OPS_FLAG_WAIT_VM_KERNEL;
>  		break;
>  	case DRM_GPUVA_OP_REMAP:
>  	{
> @@ -2409,12 +2402,12 @@ static int op_prepare(struct xe_vm *vm,
>  		if (!err && op->remap.prev && !op->remap.skip_prev) {
>  			err = bind_op_prepare(vm, tile, pt_update_ops,
>  					      op->remap.prev, false);
> -			pt_update_ops->wait_vm_bookkeep = true;
> +			vops->flags |= XE_VMA_OPS_FLAG_WAIT_VM_KERNEL;
>  		}
>  		if (!err && op->remap.next && !op->remap.skip_next) {
>  			err = bind_op_prepare(vm, tile, pt_update_ops,
>  					      op->remap.next, false);
> -			pt_update_ops->wait_vm_bookkeep = true;
> +			vops->flags |= XE_VMA_OPS_FLAG_WAIT_VM_KERNEL;
>  		}
>  		break;
>  	}

[Severity: High]
Are the flag updates in the DRM_GPUVA_OP_REMAP branch intentionally setting
XE_VMA_OPS_FLAG_WAIT_VM_KERNEL instead of XE_VMA_OPS_FLAG_WAIT_VM_BOOKKEEP?

The original code correctly set wait_vm_bookkeep to true for remap
operations, but the updated version uses the KERNEL flag. This flips the
expected synchronization dependency semantics, which could cause REMAP
operations to incorrectly wait on KERNEL fences instead of BOOKKEEP fences,
violating the synchronization contract.

[ ... ]
> @@ -2604,21 +2624,19 @@ static void unbind_op_commit(struct xe_vm *vm, struct xe_tile *tile,
>  
>  static void range_present_and_invalidated_tile(struct xe_vm *vm,
>  					       struct xe_svm_range *range,
> -					       u8 tile_id)
> +					       u8 tile_mask)
>  {
>  	/* All WRITE_ONCE pair with READ_ONCE in xe_vm_has_valid_gpu_mapping() */
>  
>  	lockdep_assert_held(&vm->svm.gpusvm.notifier_lock);
>  
> -	WRITE_ONCE(range->tile_present, range->tile_present | BIT(tile_id));
> -	WRITE_ONCE(range->tile_invalidated, range->tile_invalidated & ~BIT(tile_id));
> +	WRITE_ONCE(range->tile_present, range->tile_present | tile_mask);
> +	WRITE_ONCE(range->tile_invalidated, range->tile_invalidated & ~tile_mask);
>  }
>  
> -static void op_commit(struct xe_vm *vm,
> -		      struct xe_tile *tile,
> -		      struct xe_vm_pgtable_update_ops *pt_update_ops,
> -		      struct xe_vma_op *op, struct dma_fence *fence,
> -		      struct dma_fence *fence2)
> +static void op_commit(struct xe_vm *vm, struct xe_vma_op *op,
> +		      struct dma_fence **fences, int fence_count,
> +		      enum dma_resv_usage usage, u8 tile_mask)
>  {
>  	xe_vm_assert_held(vm);
>  
[ ... ]
> @@ -2657,8 +2672,8 @@ static void op_commit(struct xe_vm *vm,
>  		struct xe_vma *vma = gpuva_to_vma(op->base.unmap.va);
>  
>  		if (!xe_vma_is_cpu_addr_mirror(vma))
> -			unbind_op_commit(vm, tile, pt_update_ops, vma, fence,
> -					 fence2);
> +			unbind_op_commit(vm, vma, fences, fence_count,
> +					 tile_mask, usage);
>  		break;
>  	}

[Severity: Critical]
Is it possible the tile_mask and usage arguments are swapped here?

Looking at the signature for unbind_op_commit(), it expects usage before
tile_mask:

unbind_op_commit(struct xe_vm *vm, struct xe_vma *vma,
                 struct dma_fence **fences, int fence_count,
                 enum dma_resv_usage usage, u8 tile_mask)

Because the arguments are passed in reverse order, the dma_resv usage enum
is treated as the tile mask. This would mean vma->tile_present &= ~tile_mask
evaluates with the enum value (e.g. DMA_RESV_USAGE_KERNEL = 0), leaving
the VMA marked fully present, and un-cleared memory could be incorrectly
freed. Simultaneously, the tile mask is passed into the dma_resv tracking
slots as a bogus usage value.

[ ... ]
> @@ -2737,82 +2739,83 @@ static struct xe_dep_scheduler *to_dep_scheduler(struct xe_exec_queue *q,
>  struct dma_fence *
> -xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> +xe_pt_update_ops_run(struct xe_device *xe, struct xe_vma_ops *vops)
>  {
[ ... ]
> -	if (pt_update_ops->needs_invalidation) {
> -		struct xe_dep_scheduler *dep_scheduler =
> -			to_dep_scheduler(q, tile, tile->primary_gt, &type);
> -
> -		ijob = xe_tlb_inval_job_create(q, &tile->primary_gt->tlb_inval,
> -					       dep_scheduler, vm,
> -					       pt_update_ops->start,
> -					       pt_update_ops->last,
> -					       type);
> -		if (IS_ERR(ijob)) {
> -			err = PTR_ERR(ijob);
> -			goto kill_vm_tile1;
> -		}
> -		update.ijob = ijob;
> -		/*
> -		 * Only add page reclaim for the primary GT. Media GT does not have
> -		 * any PPC to flush, so enabling the PPC flush bit for media is
> -		 * effectively a NOP and provides no performance benefit nor
> -		 * interfere with primary GT.
> -		 */
> -		if (xe_page_reclaim_list_valid(&pt_update_ops->prl)) {
> -			xe_tlb_inval_job_add_page_reclaim(ijob, &pt_update_ops->prl);
> -			/* Release ref from alloc, job will now handle it */
> -			xe_page_reclaim_list_invalidate(&pt_update_ops->prl);
> -		}
> -
> -		if (tile->media_gt) {
> -			dep_scheduler = to_dep_scheduler(q, tile,
> -							 tile->media_gt, &type);
> -
> -			mjob = xe_tlb_inval_job_create(q,
> -						       &tile->media_gt->tlb_inval,
> -						       dep_scheduler, vm,
> -						       pt_update_ops->start,
> -						       pt_update_ops->last,
> -						       type);
> -			if (IS_ERR(mjob)) {
> -				err = PTR_ERR(mjob);
> +	if (needs_invalidation) {
> +		for_each_tlb_inval(q, i) {
> +			struct xe_dep_scheduler *dep_scheduler =
> +				q->tlb_inval[i].dep_scheduler;
> +			struct xe_tile *tile =
> +				&xe->tiles[i / XE_MAX_GT_PER_TILE];
> +			struct xe_vm_pgtable_update_ops *pt_update_ops =
> +				&vops->pt_update_ops[tile->id];
> +			struct xe_page_reclaim_list *prl = &pt_update_ops->prl;
> +			struct xe_tlb_inval_job *ijob;
> +			struct xe_gt *gt = i % XE_MAX_GT_PER_TILE ?
> +				tile->media_gt : tile->primary_gt;
> +
> +			ijob = xe_tlb_inval_job_create(q, &gt->tlb_inval,
> +						       dep_scheduler,
> +						       vm, pt_update_ops->start,
> +						       pt_update_ops->last, i);
> +			if (IS_ERR(ijob)) {
> +				err = PTR_ERR(ijob);
>  				goto free_ijob;
>  			}
> -			update.mjob = mjob;
> +
> +			update.ijobs[i] = ijob;
> +
> +			/*
> +			 * Only add page reclaim for the primary GT. Media GT
> +			 * does not have any PPC to flush, so enabling the PPC
> +			 * flush bit for media is effectively a NOP and provides
> +			 * no performance benefit nor interfere with primary GT.
> +			 */
> +			if (xe_page_reclaim_list_valid(prl)) {
> +				xe_tlb_inval_job_add_page_reclaim(ijob, prl);
> +				/* Release ref from alloc, job will now handle it */
> +				xe_page_reclaim_list_invalidate(prl);
> +			}
>  		}
>  	}

[Severity: Critical]
Does this code inadvertently create TLB invalidation jobs with uninitialized
bounds for secondary mirrored tiles?

When a tile shares a page table (mirroring disabled), the initialization of
pt_update_ops->start and pt_update_ops->last is skipped during the prepare
phase. Consequently, they remain 0. Here, xe_tlb_inval_job_create() relies
on these bounds, meaning the secondary tile's GTs could execute an invalid
TLB flush for the [0, 0] range, leaving stale TLB entries for the actual
modified range.

[Severity: Critical]
Can this loop trigger a use-after-free and double-free on the page reclaim
list (PRL)?

For secondary mirrored tiles, the PRL is set up as a shallow copy of the
primary tile's PRL without taking an additional page reference.
When the loop iterates over the primary tile, it adds the PRL and calls
xe_page_reclaim_list_invalidate(), which drops the page reference.

When the loop then reaches the secondary tile, its shallow-copied
prl.entries pointer is still non-NULL but points to freed memory. The code
will pass this dangling pointer to the secondary job and call
xe_page_reclaim_list_invalidate() a second time, triggering a double-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903235842.3401722-1-matthew.brost@intel.com?part=16