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

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 04, 2026 at 12:31:49AM +0000, sashiko-bot@kernel.org wrote:
> 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.
> --

All of these are correct. Will fix.

Matt

> 
> 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