Re: [PATCH V20 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:
- [High] Incomplete and inconsistent queue tagging for `PAGE_OFFLINE` ban reason due to mutually exclusive list iteration in `xe_ttm_vram_purge_page()`.
- [Medium] Missing read memory barrier (`smp_rmb`) in `guc_exec_queue_reset_status()` allows speculative loads to mask the `PAGE_OFFLINE` ban reason.
- [Medium] Multi-queue primary status check in `guc_exec_queue_reset_status()` unconditionally masks the `PAGE_OFFLINE` reason for secondary queues.
--

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

This commit extends the queue ban property UAPI to return a bitmask of ban
reasons rather than a boolean. It introduces GPU_HANG and PAGE_OFFLINE 
reasons, tracking these via a new ban_reason atomic variable in xe_exec_queue.

> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -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;
> +	}

[Severity: Medium]
When querying a secondary queue that was individually purged and killed
due to page offlining, won't this check unconditionally return the primary
queue's default status (GPU_HANG)? Because the primary queue wasn't explicitly
tagged by the purge, the secondary queue's PAGE_OFFLINE reason gets masked.

> +
> +	if (exec_queue_reset(q) || exec_queue_killed_or_banned_or_wedged(q)) {
> +		u64 reason = atomic_read_acquire(&q->ban_reason);
> +
> +		/* If no specific reason was recorded, default to GPU hang */
> +		if (!reason)
> +			reason = DRM_XE_EXEC_QUEUE_BAN_REASON_GPU_HANG;

[Severity: Medium]
Is a read memory barrier (smp_rmb) missing here?

In xe_ttm_vram_purge_page() located in drivers/gpu/drm/xe/xe_ttm_vram_mgr.c,
ban_reason is updated and then smp_wmb() is called before killing the VM
or queue. Concurrently, guc_exec_queue_reset_status() reads the state with
a relaxed load in exec_queue_killed_or_banned_or_wedged() and conditionally
reads ban_reason with an acquire load. 

Without an smp_rmb() between the state load and the ban_reason load, the CPU
could speculatively load the old ban_reason (0) before observing the killed
state, resulting in userspace receiving a false 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
[ ... ]
> @@ -637,7 +639,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: High]
Does this mutually exclusive check cause PAGE_OFFLINE to be missed for certain
queues?

The lists vm->exec_queues.list and vm->preempt.exec_queues contain disjoint sets
of queues. VM_BIND queues are explicitly excluded from vm->exec_queues.list, and
standard short-running queues are not in vm->preempt.exec_queues.

By iterating only one list based on the unrelated has_ctx_tlb_inval hardware
feature, the driver selectively tags only a subset of queues, leaving the
remaining offlined queues to falsely report a GPU_HANG.

> +		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/20260902145343.465686-17-tejas.upadhyay@intel.com?part=14