Re: [PATCH v5 2/5] drm/xe/migrate: Support copying between sysmem and stolen

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/xe/migrate: is_devmem() checks bo->ttm.resource instead of
  res
- [High] drm/xe/migrate: NULL deref and broken PTEs for stolen on
  pre-1270 GPUs
--

commit 3c7e5791762e378b27bc3e708f8e54449d9a1afa
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/xe/migrate: Support copying between sysmem and stolen

This commit updates xe_migrate to disallow identity mapping for
XE_PL_STOLEN and adds an is_devmem() helper to handle stolen device
memory during migration copies and clears.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 19c37eb315893..23b8f50d1ba21 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
[ ... ]
> @@ -1206,6 +1210,14 @@ static u32 xe_migrate_ccs_copy(struct xe_migrate *m,
>  	return flush_flags;
>  }
>  
> +static bool is_devmem(struct xe_bo *bo, struct ttm_resource *res)
> +{
> +	if (mem_type_is_vram(res->mem_type))
> +		return true;
> +
> +	return xe_bo_is_stolen_devmem(bo);
> +}

[Severity: High]
Does is_devmem() cause a NULL pointer dereference in xe_bo_sg() when
migrating or clearing an XE_PL_STOLEN BO on platforms with graphics
version < 12.70 (such as TGL, ADL, DG1, or DG2)?

mem_type_is_vram() explicitly excludes XE_PL_STOLEN, and
xe_bo_is_stolen_devmem() returns false when GRAPHICS_VERx100(xe) < 1270,
so is_devmem() returns false for stolen memory on those platforms.
__xe_migrate_copy() and xe_migrate_clear() then take the non-VRAM path
and call xe_res_first_sg(xe_bo_sg(bo), ...), but stolen BOs are managed
with use_tt = false and have bo->ttm.ttm == NULL:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_sg() {
	struct ttm_tt *tt = bo->ttm.ttm;
	struct xe_ttm_tt *xe_tt = container_of(tt, struct xe_ttm_tt, ttm);

	return xe_tt->sg;
}

In addition, emit_pte() only adds vram_region_gpu_offset(res) (which
returns mgr->stolen_base for XE_PL_STOLEN) inside the if (is_vram)
block alongside devmem = true, whereas pre-12.70 integrated stolen
memory needs xe_res_first() and vram_region_gpu_offset(res) without
setting XE_PPGTT_PTE_DM, and pre-12.70 discrete stolen memory (DG1, DG2)
resides in LMEM and needs both.

>  static struct dma_fence *__xe_migrate_copy(struct xe_migrate *m,
>  					   struct xe_bo *src_bo,
>  					   struct xe_bo *dst_bo,
> @@ -1226,8 +1238,8 @@ static struct dma_fence *__xe_migrate_copy(struct xe_migrate *m,
>  	int err;
>  	bool src_is_pltt = src->mem_type == XE_PL_TT;
>  	bool dst_is_pltt = dst->mem_type == XE_PL_TT;
> -	bool src_is_vram = mem_type_is_vram(src->mem_type);
> -	bool dst_is_vram = mem_type_is_vram(dst->mem_type);
> +	bool src_is_vram = is_devmem(src_bo, src);
> +	bool dst_is_vram = is_devmem(dst_bo, dst);

[Severity: High]
When moving or evicting a BO between XE_PL_STOLEN and XE_PL_TT, does
is_devmem() inspect bo->ttm.resource instead of the passed-in res?

xe_bo_is_stolen_devmem() calls xe_bo_is_stolen(), which checks
bo->ttm.resource->mem_type == XE_PL_STOLEN rather than res->mem_type.
During xe_bo_move(), xe_migrate_copy(migrate, bo, bo, old_mem, new_mem,
...) and xe_migrate_clear(migrate, bo, new_mem, ...) run before
bo->ttm.resource is updated to new_mem.

If a stolen BO is evicted from XE_PL_STOLEN (src) to XE_PL_TT (dst),
is_devmem(dst_bo, dst) sees bo->ttm.resource->mem_type == XE_PL_STOLEN
and returns true for the XE_PL_TT destination.  __xe_migrate_copy() then
initializes dst_it with xe_res_first(dst, ...) (which sets
dst_it.mem_type = XE_PL_TT and dst_it.size = size), passes
PTE_UPDATE_FLAG_IS_VRAM to pte_update_size(), and because
xe_migrate_allow_identity() returns true for dst_it.mem_type !=
XE_PL_STOLEN, calls xe_migrate_vram_ofs() and dereferences a NULL
xe->mem.vram on integrated GPUs.  Conversely, moving from XE_PL_TT to
XE_PL_STOLEN or clearing a newly allocated XE_PL_STOLEN resource while
bo->ttm.resource is NULL or XE_PL_TT returns false from is_devmem() and
calls xe_bo_sg() on a stolen BO.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113124453.144408-7-dev@lankhorst.se?part=2