Re: [PATCH v7 20/24] drm/xe: Add ULLS migration job support to ring ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe: GPU hang from CS prefetching uninitialized ULLS jobs
--

commit ca5d0374fab1065ae65164d64f6d5510acadd964
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Add ULLS migration job support to ring ops

Add preamble and postamble for ULLS migrations jobs. Preamble clears
current semaphore for reuse. Postamble waits on next semaphore which is
set upon next job submission, then advances the ring tail over that job
with an LRI to RING_TAIL, so submitting it costs the CPU nothing beyond
signalling the semaphore.

> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
> index 46ab1f0f35648..9f462bfecb831 100644
> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
[ ... ]
> +/* Publish the next job's tail, then park the engine on its semaphore */
> +static int emit_ulls_postamble(struct xe_gt *gt, struct xe_lrc *lrc, u32 *dw,
> +			       int i, u32 seqno, u32 head)
> +{
> +	i = emit_ulls_ring_tail(gt, lrc, dw, i, head);
> +
> +	dw[i++] = MI_SEMAPHORE_WAIT |
> +		MI_SEMW_GGTT |
> +		MI_SEMW_POLL |
> +		MI_SEMW_COMPARE(SAD_EQ_SDD);
> +	dw[i++] = LRC_MIGRATION_ULLS_SEMAPHORE_SIGNAL;

[Severity: High]
Does updating the ring tail to the next job's boundary before blocking on
the semaphore open a window where the Command Streamer (CS) prefetcher can
fetch uninitialized memory into its instruction cache?

When the CPU later writes the next job and signals the semaphore, the
absence of an MMIO tail update means the CS cache might not be invalidated.
Could this cause the CS to execute stale data and hang the GPU?

[ ... ]
> +/* Pad out to the fixed ULLS job size */
> +static int emit_ulls_pad(struct xe_gt *gt, u32 *dw, int i)
> +{
> +	xe_gt_assert(gt, i <= ULLS_JOB_SIZE_DW);
> +
> +	while (i < ULLS_JOB_SIZE_DW)
> +		dw[i++] = MI_NOOP;
> +
> +	return i;
> +}

[Severity: High]
If a ULLS job reaches the maximum size of ULLS_JOB_SIZE_DW (which is 48),
this loop will provide 0 NOOPs of padding.

Will this fail to buffer the prefetcher from the next job's uninitialized
memory, exacerbating the prefetching issue mentioned above?

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_ring_ops_types.h b/drivers/gpu/drm/xe/xe_ring_ops_types.h
> index 52ff96bc41004..ea4af321dd7c4 100644
> --- a/drivers/gpu/drm/xe/xe_ring_ops_types.h
> +++ b/drivers/gpu/drm/xe/xe_ring_ops_types.h
[ ... ]
> +/*
> + * ULLS migration jobs advance the ring tail from within the ring itself, so a
> + * job has to know where its successor will end before that successor has been
> + * emitted. Every ULLS job is therefore padded to a fixed size, letting the
> + * next tail be derived arithmetically.
> + *
> + * Sized for the largest such job, emitted by emit_migration_job_gen12():
> + * preamble (4), copy timestamp (8, its size on an SRIOV VF), start seqno
> + * store (4), arbitration off (1), batch buffer starts (2 * 3), pre-parser
> + * bracketed flush invalidate (6), seqno flush (4), user interrupt (3) and
> + * postamble (7 + 5).
> + */
> +#define ULLS_JOB_SIZE_DW 48
> +#define ULLS_JOB_SIZE_BYTES (ULLS_JOB_SIZE_DW * 4)

[Severity: High]
Since this maximum size is exactly equal to the maximal job length, it
guarantees no NOOP padding is appended for the largest jobs. 

Should this maximum size account for additional NOOPs to safely pad the
CS prefetch window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925045320.1325860-1-matthew.brost@intel.com?part=20