Re: [PATCH 1/3] drm/xe/ggtt: stop mapping unmapped GGTT pages to scratch

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Mon, Aug 24, 2026 at 10:04:56PM +0530, tilak.tirumalesh.tangudu@intel.com wrote:
>From: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>
>
>xe_ggtt_clear() wrote a scratch PTE across the range for both
>initial_clear() and the node_remove() unmap path.
>
>On unmap the scratch PTE hides stale-TLB gaps: after the range is
>invalidated a re-walk lands on the still-valid scratch PTE, so a stale
>engine TLB translation for the freed range never faults and the
>invalidation gap stays invisible.
>
>Add an explicit use_scratch bool. initial_clear() keeps writing scratch.
>On multi-queue platforms, where engines cache GGTT independently,
>node_remove() writes PTE=0 so a stale translation to a freed range
>faults and the gap becomes observable; other platforms keep scratch.
>
>v2: Add xe_gt_has_multi_queue() helper and other knits-Niranjana
>
>Assisted-by: Claude:claude-opus-4-8
>Signed-off-by: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>
>---
> drivers/gpu/drm/xe/xe_ggtt.c | 47 ++++++++++++++++++++++++++----------
> drivers/gpu/drm/xe/xe_gt.h   | 13 ++++++++++
> 2 files changed, 47 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>index 8ec23862477f..4cddd69f0e99 100644
>--- a/drivers/gpu/drm/xe/xe_ggtt.c
>+++ b/drivers/gpu/drm/xe/xe_ggtt.c
>@@ -20,6 +20,7 @@
> #include "regs/xe_regs.h"
> #include "xe_assert.h"
> #include "xe_bo.h"
>+#include "xe_gt.h"
> #include "xe_gt_printk.h"
> #include "xe_gt_types.h"
> #include "xe_map.h"
>@@ -257,23 +258,36 @@ static u64 xe_ggtt_get_pte(struct xe_ggtt *ggtt, u64 addr)
> 	return readq(&ggtt->gsm[addr >> XE_PTE_SHIFT]);
> }
>
>-static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
>+/**
>+ * xe_ggtt_clear() - Clear a range of GGTT page-table entries
>+ * @ggtt: the &xe_ggtt to clear
>+ * @start: start of the range (GGTT address)
>+ * @size: size of the range in bytes
>+ * @use_scratch: true fills the range with the scratch PTE (init-time clear so
>+ *		 engines walking free holes read zeros safely); false writes
>+ *		 PTE=0 so a stale HW TLB entry faults instead of silently
>+ *		 landing on scratch.
>+ */

NIT...Kernel-doc not really needed for static functions. General comment section
should be good. I prefer keeping it simple with general comment, but leaving it
as is is fine too.

In any case,
Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

>+static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size,
>+			  bool use_scratch)
> {
>-	u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile), XE_CACHE_WB);
> 	u64 end = start + size - 1;
>-	u64 scratch_pte;
>+	u64 pte;
>
> 	xe_tile_assert(ggtt->tile, start < end);
>
>-	if (ggtt->scratch)
>-		scratch_pte = xe_bo_addr(ggtt->scratch, 0, XE_PAGE_SIZE) |
>-			      ggtt->pt_ops->pte_encode_flags(ggtt->scratch,
>-							     pat_index);
>-	else
>-		scratch_pte = 0;
>+	if (use_scratch && ggtt->scratch) {
>+		u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile),
>+						 XE_CACHE_WB);
>+
>+		pte = xe_bo_addr(ggtt->scratch, 0, XE_PAGE_SIZE) |
>+		      ggtt->pt_ops->pte_encode_flags(ggtt->scratch, pat_index);
>+	} else {
>+		pte = 0;
>+	}
>
> 	while (start < end) {
>-		ggtt->pt_ops->ggtt_set_pte(ggtt, start, scratch_pte);
>+		ggtt->pt_ops->ggtt_set_pte(ggtt, start, pte);
> 		start += XE_PAGE_SIZE;
> 	}
> }
>@@ -459,7 +473,7 @@ static void xe_ggtt_initial_clear(struct xe_ggtt *ggtt)
> 	/* Display may have allocated inside ggtt, so be careful with clearing here */
> 	mutex_lock(&ggtt->lock);
> 	drm_mm_for_each_hole(hole, &ggtt->mm, start, end)
>-		xe_ggtt_clear(ggtt, ggtt->start + start, end - start);
>+		xe_ggtt_clear(ggtt, ggtt->start + start, end - start, true);
>
> 	xe_ggtt_invalidate(ggtt);
> 	mutex_unlock(&ggtt->lock);
>@@ -473,12 +487,19 @@ static void ggtt_node_fini(struct xe_ggtt_node *node)
> static void ggtt_node_remove(struct xe_ggtt_node *node)
> {
> 	struct xe_ggtt *ggtt = node->ggtt;
>-	bool bound;
>+	bool use_scratch, bound;
>+
>+	/*
>+	 * Addition of a secondary queue while a multi-queue context is live on
>+	 * HW requires proper GGTT TLB clearing, so use faulting PTEs here.
>+	 */
>+	use_scratch = !xe_gt_has_multi_queue(ggtt->tile->primary_gt);
>
> 	mutex_lock(&ggtt->lock);
> 	bound = ggtt->flags & XE_GGTT_FLAGS_ONLINE;
> 	if (bound)
>-		xe_ggtt_clear(ggtt, xe_ggtt_node_addr(node), xe_ggtt_node_size(node));
>+		xe_ggtt_clear(ggtt, xe_ggtt_node_addr(node),
>+			      xe_ggtt_node_size(node), use_scratch);
> 	drm_mm_remove_node(&node->base);
> 	node->base.size = 0;
> 	if (bound && node->invalidate_on_remove)
>diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
>index 65a4655b0994..92d934d50244 100644
>--- a/drivers/gpu/drm/xe/xe_gt.h
>+++ b/drivers/gpu/drm/xe/xe_gt.h
>@@ -160,4 +160,17 @@ static inline bool xe_gt_supports_multi_queue(const struct xe_gt *gt,
> 	return gt->info.multi_queue_engine_class_mask & BIT(class);
> }
>
>+/**
>+ * xe_gt_has_multi_queue() - Check if gt supports multi-queue on
>+ * any engine class.
>+ *
>+ * @gt: the GT object (may be NULL)
>+ *
>+ * Return: true if any engine class on @gt supports multi queue, else false
>+ */
>+static inline bool xe_gt_has_multi_queue(const struct xe_gt *gt)
>+{
>+	return gt && gt->info.multi_queue_engine_class_mask;
>+}
>+
> #endif
>-- 
>2.46.0
>