Re: [PATCH V17 11/12] drm/xe/uapi: Expose ban reason in EXEC_QUEUE_GET_PROPERTY_BAN

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing memory barrier in guc_exec_queue_reset_status allows speculative reads to mask PAGE_OFFLINE reason.
- [Medium] Non-LR (traditional) exec queues fail to inherit the PAGE_OFFLINE ban reason when a VM page table is purged.
- [Medium] The PAGE_OFFLINE ban reason is masked and lost for multi-queue setups if a secondary queue's LRC BO is purged.
--

--- Patch [11]: [PATCH V17 11/12] drm/xe/uapi: Expose ban reason in EXEC_QUEUE_GET_PROPERTY_BAN ---
commit 45e4d4a3182723f090e735c94ca31b96821f9856
Author: Tejas Upadhyay <[email protected]>

drm/xe/uapi: Expose ban reason in EXEC_QUEUE_GET_PROPERTY_BAN

Extend DRM_XE_EXEC_QUEUE_GET_PROPERTY_BAN to return a bitmask indicating
the reason for the ban, rather than a simple boolean.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2570,13 +2580,25 @@ 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;
> +	if (xe_exec_queue_is_multi_queue_secondary(q)) {
> +		u64 status = guc_exec_queue_reset_status(xe_exec_queue_multi_queue_primary(q));
> +
> +		if (status)
> +			return status;
> +	}

[Severity: Medium]
If a secondary queue's LRC BO is purged, xe_ttm_vram_purge_page() will tag
the secondary queue with the PAGE_OFFLINE ban reason. However, since this
delegates to the primary queue here, won't this mask the secondary queue's
specific reason if the primary queue defaults to GPU_HANG?

> +
> +	if (exec_queue_reset(q) || exec_queue_killed_or_banned_or_wedged(q)) {
> +		u64 reason = atomic_read_acquire(&q->ban_reason);

[Severity: Medium]
Is a memory barrier needed here before reading the ban reason?

When a CPU reads the queue status in guc_exec_queue_reset_status()
concurrently with xe_ttm_vram_purge_page() updating it, there is a control
dependency (the if block) before using atomic_read_acquire() on the ban
reason.

Since control dependencies do not order subsequent reads against preceding
reads, could the CPU speculatively execute the ban_reason read before the
state read resolves?

If it observes a 0, it would incorrectly default to GPU_HANG instead of
PAGE_OFFLINE. Should we add an smp_rmb() before reading the ban_reason?

> +
> +		/* 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;
>  }

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -596,7 +597,12 @@ 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;
> +
>  		down_write(&vm->lock);
> +		list_for_each_entry(eq, &vm->preempt.exec_queues, lr.link)
> +			atomic_or(DRM_XE_EXEC_QUEUE_BAN_REASON_PAGE_OFFLINE, &eq->ban_reason);

[Severity: Medium]
Does this loop miss non-LR (traditional) exec queues sharing the same VM?

This iterates strictly over vm->preempt.exec_queues, which only contains LR
queues. If a non-LR queue is associated with a purged VM, it seems it will
fail to inherit the PAGE_OFFLINE ban reason.

It would then eventually time out, trigger the TDR, and be tagged with the
default GPU_HANG reason, which might provide misleading information to
userspace.

> +		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/[email protected]?part=11