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

[sashiko-bot@kernel.org]

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.

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

> +}
> +
>  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.

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