Re: [PATCH] drm/xe: Order ring writes before ring tail updates

On Fri, 2026-08-07 at 10:17 -0700, Matthew Brost wrote:
> The ring buffer and the LRC context image are both mapped WC, so the
> ring
> tail update can become visible to the device before the ring contents
> it
> is meant to publish.
> 
> The GuC CT send does contain an xe_device_wmb(), so sending the H2G
> would
> flush the ring contents. The problem is that it comes too late:
> xe_lrc_set_ring_tail() publishes the tail before the H2G is sent, and
> the
> device samples the tail from the context image independently of it,
> either
> at context switch-in or while the context is already resident. A
> submitter
> which is interrupted between updating the tail and sending its H2G
> therefore leaves the device free to observe the new tail while the
> ring
> contents behind it are not yet visible:
> 
>   1. Thread A emits a job into the ring, sets the tail to T_A and
> sends
>      the H2G, which flushes A's ring contents. The GuC starts
> scheduling
>      the context in, but it is not executing yet.
>   2. Thread B emits a job into ring[T_A..T_B]. Those writes are not
> yet
>      visible to the device.
>   3. Thread B updates the ring tail to T_B. That write targets a
>      different page and becomes visible first.
>   4. Thread B is interrupted before it sends its H2G, so the flush
> which
>      would have published ring[T_A..T_B] has not happened yet.
>   5. The context is switched in and samples the ring tail from the
>      context image, picking up T_B rather than T_A.
>   6. The GPU executes A's job, advances HEAD to T_A, and continues on
> to
>      ring[T_A..T_B], which still holds the previous wrap's contents,
> so
>      the CS parses stale commands.
> 
> The result is command stream corruption, which typically manifests as
> a
> hang or a spurious pagefault rather than anything that points back at
> the
> submission path.
> 
> Kernel jobs are by far the most likely to hit this. Kernel queues
> such as
> the migration queue are shared and can be driven by many threads
> concurrently, producing back-to-back submissions on an LRC which is
> already executing. User queues are typically tied to a single
> submitting
> thread, so the same interleaving is much harder to produce.
> 
> Add an xe_device_wmb() at the end of xe_lrc_write_ring() so that it
> covers
> every ring tail publication site, and so the invariant is local: once
> xe_lrc_write_ring() returns, the ring contents are visible to the
> device.
> 
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
> GPUs")
> Closes:
> https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8651
> Closes:
> https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/7810
> Cc: [email protected]
> Signed-off-by: Matthew Brost <[email protected]>
> Assisted-by: GitHub_Copilot:claude-opus-5

And this gets even more likely with multi queue lite restore where we
don't even bounce out of the CS...

Yeah good catch here!
Reviewed-by: Stuart Summers <[email protected]>

> ---
>  drivers/gpu/drm/xe/xe_lrc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_lrc.c
> b/drivers/gpu/drm/xe/xe_lrc.c
> index 01d087e82205..a332c11420fd 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -1851,6 +1851,13 @@ void xe_lrc_write_ring(struct xe_lrc *lrc,
> const void *data, size_t size)
>  
>                 __xe_lrc_write_ring(lrc, ring, &noop, sizeof(noop));
>         }
> +
> +       /*
> +        * The ring and the LRC context image are both WC, so the
> ring tail
> +        * update which publishes these writes can become visible to
> the device
> +        * first. Ensure the ring contents are visible before
> returning.
> +        */
> +       xe_device_wmb(xe);
>  }
>  
>  u64 xe_lrc_descriptor(struct xe_lrc *lrc)