[PATCH v2 0/2] Huge PTE and scratch page updates

[PATCH v2 0/2] Huge PTE and scratch page updates

[Thomas Hellström]

The first patch restricts encoding of huge PTEs to 1GiB and adds a define
for the maximum level for which huge PTEs are available.

The second patch replaces scratch PTEs with NULL PTEs.

Cc: Brian Welty <brian.welty@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>

Thomas Hellström (2):
  drm/xe: Restrict huge PTEs to 1GiB
  drm/xe: Use NULL PTEs as scratch PTEs

 drivers/gpu/drm/xe/xe_pt.c       | 68 +++++---------------------
 drivers/gpu/drm/xe/xe_pt.h       |  6 +--
 drivers/gpu/drm/xe/xe_vm.c       | 84 ++++++++++++++++++++++----------
 drivers/gpu/drm/xe/xe_vm.h       | 11 +++++
 drivers/gpu/drm/xe/xe_vm_types.h |  1 -
 5 files changed, 82 insertions(+), 88 deletions(-)

-- 
2.42.0

[PATCH v2 1/2] drm/xe: Restrict huge PTEs to 1GiB

[Thomas Hellström]

Add a define for the highest level for which we can encode a huge PTE,
and use it for page-table building. Also update an assert that checks that
we don't try to encode for larger sizes.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_pt.c | 3 +++
 drivers/gpu/drm/xe/xe_pt.h | 3 +++
 drivers/gpu/drm/xe/xe_vm.c | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
