Re: [PATCH V21 14/15] drm/xe/uapi: Expose ban reason in EXEC_QUEUE_GET_PROPERTY_BAN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing memory barriers when updating and checking ban_reason and queue state allow race conditions where the wrong ban reason is reported.
- [Low] The GPU_HANG ban reason is erroneously appended to non-LR queues that time out as a consequence of a VM ban (e.g., from page offline).
- [Low] The PAGE_OFFLINE ban reason is not recorded for non-LR exec queues on platforms without context-based TLB invalidations.
--

commit 67aaa673382d33c9ca0db014fb3ae57c32097a8e
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>
Subject: drm/xe/uapi: Expose ban reason in EXEC_QUEUE_GET_PROPERTY_BAN

This commit extends DRM_XE_EXEC_QUEUE_GET_PROPERTY_BAN to return a bitmask 
indicating the reason for a queue ban instead of a simple boolean. It adds 
a ban_reason field to xe_exec_queue, updates the VRAM purge and TDR paths 
to populate this field, and modifies the reset_status op to expose it.

> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -1599,6 +1600,12 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>  	else
>  		wedged = xe_device_wedged(xe);
>  
> +	/*
> +	 * Only tag as GPU hang if this is the original timeout, not a
> +	 * consequence of a prior kill (e.g., page-offline).
> +	 */
> +	if (!exec_queue_killed(q))
> +		atomic_or(DRM_XE_EXEC_QUEUE_BAN_REASON_GPU_HANG, &q->ban_reason);

[Severity: Medium]
Does this sequence require an smp_wmb() before set_exec_queue_banned(q)?

atomic_or() is a relaxed operation. Without a write barrier, the queue's
BANNED state update might become visible to concurrent readers before the
ban_reason update is visible. This could allow a userspace query to see
the banned state but get the wrong (default) reason.

[Severity: Low]
Could this erroneously tag non-LR queues with the GPU_HANG reason after a
VM ban?

The commit message states this guard avoids masking the page-offline
reason. However, xe_vm_kill() only kills LR queues. As a result,
exec_queue_killed(q) remains false for non-LR queues, causing them to
incorrectly receive both the PAGE_OFFLINE and GPU_HANG bits when they
eventually time out due to the unmapped memory.

>  	set_exec_queue_banned(q);
>  
>  	/* Kick job / queue off hardware */
[ ... ]
> @@ -2580,13 +2590,29 @@ static void guc_exec_queue_multi_queue_drop_suspend(struct xe_exec_queue *q)
>  	}
>  }
>  
> -static bool guc_exec_queue_reset_status(struct xe_exec_queue *q)
> +static u64 guc_exec_queue_reset_status(struct xe_exec_queue *q)
>  {
> -	if (xe_exec_queue_is_multi_queue_secondary(q) &&
> -	    guc_exec_queue_reset_status(xe_exec_queue_multi_queue_primary(q)))
> -		return true;
> +	/* TODO: In case of multiqueue, if a secondary queue is banned due to
> +	 * page offlining, checking only the primary queue's GuC reset status
> +	 * may mask the true reason or race with it.
> +	 */
> +	if (xe_exec_queue_is_multi_queue_secondary(q)) {
> +		u64 status = guc_exec_queue_reset_status(xe_exec_queue_multi_queue_primary(q));
>  
> -	return exec_queue_reset(q) || exec_queue_killed_or_banned_or_wedged(q);
> +		if (status)
> +			return status;
> +	}
> +
> +	if (exec_queue_reset(q) || exec_queue_killed_or_banned_or_wedged(q)) {
> +		u64 reason = atomic_read_acquire(&q->ban_reason);

[Severity: Medium]
Is an smp_rmb() needed here, or an acquire barrier on the state check?

While atomic_read_acquire() prevents subsequent loads from being reordered
before this point, the relaxed reads inside
exec_queue_killed_or_banned_or_wedged(q) (e.g., checking q->guc->state)
could be reordered with the read of q->ban_reason because control
dependencies do not order subsequent loads. This could result in loading a
stale ban_reason before the state check is fully verified.

> +
> +		/* If no specific reason was recorded, default to GPU hang */
> +		if (!reason)
> +			reason = DRM_XE_EXEC_QUEUE_BAN_REASON_GPU_HANG;
> +		return reason;
> +	}
> +
> +	return 0;
>  }
[ ... ]
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
[ ... ]
> @@ -639,7 +641,30 @@ static int xe_ttm_vram_purge_page(struct xe_device *xe, struct xe_bo *bo)
>  	xe_bo_unlock(bo);
>  	/*  Ban VM if BO is PPGTT */
>  	if (vm && (flags & XE_BO_FLAG_PAGETABLE)) {
> +		struct xe_exec_queue *eq;
> +		int id;
> +
>  		down_write(&vm->lock);
> +		if (xe->info.has_ctx_tlb_inval) {
> +			/*
> +			 * Must be the write lock: send_tlb_inval_ctx_ppgtt()
> +			 * mutates this list (list_move_tail() onto an on-stack
> +			 * head) while holding only the read lock, relying on
> +			 * tlb_inval->seqno_lock to keep itself the sole
> +			 * mutator. Traversing it under down_read() would let
> +			 * this walk follow entries onto that stack list.
> +			 */
> +			down_write(&vm->exec_queues.lock);
> +			for (id = 0; id < ARRAY_SIZE(vm->exec_queues.list); id++)
> +				list_for_each_entry(eq, &vm->exec_queues.list[id],
> +						    vm_exec_queue_link)
> +					atomic_or(q_flag, &eq->ban_reason);
> +			up_write(&vm->exec_queues.lock);
> +		} else {
> +			list_for_each_entry(eq, &vm->preempt.exec_queues, lr.link)
> +				atomic_or(q_flag, &eq->ban_reason);

[Severity: Low]
Does this fallback path properly handle non-LR queues on older platforms?

When has_ctx_tlb_inval is false, this iteration loops over
vm->preempt.exec_queues, which exclusively contains LR queues. Since
non-LR queues aren't tracked in this list, they will miss the PAGE_OFFLINE
tag during a VRAM purge and will only report a generic GPU_HANG when they
eventually time out.

> +		}
> +		smp_wmb(); /* Force all queue bits to be visible before killing the VM */
>  		xe_vm_kill(vm, true);
>  		up_write(&vm->lock);
>  	}

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