Re: [CI] drm/xe/vram: Add early VRAM health check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] VRAM memtest BOs are leaked on error paths and skipped tiles.
- [High] `xe_migrate_debug_ccs_overlap` fails to acquire the necessary `job_mutex` lock.
- [High] `xe_migrate_debug_ccs_overlap` fails to initialize the second migration batch buffer, causing execution of uninitialized GPU memory.
- [High] Hardcoded 256MB VRAM bounds in `xe_migrate_debug_ccs_overlap` causes out-of-bounds GPU reads on systems with small VRAM.
- [High] The CCS overlap test writes a canary to the end of VRAM but checks the CCS metadata for the beginning of VRAM, rendering the test useless.
- [Medium] Mismatch between code implementation and comment for flat CCS offset rounding.
- [Medium] Config guard mismatch for `memtest_bo` causes build failures.
- [Medium] Ignoring the return value of `xe_migrate_clear()` masks software errors as hardware failures.
--

commit c386b266adaf1a488d11cddd5ddf577c88467fb1
Author: Matthew Auld <matthew.auld@intel.com>
Subject: drm/xe/vram: Add early VRAM health check

This commit introduces an early VRAM health check to detect BAR sizing, CCS
sizing, and identity map setup issues using a canary page at the end of VRAM.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 71ce153737abe..5d9e3dc133fa8 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -1040,6 +1040,8 @@ int xe_device_probe(struct xe_device *xe)
>  	if (err)
>  		return err;
>  
> +	xe_vram_reserve_memtest_bo(xe);
> +
>  	for_each_tile(tile, xe, id) {
>  		err = xe_tile_init(tile);
>  		if (err)
>  			return err;

[Severity: High]
If xe_tile_init() fails, does this code leak the reserved memtest BOs since
there is no cleanup before returning the error?

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index f79d0047bec69..644be4c2d4404 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
[ ... ]
> @@ -2635,3 +2635,59 @@ void xe_migrate_job_lock_assert(struct xe_exec_queue *q)
>  #if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
>  #include "tests/xe_migrate.c"
>  #endif
> +
> +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG_MEM)
> +void xe_migrate_debug_ccs_overlap(struct xe_migrate *m, struct xe_bo *last_page_bo, struct xe_bo *scratch_bo)
> +{
> +	struct xe_device *xe = tile_to_xe(m->tile);
> +	struct xe_gt *gt = m->tile->primary_gt;
> +	struct dma_fence *fence;
> +	struct xe_bb *bb;
> +	struct xe_sched_job *job;
> +	u64 first_page_dpa, clear_L0_ofs, scratch_dpa, scratch_L0_ofs;
> +
> +	if (!xe_device_has_flat_ccs(xe))
> +		return;
> +
> +	first_page_dpa = xe_vram_region_dpa_base(m->tile->mem.vram);
> +	clear_L0_ofs = xe_migrate_vram_ofs(xe, first_page_dpa, true);

[Severity: High]
The test writes a canary pattern to the end of VRAM (vram_size - SZ_64K) later,
but clear_L0_ofs is based on first_page_dpa (the very beginning of VRAM). Does
this check the wrong CCS metadata region and miss the canary pattern entirely?

> +
> +	scratch_dpa = xe_bo_addr(scratch_bo, 0, XE_PAGE_SIZE);
> +	scratch_L0_ofs = xe_migrate_vram_ofs(xe, scratch_dpa, false);
> +
> +	for (int j = 0; j < 4; j++) {
> +		bb = xe_bb_new(gt, 256, xe->info.has_usm);
> +		if (IS_ERR(bb)) {
> +			drm_warn(&xe->drm, "Failed to create bb for VRAM overlap check\n");
> +			return;
> +		}
> +
> +		for (int i = 0; i < 16; i++) {
> +			int idx = (j * 16) + i;
> +			/* 4MB payload = 16KB CCS metadata */
> +			emit_copy_ccs(gt, bb, scratch_L0_ofs + (idx * 16384), false,
> +				      clear_L0_ofs + (idx * SZ_4M), true, SZ_4M);

[Severity: High]
Is it possible for a tile's VRAM to be smaller than 256MB? This loop
unconditionally tests 256MB (64 iterations of 4MB chunks), which might result
in out-of-bounds GPU reads on smaller VRAM configurations.

> +		}
> +
> +		bb->cs[bb->len++] = MI_BATCH_BUFFER_END;
> +
> +		job = xe_bb_create_migration_job(m->q, bb,
> +						 xe_migrate_batch_base(m, xe->info.has_usm),
> +						 bb->len);

[Severity: High]
By passing bb->len as the second_idx right after writing MI_BATCH_BUFFER_END,
does xe_bb_create_migration_job() leave the second batch buffer empty and
unterminated? This might cause the GPU to execute uninitialized memory.

> +		if (!IS_ERR(job)) {
> +			xe_sched_job_add_migrate_flush(job, MI_FLUSH_DW_CCS);
> +			xe_sched_job_arm(job);
> +
> +			fence = dma_fence_get(&job->drm.s_fence->finished);
> +			xe_sched_job_push(job);

[Severity: High]
Does this code need to acquire m->job_mutex before calling xe_sched_job_arm()
and xe_sched_job_push()? Other migration jobs hold this lock to serialize
queue accesses.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_tile_types.h b/drivers/gpu/drm/xe/xe_tile_types.h
> index 0048100ccb723..75097e4d1c643 100644
> --- a/drivers/gpu/drm/xe/xe_tile_types.h
> +++ b/drivers/gpu/drm/xe/xe_tile_types.h
> @@ -97,6 +97,10 @@ struct xe_tile {
>  		 * Only main GT has page reclaim list allocations.
>  		 */
>  		struct xe_sa_manager *reclaim_pool;
> +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> +		/** @mem.memtest_bo: VRAM overlap check BO */
> +		struct xe_bo *memtest_bo;
> +#endif

[Severity: Medium]
In this header, memtest_bo is guarded by CONFIG_DRM_XE_DEBUG, but in
xe_vram.c and xe_migrate.c its usage is guarded by CONFIG_DRM_XE_DEBUG_MEM.
Will this cause a build failure if CONFIG_DRM_XE_DEBUG_MEM is enabled without
CONFIG_DRM_XE_DEBUG?

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
> index 23eb7edbdd572..eb3d7b42371cd 100644
> --- a/drivers/gpu/drm/xe/xe_vram.c
> +++ b/drivers/gpu/drm/xe/xe_vram.c
[ ... ]
> @@ -89,7 +92,7 @@ static int get_flat_ccs_offset(struct xe_gt *gt, u64 tile_size, u64 *poffset)
>  		offset = offset_hi << 32; /* HW view bits 39:32 */
>  		offset |= offset_lo << 6; /* HW view bits 31:6 */
>  		offset *= num_enabled; /* convert to SW view */
> -		offset = round_up(offset, SZ_128K); /* SW must round up to nearest 128K */
> +		offset = round_up(offset, SZ_128K); /* SW must round down to nearest 2M */

[Severity: Medium]
The comment states we must round down to nearest 2M, but the code still calls
round_up(offset, SZ_128K). Which one is the intended behavior?

[ ... ]
> @@ -387,3 +390,160 @@ resource_size_t xe_vram_region_actual_physical_size(const struct xe_vram_region
[ ... ]
> +int xe_vram_memtest(struct xe_device *xe)
> +{
> +	struct xe_tile *tile;
> +	u8 id;
> +	int err = 0;
> +
> +	for_each_tile(tile, xe, id) {
> +		struct xe_bo *last_page_bo = tile->mem.memtest_bo;
> +		struct dma_fence *fence;
> +		bool overlap = false;
> +		int i, consecutive = 0;
> +		u8 val;
> +
> +		if (!last_page_bo || !tile->migrate)
> +			continue;

[Severity: High]
For tiles without a migrate engine, does this continue bypass the unpin and
cleanup of tile->mem.memtest_bo, leading to a resource leak?

[ ... ]
> +		/* 2. Non-CCS access via GPU on the last page */
> +		fence = xe_migrate_clear(tile->migrate, last_page_bo,
> +					 last_page_bo->ttm.resource,
> +					 XE_MIGRATE_CLEAR_FLAG_BO_DATA);
> +		if (!IS_ERR(fence)) {
> +			dma_fence_wait(fence, false);
> +			dma_fence_put(fence);
> +		}
> +
> +		rmb();
> +
> +		val = xe_map_rd(xe, &last_page_bo->vmap, 0, u8);
> +		if (drm_WARN(&xe->drm, val != 0x00,
> +			     "Tile %d: GPU memtest clear failed at offset 0 (expected 0x00, got 0x%02x)\n",
> +			     id, val)) {

[Severity: Medium]
If xe_migrate_clear() fails and returns an ERR_PTR, this skips the wait but
proceeds to read the VRAM validation byte. Will this falsely report a GPU
hardware failure because it reads the old 0xA5 value instead of 0x00?

[ ... ]
> +
> +unpin:
> +		xe_bo_unpin_map_no_vm(last_page_bo);
> +		tile->mem.memtest_bo = NULL;
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
On error, this early return skips cleanup for the remaining tiles in the loop.
Does this code leak the memtest_bo for subsequent tiles?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826125304.378173-2-matthew.auld@intel.com?part=1