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

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

On Fri, Sep 25, 2026 at 06:38:44AM +0000, sashiko-bot@kernel.org wrote:
> 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]

All semmly non-issues during testing. I believe the semaphore wait acts
as prefetch flush.

Matt 

> 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