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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Memory leak of `scratch_bo_before` and `scratch_bo_after` on error paths in `xe_vram_memtest()`.
- [Medium] L2 cache flush in `xe_device_l2_flush()` always targets the root GT, missing remote tiles on multi-tile devices.
- [Medium] Ignored return value of `devm_add_action_or_reset()` in `xe_vram_reserve_memtest_bo()`.
--

commit 2f7e446013d1817f370eab63e573b6ed23bda268
Author: Matthew Auld <matthew.auld@intel.com>

    drm/xe/vram: Add early VRAM health check

This commit introduces an early VRAM health check that reserves the last page
of VRAM as a canary. It performs CPU and GPU memory operations to verify
mapping and check for flat CCS metadata overlaps.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 396d02eb2af83..601c372775eff 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -1270,7 +1276,7 @@ bool xe_device_is_l2_flush_optimized(struct xe_device *xe)
>  	return false;
>  }
>  
> -void xe_device_l2_flush(struct xe_device *xe)
> +void xe_device_l2_flush(struct xe_device *xe, bool force)
>  {
>  	struct xe_gt *gt;
>  
> @@ -1278,7 +1284,7 @@ void xe_device_l2_flush(struct xe_device *xe)
>  	if (!gt)
>  		return;
>  
> -	if (!XE_GT_WA(gt, 16023588340))
> +	if (!force && !XE_GT_WA(gt, 16023588340))
>  		return;

[Severity: Medium]
Since this function hardcodes the target to the root GT via xe_root_mmio_gt(),
will this miss flushing the L2 cache on remote tiles?

When called in a multi-tile context (like during xe_vram_memtest() below),
flushing only the root GT could cause CPU writes to VRAM on remote tiles to
remain in the cache, preventing the GPU migrator from seeing them.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
> index 7b4478fb1697c..76cb87dc48578 100644
> --- a/drivers/gpu/drm/xe/xe_vram.c
> +++ b/drivers/gpu/drm/xe/xe_vram.c
[ ... ]
> +void xe_vram_reserve_memtest_bo(struct xe_device *xe)
> +{
> +	struct xe_tile *tile;
> +	u8 id;
> +
> +	if (IS_SRIOV_VF(xe))
> +		return;
> +
> +	for_each_tile(tile, xe, id) {
> +		u64 vram_size;
[ ... ]
> +		tile->mem.memtest_bo = xe_bo_create_pin_map_at_novm(xe, tile, SZ_64K,
> +								    vram_size - SZ_64K,
> +								    ttm_bo_type_kernel,
> +								    XE_BO_FLAG_VRAM_IF_DGFX(tile),
> +								    0, false);
> +		if (IS_ERR(tile->mem.memtest_bo)) {
> +			drm_warn(&xe->drm, "Tile %d: Failed to reserve memtest BO\n", id);
> +			tile->mem.memtest_bo = NULL;
> +			continue;
> +		}
> +
> +		drm_info(&xe->drm, "Tile %d: Reserved memtest BO at offset 0x%llx\n",
> +			 id, vram_size - SZ_64K);
> +	}
> +
> +	devm_add_action_or_reset(xe->drm.dev, memtest_bo_cleanup, xe);

[Severity: Medium]
Is it safe to ignore the return value of devm_add_action_or_reset() here?

If devres allocation fails due to memory pressure, it will return -ENOMEM
and immediately execute memtest_bo_cleanup(), freeing the BOs. The function
will then silently exit instead of properly propagating the error, which
could leave the driver state expecting those BOs to exist.

> +}
[ ... ]
> +int xe_vram_memtest(struct xe_device *xe)
> +{
> +	struct xe_tile *tile;
> +	u8 id;
> +	int err = 0;
[ ... ]
> +		if (xe_device_has_flat_ccs(xe) && GRAPHICS_VERx100(xe) >= 2000) {
> +			struct xe_bo *scratch_bo_before;
> +			struct xe_bo *scratch_bo_after;
> +
> +			scratch_bo_before = xe_bo_create_pin_map_novm(xe, tile, SZ_1M,
> +								      ttm_bo_type_kernel,
> +								      XE_BO_FLAG_VRAM_IF_DGFX(tile),
> +								      false);
> +			if (IS_ERR(scratch_bo_before)) {
> +				err = PTR_ERR(scratch_bo_before);
> +				goto unpin;
> +			}
> +
> +			scratch_bo_after = xe_bo_create_pin_map_novm(xe, tile, SZ_1M,
> +								     ttm_bo_type_kernel,
> +								     XE_BO_FLAG_VRAM_IF_DGFX(tile),
> +								     false);
> +			if (IS_ERR(scratch_bo_after)) {
> +				xe_bo_unpin_map_no_vm(scratch_bo_before);
> +				err = PTR_ERR(scratch_bo_after);
> +				goto unpin;
> +			}
> +
> +			/* Save original CCS metadata for PA 0 */
> +			err = xe_migrate_debug_ccs_overlap(tile->migrate, scratch_bo_before, false);
> +			if (err)
> +				goto unpin;

[Severity: Medium]
Does this code leak both scratch_bo_before and scratch_bo_after?

If xe_migrate_debug_ccs_overlap() fails here, the execution jumps directly
to the unpin label. Because the cleanup block down there only handles the
global memtest_bo through xe_vram_free_memtest_bos(), both of these
temporary pinned BOs will be permanently lost if the jump occurs.

> +
> +			/* Fill last page with CPU */
> +			xe_map_memset(xe, &last_page_bo->vmap, 0, 0x5A, SZ_64K);
> +			xe_device_wmb(xe);
> +			xe_device_l2_flush(xe, true);
> +
> +			/* Use GPU to clear CCS state for PA 0 */
> +			xe_map_memset(xe, &scratch_bo_after->vmap, 0, 0x00, SZ_1M);
> +			err = xe_migrate_debug_ccs_overlap(tile->migrate, scratch_bo_after, true);
> +			if (err)
> +				goto unpin;

[Severity: Medium]
This is another error path where the scratch BOs appear to be leaked by
skipping the xe_bo_unpin_map_no_vm() calls before jumping to the unpin label.

> +			xe_device_l2_flush(xe, true);
> +
> +			/* Check if last_page_bo was corrupted by the GPU CCS clear */
> +			for (i = 0; i < SZ_64K; i += 8) {
> +				u64 payload = xe_map_rd(xe, &last_page_bo->vmap, i, u64);
> +
> +				if (payload != 0x5A5A5A5A5A5A5A5AULL) {
> +					overlap = true;
> +					break;
> +				}
> +			}
> +
> +			/* Restore original CCS metadata for PA 0 */
> +			err = xe_migrate_debug_ccs_overlap(tile->migrate, scratch_bo_before, true);
> +			if (err)
> +				drm_warn(&xe->drm, "Failed to restore CCS metadata\n");
> +
> +			xe_bo_unpin_map_no_vm(scratch_bo_before);
> +			xe_bo_unpin_map_no_vm(scratch_bo_after);
> +		}

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