Re: [PATCH 18/20] drm/xe/eudebug: Introduce per device attention scan worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/xe/eudebug: attention seqno race with discover_client()
  queue create
--

commit 9b3fd09a4bd67b0862bac3d3c0bc36809193f44f
Author: Dominik Grzegorzek <dominik.grzegorzek@intel.com>

drm/xe/eudebug: Introduce per device attention scan worker

This commit introduces a periodic per-device delayed worker to scan for EU
debugging attention bits and send DRM_XE_EUDEBUG_EVENT_EU_ATTENTION events
when an attached debugger is present, or reset the GT if no debugger will
service the halted threads.

> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
> index 4fab8304046b3..9b1f2f97b603b 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
[ ... ]
> @@ -2218,6 +2233,362 @@ static const struct file_operations fops = {
>  	.compat_ioctl	= compat_ptr_ioctl,
>  };
>  
> +/*
> + * The uapi tells userspace the attention bitmask sits 8 byte aligned within
> + * the event, so it may be scanned as u32 or u64 words. That holds today
> + * because the preceding members are u64, but a field added ahead of the
> + * array could quietly break it, so pin it down here. Only the offset can
> + * be: plain __u64 members are 4 byte aligned on i386.
> + */
> +static_assert(offsetof(struct drm_xe_eudebug_event_eu_attention, bitmask) %
> +	      sizeof(__u64) == 0);
> +
> +static int send_attention_event(struct xe_eudebug *d, struct xe_exec_queue *q,
> +				int lrc_idx, void *bitmap, unsigned int size,
> +				u64 seqno)
> +{
> +	struct drm_xe_eudebug_event_eu_attention *e;
> +	struct drm_xe_eudebug_event *event;
> +	const u32 sz = struct_size(e, bitmask, size);
> +	int h_queue, h_lrc;
> +	int ret;
> +
> +	if (lrc_idx < 0 || lrc_idx >= q->width)
> +		return -EINVAL;
> +
> +	h_queue = find_handle(d, XE_EUDEBUG_RES_TYPE_EXEC_QUEUE, q);
> +	if (h_queue < 0)
> +		return h_queue;
> +
> +	h_lrc = find_handle(d, XE_EUDEBUG_RES_TYPE_LRC, q->lrc[lrc_idx]);
> +	if (h_lrc < 0)
> +		return h_lrc;
> +
> +	mutex_lock(&d->lock);
> +	event = xe_eudebug_prepare_event(d, DRM_XE_EUDEBUG_EVENT_EU_ATTENTION,
> +					 seqno, DRM_XE_EUDEBUG_EVENT_STATE_CHANGE,
> +					 sz);
> +
> +	e = cast_event(e, event);
> +	e->exec_queue_handle = h_queue;
> +	e->lrc_handle = h_lrc;
> +	e->bitmask_size = size;
> +
> +	memcpy(e->bitmask, bitmap, size);
> +	ret = xe_eudebug_queue_event(d, event);
> +	mutex_unlock(&d->lock);
> +
> +	return ret;
> +}
> +
> +static int relay_gt_attention(struct xe_eudebug *d, struct xe_exec_queue *q,
> +			      struct xe_hw_engine *hwe, int lrc_idx)
> +{
> +	const u32 size = xe_gt_eu_attention_bitmap_size(q->gt);
> +	void *bitmask;
> +	u64 seqno;
> +	int ret;
> +
> +	/* If we fail at this time, assume we manage to send eventually */
> +	bitmask = kvzalloc(size, GFP_KERNEL);
> +	if (!bitmask)
> +		return 0;
> +
> +	/*
> +	 * Attention is state, so the event is stamped where that state is
> +	 * observed: as the bits are read. That is also before the handle
> +	 * lookups in send_attention_event(), so a handle still found there is
> +	 * destroyed with a later seqno. A round that ends up sending nothing
> +	 * leaves a gap in the numbering, which the uapi allows.
> +	 */
> +	seqno = atomic64_inc_return(&d->events.seqno);
> +
> +	ret = xe_gt_eu_attention_bitmap(q->gt, bitmask, size);

[Severity: Medium]
Can relay_gt_attention() send a DRM_XE_EUDEBUG_EVENT_EU_ATTENTION event
with a smaller seqno than the queue's DRM_XE_EUDEBUG_EVENT_CREATE event
when racing with discover_client()?

In xe_eudebug_attach(), add_debugger() publishes xef->eudebug.debugger
before discover_client() runs, and handle_gt_attention() looks up the
debugger via xe_eudebug_get_nolock() without holding xef->eudebug.ioctl_lock:

CPU0 (attention_wq)               CPU1 (connect ioctl)
handle_gt_attention()             xe_eudebug_attach()
  d = xe_eudebug_get_nolock(...)    discover_client()
  relay_gt_attention()                ...
    seqno = atomic64_inc_return()       exec_queue_create_event()
      --> S1                              _xe_eudebug_add_handle(LRC)
    xe_gt_eu_attention_bitmap()           mutex_lock(&d->lock)
                                          __handle_insert(EXEC_QUEUE)
                                            --> seqno = S2 (S2 > S1)
                                          mutex_unlock(&d->lock)
    send_attention_event()
      find_handle(EXEC_QUEUE)
      find_handle(LRC)
      mutex_lock(&d->lock)
      xe_eudebug_queue_event(S1)
      mutex_unlock(&d->lock)
                                          send_exec_queue_event(S2)

The attention event for q is stamped with seqno S1 < S2 and can also be
queued into d->events.fifo ahead of the EXEC_QUEUE CREATE event because
exec_queue_create_event() drops d->lock between __handle_insert() and
send_exec_queue_event(), so userspace sees an attention event for handles
that have not been created yet.

Looking up h_queue and h_lrc before stamping seqno in relay_gt_attention()
(and re-checking that the handles still exist before queueing, or holding
xef->eudebug.ioctl_lock) would ensure discovery has already assigned the
CREATE seqno first.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109135549.4128280-1-mika.kuoppala@linux.intel.com?part=18