Re: [PATCH v4 17/25] drm/xe: Add device flag to enable PT mirroring across tiles

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

On Fri, Sep 04, 2026 at 12:29:53AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Incorrect tile mask generation causes missing TLB invalidations on secondary tiles
> - [High] Shallow copy of Page Reclaim List (PRL) leads to Double Free during TLB invalidation
> - [Medium] Unnecessary allocation of unused page table roots and scratch tables
> --
> 
> commit 084f96016724c31d7bcd97e7c49979198c29fe71
> Author: Matthew Brost <matthew.brost@intel.com>
> Subject: drm/xe: Add device flag to enable PT mirroring across tiles
> 
> Some multi-tile devices may want to mirror page tables across tiles for
> memory-bandwidth reasons, while others may not. Add a device flag that
> allows enabling or disabling page-table mirroring across tiles.
> 
> Link: https://patch.msgid.link/20260228013501.106680-18-matthew.brost@intel.com
> 
> > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > index 821edeaecaed9..deb33e85e6eb7 100644
> > --- a/drivers/gpu/drm/xe/xe_pt.c
> > +++ b/drivers/gpu/drm/xe/xe_pt.c
> [ ... ]
> > @@ -2542,9 +2553,20 @@ int xe_pt_update_ops_prepare(struct xe_device *xe, struct xe_vma_ops *vops)
> >  	int id, err;
> >  
> >  	for_each_tile(tile, xe, id) {
> > +		struct xe_vm_pgtable_update_ops *pt_update_ops =
> > +			&vops->pt_update_ops[id];
> > +
> >  		if (!vops->pt_update_ops[id].num_ops)
> >  			continue;
> >  
> > +		if (pt_mirroring_disabled_for_tile(vops->vm, id)) {
> > +			struct xe_page_reclaim_list *prl = &pt_update_ops->prl;
> > +
> > +			/* Transfer root PT update ops PRL to current */
> > +			*prl = vops->pt_update_ops[0].prl;
> 
> [Severity: High]
> Does this shallow copy of the Page Reclaim List (PRL) cause a double free?
> 
> By copying the structure directly without incrementing a reference count,
> multiple tiles now hold a pointer to the same page. Later, when
> xe_tlb_inval_job_create() adds this to jobs in xe_pt_update_ops_run(), both
> jobs take ownership. When the jobs finish, they will both call
> xe_page_reclaim_entries_put(), freeing the same page twice.
> 

This is correct. Will fix.

> > +			continue;
> > +		}
> > +
> >  		err = __xe_pt_update_ops_prepare(tile, vops);
> >  		if (err)
> >  			return err;
> [ ... ]
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 33ef4ccc3550b..d9a9c0844c5e3 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -844,6 +844,14 @@ int xe_vm_rebind(struct xe_vm *vm, bool rebind_worker)
> >  	return err;
> >  }
> >  
> > +static u8 adjust_rebind_tile_mask(struct xe_vm *vm, u8 tile_mask)
> > +{
> > +	if (vm->xe->info.has_pt_mirror)
> > +		return tile_mask;
> > +
> > +	return (0x1 << vm->xe->info.max_gt_per_tile) - 1;
> 
> [Severity: High]
> Is max_gt_per_tile being used here incorrectly instead of the total tile count?
> 
> If a device (like PVC) has 1 GT per tile, this evaluates to 1, causing the
> generated mask to omit secondary tiles. During xe_vma_rebind(), the num_ops
> will only be incremented for tile 0.
> 
> Consequently, xe_pt_update_ops_prepare() skips preparation for the secondary
> tiles, leaving their start and last variables uninitialized (zeroed). When
> TLB invalidation jobs are created later, they use a range of [0, 0] for
> these tiles, leaving stale TLB entries that could cause memory corruption.
> 

This is correct. Will fix.

> > +}
> > +
> >  struct dma_fence *xe_vma_rebind(struct xe_vm *vm, struct xe_vma *vma, u8 tile_mask)
> >  {
> >  	struct dma_fence *fence = NULL;
> [ ... ]
> > @@ -1737,7 +1747,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags, struct xe_file *xef)
> >  
> >  		for_each_tile(tile, xe, id) {
> >  			if (flags & XE_VM_FLAG_MIGRATION &&
> > -			    tile->id != XE_VM_FLAG_TILE_ID(flags))
> > +			    tile->id != XE_VM_FLAG_TILE_ID(flags) &&
> > +			    (vm->xe->info.has_pt_mirror || id))
> >  				continue;
> >  
> >  			vm->pt_root[id] = xe_pt_create(vm, tile, xe->info.vm_max_level,
> >  						       &exec);
> 
> [Severity: Medium]
> For standard user VMs, does this unconditionally allocate page table roots
> and scratch tables for secondary tiles even when PT mirroring is disabled?
> 
> When has_pt_mirror is false, xe_vm_pt_root() is hardcoded to return
> vm->pt_root[0]. As a result, the allocations made here for id > 0 (and their
> associated scratch tables) are never used, wasting VRAM.
> 

Techincally, but I'm not going to worry about 4k vs functionality
required for the migrate layer.

Matt 

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