Re: [PATCH V17 08/12] drm/xe/vram: Add VRAM page offline fault handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Asynchronous queue termination during BO page purging causes GPU use-after-free for LRC and Page Table BOs.
- [High] Forceful unbalanced unpinning of a BO without ownership corrupts pin count tracking.
- [High] GSMBASE check incorrectly matches valid VRAM addresses on multi-tile devices, causing spurious system resets.
- [High] Dropping vram_mgr->lock before reserving the faulty page introduces a race window allowing duplicate tracking entries and memory corruption.
- [High] Racing with BO destruction returns -ENOENT, triggering an unnecessary System Reset (SBR).
- [High] Checking if a user BO is pinned without holding its reservation lock allows it to be pinned immediately before being purged.
- [High] Missing page alignment for HW-reported faulty addresses causes buddy allocator to fail, preventing page offlining.
- [Medium] The relative VRAM offset is stored in tracking structures and reported in warnings instead of the absolute Device Physical Address (DPA), breaking multi-tile hardware tracking.
- [Medium] The exec_queue and VM ban reasons are not set before killing the queues, leading to incorrect error reporting and telemetry.
--

--- Patch [8]: [PATCH V17 08/12] drm/xe/vram: Add VRAM page offline fault handler ---
commit 5eda8f6c71d41d8599cf1e30faca069cde3189fb
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>
Subject: drm/xe/vram: Add VRAM page offline fault handler

Add the core VRAM page offlining logic to handle HW-reported faulty
physical addresses.

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index 5513dc014f3e7..9311aa3728f84 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c

[ ... ]

> @@ -572,3 +578,289 @@ u64 xe_ttm_vram_get_avail(struct ttm_resource_manager *man)
> +static int xe_ttm_vram_purge_page(struct xe_device *xe, struct xe_bo *bo)
> +{
> +	struct ttm_operation_ctx ctx = {};
> +	struct xe_exec_queue *q_to_put = NULL;
> +	struct xe_exec_queue *q = NULL;
> +	struct xe_vm *vm = NULL;
> +	u32	flags;
> +	int ret = 0;
> +
> +	xe_bo_lock(bo, false);
> +	if (bo->vm)
> +		vm = xe_vm_get(bo->vm);
> +	flags = bo->flags;
> +	xe_bo_unlock(bo);
> +	/*  Ban VM if BO is PPGTT */
> +	if (vm && (flags & XE_BO_FLAG_PAGETABLE)) {
> +		down_write(&vm->lock);

[Severity: Medium]
Will this lead to incorrect error reporting and telemetry?

When killing the PPGTT VM here, its ban reasons are not updated to include
DRM_XE_EXEC_QUEUE_BAN_REASON_PAGE_OFFLINE. This could cause the system logs
and userspace APIs to report a generic context reset rather than a hardware
memory fault mitigation event.

> +		xe_vm_kill(vm, true);
> +		up_write(&vm->lock);
> +	}
> +	if (vm)
> +		xe_vm_put(vm);
> +
> +	xe_bo_lock(bo, false);
> +	q = READ_ONCE(bo->q);
> +	/*  Ban exec queue if BO is lrc */
> +	if (q && xe_exec_queue_get_unless_zero(q)) {
> +		/* ban queue */
> +		q_to_put = q;
> +	}
> +
> +	if (bo->purgeable.state == XE_MADV_PURGEABLE_PURGED) {
> +		/* Already purged by shrinker during unlocked window — nothing to do */
> +		xe_bo_unlock(bo);
> +		goto out;
> +	}
> +
> +	xe_bo_set_purgeable_state(bo, XE_MADV_PURGEABLE_DONTNEED);
> +	ttm_bo_unmap_virtual(&bo->ttm);   /* nuke CPU mmap + VRAM IO mappings */
> +	if (xe_bo_is_pinned(bo))
> +		xe_bo_unpin(bo);

[Severity: High]
Does this forceful unpinning without ownership corrupt pin count tracking?

When the offline handler processes a pinned BO, it unconditionally calls
xe_bo_unpin(bo), decreasing the pin count without taking ownership of the pin.
When the original legitimate owner later attempts to unpin the BO, the pin
count will underflow, triggering xe_assert(xe, xe_bo_is_pinned(bo)) inside
xe_bo_unpin() and resulting in a kernel crash.

> +	ret = xe_ttm_bo_purge(&bo->ttm, &ctx);
> +	xe_bo_unlock(bo);
> +
> +out:
> +	if (q_to_put) {

[Severity: Critical]
Does this sequence cause a GPU use-after-free for LRC BOs?

xe_ttm_bo_purge() synchronously frees the VRAM backing pages into the buddy
allocator before the actively running GPU queue is explicitly halted here.
Since LRC BOs are actively accessed by running engines without per-job
execution fences, the hardware may still be running and writing to the
now-freed pages when they are reallocated.

[Severity: Medium]
Also, similar to the VM kill above, is it intentional that the exec queue is
killed here without having its ban_reason updated to
DRM_XE_EXEC_QUEUE_BAN_REASON_PAGE_OFFLINE?

> +		xe_exec_queue_kill(q_to_put);
> +		xe_exec_queue_put(q_to_put);
> +	}
> +
> +	return ret;
> +}

