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

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Sep 02, 2026 at 05:10:59PM +0000, sashiko-bot@kernel.org wrote:
> 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.
> 

This is right - we should probably just use exec_queues.list. I have
various code floating around which use exec_queues.list regardless if
info.has_ctx_tlb_inval is enabled. We should probably extract that into
a standalone patch and then fixup this code. Another thing we mostly
likely want to do is kill all exec queues too.

Both can be done in a follow up. Can we get a Jira filed for this?

Matt

> > +		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