index 35bd7940a571..699a255d75f5 100644
--- a/drivers/gpu/drm/xe/xe_pt.c
+++ b/drivers/gpu/drm/xe/xe_pt.c
@@ -430,6 +430,9 @@ static bool xe_pt_hugepte_possible(u64 addr, u64 next, unsigned int level,
 {
 	u64 size, dma;
 
+	if (level > MAX_HUGEPTE_LEVEL)
+		return false;
+
 	/* Does the virtual range requested cover a huge pte? */
 	if (!xe_pt_covers(addr, next, level, &xe_walk->base))
 		return false;
diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
index d5460e58dbbf..ba2f3325c84d 100644
--- a/drivers/gpu/drm/xe/xe_pt.h
+++ b/drivers/gpu/drm/xe/xe_pt.h
@@ -18,6 +18,9 @@ struct xe_tile;
 struct xe_vm;
 struct xe_vma;
 
+/* Largest huge pte is currently 1GiB. May become device dependent. */
+#define MAX_HUGEPTE_LEVEL 2
+
 #define xe_pt_write(xe, map, idx, data) \
 	xe_map_wr(xe, map, (idx) * sizeof(u64), u64, data)
 
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index e09050f16f07..f585cc7df071 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1239,7 +1239,7 @@ static u64 pte_encode_pat_index(struct xe_device *xe, u16 pat_index,
 
 static u64 pte_encode_ps(u32 pt_level)
 {
-	XE_WARN_ON(pt_level > 2);
+	XE_WARN_ON(pt_level > MAX_HUGEPTE_LEVEL);
 
 	if (pt_level == 1)
 		return XE_PDE_PS_2M;
-- 
2.42.0

Re: [PATCH v2 1/2] drm/xe: Restrict huge PTEs to 1GiB

[Matthew Brost]

On Fri, Dec 08, 2023 at 12:29:17PM +0100, Thomas Hellström wrote:
> Add a define for the highest level for which we can encode a huge PTE,
> and use it for page-table building. Also update an assert that checks that
> we don't try to encode for larger sizes.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pt.c | 3 +++
>  drivers/gpu/drm/xe/xe_pt.h | 3 +++
>  drivers/gpu/drm/xe/xe_vm.c | 2 +-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 35bd7940a571..699a255d75f5 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -430,6 +430,9 @@ static bool xe_pt_hugepte_possible(u64 addr, u64 next, unsigned int level,
>  {
>  	u64 size, dma;
>  
> +	if (level > MAX_HUGEPTE_LEVEL)
> +		return false;
> +
>  	/* Does the virtual range requested cover a huge pte? */
>  	if (!xe_pt_covers(addr, next, level, &xe_walk->base))
>  		return false;
> diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
> index d5460e58dbbf..ba2f3325c84d 100644
> --- a/drivers/gpu/drm/xe/xe_pt.h
> +++ b/drivers/gpu/drm/xe/xe_pt.h
> @@ -18,6 +18,9 @@ struct xe_tile;
>  struct xe_vm;
>  struct xe_vma;
>  
> +/* Largest huge pte is currently 1GiB. May become device dependent. */
> +#define MAX_HUGEPTE_LEVEL 2
> +

I thought devices with 57 bits of address space support 512GiB pages?

Anyways the patch does LGTM and comment seems to address that is can
change.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

>  #define xe_pt_write(xe, map, idx, data) \
>  	xe_map_wr(xe, map, (idx) * sizeof(u64), u64, data)
>  
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index e09050f16f07..f585cc7df071 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1239,7 +1239,7 @@ static u64 pte_encode_pat_index(struct xe_device *xe, u16 pat_index,
>  
>  static u64 pte_encode_ps(u32 pt_level)
>  {
> -	XE_WARN_ON(pt_level > 2);
> +	XE_WARN_ON(pt_level > MAX_HUGEPTE_LEVEL);
>  
>  	if (pt_level == 1)
>  		return XE_PDE_PS_2M;
> -- 
> 2.42.0
> 

Re: [PATCH v2 1/2] drm/xe: Restrict huge PTEs to 1GiB

[Welty, Brian]

On 12/8/2023 3:29 AM, Thomas Hellström wrote:
> Add a define for the highest level for which we can encode a huge PTE,
> and use it for page-table building. Also update an assert that checks that
> we don't try to encode for larger sizes.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Brian Welty <brian.welty@intel.com>

> ---
>   drivers/gpu/drm/xe/xe_pt.c | 3 +++
>   drivers/gpu/drm/xe/xe_pt.h | 3 +++
>   drivers/gpu/drm/xe/xe_vm.c | 2 +-
>   3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 35bd7940a571..699a255d75f5 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -430,6 +430,9 @@ static bool xe_pt_hugepte_possible(u64 addr, u64 next, unsigned int level,
>   {
>   	u64 size, dma;
>   
> +	if (level > MAX_HUGEPTE_LEVEL)
> +		return false;
> +
>   	/* Does the virtual range requested cover a huge pte? */
>   	if (!xe_pt_covers(addr, next, level, &xe_walk->base))
>   		return false;
> diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
> index d5460e58dbbf..ba2f3325c84d 100644
> --- a/drivers/gpu/drm/xe/xe_pt.h
> +++ b/drivers/gpu/drm/xe/xe_pt.h
> @@ -18,6 +18,9 @@ struct xe_tile;
>   struct xe_vm;
>   struct xe_vma;
>   
> +/* Largest huge pte is currently 1GiB. May become device dependent. */
> +#define MAX_HUGEPTE_LEVEL 2
> +
>   #define xe_pt_write(xe, map, idx, data) \
>   	xe_map_wr(xe, map, (idx) * sizeof(u64), u64, data)
>   
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index e09050f16f07..f585cc7df071 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1239,7 +1239,7 @@ static u64 pte_encode_pat_index(struct xe_device *xe, u16 pat_index,
>   
>   static u64 pte_encode_ps(u32 pt_level)
>   {
> -	XE_WARN_ON(pt_level > 2);
> +	XE_WARN_ON(pt_level > MAX_HUGEPTE_LEVEL);
>   
>   	if (pt_level == 1)
>   		return XE_PDE_PS_2M;

[PATCH v2 2/2] drm/xe: Use NULL PTEs as scratch PTEs

[Thomas Hellström]

Currently scratch PTEs are write-enabled and points to a single scratch
page. This has the side effect that buggy applications with out-of-bounds
memory accesses may not notice the bad access since what's written may
be read back.

Instead use NULL PTEs as scratch PTEs. These always return 0 when reading,
and writing has no effect. As a slight benefit, we can also use huge NULL
PTEs.

One drawback pointed out is that debugging may be hampered since previously
when inspecting the content of the scratch page, it might be possible to
detect writes to out-of-bound addresses and possibly also
from where the out-of-bounds address originated. However since the scratch
page-table structure is kept, it will be easy to add back the single
RW-enabled scratch page under a debug define if needed.

Also update the kerneldoc accordingly and move the function to create the
scratch page-tables from xe_pt.c to xe_pt.h since it is accessing
vm structure internals and this also makes it possible to make it static.

v2:
- Don't try to encode scratch PTEs larger than 1GiB.
- Move xe_pt_create_scratch(), Update kerneldoc.

Cc: Brian Welty <brian.welty@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Acked-by: Lucas De Marchi <lucas.demarchi@intel.com> #for general direction.
---
 drivers/gpu/drm/xe/xe_pt.c       | 65 ++++---------------------
 drivers/gpu/drm/xe/xe_pt.h       |  3 --
 drivers/gpu/drm/xe/xe_vm.c       | 82 ++++++++++++++++++++++----------
 drivers/gpu/drm/xe/xe_vm.h       | 11 +++++
 drivers/gpu/drm/xe/xe_vm_types.h |  1 -
 5 files changed, 75 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
index 699a255d75f5..bf33e56715d8 100644
--- a/drivers/gpu/drm/xe/xe_pt.c
+++ b/drivers/gpu/drm/xe/xe_pt.c
@@ -50,17 +50,19 @@ static struct xe_pt *xe_pt_entry(struct xe_pt_dir *pt_dir, unsigned int index)
 static u64 __xe_pt_empty_pte(struct xe_tile *tile, struct xe_vm *vm,
 			     unsigned int level)
 {
-	u16 pat_index = tile_to_xe(tile)->pat.idx[XE_CACHE_WB];
+	struct xe_device *xe = tile_to_xe(tile);
+	u16 pat_index = xe->pat.idx[XE_CACHE_WB];
 	u8 id = tile->id;
 
-	if (!vm->scratch_bo[id])
+	if (!xe_vm_has_scratch(vm))
 		return 0;
 
-	if (level > 0)
+	if (level > MAX_HUGEPTE_LEVEL)
 		return vm->pt_ops->pde_encode_bo(vm->scratch_pt[id][level - 1]->bo,
 						 0, pat_index);
 
-	return vm->pt_ops->pte_encode_bo(vm->scratch_bo[id], 0, pat_index, 0);
+	return vm->pt_ops->pte_encode_addr(xe, 0, pat_index, level, IS_DGFX(xe), 0) |
+		XE_PTE_NULL;
 }
 
 /**
@@ -135,7 +137,7 @@ void xe_pt_populate_empty(struct xe_tile *tile, struct xe_vm *vm,
 	u64 empty;
 	int i;
 
-	if (!vm->scratch_bo[tile->id]) {
+	if (!xe_vm_has_scratch(vm)) {
 		/*
 		 * FIXME: Some memory is allocated already allocated to zero?
 		 * Find out which memory that is and avoid this memset...
@@ -194,57 +196,6 @@ void xe_pt_destroy(struct xe_pt *pt, u32 flags, struct llist_head *deferred)
 	kfree(pt);
 }
 
-/**
- * xe_pt_create_scratch() - Setup a scratch memory pagetable tree for the
- * given tile and vm.
- * @xe: xe device.
- * @tile: tile to set up for.
- * @vm: vm to set up for.
- *
- * Sets up a pagetable tree with one page-table per level and a single
- * leaf bo. All pagetable entries point to the single page-table or,
- * for L0, the single bo one level below.
- *
- * Return: 0 on success, negative error code on error.
- */
-int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
-			 struct xe_vm *vm)
-{
-	u8 id = tile->id;
-	unsigned int flags;
-	int i;
-
-	/*
-	 * So we don't need to worry about 64K TLB hints when dealing with
-	 * scratch entires, rather keep the scratch page in system memory on
-	 * platforms where 64K pages are needed for VRAM.
-	 */
-	flags = XE_BO_CREATE_PINNED_BIT;
-	if (vm->flags & XE_VM_FLAG_64K)
-		flags |= XE_BO_CREATE_SYSTEM_BIT;
-	else
-		flags |= XE_BO_CREATE_VRAM_IF_DGFX(tile);
-
-	vm->scratch_bo[id] = xe_bo_create_pin_map(xe, tile, vm, SZ_4K,
-						  ttm_bo_type_kernel,
-						  flags);
-	if (IS_ERR(vm->scratch_bo[id]))
-		return PTR_ERR(vm->scratch_bo[id]);
-
-	xe_map_memset(vm->xe, &vm->scratch_bo[id]->vmap, 0, 0,
-		      vm->scratch_bo[id]->size);
-
-	for (i = 0; i < vm->pt_root[id]->level; i++) {
-		vm->scratch_pt[id][i] = xe_pt_create(vm, tile, i);
-		if (IS_ERR(vm->scratch_pt[id][i]))
-			return PTR_ERR(vm->scratch_pt[id][i]);
-
-		xe_pt_populate_empty(tile, vm, vm->scratch_pt[id][i]);
-	}
-
-	return 0;
-}
-
 /**
  * DOC: Pagetable building
  *
@@ -1289,7 +1240,7 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
 	 * it needs to be done here.
 	 */
 	if ((rebind && !xe_vm_in_lr_mode(vm) && !vm->batch_invalidate_tlb) ||
-	    (!rebind && vm->scratch_bo[tile->id] && xe_vm_in_preempt_fence_mode(vm))) {
+	    (!rebind && xe_vm_has_scratch(vm) && xe_vm_in_preempt_fence_mode(vm))) {
 		ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
 		if (!ifence)
 			return ERR_PTR(-ENOMEM);
diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
index ba2f3325c84d..71a4fbfcff43 100644
--- a/drivers/gpu/drm/xe/xe_pt.h
+++ b/drivers/gpu/drm/xe/xe_pt.h
@@ -29,9 +29,6 @@ unsigned int xe_pt_shift(unsigned int level);
 struct xe_pt *xe_pt_create(struct xe_vm *vm, struct xe_tile *tile,
 			   unsigned int level);
 
-int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
-			 struct xe_vm *vm);
-
 void xe_pt_populate_empty(struct xe_tile *tile, struct xe_vm *vm,
 			  struct xe_pt *pt);
 
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index f585cc7df071..c58da0c390f5 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1329,10 +1329,61 @@ static const struct xe_pt_ops xelp_pt_ops = {
 
 static void vm_destroy_work_func(struct work_struct *w);
 
+/**
+ * xe_vm_create_scratch() - Setup a scratch memory pagetable tree for the
+ * given tile and vm.
+ * @xe: xe device.
+ * @tile: tile to set up for.
+ * @vm: vm to set up for.
+ *
+ * Sets up a pagetable tree with one page-table per level and a single
+ * leaf PTE. All pagetable entries point to the single page-table or,
+ * for MAX_HUGEPTE_LEVEL, a NULL huge PTE returning 0 on read and
+ * writes become NOPs.
+ *
+ * Return: 0 on success, negative error code on error.
+ */
+static int xe_vm_create_scratch(struct xe_device *xe, struct xe_tile *tile,
+				struct xe_vm *vm)
+{
+	u8 id = tile->id;
+	int i;
+
+	for (i = MAX_HUGEPTE_LEVEL; i < vm->pt_root[id]->level; i++) {
+		vm->scratch_pt[id][i] = xe_pt_create(vm, tile, i);
+		if (IS_ERR(vm->scratch_pt[id][i]))
+			return PTR_ERR(vm->scratch_pt[id][i]);
+
+		xe_pt_populate_empty(tile, vm, vm->scratch_pt[id][i]);
+	}
+
+	return 0;
+}
+
+static void xe_vm_free_scratch(struct xe_vm *vm)
+{
+	struct xe_tile *tile;
+	u8 id;
+
+	if (!xe_vm_has_scratch(vm))
+		return;
+
+	for_each_tile(tile, vm->xe, id) {
+		u32 i;
+
+		if (!vm->pt_root[id])
+			continue;
+
+		for (i = MAX_HUGEPTE_LEVEL; i < vm->pt_root[id]->level; ++i)
+			if (vm->scratch_pt[id][i])
+				xe_pt_destroy(vm->scratch_pt[id][i], vm->flags, NULL);
+	}
+}
+
 struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
 {
 	struct xe_vm *vm;
-	int err, i = 0, number_tiles = 0;
+	int err, number_tiles = 0;
 	struct xe_tile *tile;
 	u8 id;
 
@@ -1397,12 +1448,12 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
 		}
 	}
 
-	if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
+	if (xe_vm_has_scratch(vm)) {
 		for_each_tile(tile, xe, id) {
 			if (!vm->pt_root[id])
 				continue;
 
-			err = xe_pt_create_scratch(xe, tile, vm);
+			err = xe_vm_create_scratch(xe, tile, vm);
 			if (err)
 				goto err_scratch_pt;
 		}
@@ -1466,18 +1517,7 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
 	return vm;
 
 err_scratch_pt:
-	for_each_tile(tile, xe, id) {
-		if (!vm->pt_root[id])
-			continue;
-
-		i = vm->pt_root[id]->level;
-		while (i)
-			if (vm->scratch_pt[id][--i])
-				xe_pt_destroy(vm->scratch_pt[id][i],
-					      vm->flags, NULL);
-		xe_bo_unpin(vm->scratch_bo[id]);
-		xe_bo_put(vm->scratch_bo[id]);
-	}
+	xe_vm_free_scratch(vm);
 err_destroy_root:
 	for_each_tile(tile, xe, id) {
 		if (vm->pt_root[id])
@@ -1563,17 +1603,7 @@ void xe_vm_close_and_put(struct xe_vm *vm)
 	 * install a fence to resv. Hence it's safe to
 	 * destroy the pagetables immediately.
 	 */
-	for_each_tile(tile, xe, id) {
-		if (vm->scratch_bo[id]) {
-			u32 i;
-
-			xe_bo_unpin(vm->scratch_bo[id]);
-			xe_bo_put(vm->scratch_bo[id]);
-			for (i = 0; i < vm->pt_root[id]->level; i++)
-				xe_pt_destroy(vm->scratch_pt[id][i], vm->flags,
-					      NULL);
-		}
-	}
+	xe_vm_free_scratch(vm);
 	xe_vm_unlock(vm);
 
 	/*
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 9a0ae19c47b7..6563ca33caf2 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -64,6 +64,17 @@ static inline bool xe_vm_is_closed_or_banned(struct xe_vm *vm)
 struct xe_vma *
 xe_vm_find_overlapping_vma(struct xe_vm *vm, u64 start, u64 range);
 
+/**
+ * xe_vm_has_scratch() - Whether the vm is configured for scratch PTEs
+ * @vm: The vm
+ *
+ * Return: whether the vm populates unmapped areas with scratch PTEs
+ */
+static inline bool xe_vm_has_scratch(const struct xe_vm *vm)
+{
+	return vm->flags & XE_VM_FLAG_SCRATCH_PAGE;
+}
+
 static inline struct xe_vm *gpuva_to_vm(struct drm_gpuva *gpuva)
 {
 	return container_of(gpuva->vm, struct xe_vm, gpuvm);
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 23abdfd8622f..f628a7081f27 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -158,7 +158,6 @@ struct xe_vm {
 	u64 size;
 
 	struct xe_pt *pt_root[XE_MAX_TILES_PER_DEVICE];
-	struct xe_bo *scratch_bo[XE_MAX_TILES_PER_DEVICE];
 	struct xe_pt *scratch_pt[XE_MAX_TILES_PER_DEVICE][XE_VM_MAX_LEVEL];
 
 	/**
-- 
2.42.0

Re: [PATCH v2 2/2] drm/xe: Use NULL PTEs as scratch PTEs

[Welty, Brian]

On 12/8/2023 3:29 AM, Thomas Hellström wrote:
> Currently scratch PTEs are write-enabled and points to a single scratch
> page. This has the side effect that buggy applications with out-of-bounds
> memory accesses may not notice the bad access since what's written may
> be read back.
> 
> Instead use NULL PTEs as scratch PTEs. These always return 0 when reading,
> and writing has no effect. As a slight benefit, we can also use huge NULL
> PTEs.
> 
> One drawback pointed out is that debugging may be hampered since previously
> when inspecting the content of the scratch page, it might be possible to
> detect writes to out-of-bound addresses and possibly also
> from where the out-of-bounds address originated. However since the scratch
> page-table structure is kept, it will be easy to add back the single
> RW-enabled scratch page under a debug define if needed.
> 
> Also update the kerneldoc accordingly and move the function to create the
> scratch page-tables from xe_pt.c to xe_pt.h since it is accessing
> vm structure internals and this also makes it possible to make it static.
> 
> v2:
> - Don't try to encode scratch PTEs larger than 1GiB.
> - Move xe_pt_create_scratch(), Update kerneldoc.
> 
> Cc: Brian Welty <brian.welty@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com> #for general direction.

Reviewed-by: Brian Welty <brian.welty@intel.com>

But one nit, and one question further below.


> ---
>   drivers/gpu/drm/xe/xe_pt.c       | 65 ++++---------------------
>   drivers/gpu/drm/xe/xe_pt.h       |  3 --
>   drivers/gpu/drm/xe/xe_vm.c       | 82 ++++++++++++++++++++++----------
>   drivers/gpu/drm/xe/xe_vm.h       | 11 +++++
>   drivers/gpu/drm/xe/xe_vm_types.h |  1 -
>   5 files changed, 75 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 699a255d75f5..bf33e56715d8 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -50,17 +50,19 @@ static struct xe_pt *xe_pt_entry(struct xe_pt_dir *pt_dir, unsigned int index)
>   static u64 __xe_pt_empty_pte(struct xe_tile *tile, struct xe_vm *vm,
>   			     unsigned int level)
>   {
> -	u16 pat_index = tile_to_xe(tile)->pat.idx[XE_CACHE_WB];
> +	struct xe_device *xe = tile_to_xe(tile);
> +	u16 pat_index = xe->pat.idx[XE_CACHE_WB];
>   	u8 id = tile->id;
>   
> -	if (!vm->scratch_bo[id])
> +	if (!xe_vm_has_scratch(vm))
>   		return 0;
>   
> -	if (level > 0)
> +	if (level > MAX_HUGEPTE_LEVEL)
>   		return vm->pt_ops->pde_encode_bo(vm->scratch_pt[id][level - 1]->bo,
>   						 0, pat_index);
>   
> -	return vm->pt_ops->pte_encode_bo(vm->scratch_bo[id], 0, pat_index, 0);
> +	return vm->pt_ops->pte_encode_addr(xe, 0, pat_index, level, IS_DGFX(xe), 0) |
> +		XE_PTE_NULL;

Nit, but seems a little cleaner to utilize the flags argument?
	return vm->pt_ops->pte_encode_addr(xe, 0, pat_index, level, 
IS_DGFX(xe), XE_PTE_NULL);

>   }
>   
>   /**
> @@ -135,7 +137,7 @@ void xe_pt_populate_empty(struct xe_tile *tile, struct xe_vm *vm,
>   	u64 empty;
>   	int i;
>   
> -	if (!vm->scratch_bo[tile->id]) {
> +	if (!xe_vm_has_scratch(vm)) {
>   		/*
>   		 * FIXME: Some memory is allocated already allocated to zero?
>   		 * Find out which memory that is and avoid this memset...
> @@ -194,57 +196,6 @@ void xe_pt_destroy(struct xe_pt *pt, u32 flags, struct llist_head *deferred)
>   	kfree(pt);
>   }
>   
> -/**
> - * xe_pt_create_scratch() - Setup a scratch memory pagetable tree for the
> - * given tile and vm.
> - * @xe: xe device.
> - * @tile: tile to set up for.
> - * @vm: vm to set up for.
> - *
> - * Sets up a pagetable tree with one page-table per level and a single
> - * leaf bo. All pagetable entries point to the single page-table or,
> - * for L0, the single bo one level below.
> - *
> - * Return: 0 on success, negative error code on error.
> - */
> -int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
> -			 struct xe_vm *vm)
> -{
> -	u8 id = tile->id;
> -	unsigned int flags;
> -	int i;
> -
> -	/*
> -	 * So we don't need to worry about 64K TLB hints when dealing with
> -	 * scratch entires, rather keep the scratch page in system memory on
> -	 * platforms where 64K pages are needed for VRAM.
> -	 */
> -	flags = XE_BO_CREATE_PINNED_BIT;
> -	if (vm->flags & XE_VM_FLAG_64K)
> -		flags |= XE_BO_CREATE_SYSTEM_BIT;
> -	else
> -		flags |= XE_BO_CREATE_VRAM_IF_DGFX(tile);
> -
> -	vm->scratch_bo[id] = xe_bo_create_pin_map(xe, tile, vm, SZ_4K,
> -						  ttm_bo_type_kernel,
> -						  flags);
> -	if (IS_ERR(vm->scratch_bo[id]))
> -		return PTR_ERR(vm->scratch_bo[id]);
> -
> -	xe_map_memset(vm->xe, &vm->scratch_bo[id]->vmap, 0, 0,
> -		      vm->scratch_bo[id]->size);
> -
> -	for (i = 0; i < vm->pt_root[id]->level; i++) {
> -		vm->scratch_pt[id][i] = xe_pt_create(vm, tile, i);
> -		if (IS_ERR(vm->scratch_pt[id][i]))
> -			return PTR_ERR(vm->scratch_pt[id][i]);
> -
> -		xe_pt_populate_empty(tile, vm, vm->scratch_pt[id][i]);
> -	}
> -
> -	return 0;
> -}
> -
>   /**
>    * DOC: Pagetable building
>    *
> @@ -1289,7 +1240,7 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
>   	 * it needs to be done here.
>   	 */
>   	if ((rebind && !xe_vm_in_lr_mode(vm) && !vm->batch_invalidate_tlb) ||
> -	    (!rebind && vm->scratch_bo[tile->id] && xe_vm_in_preempt_fence_mode(vm))) {
> +	    (!rebind && xe_vm_has_scratch(vm) && xe_vm_in_preempt_fence_mode(vm))) {
>   		ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
>   		if (!ifence)
>   			return ERR_PTR(-ENOMEM);
> diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
> index ba2f3325c84d..71a4fbfcff43 100644
> --- a/drivers/gpu/drm/xe/xe_pt.h
> +++ b/drivers/gpu/drm/xe/xe_pt.h
> @@ -29,9 +29,6 @@ unsigned int xe_pt_shift(unsigned int level);
>   struct xe_pt *xe_pt_create(struct xe_vm *vm, struct xe_tile *tile,
>   			   unsigned int level);
>   
> -int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
> -			 struct xe_vm *vm);
> -
>   void xe_pt_populate_empty(struct xe_tile *tile, struct xe_vm *vm,
>   			  struct xe_pt *pt);
>   
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f585cc7df071..c58da0c390f5 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1329,10 +1329,61 @@ static const struct xe_pt_ops xelp_pt_ops = {
>   
>   static void vm_destroy_work_func(struct work_struct *w);
>   
> +/**
> + * xe_vm_create_scratch() - Setup a scratch memory pagetable tree for the
> + * given tile and vm.
> + * @xe: xe device.
> + * @tile: tile to set up for.
> + * @vm: vm to set up for.
> + *
> + * Sets up a pagetable tree with one page-table per level and a single
> + * leaf PTE. All pagetable entries point to the single page-table or,
> + * for MAX_HUGEPTE_LEVEL, a NULL huge PTE returning 0 on read and
> + * writes become NOPs.
> + *
> + * Return: 0 on success, negative error code on error.
> + */
> +static int xe_vm_create_scratch(struct xe_device *xe, struct xe_tile *tile,
> +				struct xe_vm *vm)
> +{
> +	u8 id = tile->id;
> +	int i;
> +
> +	for (i = MAX_HUGEPTE_LEVEL; i < vm->pt_root[id]->level; i++) {
> +		vm->scratch_pt[id][i] = xe_pt_create(vm, tile, i);
> +		if (IS_ERR(vm->scratch_pt[id][i]))
> +			return PTR_ERR(vm->scratch_pt[id][i]);
> +
> +		xe_pt_populate_empty(tile, vm, vm->scratch_pt[id][i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static void xe_vm_free_scratch(struct xe_vm *vm)
> +{
> +	struct xe_tile *tile;
> +	u8 id;
> +
> +	if (!xe_vm_has_scratch(vm))
> +		return;
> +
> +	for_each_tile(tile, vm->xe, id) {
> +		u32 i;
> +
> +		if (!vm->pt_root[id])
> +			continue;
> +
> +		for (i = MAX_HUGEPTE_LEVEL; i < vm->pt_root[id]->level; ++i)
> +			if (vm->scratch_pt[id][i])
> +				xe_pt_destroy(vm->scratch_pt[id][i], vm->flags, NULL);
> +	}
> +}
> +
>   struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
>   {
>   	struct xe_vm *vm;
> -	int err, i = 0, number_tiles = 0;
> +	int err, number_tiles = 0;
>   	struct xe_tile *tile;
>   	u8 id;
>   
> @@ -1397,12 +1448,12 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
>   		}
>   	}
>   
> -	if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> +	if (xe_vm_has_scratch(vm)) {
>   		for_each_tile(tile, xe, id) {
>   			if (!vm->pt_root[id])
>   				continue;
>   
> -			err = xe_pt_create_scratch(xe, tile, vm);
> +			err = xe_vm_create_scratch(xe, tile, vm);
>   			if (err)
>   				goto err_scratch_pt;
>   		}
> @@ -1466,18 +1517,7 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
>   	return vm;
>   
>   err_scratch_pt:
> -	for_each_tile(tile, xe, id) {
> -		if (!vm->pt_root[id])
> -			continue;
> -
> -		i = vm->pt_root[id]->level;
> -		while (i)
> -			if (vm->scratch_pt[id][--i])
> -				xe_pt_destroy(vm->scratch_pt[id][i],
> -					      vm->flags, NULL);
> -		xe_bo_unpin(vm->scratch_bo[id]);
> -		xe_bo_put(vm->scratch_bo[id]);
> -	}
> +	xe_vm_free_scratch(vm);
>   err_destroy_root:
>   	for_each_tile(tile, xe, id) {
>   		if (vm->pt_root[id])
> @@ -1563,17 +1603,7 @@ void xe_vm_close_and_put(struct xe_vm *vm)
>   	 * install a fence to resv. Hence it's safe to
>   	 * destroy the pagetables immediately.
>   	 */
> -	for_each_tile(tile, xe, id) {
> -		if (vm->scratch_bo[id]) {
> -			u32 i;
> -
> -			xe_bo_unpin(vm->scratch_bo[id]);
> -			xe_bo_put(vm->scratch_bo[id]);
> -			for (i = 0; i < vm->pt_root[id]->level; i++)
> -				xe_pt_destroy(vm->scratch_pt[id][i], vm->flags,
> -					      NULL);
> -		}
> -	}

Just curious. Is user-space exec_queue able to still be performing 
memory accesses and causing page table walks?  In other words, is
the page table possibly being walked as we teardown here?
Partly, I'm asking as I noticed you switched the order it tears down
the levels.


> +	xe_vm_free_scratch(vm);
>   	xe_vm_unlock(vm);
>   
>   	/*
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 9a0ae19c47b7..6563ca33caf2 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -64,6 +64,17 @@ static inline bool xe_vm_is_closed_or_banned(struct xe_vm *vm)
>   struct xe_vma *
>   xe_vm_find_overlapping_vma(struct xe_vm *vm, u64 start, u64 range);
>   
> +/**
> + * xe_vm_has_scratch() - Whether the vm is configured for scratch PTEs
> + * @vm: The vm
> + *
> + * Return: whether the vm populates unmapped areas with scratch PTEs
> + */
> +static inline bool xe_vm_has_scratch(const struct xe_vm *vm)
> +{
> +	return vm->flags & XE_VM_FLAG_SCRATCH_PAGE;
> +}
> +
>   static inline struct xe_vm *gpuva_to_vm(struct drm_gpuva *gpuva)
>   {
>   	return container_of(gpuva->vm, struct xe_vm, gpuvm);
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> index 23abdfd8622f..f628a7081f27 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -158,7 +158,6 @@ struct xe_vm {
>   	u64 size;
>   
>   	struct xe_pt *pt_root[XE_MAX_TILES_PER_DEVICE];
> -	struct xe_bo *scratch_bo[XE_MAX_TILES_PER_DEVICE];
>   	struct xe_pt *scratch_pt[XE_MAX_TILES_PER_DEVICE][XE_VM_MAX_LEVEL];
>   
>   	/**

Re: [PATCH v2 2/2] drm/xe: Use NULL PTEs as scratch PTEs

[Thomas Hellström]

On 12/8/23 20:37, Welty, Brian wrote:
>
>
> On 12/8/2023 3:29 AM, Thomas Hellström wrote:
>> Currently scratch PTEs are write-enabled and points to a single scratch
>> page. This has the side effect that buggy applications with 
>> out-of-bounds
>> memory accesses may not notice the bad access since what's written may
>> be read back.
>>
>> Instead use NULL PTEs as scratch PTEs. These always return 0 when 
>> reading,
>> and writing has no effect. As a slight benefit, we can also use huge 
>> NULL
>> PTEs.
>>
>> One drawback pointed out is that debugging may be hampered since 
>> previously
>> when inspecting the content of the scratch page, it might be possible to
>> detect writes to out-of-bound addresses and possibly also
>> from where the out-of-bounds address originated. However since the 
>> scratch
>> page-table structure is kept, it will be easy to add back the single
>> RW-enabled scratch page under a debug define if needed.
>>
>> Also update the kerneldoc accordingly and move the function to create 
>> the
>> scratch page-tables from xe_pt.c to xe_pt.h since it is accessing
>> vm structure internals and this also makes it possible to make it 
>> static.
>>
>> v2:
>> - Don't try to encode scratch PTEs larger than 1GiB.
>> - Move xe_pt_create_scratch(), Update kerneldoc.
>>
>> Cc: Brian Welty <brian.welty@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com> #for general 
>> direction.
>
> Reviewed-by: Brian Welty <brian.welty@intel.com>
>
Thanks for reviewing, Brian.


> But one nit, and one question further below.
>
>
>> ---
>>   drivers/gpu/drm/xe/xe_pt.c       | 65 ++++---------------------
>>   drivers/gpu/drm/xe/xe_pt.h       |  3 --
>>   drivers/gpu/drm/xe/xe_vm.c       | 82 ++++++++++++++++++++++----------
>>   drivers/gpu/drm/xe/xe_vm.h       | 11 +++++
>>   drivers/gpu/drm/xe/xe_vm_types.h |  1 -
>>   5 files changed, 75 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index 699a255d75f5..bf33e56715d8 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -50,17 +50,19 @@ static struct xe_pt *xe_pt_entry(struct xe_pt_dir 
>> *pt_dir, unsigned int index)
>>   static u64 __xe_pt_empty_pte(struct xe_tile *tile, struct xe_vm *vm,
>>                    unsigned int level)
>>   {
>> -    u16 pat_index = tile_to_xe(tile)->pat.idx[XE_CACHE_WB];
>> +    struct xe_device *xe = tile_to_xe(tile);
>> +    u16 pat_index = xe->pat.idx[XE_CACHE_WB];
>>       u8 id = tile->id;
>>   -    if (!vm->scratch_bo[id])
>> +    if (!xe_vm_has_scratch(vm))
>>           return 0;
>>   -    if (level > 0)
>> +    if (level > MAX_HUGEPTE_LEVEL)
>>           return vm->pt_ops->pde_encode_bo(vm->scratch_pt[id][level - 
>> 1]->bo,
>>                            0, pat_index);
>>   -    return vm->pt_ops->pte_encode_bo(vm->scratch_bo[id], 0, 
>> pat_index, 0);
>> +    return vm->pt_ops->pte_encode_addr(xe, 0, pat_index, level, 
>> IS_DGFX(xe), 0) |
>> +        XE_PTE_NULL;
>
> Nit, but seems a little cleaner to utilize the flags argument?
>     return vm->pt_ops->pte_encode_addr(xe, 0, pat_index, level, 
> IS_DGFX(xe), XE_PTE_NULL);
>
Yes, that's true. Although there is an assert in the function that 
blocks that.  I've been discussing with Lucas to change the interface of 
this function to take a set of generic generic flags.

>>   }
>>     /**
>> @@ -135,7 +137,7 @@ void xe_pt_populate_empty(struct xe_tile *tile, 
>> struct xe_vm *vm,
>>       u64 empty;
>>       int i;
>>   -    if (!vm->scratch_bo[tile->id]) {
>> +    if (!xe_vm_has_scratch(vm)) {
>>           /*
>>            * FIXME: Some memory is allocated already allocated to zero?
>>            * Find out which memory that is and avoid this memset...
>> @@ -194,57 +196,6 @@ void xe_pt_destroy(struct xe_pt *pt, u32 flags, 
>> struct llist_head *deferred)
>>       kfree(pt);
>>   }
>>   -/**
>> - * xe_pt_create_scratch() - Setup a scratch memory pagetable tree 
>> for the
>> - * given tile and vm.
>> - * @xe: xe device.
>> - * @tile: tile to set up for.
>> - * @vm: vm to set up for.
>> - *
>> - * Sets up a pagetable tree with one page-table per level and a single
>> - * leaf bo. All pagetable entries point to the single page-table or,
>> - * for L0, the single bo one level below.
>> - *
>> - * Return: 0 on success, negative error code on error.
>> - */
>> -int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
>> -             struct xe_vm *vm)
>> -{
>> -    u8 id = tile->id;
>> -    unsigned int flags;
>> -    int i;
>> -
>> -    /*
>> -     * So we don't need to worry about 64K TLB hints when dealing with
>> -     * scratch entires, rather keep the scratch page in system 
>> memory on
>> -     * platforms where 64K pages are needed for VRAM.
>> -     */
>> -    flags = XE_BO_CREATE_PINNED_BIT;
>> -    if (vm->flags & XE_VM_FLAG_64K)
>> -        flags |= XE_BO_CREATE_SYSTEM_BIT;
>> -    else
>> -        flags |= XE_BO_CREATE_VRAM_IF_DGFX(tile);
>> -
>> -    vm->scratch_bo[id] = xe_bo_create_pin_map(xe, tile, vm, SZ_4K,
>> -                          ttm_bo_type_kernel,
>> -                          flags);
>> -    if (IS_ERR(vm->scratch_bo[id]))
>> -        return PTR_ERR(vm->scratch_bo[id]);
>> -
>> -    xe_map_memset(vm->xe, &vm->scratch_bo[id]->vmap, 0, 0,
>> -              vm->scratch_bo[id]->size);
>> -
>> -    for (i = 0; i < vm->pt_root[id]->level; i++) {
>> -        vm->scratch_pt[id][i] = xe_pt_create(vm, tile, i);
>> -        if (IS_ERR(vm->scratch_pt[id][i]))
>> -            return PTR_ERR(vm->scratch_pt[id][i]);
>> -
>> -        xe_pt_populate_empty(tile, vm, vm->scratch_pt[id][i]);
>> -    }
>> -
>> -    return 0;
>> -}
>> -
>>   /**
>>    * DOC: Pagetable building
>>    *
>> @@ -1289,7 +1240,7 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct 
>> xe_vma *vma, struct xe_exec_queue
>>        * it needs to be done here.
>>        */
>>       if ((rebind && !xe_vm_in_lr_mode(vm) && 
>> !vm->batch_invalidate_tlb) ||
>> -        (!rebind && vm->scratch_bo[tile->id] && 
>> xe_vm_in_preempt_fence_mode(vm))) {
>> +        (!rebind && xe_vm_has_scratch(vm) && 
>> xe_vm_in_preempt_fence_mode(vm))) {
>>           ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
>>           if (!ifence)
>>               return ERR_PTR(-ENOMEM);
>> diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
>> index ba2f3325c84d..71a4fbfcff43 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.h
>> +++ b/drivers/gpu/drm/xe/xe_pt.h
>> @@ -29,9 +29,6 @@ unsigned int xe_pt_shift(unsigned int level);
>>   struct xe_pt *xe_pt_create(struct xe_vm *vm, struct xe_tile *tile,
>>                  unsigned int level);
>>   -int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
>> -             struct xe_vm *vm);
>> -
>>   void xe_pt_populate_empty(struct xe_tile *tile, struct xe_vm *vm,
>>                 struct xe_pt *pt);
>>   diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index f585cc7df071..c58da0c390f5 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -1329,10 +1329,61 @@ static const struct xe_pt_ops xelp_pt_ops = {
>>     static void vm_destroy_work_func(struct work_struct *w);
>>   +/**
>> + * xe_vm_create_scratch() - Setup a scratch memory pagetable tree 
>> for the
>> + * given tile and vm.
>> + * @xe: xe device.
>> + * @tile: tile to set up for.
>> + * @vm: vm to set up for.
>> + *
>> + * Sets up a pagetable tree with one page-table per level and a single
>> + * leaf PTE. All pagetable entries point to the single page-table or,
>> + * for MAX_HUGEPTE_LEVEL, a NULL huge PTE returning 0 on read and
>> + * writes become NOPs.
>> + *
>> + * Return: 0 on success, negative error code on error.
>> + */
>> +static int xe_vm_create_scratch(struct xe_device *xe, struct xe_tile 
>> *tile,
>> +                struct xe_vm *vm)
>> +{
>> +    u8 id = tile->id;
>> +    int i;
>> +
>> +    for (i = MAX_HUGEPTE_LEVEL; i < vm->pt_root[id]->level; i++) {
>> +        vm->scratch_pt[id][i] = xe_pt_create(vm, tile, i);
>> +        if (IS_ERR(vm->scratch_pt[id][i]))
>> +            return PTR_ERR(vm->scratch_pt[id][i]);
>> +
>> +        xe_pt_populate_empty(tile, vm, vm->scratch_pt[id][i]);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void xe_vm_free_scratch(struct xe_vm *vm)
>> +{
>> +    struct xe_tile *tile;
>> +    u8 id;
>> +
>> +    if (!xe_vm_has_scratch(vm))
>> +        return;
>> +
>> +    for_each_tile(tile, vm->xe, id) {
>> +        u32 i;
>> +
>> +        if (!vm->pt_root[id])
>> +            continue;
>> +
>> +        for (i = MAX_HUGEPTE_LEVEL; i < vm->pt_root[id]->level; ++i)
>> +            if (vm->scratch_pt[id][i])
>> +                xe_pt_destroy(vm->scratch_pt[id][i], vm->flags, NULL);
>> +    }
>> +}
>> +
>>   struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
>>   {
>>       struct xe_vm *vm;
>> -    int err, i = 0, number_tiles = 0;
>> +    int err, number_tiles = 0;
>>       struct xe_tile *tile;
>>       u8 id;
>>   @@ -1397,12 +1448,12 @@ struct xe_vm *xe_vm_create(struct xe_device 
>> *xe, u32 flags)
>>           }
>>       }
>>   -    if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
>> +    if (xe_vm_has_scratch(vm)) {
>>           for_each_tile(tile, xe, id) {
>>               if (!vm->pt_root[id])
>>                   continue;
>>   -            err = xe_pt_create_scratch(xe, tile, vm);
>> +            err = xe_vm_create_scratch(xe, tile, vm);
>>               if (err)
>>                   goto err_scratch_pt;
>>           }
>> @@ -1466,18 +1517,7 @@ struct xe_vm *xe_vm_create(struct xe_device 
>> *xe, u32 flags)
>>       return vm;
>>     err_scratch_pt:
>> -    for_each_tile(tile, xe, id) {
>> -        if (!vm->pt_root[id])
>> -            continue;
>> -
>> -        i = vm->pt_root[id]->level;
>> -        while (i)
>> -            if (vm->scratch_pt[id][--i])
>> -                xe_pt_destroy(vm->scratch_pt[id][i],
>> -                          vm->flags, NULL);
>> -        xe_bo_unpin(vm->scratch_bo[id]);
>> -        xe_bo_put(vm->scratch_bo[id]);
>> -    }
>> +    xe_vm_free_scratch(vm);
>>   err_destroy_root:
>>       for_each_tile(tile, xe, id) {
>>           if (vm->pt_root[id])
>> @@ -1563,17 +1603,7 @@ void xe_vm_close_and_put(struct xe_vm *vm)
>>        * install a fence to resv. Hence it's safe to
>>        * destroy the pagetables immediately.
>>        */
>> -    for_each_tile(tile, xe, id) {
>> -        if (vm->scratch_bo[id]) {
>> -            u32 i;
>> -
>> -            xe_bo_unpin(vm->scratch_bo[id]);
>> -            xe_bo_put(vm->scratch_bo[id]);
>> -            for (i = 0; i < vm->pt_root[id]->level; i++)
>> -                xe_pt_destroy(vm->scratch_pt[id][i], vm->flags,
>> -                          NULL);
>> -        }
>> -    }
>
> Just curious. Is user-space exec_queue able to still be performing 
> memory accesses and causing page table walks?  In other words, is
> the page table possibly being walked as we teardown here?

Yes, it is. But the page-table buffer objects aren't actually freed 
until any ongoing GPU activity on them is complete, so should be safe.


> Partly, I'm asking as I noticed you switched the order it tears down
> the levels.
>
Were still going from low- to high level in the teardown (but the error 
path on VM creation indeed had that reversed before). There should be no 
GPU accesses of the page tables in the VM creation error path though, so 
I figure it won't matter, but let's see what CI says..

/Thomas


>
>> +    xe_vm_free_scratch(vm);
>>       xe_vm_unlock(vm);
>>         /*
>> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
>> index 9a0ae19c47b7..6563ca33caf2 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.h
>> +++ b/drivers/gpu/drm/xe/xe_vm.h
>> @@ -64,6 +64,17 @@ static inline bool 
>> xe_vm_is_closed_or_banned(struct xe_vm *vm)
>>   struct xe_vma *
>>   xe_vm_find_overlapping_vma(struct xe_vm *vm, u64 start, u64 range);
>>   +/**
>> + * xe_vm_has_scratch() - Whether the vm is configured for scratch PTEs
>> + * @vm: The vm
>> + *
>> + * Return: whether the vm populates unmapped areas with scratch PTEs
>> + */
>> +static inline bool xe_vm_has_scratch(const struct xe_vm *vm)
>> +{
>> +    return vm->flags & XE_VM_FLAG_SCRATCH_PAGE;
>> +}
>> +
>>   static inline struct xe_vm *gpuva_to_vm(struct drm_gpuva *gpuva)
>>   {
>>       return container_of(gpuva->vm, struct xe_vm, gpuvm);
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h 
>> b/drivers/gpu/drm/xe/xe_vm_types.h
>> index 23abdfd8622f..f628a7081f27 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -158,7 +158,6 @@ struct xe_vm {
>>       u64 size;
>>         struct xe_pt *pt_root[XE_MAX_TILES_PER_DEVICE];
>> -    struct xe_bo *scratch_bo[XE_MAX_TILES_PER_DEVICE];
>>       struct xe_pt 
>> *scratch_pt[XE_MAX_TILES_PER_DEVICE][XE_VM_MAX_LEVEL];
>>         /**

✗ CI.Patch_applied: failure for Huge PTE and scratch page updates

[Patchwork]

== Series Details ==

Series: Huge PTE and scratch page updates
URL   : https://patchwork.freedesktop.org/series/127565/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: c93e733ab fixup! drm/xe: Port Xe to GPUVA
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_vm.c:1329
error: drivers/gpu/drm/xe/xe_vm.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/xe: Restrict huge PTEs to 1GiB
Applying: drm/xe: Use NULL PTEs as scratch PTEs
Patch failed at 0002 drm/xe: Use NULL PTEs as scratch PTEs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".