[ ... ]

> +static int xe_ttm_vram_reserve_page_at_addr(struct xe_device *xe, u64 addr,
> +					    struct xe_ttm_vram_mgr *vram_mgr, struct gpu_buddy *mm)
> +{
> +	struct xe_ttm_vram_offline_resource *nentry;
> +	struct ttm_buffer_object *tbo = NULL;
> +	struct xe_bo *pbo_to_put = NULL;
> +	struct gpu_buddy_block *block;
> +	u64 size = SZ_4K;
> +	int ret = 0;
> +
> +	scoped_guard(mutex, &vram_mgr->lock) {
> +		if (xe_ttm_vram_page_already_processed(vram_mgr, addr))
> +			return -EEXIST;
> +		block = gpu_buddy_allocated_addr_to_block(mm, addr);
> +		if (WARN_ON(IS_ERR(block)))
> +			return PTR_ERR(block);
> +
> +		nentry = kzalloc_obj(*nentry);
> +		if (!nentry)
> +			return -ENOMEM;
> +		INIT_LIST_HEAD(&nentry->blocks);
> +		nentry->status = XE_PAGE_RESERVE_PENDING;
> +		nentry->addr = addr;

[Severity: Medium]
Will this store the relative offset instead of the absolute Device Physical
Address?

The entry point xe_ttm_vram_handle_addr_fault() translates the absolute DPA
to a relative offset (addr - vr->dpa_base) before passing it to this function.
Storing the relative offset here means debug messages and FW offlining
mechanisms reading this struct will consume the wrong physical address.

> +
> +		if (block) {
> +			struct xe_bo *pbo;
> +
> +			if (!block->private) {
> +				/* Race: another thread just reserved this block */
> +				kfree(nentry);
> +				return -EEXIST;
> +			}
> +			tbo = block->private;
> +			pbo = ttm_to_xe_bo(tbo);
> +
> +			/* Get reference safely - BO may have zero refcount */
> +			if (!xe_bo_get_unless_zero(pbo)) {
> +				kfree(nentry);
> +				return -ENOENT;
> +			}

[Severity: High]
Does returning -ENOENT here trigger an unnecessary System Reset?

If a hardware memory fault lands on a BO that is concurrently being freed or
destroyed, xe_bo_get_unless_zero() fails and the function returns -ENOENT.
The caller xe_ttm_vram_handle_addr_fault() treats any negative error code
other than -EEXIST or -EOPNOTSUPP as a signal to reset the system. Triggering
a system reset because a BO was freed seems inappropriate, since the physical
page will soon become free and can be safely offlined anyway.

> +			/*
> +			 * Critical kernel BO? Best-effort check without resv lock;
> +			 * worst case a concurrent pin causes reset path unnecessarily.
> +			 */
> +			if ((pbo->ttm.type == ttm_bo_type_kernel &&
> +			     !(pbo->flags & XE_BO_FLAG_PINNED_LATE_RESTORE)) ||
> +			    (xe_bo_is_user(pbo) && xe_bo_is_pinned(pbo))) {

[Severity: High]
Could this lockless check lead to purging an actively pinned user BO?

Checking xe_bo_is_pinned(pbo) without holding the BO's reservation lock means
a user BO might not be pinned at the time of the check, but could be pinned
by another thread right before xe_ttm_vram_purge_page() locks the BO.
The purge logic will then forcefully unpin and destroy the BO, leaving the
concurrent thread with an invalid, unbacked BO.

> +				kfree(nentry);
> +				pbo_to_put = pbo;
> +				drm_err(&xe->drm,
> +					"%s: addr: 0x%llx is critical kernel bo, requesting SBR\n",
> +					__func__, addr);
> +				break;
> +			}
> +			++vram_mgr->n_queued_pages;
> +			list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
> +		}
> +	}
> +
> +	/* Deferred put outside lock to avoid recursive deadlock */
> +	if (pbo_to_put) {
> +		xe_bo_put(pbo_to_put);
> +		/* Hint System controller driver for reset with -EIO  */
> +		return -EIO;
> +	}
> +
> +	if (block) {
> +		struct xe_ttm_vram_offline_resource *pos, *n;
> +		struct xe_bo *pbo = ttm_to_xe_bo(tbo);
> +
> +		/*
> +		 * Purge BO containing address - reference held from above.
> +		 * Note: brief window between purge (freeing blocks) and re-reserve
> +		 * below. If another allocation claims the block, buddy_alloc fails
> +		 * and the status will be shown as failed reservation.
> +		 */
> +		ret = xe_ttm_vram_purge_page(xe, pbo);
> +		xe_bo_put(pbo);
> +		if (ret)
> +			drm_warn(&xe->drm, "Purge failed at addr:0x%llx, ret:%d\n", addr, ret);
> +
> +		/* Reserve page at address addr*/
> +		scoped_guard(mutex, &vram_mgr->lock) {
> +			ret = xe_ttm_vram_buddy_alloc(vram_mgr, addr, addr + size,
> +						      size, size, &nentry->blocks,
> +						      GPU_BUDDY_RANGE_ALLOCATION,
> +						      NULL, &nentry->used_visible_size);
> +			if (ret) {
> +				drm_warn(&xe->drm,
> +					 "Could not reserve page at addr:0x%llx, ret:%d\n",
> +					 addr, ret);
> +				nentry->status = XE_PAGE_RESERVE_FAIL;
> +				return ret;
> +			}
> +
> +			list_for_each_entry_safe(pos, n, &vram_mgr->queued_pages, queued_link) {
> +				if (pos->addr == nentry->addr) {
> +					--vram_mgr->n_queued_pages;
> +					list_del_rcu(&pos->queued_link);
> +					break;
> +				}
> +			}
> +			list_add_rcu(&nentry->offlined_link, &vram_mgr->offlined_pages);
> +			/* RAS will send command to FW for offlining page based on ret value */
> +			++vram_mgr->n_offlined_pages;
> +			return ret;
> +		}
> +	} else {
> +		struct xe_ttm_vram_offline_resource *pos, *n;
> +
> +		scoped_guard(mutex, &vram_mgr->lock) {
> +			++vram_mgr->n_queued_pages;

[Severity: High]
Does dropping vram_mgr->lock before this point introduce a race window?

When the fault occurs on a free page, the function drops vram_mgr->lock after
the initial xe_ttm_vram_page_already_processed() check, without adding the new
entry to any tracking lists.
Reacquiring the lock here to perform the buddy allocation allows a race where
another fault for the same address arrives during the lockless window,
resulting in two concurrent buddy allocations for the same physical page and
duplicate list insertions. Normal GPU memory allocation could also claim the
buddy block before it is safely reserved.

> +			list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
> +			ret = xe_ttm_vram_buddy_alloc(vram_mgr, addr, addr + size,
> +						      size, size, &nentry->blocks,
> +						      GPU_BUDDY_RANGE_ALLOCATION,
> +						      NULL, &nentry->used_visible_size);
> +			if (ret) {
> +				drm_warn(&xe->drm,
> +					 "Could not reserve page at addr:0x%llx, ret:%d\n",
> +					 addr, ret);
> +				nentry->status = XE_PAGE_RESERVE_FAIL;
> +				return ret;
> +			}

[ ... ]

> +static struct xe_vram_region *xe_ttm_vram_addr_to_region(struct xe_device *xe, u64 addr)
> +{
> +	u64 raw_offset = xe_mmio_read64_2x32(&xe_device_get_root_tile(xe)->mmio, GSMBASE);
> +	/* force a 4K (4096 bytes) page alignment */
> +	u64 gsmbase_dpa = raw_offset & ~(u64)(PAGE_SIZE - 1);
> +	struct xe_vram_region *vr;
> +	struct xe_tile *tile;
> +	int id;
> +
> +	/* Addr from GSM? */
> +	if (addr >= gsmbase_dpa)
> +		/* Return NULL so the caller can request reset (SBR) */
> +		return NULL;

[Severity: High]
Can this GSMBASE check incorrectly match valid VRAM addresses on multi-tile
devices?

In a multi-tile configuration, the VRAM device physical address (DPA) of
secondary tiles (e.g., Tile 1) is typically mapped higher than the root tile's
VRAM and GSM base. A valid hardware memory fault on a secondary tile will have
addr >= gsmbase_dpa evaluate to true. The function then falsely identifies the
fault as occurring in Graphics Stolen Memory and returns NULL, triggering an
unnecessary System Reset instead of offlining the faulty page.

> +
> +	for_each_tile(tile, xe, id) {
> +		vr = tile->mem.vram;
> +		if (addr >= vr->dpa_base &&
> +		    addr < vr->dpa_base + vr->usable_size)
> +			return vr;
> +	}
> +
> +	/*
> +	 * Return an explicit error pointer so the caller knows the addr
> +	 * is invalid and should be ignored, NOT SBR.
> +	 */
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +/**
> + * xe_ttm_vram_handle_addr_fault - Handle vram physical address error flaged
> + * @xe: pointer to parent device
> + * @addr: physical faulty address
> + *
> + * Handle the physcial faulty address error on specific tile.
> + *
> + * Returns 0 for success, negative error code otherwise as follow:
> + * * %-EIO - critical BO or address outside any VRAM region; next action is reset.
> + * * %-EOPNOTSUPP - log-only policy or unknown address; no further action.
> + * * %-ENOMEM - allocation failure; next action is reset.
> + * * %-ENXIO - address not found in buddy; next action is reset.
> + * * %-EEXIST - address already processed; no further action.
> + * * % Any other negative error - next action is reset.
> + */
> +int xe_ttm_vram_handle_addr_fault(struct xe_device *xe, u64 addr)
> +{
> +	struct xe_ttm_vram_mgr *vram_mgr;
> +	struct xe_vram_region *vr;
> +	struct gpu_buddy *mm;
> +
> +	vr = xe_ttm_vram_addr_to_region(xe, addr);
> +	if (IS_ERR(vr)) {
> +		/*
> +		 * The addr is outside VRAM and GSM.
> +		 * Log a debug message if needed, and safely exit/ignore.
> +		 */
> +		drm_dbg(&xe->drm, "Address %llx is out of bounds, ignoring fault.\n", addr);
> +		return PTR_ERR(vr);
> +	}
> +	if (!vr) {
> +		drm_err(&xe->drm, "%s:%d GSM addr:%llx error requesting SBR\n",
> +			__func__, __LINE__, addr);
> +		/* Hint System controller driver for reset with -EIO  */
> +		return -EIO;
> +	}
> +	vram_mgr = &vr->ttm;
> +	mm = &vram_mgr->mm;
> +
> +	/* Reserve page at address */
> +	return xe_ttm_vram_reserve_page_at_addr(xe, addr - vr->dpa_base, vram_mgr, mm);
> +}

[Severity: High]
Does the missing page alignment for HW-reported faulty addresses cause the
buddy allocator to fail?

Hardware RAS components report faults using exact, unaligned byte addresses.
Since the unaligned address is passed directly as 'start' for a 4K buddy
allocation in xe_ttm_vram_buddy_alloc(), the unaligned range [start, start+4096)
spans two actual physical pages. The buddy allocator will fail to find a fully
contained 4K block and return -ENOSPC, preventing page offlining.
Should ALIGN_DOWN(addr, PAGE_SIZE) be applied before passing it down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818104055.3833974-14-tejas.upadhyay@intel.com?part=8