Re: [PATCH 3/3] drm/xe/ggtt: invalidate engine GGTT TLBs for multi-queue GTs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Engine TLB invalidation is incorrectly gated on a mutable software configuration flag, leading to stale TLBs and memory corruption when multi-queue scheduling is disabled.
--

--- Patch [3]: [PATCH 3/3] drm/xe/ggtt: invalidate engine GGTT TLBs for multi-queue GTs ---
commit 1bb3febf0df767bbdf46eda5e034dfd13af1e727
Author: Tangudu Tilak Tirumalesh <[email protected]>

drm/xe/ggtt: invalidate engine GGTT TLBs for multi-queue GTs

This commit introduces an engine TLB invalidation on GGTT unmap to ensure
that engines do not hit stale cached translations after the memory is freed.
The invalidation logic is gated on the GT having a multi-queue engine
class mask enabled.

> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 3564345fd7403..60eb9a3d29cc2 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c

[ ... ]

> @@ -603,6 +607,23 @@ static void xe_ggtt_invalidate(struct xe_ggtt *ggtt)
>  	ggtt_invalidate_gt_tlb(ggtt->tile->media_gt);
>  }
>  
> +/*
> + * Drain engine-side GGTT TLBs on teardown so a recycled range's next
> + * occupant can't hit a predecessor's cached translation.
> + */
> +static void xe_ggtt_invalidate_engine(struct xe_ggtt *ggtt)
> +{
> +	struct xe_gt *gt = ggtt->tile->primary_gt;
> +	int err;
> +
> +	if (!gt || !gt->info.multi_queue_engine_class_mask)
> +		return;

[Severity: High]
Does this skip necessary hardware invalidation if the multi-queue scheduling
feature is disabled in software?

If a privileged user disables the multi-queue feature via configfs,
gt->info.multi_queue_engine_class_mask is cleared to 0. However, the hardware
physical capability of having independent engine TLBs does not disappear when
this software feature is disabled.

When the mask is cleared, xe_ggtt_invalidate_engine() will silently skip
the engine TLB invalidation on GGTT unmap, leaving the independent engine
TLBs unflushed and potentially causing data corruption or a use-after-free
when a recycled range is accessed. (Also note that in ggtt_node_remove(),
the unmap path falls back to writing a scratch PTE when this mask is 0,
which does not prevent engines from using their still-valid, stale TLB
entries.)

Should this check use an immutable hardware capability flag instead of the
active software configuration mask?

> +
> +	err = xe_tlb_inval_ggtt_full(&gt->tlb_inval);
> +	xe_gt_WARN(gt, err, "Failed to invalidate engine GGTT TLBs (%pe)",
> +		   ERR_PTR(err));
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3