[PATCH 0/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[PATCH 0/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Jonathan Cavitt]

Add additional information to the xe_vm so it can report the last 50
relevant exec queues that have been banned on it, as well as the
associated pagefault address and address type that caused the ban when
applicable.  Since we cannot reasonably associate a pagefault to a
specific exec queue, whenever a CAT error causes an exec queue to become
banned, we blame the last seen pagefault on said exec queue.

The last pagefault seen per exec queue is saved to the xe_vm, and the
pagefault is updated when a new pagefault is reported or when the last
pagefault has been associated with an exec queue, whichever happens
first.  All new pagefault reports come from xe_gt_pagefault.

Also add a tracker that counts the number of times the VM has
experienced an engine reset.

Finally, add a new ioctl - xe_vm_get_property_ioctl - that allows the
user to query this additional information.

Signed-off-by: Jonathan Cavitt <joanthan.cavitt@intel.com>
Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Suggested-by: Matthew Brost <matthew.brost@intel.com>
CC: Zhang Jianxun <jianxun.zhang@intel.com>

Jonathan Cavitt (6):
  drm/xe/xe_gt_pagefault: Migrate lookup_vma to xe_vm.h
  drm/xe/xe_exec_queue: Add ID param to exec queue struct
  drm/xe/xe_gt_pagefault: Migrate pagefault struct to header
  drm/xe/xe_vm: Add per VM pagefault info
  drm/xe/xe_vm: Add per VM reset stats
  drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

 drivers/gpu/drm/xe/xe_device.c           |   2 +
 drivers/gpu/drm/xe/xe_exec_queue.c       |   7 +
 drivers/gpu/drm/xe/xe_exec_queue_types.h |   2 +
 drivers/gpu/drm/xe/xe_gt_pagefault.c     |  82 ++++-------
 drivers/gpu/drm/xe/xe_gt_pagefault.h     |  28 ++++
 drivers/gpu/drm/xe/xe_guc_submit.c       |   4 +
 drivers/gpu/drm/xe/xe_vm.c               | 175 +++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_vm.h               |  32 +++++
 drivers/gpu/drm/xe/xe_vm_types.h         |  34 +++++
 include/uapi/drm/xe_drm.h                |  73 ++++++++++
 10 files changed, 381 insertions(+), 58 deletions(-)

-- 
2.43.0

[PATCH 1/6] drm/xe/xe_gt_pagefault: Migrate lookup_vma to xe_vm.h

[Jonathan Cavitt]

Make lookup_vma a static inline header function for xe_vm.

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
---
 drivers/gpu/drm/xe/xe_gt_pagefault.c | 25 +------------------------
 drivers/gpu/drm/xe/xe_vm.h           | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
index 17d69039b866..4a4cf0c4b68d 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
@@ -71,29 +71,6 @@ static bool vma_is_valid(struct xe_tile *tile, struct xe_vma *vma)
 		!(BIT(tile->id) & vma->tile_invalidated);
 }
 
-static bool vma_matches(struct xe_vma *vma, u64 page_addr)
-{
-	if (page_addr > xe_vma_end(vma) - 1 ||
-	    page_addr + SZ_4K - 1 < xe_vma_start(vma))
-		return false;
-
-	return true;
-}
-
-static struct xe_vma *lookup_vma(struct xe_vm *vm, u64 page_addr)
-{
-	struct xe_vma *vma = NULL;
-
-	if (vm->usm.last_fault_vma) {   /* Fast lookup */
-		if (vma_matches(vm->usm.last_fault_vma, page_addr))
-			vma = vm->usm.last_fault_vma;
-	}
-	if (!vma)
-		vma = xe_vm_find_overlapping_vma(vm, page_addr, SZ_4K);
-
-	return vma;
-}
-
 static int xe_pf_begin(struct drm_exec *exec, struct xe_vma *vma,
 		       bool atomic, unsigned int id)
 {
@@ -229,7 +206,7 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
 		goto unlock_vm;
 	}
 
-	vma = lookup_vma(vm, pf->page_addr);
+	vma = xe_vm_lookup_vma(vm, pf->page_addr);
 	if (!vma) {
 		err = -EINVAL;
 		goto unlock_vm;
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index f66075f8a6fe..fb3f15ee89ec 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -248,6 +248,30 @@ bool xe_vm_validate_should_retry(struct drm_exec *exec, int err, ktime_t *end);
 
 int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma);
 
+static bool vma_matches(struct xe_vma *vma, u64 page_addr)
+{
+	if (page_addr > xe_vma_end(vma) - 1 ||
+	    page_addr + SZ_4K - 1 < xe_vma_start(vma))
+		return false;
+
+	return true;
+}
+
+static inline struct xe_vma *xe_vm_lookup_vma(struct xe_vm *vm, u64 page_addr)
+{
+	struct xe_vma *vma = NULL;
+
+	if (vm->usm.last_fault_vma) {   /* Fast lookup */
+		if (vma_matches(vm->usm.last_fault_vma, page_addr))
+			vma = vm->usm.last_fault_vma;
+	}
+	if (!vma)
+		vma = xe_vm_find_overlapping_vma(vm, page_addr, SZ_4K);
+
+	return vma;
+}
+
+
 int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
 			  unsigned int num_fences);
 
-- 
2.43.0

[PATCH 2/6] drm/xe/xe_exec_queue: Add ID param to exec queue struct

[Jonathan Cavitt]

Add the exec queue id to the exec queue struct.  This is useful for
performing a reverse lookup into the xef->exec_queue xarray.

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
---
 drivers/gpu/drm/xe/xe_exec_queue.c       | 1 +
 drivers/gpu/drm/xe/xe_exec_queue_types.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
index 23a9f519ce1c..4a98a5d0e405 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue.c
+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
@@ -709,6 +709,7 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
 	if (err)
 		goto kill_exec_queue;
 
+	q->id = id;
 	args->exec_queue_id = id;
 
 	return 0;
diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
index 6eb7ff091534..088d838218e9 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
+++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
@@ -55,6 +55,8 @@ struct xe_exec_queue {
 	struct xe_vm *vm;
 	/** @class: class of this exec queue */
 	enum xe_engine_class class;
+	/** @id: exec queue ID as reported during create ioctl */
+	u32 id;
 	/**
 	 * @logical_mask: logical mask of where job submitted to exec queue can run
 	 */
-- 
2.43.0

[PATCH 3/6] drm/xe/xe_gt_pagefault: Migrate pagefault struct to header

[Jonathan Cavitt]

Migrate the pagefault struct from xe_gt_pagefault.c to the
xe_gt_pagefault.h header file, along with the associated enum values.

v2: Normalize names for common header (Matt Brost)

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
---
 drivers/gpu/drm/xe/xe_gt_pagefault.c | 41 +++++-----------------------
 drivers/gpu/drm/xe/xe_gt_pagefault.h | 28 +++++++++++++++++++
 2 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
index 4a4cf0c4b68d..76d7feecf98e 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
@@ -22,33 +22,6 @@
 #include "xe_trace_bo.h"
 #include "xe_vm.h"
 
-struct pagefault {
-	u64 page_addr;
-	u32 asid;
-	u16 pdata;
-	u8 vfid;
-	u8 access_type;
-	u8 fault_type;
-	u8 fault_level;
-	u8 engine_class;
-	u8 engine_instance;
-	u8 fault_unsuccessful;
-	bool trva_fault;
-};
-
-enum access_type {
-	ACCESS_TYPE_READ = 0,
-	ACCESS_TYPE_WRITE = 1,
-	ACCESS_TYPE_ATOMIC = 2,
-	ACCESS_TYPE_RESERVED = 3,
-};
-
-enum fault_type {
-	NOT_PRESENT = 0,
-	WRITE_ACCESS_VIOLATION = 1,
-	ATOMIC_ACCESS_VIOLATION = 2,
-};
-
 struct acc {
 	u64 va_range_base;
 	u32 asid;
@@ -60,9 +33,9 @@ struct acc {
 	u8 engine_instance;
 };
 
-static bool access_is_atomic(enum access_type access_type)
+static bool access_is_atomic(enum xe_pagefault_access_type access_type)
 {
-	return access_type == ACCESS_TYPE_ATOMIC;
+	return access_type == XE_PAGEFAULT_ACCESS_TYPE_ATOMIC;
 }
 
 static bool vma_is_valid(struct xe_tile *tile, struct xe_vma *vma)
@@ -102,7 +75,7 @@ static int xe_pf_begin(struct drm_exec *exec, struct xe_vma *vma,
 	return 0;
 }
 
-static int handle_vma_pagefault(struct xe_gt *gt, struct pagefault *pf,
+static int handle_vma_pagefault(struct xe_gt *gt, struct xe_pagefault *pf,
 				struct xe_vma *vma)
 {
 	struct xe_vm *vm = xe_vma_vm(vma);
@@ -181,7 +154,7 @@ static struct xe_vm *asid_to_vm(struct xe_device *xe, u32 asid)
 	return vm;
 }
 
-static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
+static int handle_pagefault(struct xe_gt *gt, struct xe_pagefault *pf)
 {
 	struct xe_device *xe = gt_to_xe(gt);
 	struct xe_vm *vm;
@@ -235,7 +208,7 @@ static int send_pagefault_reply(struct xe_guc *guc,
 	return xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
 }
 
-static void print_pagefault(struct xe_device *xe, struct pagefault *pf)
+static void print_pagefault(struct xe_device *xe, struct xe_pagefault *pf)
 {
 	drm_dbg(&xe->drm, "\n\tASID: %d\n"
 		 "\tVFID: %d\n"
@@ -255,7 +228,7 @@ static void print_pagefault(struct xe_device *xe, struct pagefault *pf)
 
 #define PF_MSG_LEN_DW	4
 
-static bool get_pagefault(struct pf_queue *pf_queue, struct pagefault *pf)
+static bool get_pagefault(struct pf_queue *pf_queue, struct xe_pagefault *pf)
 {
 	const struct xe_guc_pagefault_desc *desc;
 	bool ret = false;
@@ -342,7 +315,7 @@ static void pf_queue_work_func(struct work_struct *w)
 	struct xe_gt *gt = pf_queue->gt;
 	struct xe_device *xe = gt_to_xe(gt);
 	struct xe_guc_pagefault_reply reply = {};
-	struct pagefault pf = {};
+	struct xe_pagefault pf = {};
 	unsigned long threshold;
 	int ret;
 
diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.h b/drivers/gpu/drm/xe/xe_gt_pagefault.h
index 839c065a5e4c..33616043d17a 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.h
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.h
@@ -11,6 +11,34 @@
 struct xe_gt;
 struct xe_guc;
 
+struct xe_pagefault {
+	u64 page_addr;
+	u32 asid;
+	u16 pdata;
+	u8 vfid;
+	u8 access_type;
+	u8 fault_type;
+	u8 fault_level;
+	u8 engine_class;
+	u8 engine_instance;
+	u8 fault_unsuccessful;
+	bool prefetch;
+	bool trva_fault;
+};
+
+enum xe_pagefault_access_type {
+	XE_PAGEFAULT_ACCESS_TYPE_READ = 0,
+	XE_PAGEFAULT_ACCESS_TYPE_WRITE = 1,
+	XE_PAGEFAULT_ACCESS_TYPE_ATOMIC = 2,
+	XE_PAGEFAULT_ACCESS_TYPE_RESERVED = 3,
+};
+
+enum xe_pagefault_type {
+	XE_PAGEFAULT_TYPE_NOT_PRESENT = 0,
+	XE_PAGEFAULT_TYPE_WRITE_ACCESS_VIOLATION = 1,
+	XE_PAGEFAULT_TYPE_ATOMIC_ACCESS_VIOLATION = 2,
+};
+
 int xe_gt_pagefault_init(struct xe_gt *gt);
 void xe_gt_pagefault_reset(struct xe_gt *gt);
 int xe_guc_pagefault_handler(struct xe_guc *guc, u32 *msg, u32 len);
-- 
2.43.0

[PATCH 4/6] drm/xe/xe_vm: Add per VM pagefault info

[Jonathan Cavitt]

Add additional information to vm so it can report up to the last 50
relevant exec queues to have been banned on it, as well as the last
pagefault seen when said exec queues were banned.  Since we cannot
reasonably associate a pagefault to a specific exec queue, we
currently report the last seen pagefault on the associated vm instead.

The last pagefault seen per exec queue is saved to the vm, and the
pagefault is updated during the pagefault handling process in
xe_gt_pagefault.  The last seen pagefault is reset once it has been
associated to the next banned exec queue.

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Suggested-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_exec_queue.c   |  6 +++
 drivers/gpu/drm/xe/xe_gt_pagefault.c | 16 +++++++
 drivers/gpu/drm/xe/xe_guc_submit.c   |  2 +
 drivers/gpu/drm/xe/xe_vm.c           | 69 ++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_vm.h           |  6 +++
 drivers/gpu/drm/xe/xe_vm_types.h     | 31 +++++++++++++
 6 files changed, 130 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
index 4a98a5d0e405..e0764f3dfd76 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue.c
+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
@@ -712,6 +712,12 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
 	q->id = id;
 	args->exec_queue_id = id;
 
+	/**
+	 * If an exec queue in the ban list shares the same exec queue
+	 * ID, remove it from the ban list to avoid confusion.
+	 */
+	xe_vm_remove_ban_entry(q->vm, q);
+
 	return 0;
 
 kill_exec_queue:
diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
index 76d7feecf98e..899b687de3e7 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
@@ -307,6 +307,21 @@ int xe_guc_pagefault_handler(struct xe_guc *guc, u32 *msg, u32 len)
 	return full ? -ENOSPC : 0;
 }
 
+static void save_pagefault_to_vm(struct xe_device *xe, struct xe_pagefault *pf)
+{
+	struct xe_vm *vm;
+
+	vm = asid_to_vm(xe, pf->asid);
+	if (IS_ERR(vm))
+		return;
+
+	spin_lock(&vm->pf.lock);
+	if (!vm->pf.info)
+		vm->pf.info = kzalloc(sizeof(*pf), GFP_KERNEL);
+	memcpy(vm->pf.info, pf, sizeof(*pf));
+	spin_unlock(&vm->pf.lock);
+}
+
 #define USM_QUEUE_MAX_RUNTIME_MS	20
 
 static void pf_queue_work_func(struct work_struct *w)
@@ -325,6 +340,7 @@ static void pf_queue_work_func(struct work_struct *w)
 		ret = handle_pagefault(gt, &pf);
 		if (unlikely(ret)) {
 			print_pagefault(xe, &pf);
+			save_pagefault_to_vm(xe, &pf);
 			pf.fault_unsuccessful = 1;
 			drm_dbg(&xe->drm, "Fault response: Unsuccessful %d\n", ret);
 		}
diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index b6a2dd742ebd..f0bfc9d109cb 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -2066,6 +2066,8 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
 	if (!exec_queue_banned(q) && !exec_queue_check_timeout(q))
 		xe_guc_exec_queue_trigger_cleanup(q);
 
+	xe_vm_add_ban_entry(q->vm, q);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 996000f2424e..3e88652670e6 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -746,6 +746,62 @@ int xe_vm_userptr_check_repin(struct xe_vm *vm)
 		list_empty_careful(&vm->userptr.invalidated)) ? 0 : -EAGAIN;
 }
 
+static void free_ban_entry(struct xe_exec_queue_ban_entry *b)
+{
+	list_del(&b->list);
+	kfree(b->pf);
+	kfree(b);
+}
+
+void xe_vm_add_ban_entry(struct xe_vm *vm, struct xe_exec_queue *q)
+{
+	struct xe_exec_queue_ban_entry *b = NULL;
+	struct xe_file *xef = q->xef;
+
+	b = kzalloc(sizeof(*b), GFP_KERNEL);
+	xe_assert(xef->xe, b);
+
+	spin_lock(&vm->bans.lock);
+	list_add_tail(&b->list, &vm->bans.list);
+	vm->bans.len++;
+	/**
+	 * Limit the number of bans in the bans list to prevent memory overuse.
+	 */
+	if (vm->bans.len > MAX_BANS) {
+		struct xe_exec_queue_ban_entry *rem =
+			list_first_entry(&vm->bans.list, struct xe_exec_queue_ban_entry, list);
+
+		free_ban_entry(rem);
+		vm->bans.len--;
+	}
+	spin_unlock(&vm->bans.lock);
+
+	/**
+	 * Associate the current pagefault saved to the VM to the ban entry, and clear
+	 * the VM pagefault cache.  This is still valid if vm->pf.info is NULL.
+	 */
+	spin_lock(&vm->pf.lock);
+	b->pf = vm->pf.info;
+	vm->pf.info = NULL;
+	spin_unlock(&vm->pf.lock);
+
+	/** Save blame data to list element */
+	b->exec_queue_id = q->id;
+}
+
+void xe_vm_remove_ban_entry(struct xe_vm *vm, struct xe_exec_queue *q)
+{
+	struct xe_exec_queue_ban_entry *b, *tmp;
+
+	spin_lock(&vm->bans.lock);
+	list_for_each_entry_safe(b, tmp, &vm->bans.list, list)
+		if (b->exec_queue_id == q->id) {
+			free_ban_entry(b);
+			vm->bans.len--;
+		}
+	spin_unlock(&vm->bans.lock);
+}
+
 static int xe_vma_ops_alloc(struct xe_vma_ops *vops, bool array_of_binds)
 {
 	int i;
@@ -1448,6 +1504,10 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
 	init_rwsem(&vm->userptr.notifier_lock);
 	spin_lock_init(&vm->userptr.invalidated_lock);
 
+	INIT_LIST_HEAD(&vm->bans.list);
+	spin_lock_init(&vm->bans.lock);
+	spin_lock_init(&vm->pf.lock);
+
 	ttm_lru_bulk_move_init(&vm->lru_bulk_move);
 
 	INIT_WORK(&vm->destroy_work, vm_destroy_work_func);
@@ -1672,6 +1732,15 @@ void xe_vm_close_and_put(struct xe_vm *vm)
 	}
 	up_write(&xe->usm.lock);
 
+	if (vm->bans.len) {
+		struct xe_exec_queue_ban_entry *b, *tmp;
+
+		spin_lock(&vm->bans.lock);
+		list_for_each_entry_safe(b, tmp, &vm->bans.list, list)
+			free_ban_entry(b);
+		spin_unlock(&vm->bans.lock);
+	}
+
 	for_each_tile(tile, xe, id)
 		xe_range_fence_tree_fini(&vm->rftree[id]);
 
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index fb3f15ee89ec..78dbc5d57cd3 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -12,6 +12,8 @@
 #include "xe_map.h"
 #include "xe_vm_types.h"
 
+#define MAX_BANS 50
+
 struct drm_device;
 struct drm_printer;
 struct drm_file;
@@ -244,6 +246,10 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma);
 
 int xe_vma_userptr_check_repin(struct xe_userptr_vma *uvma);
 
+void xe_vm_add_ban_entry(struct xe_vm *vm, struct xe_exec_queue *q);
+
+void xe_vm_remove_ban_entry(struct xe_vm *vm, struct xe_exec_queue *q);
+
 bool xe_vm_validate_should_retry(struct drm_exec *exec, int err, ktime_t *end);
 
 int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma);
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 52467b9b5348..e7e2d682b1b6 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -18,6 +18,7 @@
 #include "xe_range_fence.h"
 
 struct xe_bo;
+struct xe_pagefault;
 struct xe_sync_entry;
 struct xe_user_fence;
 struct xe_vm;
@@ -135,6 +136,15 @@ struct xe_userptr_vma {
 
 struct xe_device;
 
+struct xe_exec_queue_ban_entry {
+	/** @exec_queue_id: ID number of banned exec queue */
+	u32 exec_queue_id;
+	/** @pf: pagefault on engine of banned exec queue, if any at time */
+	struct xe_pagefault *pf;
+	/** @list: link into @xe_vm.bans.list */
+	struct list_head list;
+};
+
 struct xe_vm {
 	/** @gpuvm: base GPUVM used to track VMAs */
 	struct drm_gpuvm gpuvm;
@@ -274,6 +284,27 @@ struct xe_vm {
 		bool capture_once;
 	} error_capture;
 
+	/**
+	 * @ban_list: List of relevant banned exec queues associated with this
+	 * vm, as well as any pagefaults at time of ban.
+	 */
+	struct {
+		/** @lock: lock protecting @bans.list */
+		spinlock_t lock;
+		/** @list: list of xe_exec_queue_ban_entry entries */
+		struct list_head list;
+		/** @len: length of @bans.list */
+		unsigned int len;
+	} bans;
+
+	/** @pf: the last pagefault seen on this VM */
+	struct {
+		/** @pf.info: info containing last seen pagefault details */
+		struct xe_pagefault *info;
+		/** @pf.lock: lock protecting @pf.info */
+		spinlock_t lock;
+	} pf;
+
 	/**
 	 * @tlb_flush_seqno: Required TLB flush seqno for the next exec.
 	 * protected by the vm resv.
-- 
2.43.0

[PATCH 5/6] drm/xe/xe_vm: Add per VM reset stats

[Jonathan Cavitt]

Add a counter to xe_vm that tracks the number of times an engine reset
has been observed with respect to the VM since creation.

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_submit.c | 2 ++
 drivers/gpu/drm/xe/xe_vm_types.h   | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index f0bfc9d109cb..e4c2413ed47e 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1990,6 +1990,8 @@ int xe_guc_exec_queue_reset_handler(struct xe_guc *guc, u32 *msg, u32 len)
 
 	trace_xe_exec_queue_reset(q);
 
+	atomic_inc(&q->vm->reset_count);
+
 	/*
 	 * A banned engine is a NOP at this point (came from
 	 * guc_exec_queue_timedout_job). Otherwise, kick drm scheduler to cancel
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index e7e2d682b1b6..a448402250e5 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -305,6 +305,9 @@ struct xe_vm {
 		spinlock_t lock;
 	} pf;
 
+	/** @reset_count: number of times this VM has seen an engine reset */
+	atomic_t reset_count;
+
 	/**
 	 * @tlb_flush_seqno: Required TLB flush seqno for the next exec.
 	 * protected by the vm resv.
-- 
2.43.0

[PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Jonathan Cavitt]

Add support for userspace to get various properties from a specified VM.
The currently supported properties are:

- The number of engine resets the VM has observed
- The number of exec queue bans the VM has observed, up to the last 50
  relevant ones, and how many of those were caused by faults.

The latter request also includes information on the exec queue bans,
such as the ID of the banned exec queue, whether the ban was caused by a
pagefault or not, and the address and address type of the associated
fault (if one exists).

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Suggested-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_device.c |   2 +
 drivers/gpu/drm/xe/xe_vm.c     | 106 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_vm.h     |   2 +
 include/uapi/drm/xe_drm.h      |  73 +++++++++++++++++++++++
 4 files changed, 183 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 9454b51f7ad8..3a509a69062c 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -193,6 +193,8 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl,
 			  DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(XE_OBSERVATION, xe_observation_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(XE_VM_GET_PROPERTY, xe_vm_get_property_ioctl,
+			  DRM_RENDER_ALLOW),
 };
 
 static long xe_drm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 3e88652670e6..047908eb9ff7 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -3258,6 +3258,112 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	return err;
 }
 
+static u32 xe_vm_get_property_size(struct xe_vm *vm, u32 property)
+{
+	u32 size = -EINVAL;
+
+	switch (property) {
+	case DRM_XE_VM_GET_PROPERTY_FAULTS:
+		spin_lock(&vm->bans.lock);
+		size = vm->bans.len * sizeof(struct drm_xe_ban);
+		spin_unlock(&vm->bans.lock);
+		size += sizeof(struct drm_xe_faults);
+		break;
+	case DRM_XE_VM_GET_PROPERTY_NUM_RESETS:
+		size = sizeof(u64);
+		break;
+	default:
+		break;
+	}
+
+	return size;
+}
+
+static enum drm_xe_fault_address_type
+xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
+{
+	if (!pf)
+		return 0;
+
+	vma = lookup_vma(vm, pf->page_addr);
+	if (!vma)
+		return DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT;
+	if (xe_vma_read_only(vma) && pf->access_type != XE_PAGEFAULT_ACCESS_TYPE_READ)
+		return DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT;
+	return 0;
+}
+
+int xe_vm_get_property_ioctl(struct drm_device *drm, void *data,
+			     struct drm_file *file)
+{
+	struct xe_device *xe = to_xe_device(drm);
+	struct xe_file *xef = to_xe_file(file);
+	struct drm_xe_vm_get_property *args = data;
+	struct xe_vm *vm;
+	u32 size;
+
+	if (XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
+		return -EINVAL;
+
+	vm = xe_vm_lookup(xef, args->vm_id);
+	if (XE_IOCTL_DBG(xe, !vm))
+		return -ENOENT;
+
+	size = xe_vm_get_property_size(vm, args->property);
+	if (size < 0) {
+		return size;
+	} else if (!args->size) {
+		args->size = size;
+		return 0;
+	} else if (args->size != size) {
+		return -EINVAL;
+	}
+
+	if (args->property == DRM_XE_VM_GET_PROPERTY_FAULTS) {
+		struct drm_xe_faults __user *usr_ptr = u64_to_user_ptr(args->data);
+		struct drm_xe_faults fault_list;
+		struct drm_xe_ban *ban;
+		struct xe_exec_queue_ban_entry *entry;
+		int i = 0;
+
+		if (copy_from_user(&fault_list, usr_ptr, size))
+			return -EFAULT;
+
+		fault_list.num_faults = 0;
+
+		spin_lock(&vm->bans.lock);
+		list_for_each_entry(entry, &vm->bans.list, list) {
+			struct xe_pagefault *pf = entry->pf;
+
+			ban = &fault_list.list[i++];
+			ban->exec_queue_id = entry->exec_queue_id;
+			ban->faulted = !!pf ? 1 : 0;
+			ban->address = pf ? pf->page_addr : 0;
+			ban->address_type = xe_pagefault_access_type_to_address_type(vm, pf);
+			ban->address_type = pf ? pf->fault_type : 0;
+			fault_list.num_faults += ban->faulted;
+		}
+		spin_unlock(&vm->bans.lock);
+
+		fault_list.num_bans = i;
+
+		if (copy_to_user(usr_ptr, &fault_list, size))
+			return -EFAULT;
+
+	} else if (args->property == DRM_XE_VM_GET_PROPERTY_NUM_RESETS) {
+		u64 __user *usr_ptr = u64_to_user_ptr(args->data);
+		u64 num_resets = atomic_read(&vm->reset_count);
+
+		if (copy_to_user(usr_ptr, &num_resets, size))
+			return -EFAULT;
+
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * xe_vm_bind_kernel_bo - bind a kernel BO to a VM
  * @vm: VM to bind the BO to
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 78dbc5d57cd3..84653539d8db 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -184,6 +184,8 @@ int xe_vm_destroy_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file);
 int xe_vm_bind_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *file);
+int xe_vm_get_property_ioctl(struct drm_device *dev, void *data,
+			     struct drm_file *file);
 
 void xe_vm_close_and_put(struct xe_vm *vm);
 
diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index 76a462fae05f..00328d8a15dd 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -81,6 +81,7 @@ extern "C" {
  *  - &DRM_IOCTL_XE_EXEC
  *  - &DRM_IOCTL_XE_WAIT_USER_FENCE
  *  - &DRM_IOCTL_XE_OBSERVATION
+ *  - &DRM_IOCTL_XE_VM_GET_BANS
  */
 
 /*
@@ -102,6 +103,7 @@ extern "C" {
 #define DRM_XE_EXEC			0x09
 #define DRM_XE_WAIT_USER_FENCE		0x0a
 #define DRM_XE_OBSERVATION		0x0b
+#define DRM_XE_VM_GET_PROPERTY		0x0c
 
 /* Must be kept compact -- no holes */
 
@@ -117,6 +119,7 @@ extern "C" {
 #define DRM_IOCTL_XE_EXEC			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
 #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
 #define DRM_IOCTL_XE_OBSERVATION		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_OBSERVATION, struct drm_xe_observation_param)
+#define DRM_IOCTL_XE_VM_GET_PROPERTY		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_GET_PROPERTY, struct drm_xe_vm_get_property)
 
 /**
  * DOC: Xe IOCTL Extensions
@@ -1166,6 +1169,76 @@ struct drm_xe_vm_bind {
 	__u64 reserved[2];
 };
 
+/** Types of fault address */
+enum drm_xe_fault_address_type {
+	DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT,
+	DRM_XE_FAULT_ADDRESS_TYPE_READ_INVALID_EXT,
+	DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT,
+};
+
+struct drm_xe_ban {
+	/** @exec_queue_id: ID of banned exec queue */
+	__u32 exec_queue_id;
+	/** @faulted: Whether or not the ban has an associated pagefault.  0 is no, 1 is yes */
+	__u32 faulted;
+	/** @address: Address of the fault, if relevant */
+	__u64 address;
+	/** @address_type: enum drm_xe_fault_address_type, if relevant */
+	__u32 address_type;
+	/** @pad: MBZ */
+	__u32 pad;
+	/** @reserved: MBZ */
+	__u64 reserved[3];
+};
+
+struct drm_xe_faults {
+	/** @num_faults: Number of faults observed on the VM */
+	__u32 num_faults;
+	/** @num_bans: Number of bans observed on the VM */
+	__u32 num_bans;
+	/** @reserved: MBZ */
+	__u64 reserved[2];
+	/** @list: Dynamic sized array of drm_xe_ban bans */
+	struct drm_xe_ban list[];
+};
+
+/**
+ * struct drm_xe_vm_get_property - Input of &DRM_IOCTL_XE_VM_GET_PROPERTY
+ *
+ * The user provides a VM ID and a property to query to this ioctl,
+ * and the ioctl returns the size of the return value.  Calling the
+ * ioctl again with memory reserved for the data will save the
+ * requested property data to the data pointer.
+ *
+ * The valid properties are:
+ *  - %DRM_XE_VM_GET_PROPERTY_FAULTS : Property is a drm_xe_faults struct of dynamic size
+ *  - %DRM_XE_VM_GET_PROPERTY_NUM_RESETS: Property is a scalar
+ */
+struct drm_xe_vm_get_property {
+	/** @extensions: Pointer to the first extension struct, if any */
+	__u64 extensions;
+
+	/** @vm_id: The ID of the VM to query the properties of */
+	__u32 vm_id;
+
+#define DRM_XE_VM_GET_PROPERTY_FAULTS		0
+#define DRM_XE_VM_GET_PROPERTY_NUM_RESETS	1
+	/** @property: The property to get */
+	__u32 property;
+
+	/** @size: Size of returned property @data */
+	__u32 size;
+
+	/** @pad: MBZ */
+	__u32 pad;
+
+	/** @reserved: MBZ */
+	__u64 reserved[2];
+
+	/** @data: Pointer storing return data */
+	__u64 data;
+};
+
 /**
  * struct drm_xe_exec_queue_create - Input of &DRM_IOCTL_XE_EXEC_QUEUE_CREATE
  *
-- 
2.43.0

Re: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[kernel test robot]

Hi Jonathan,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-xe/drm-xe-next]
[also build test ERROR on next-20250226]
[cannot apply to linus/master v6.14-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Cavitt/drm-xe-xe_gt_pagefault-Migrate-lookup_vma-to-xe_vm-h/20250227-070008
base:   https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next
patch link:    https://lore.kernel.org/r/20250226225557.133076-7-jonathan.cavitt%40intel.com
patch subject: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
config: s390-randconfig-001-20250227 (https://download.01.org/0day-ci/archive/20250227/202502271029.67aYhWm6-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250227/202502271029.67aYhWm6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502271029.67aYhWm6-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/xe/xe_vm.c:3288:2: error: use of undeclared identifier 'vma'; did you mean 'vm'?
    3288 |         vma = lookup_vma(vm, pf->page_addr);
         |         ^~~
         |         vm
   drivers/gpu/drm/xe/xe_vm.c:3283:56: note: 'vm' declared here
    3283 | xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
         |                                                        ^
>> drivers/gpu/drm/xe/xe_vm.c:3288:8: error: call to undeclared function 'lookup_vma'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    3288 |         vma = lookup_vma(vm, pf->page_addr);
         |               ^
>> drivers/gpu/drm/xe/xe_vm.c:3288:6: error: incompatible integer to pointer conversion assigning to 'struct xe_vm *' from 'int' [-Wint-conversion]
    3288 |         vma = lookup_vma(vm, pf->page_addr);
         |             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/xe/xe_vm.c:3289:7: error: use of undeclared identifier 'vma'; did you mean 'vm'?
    3289 |         if (!vma)
         |              ^~~
         |              vm
   drivers/gpu/drm/xe/xe_vm.c:3283:56: note: 'vm' declared here
    3283 | xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
         |                                                        ^
   drivers/gpu/drm/xe/xe_vm.c:3291:23: error: use of undeclared identifier 'vma'; did you mean 'vm'?
    3291 |         if (xe_vma_read_only(vma) && pf->access_type != XE_PAGEFAULT_ACCESS_TYPE_READ)
         |                              ^~~
         |                              vm
   drivers/gpu/drm/xe/xe_vm.c:3283:56: note: 'vm' declared here
    3283 | xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
         |                                                        ^
   5 errors generated.


vim +3288 drivers/gpu/drm/xe/xe_vm.c

  3281	
  3282	static enum drm_xe_fault_address_type
  3283	xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
  3284	{
  3285		if (!pf)
  3286			return 0;
  3287	
> 3288		vma = lookup_vma(vm, pf->page_addr);
  3289		if (!vma)
  3290			return DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT;
  3291		if (xe_vma_read_only(vma) && pf->access_type != XE_PAGEFAULT_ACCESS_TYPE_READ)
  3292			return DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT;
  3293		return 0;
  3294	}
  3295	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[kernel test robot]

Hi Jonathan,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-xe/drm-xe-next]
[also build test ERROR on next-20250226]
[cannot apply to linus/master v6.14-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Cavitt/drm-xe-xe_gt_pagefault-Migrate-lookup_vma-to-xe_vm-h/20250227-070008
base:   https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next
patch link:    https://lore.kernel.org/r/20250226225557.133076-7-jonathan.cavitt%40intel.com
patch subject: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
config: i386-buildonly-randconfig-002-20250227 (https://download.01.org/0day-ci/archive/20250227/202502271226.qHKoHhA5-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250227/202502271226.qHKoHhA5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502271226.qHKoHhA5-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/xe/xe_vm.c: In function 'xe_pagefault_access_type_to_address_type':
>> drivers/gpu/drm/xe/xe_vm.c:3288:9: error: 'vma' undeclared (first use in this function); did you mean 'vm'?
    3288 |         vma = lookup_vma(vm, pf->page_addr);
         |         ^~~
         |         vm
   drivers/gpu/drm/xe/xe_vm.c:3288:9: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/xe/xe_vm.c:3288:15: error: implicit declaration of function 'lookup_vma' [-Werror=implicit-function-declaration]
    3288 |         vma = lookup_vma(vm, pf->page_addr);
         |               ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +3288 drivers/gpu/drm/xe/xe_vm.c

  3281	
  3282	static enum drm_xe_fault_address_type
  3283	xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
  3284	{
  3285		if (!pf)
  3286			return 0;
  3287	
> 3288		vma = lookup_vma(vm, pf->page_addr);
  3289		if (!vma)
  3290			return DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT;
  3291		if (xe_vma_read_only(vma) && pf->access_type != XE_PAGEFAULT_ACCESS_TYPE_READ)
  3292			return DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT;
  3293		return 0;
  3294	}
  3295	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Matthew Brost]

On Wed, Feb 26, 2025 at 10:55:56PM +0000, Jonathan Cavitt wrote:
> Add support for userspace to get various properties from a specified VM.
> The currently supported properties are:
> 
> - The number of engine resets the VM has observed
> - The number of exec queue bans the VM has observed, up to the last 50
>   relevant ones, and how many of those were caused by faults.
> 
> The latter request also includes information on the exec queue bans,
> such as the ID of the banned exec queue, whether the ban was caused by a
> pagefault or not, and the address and address type of the associated
> fault (if one exists).
> 

> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c |   2 +
>  drivers/gpu/drm/xe/xe_vm.c     | 106 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_vm.h     |   2 +
>  include/uapi/drm/xe_drm.h      |  73 +++++++++++++++++++++++
>  4 files changed, 183 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 9454b51f7ad8..3a509a69062c 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -193,6 +193,8 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl,
>  			  DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(XE_OBSERVATION, xe_observation_ioctl, DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(XE_VM_GET_PROPERTY, xe_vm_get_property_ioctl,
> +			  DRM_RENDER_ALLOW),
>  };
>  
>  static long xe_drm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 3e88652670e6..047908eb9ff7 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -3258,6 +3258,112 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  	return err;
>  }
>  
> +static u32 xe_vm_get_property_size(struct xe_vm *vm, u32 property)
> +{
> +	u32 size = -EINVAL;
> +
> +	switch (property) {
> +	case DRM_XE_VM_GET_PROPERTY_FAULTS:
> +		spin_lock(&vm->bans.lock);
> +		size = vm->bans.len * sizeof(struct drm_xe_ban);
> +		spin_unlock(&vm->bans.lock);
> +		size += sizeof(struct drm_xe_faults);
> +		break;
> +	case DRM_XE_VM_GET_PROPERTY_NUM_RESETS:
> +		size = sizeof(u64);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return size;
> +}
> +
> +static enum drm_xe_fault_address_type
> +xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
> +{
> +	if (!pf)
> +		return 0;
> +
> +	vma = lookup_vma(vm, pf->page_addr);

The VMA state is not stable; for example, it can change between the time
of the fault and the time this IOCTL is called. Therefore, we need an
intermediate structure fully populated at fault time, which is then
converted to user output upon IOCTL.

> +	if (!vma)
> +		return DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT;
> +	if (xe_vma_read_only(vma) && pf->access_type != XE_PAGEFAULT_ACCESS_TYPE_READ)
> +		return DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT;
> +	return 0;
> +}
> +
> +int xe_vm_get_property_ioctl(struct drm_device *drm, void *data,
> +			     struct drm_file *file)
> +{
> +	struct xe_device *xe = to_xe_device(drm);
> +	struct xe_file *xef = to_xe_file(file);
> +	struct drm_xe_vm_get_property *args = data;
> +	struct xe_vm *vm;
> +	u32 size;
> +
> +	if (XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> +		return -EINVAL;
> +
> +	vm = xe_vm_lookup(xef, args->vm_id);
> +	if (XE_IOCTL_DBG(xe, !vm))
> +		return -ENOENT;
> +
> +	size = xe_vm_get_property_size(vm, args->property);

For both this size calculation and the population if statements below,
I'd lean toward using vfunc tables for the implementations based on the
property index. Maybe overkill, though—not a strongly held opinion.

> +	if (size < 0) {
> +		return size;
> +	} else if (!args->size) {
> +		args->size = size;
> +		return 0;
> +	} else if (args->size != size) {
> +		return -EINVAL;
> +	}
> +
> +	if (args->property == DRM_XE_VM_GET_PROPERTY_FAULTS) {
> +		struct drm_xe_faults __user *usr_ptr = u64_to_user_ptr(args->data);
> +		struct drm_xe_faults fault_list;
> +		struct drm_xe_ban *ban;
> +		struct xe_exec_queue_ban_entry *entry;
> +		int i = 0;
> +
> +		if (copy_from_user(&fault_list, usr_ptr, size))
> +			return -EFAULT;
> +
> +		fault_list.num_faults = 0;
> +
> +		spin_lock(&vm->bans.lock);
> +		list_for_each_entry(entry, &vm->bans.list, list) {
> +			struct xe_pagefault *pf = entry->pf;
> +
> +			ban = &fault_list.list[i++];
> +			ban->exec_queue_id = entry->exec_queue_id;
> +			ban->faulted = !!pf ? 1 : 0;
> +			ban->address = pf ? pf->page_addr : 0;
> +			ban->address_type = xe_pagefault_access_type_to_address_type(vm, pf);
> +			ban->address_type = pf ? pf->fault_type : 0;
> +			fault_list.num_faults += ban->faulted;
> +		}
> +		spin_unlock(&vm->bans.lock);
> +
> +		fault_list.num_bans = i;
> +
> +		if (copy_to_user(usr_ptr, &fault_list, size))
> +			return -EFAULT;
> +
> +	} else if (args->property == DRM_XE_VM_GET_PROPERTY_NUM_RESETS) {
> +		u64 __user *usr_ptr = u64_to_user_ptr(args->data);
> +		u64 num_resets = atomic_read(&vm->reset_count);
> +
> +		if (copy_to_user(usr_ptr, &num_resets, size))
> +			return -EFAULT;
> +
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * xe_vm_bind_kernel_bo - bind a kernel BO to a VM
>   * @vm: VM to bind the BO to
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 78dbc5d57cd3..84653539d8db 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -184,6 +184,8 @@ int xe_vm_destroy_ioctl(struct drm_device *dev, void *data,
>  			struct drm_file *file);
>  int xe_vm_bind_ioctl(struct drm_device *dev, void *data,
>  		     struct drm_file *file);
> +int xe_vm_get_property_ioctl(struct drm_device *dev, void *data,
> +			     struct drm_file *file);
>  
>  void xe_vm_close_and_put(struct xe_vm *vm);
>  
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 76a462fae05f..00328d8a15dd 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -81,6 +81,7 @@ extern "C" {
>   *  - &DRM_IOCTL_XE_EXEC
>   *  - &DRM_IOCTL_XE_WAIT_USER_FENCE
>   *  - &DRM_IOCTL_XE_OBSERVATION
> + *  - &DRM_IOCTL_XE_VM_GET_BANS

s/DRM_IOCTL_XE_VM_GET_BANS/DRM_IOCTL_XE_VM_GET_PROPERTY

>   */
>  
>  /*
> @@ -102,6 +103,7 @@ extern "C" {
>  #define DRM_XE_EXEC			0x09
>  #define DRM_XE_WAIT_USER_FENCE		0x0a
>  #define DRM_XE_OBSERVATION		0x0b
> +#define DRM_XE_VM_GET_PROPERTY		0x0c
>  
>  /* Must be kept compact -- no holes */
>  
> @@ -117,6 +119,7 @@ extern "C" {
>  #define DRM_IOCTL_XE_EXEC			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
>  #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
>  #define DRM_IOCTL_XE_OBSERVATION		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_OBSERVATION, struct drm_xe_observation_param)
> +#define DRM_IOCTL_XE_VM_GET_PROPERTY		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_GET_PROPERTY, struct drm_xe_vm_get_property)

s/DRM_IOW/DRM_IOWR as we write back the size.

For reference: DRM_IOW does not copy back the modified IOCTL structure
(e.g., struct drm_xe_vm_get_property), while DRM_IOWR does.

>  
>  /**
>   * DOC: Xe IOCTL Extensions
> @@ -1166,6 +1169,76 @@ struct drm_xe_vm_bind {
>  	__u64 reserved[2];
>  };
>  
> +/** Types of fault address */
> +enum drm_xe_fault_address_type {
> +	DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT,
> +	DRM_XE_FAULT_ADDRESS_TYPE_READ_INVALID_EXT,
> +	DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT,
> +};

No enum uAPI in header files—use defines instead. Enums can change at
the compiler's discretion, while defines cannot.

> +
> +struct drm_xe_ban {
> +	/** @exec_queue_id: ID of banned exec queue */
> +	__u32 exec_queue_id;

I don't think we can reliably associate a page fault with an
exec_queue_id at the moment, given my above statement about having to
capture all state at the time of the page fault. Maybe we could with
some tricks between the page fault and the IOMMU CAT error G2H?
Regardless, let's ask the UMD we are targeting [1] if this information
would be helpful. It would seemingly have to be vendor-specific
information, not part of the generic Vk information.

Additionally, it might be good to ask what other vendor-specific
information, if any, we'd need here based on what the current page fault
interface supports.

[1] https://registry.khronos.org/vulkan/specs/latest/man/html/VK_EXT_device_fault.html

> +	/** @faulted: Whether or not the ban has an associated pagefault.  0 is no, 1 is yes */
> +	__u32 faulted;
> +	/** @address: Address of the fault, if relevant */
> +	__u64 address;
> +	/** @address_type: enum drm_xe_fault_address_type, if relevant */
> +	__u32 address_type;

We likely need a fault_size field to support VkDeviceSize
addressPrecision; as defined here [2]. I believe we can extract this
information from pagefault.fault_level.

[2] https://registry.khronos.org/vulkan/specs/latest/man/html/VkDeviceFaultAddressInfoEXT.html

> +	/** @pad: MBZ */
> +	__u32 pad;
> +	/** @reserved: MBZ */
> +	__u64 reserved[3];
> +};
> +
> +struct drm_xe_faults {
> +	/** @num_faults: Number of faults observed on the VM */
> +	__u32 num_faults;
> +	/** @num_bans: Number of bans observed on the VM */
> +	__u32 num_bans;

I don't think num_bans and num_faults really provide any benefit for
supporting [1]. The requirement for [1] is device faults—nothing more.
With that in mind, I'd lean toward an array of a single structure
(returned in drm_xe_vm_get_property.data, number of faults can be
inferred from the returned size) reporting all faults, with each entry
containing all the fault information. If another use case arises for
reporting all banned queues, we can add a property for that.

> +	/** @reserved: MBZ */
> +	__u64 reserved[2];
> +	/** @list: Dynamic sized array of drm_xe_ban bans */
> +	struct drm_xe_ban list[];

list[0] would be the prefered way.

> +};
> +
> +/**
> + * struct drm_xe_vm_get_property - Input of &DRM_IOCTL_XE_VM_GET_PROPERTY
> + *
> + * The user provides a VM ID and a property to query to this ioctl,
> + * and the ioctl returns the size of the return value.  Calling the
> + * ioctl again with memory reserved for the data will save the
> + * requested property data to the data pointer.
> + *
> + * The valid properties are:
> + *  - %DRM_XE_VM_GET_PROPERTY_FAULTS : Property is a drm_xe_faults struct of dynamic size
> + *  - %DRM_XE_VM_GET_PROPERTY_NUM_RESETS: Property is a scalar

We need to consider where the number of resets requirement is coming
from. As far as I know, the Vk extension [1] we are targeting does not
need this information. I'm unsure about the compute UMD requirements at
the moment, so let's focus on supporting the Vk extension first.

Any uAPI must also have a UMD component, so focusing on one issue at a
time makes sense.

> + */
> +struct drm_xe_vm_get_property {
> +	/** @extensions: Pointer to the first extension struct, if any */
> +	__u64 extensions;
> +
> +	/** @vm_id: The ID of the VM to query the properties of */
> +	__u32 vm_id;
> +
> +#define DRM_XE_VM_GET_PROPERTY_FAULTS		0
> +#define DRM_XE_VM_GET_PROPERTY_NUM_RESETS	1
> +	/** @property: The property to get */
> +	__u32 property;
> +
> +	/** @size: Size of returned property @data */
> +	__u32 size;
> +
> +	/** @pad: MBZ */
> +	__u32 pad;
> +
> +	/** @reserved: MBZ */
> +	__u64 reserved[2];

I'd put the reserved bits at the end.

> +
> +	/** @data: Pointer storing return data */
> +	__u64 data;

union {
	__u64 data;
	__u64 ptr;
};

We would simply return 'data' for properties that fit in a u64 and
perform the size=0, return size process for properties that require a
user allocation.

This may not be relevant at the moment if we drop
DRM_XE_VM_GET_PROPERTY_NUM_RESET.

Matt

> +};
> +
>  /**
>   * struct drm_xe_exec_queue_create - Input of &DRM_IOCTL_XE_EXEC_QUEUE_CREATE
>   *
> -- 
> 2.43.0
> 

RE: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Cavitt, Jonathan]

Some responses below.  If I skip over anything, just assume that I'm taking the request
into consideration and that it will be fixed for version 2 of this patch series.

-----Original Message-----
From: Brost, Matthew <matthew.brost@intel.com> 
Sent: Thursday, February 27, 2025 12:25 AM
To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
Cc: intel-xe@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; joonas.lahtinen@linux.intel.com; Zhang, Jianxun <jianxun.zhang@intel.com>; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
> 
> On Wed, Feb 26, 2025 at 10:55:56PM +0000, Jonathan Cavitt wrote:
> > Add support for userspace to get various properties from a specified VM.
> > The currently supported properties are:
> > 
> > - The number of engine resets the VM has observed
> > - The number of exec queue bans the VM has observed, up to the last 50
> >   relevant ones, and how many of those were caused by faults.
> > 
> > The latter request also includes information on the exec queue bans,
> > such as the ID of the banned exec queue, whether the ban was caused by a
> > pagefault or not, and the address and address type of the associated
> > fault (if one exists).
> > 
> 
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > ---
[...]
> 
> > +
> > +struct drm_xe_ban {
> > +	/** @exec_queue_id: ID of banned exec queue */
> > +	__u32 exec_queue_id;
> 
> I don't think we can reliably associate a page fault with an
> exec_queue_id at the moment, given my above statement about having to
> capture all state at the time of the page fault. Maybe we could with
> some tricks between the page fault and the IOMMU CAT error G2H?
> Regardless, let's ask the UMD we are targeting [1] if this information
> would be helpful. It would seemingly have to be vendor-specific
> information, not part of the generic Vk information.
> 
> Additionally, it might be good to ask what other vendor-specific
> information, if any, we'd need here based on what the current page fault
> interface supports.
> 
> [1] https://registry.khronos.org/vulkan/specs/latest/man/html/VK_EXT_device_fault.html

The original request was something along the lines of having a mirror of the
DRM_IOCTL_I915_GET_RESET_STATS on XeKMD.  Those reset stats contain
information on the "context" ID, which maps to the exec queue ID on XeKMD.

Even if we can't reasonably blame a pagefault on a particular exec queue, in
order to match the request correctly, this information needs to be returned.

The I915 reset stats also contain information on the number of observed engine
resets, so that needs to be returned as well.

@joonas.lahtinen@linux.intel.com can provide more details.  Or maybe
@Mistat, Tomasz .

> 
> > +	/** @faulted: Whether or not the ban has an associated pagefault.  0 is no, 1 is yes */
> > +	__u32 faulted;
> > +	/** @address: Address of the fault, if relevant */
> > +	__u64 address;
> > +	/** @address_type: enum drm_xe_fault_address_type, if relevant */
> > +	__u32 address_type;
> 
> We likely need a fault_size field to support VkDeviceSize
> addressPrecision; as defined here [2]. I believe we can extract this
> information from pagefault.fault_level.
> 
> [2] https://registry.khronos.org/vulkan/specs/latest/man/html/VkDeviceFaultAddressInfoEXT.html

I can add this field as a prototype, though it will always return SZ_4K until we
can have a longer discussion on how to map between the fault_level and the
fault_size.

> 
> > +	/** @pad: MBZ */
> > +	__u32 pad;
> > +	/** @reserved: MBZ */
> > +	__u64 reserved[3];
> > +};
> > +
> > +struct drm_xe_faults {
> > +	/** @num_faults: Number of faults observed on the VM */
> > +	__u32 num_faults;
> > +	/** @num_bans: Number of bans observed on the VM */
> > +	__u32 num_bans;
> 
> I don't think num_bans and num_faults really provide any benefit for
> supporting [1]. The requirement for [1] is device faults-nothing more.
> With that in mind, I'd lean toward an array of a single structure
> (returned in drm_xe_vm_get_property.data, number of faults can be
> inferred from the returned size) reporting all faults, with each entry
> containing all the fault information. If another use case arises for
> reporting all banned queues, we can add a property for that.

I'm fairly certain the full ban list was directly requested, but I can break
it into a third query at least.

Also, the abstraction is done here because that's how copy_from_user
has historically been used.  I'd rather not experiment with trying to
copy_from_user a structure array and bungling it, but I guess I can give
it a try at least...

> 
> > +	/** @reserved: MBZ */
> > +	__u64 reserved[2];
> > +	/** @list: Dynamic sized array of drm_xe_ban bans */
> > +	struct drm_xe_ban list[];
> 
> list[0] would be the prefered way.

That is not how dynamic arrays are handled for
struct drm_xe_query_engines,
struct drm_xe_query_mem_regions,
struct drm_xe_query_config,
struct drm_xe_query_gt_list,
struct drm_xe_query_topology_mask,
struct drm_xe_oa_unit,
or
struct drm_xe_query_oa_units

> 
> > +};
> > +
> > +/**
> > + * struct drm_xe_vm_get_property - Input of &DRM_IOCTL_XE_VM_GET_PROPERTY
> > + *
> > + * The user provides a VM ID and a property to query to this ioctl,
> > + * and the ioctl returns the size of the return value.  Calling the
> > + * ioctl again with memory reserved for the data will save the
> > + * requested property data to the data pointer.
> > + *
> > + * The valid properties are:
> > + *  - %DRM_XE_VM_GET_PROPERTY_FAULTS : Property is a drm_xe_faults struct of dynamic size
> > + *  - %DRM_XE_VM_GET_PROPERTY_NUM_RESETS: Property is a scalar
> 
> We need to consider where the number of resets requirement is coming
> from. As far as I know, the Vk extension [1] we are targeting does not
> need this information. I'm unsure about the compute UMD requirements at
> the moment, so let's focus on supporting the Vk extension first.
> 
> Any uAPI must also have a UMD component, so focusing on one issue at a
> time makes sense.

See first reply.
-Jonathan Cavitt

> 
> > + */
> > +struct drm_xe_vm_get_property {
> > +	/** @extensions: Pointer to the first extension struct, if any */
> > +	__u64 extensions;
> > +
> > +	/** @vm_id: The ID of the VM to query the properties of */
> > +	__u32 vm_id;
> > +
> > +#define DRM_XE_VM_GET_PROPERTY_FAULTS		0
> > +#define DRM_XE_VM_GET_PROPERTY_NUM_RESETS	1
> > +	/** @property: The property to get */
> > +	__u32 property;
> > +
> > +	/** @size: Size of returned property @data */
> > +	__u32 size;
> > +
> > +	/** @pad: MBZ */
> > +	__u32 pad;
> > +
> > +	/** @reserved: MBZ */
> > +	__u64 reserved[2];
> 
> I'd put the reserved bits at the end.
> 
> > +
> > +	/** @data: Pointer storing return data */
> > +	__u64 data;
> 
> union {
> 	__u64 data;
> 	__u64 ptr;
> };
> 
> We would simply return 'data' for properties that fit in a u64 and
> perform the size=0, return size process for properties that require a
> user allocation.
> 
> This may not be relevant at the moment if we drop
> DRM_XE_VM_GET_PROPERTY_NUM_RESET.
> 
> Matt
> 
> > +};
> > +
> >  /**
> >   * struct drm_xe_exec_queue_create - Input of &DRM_IOCTL_XE_EXEC_QUEUE_CREATE
> >   *
> > -- 
> > 2.43.0
> > 
> 

Re: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Matthew Brost]

On Thu, Feb 27, 2025 at 09:51:15AM -0700, Cavitt, Jonathan wrote:
> Some responses below.  If I skip over anything, just assume that I'm taking the request
> into consideration and that it will be fixed for version 2 of this patch series.
> 
> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com> 
> Sent: Thursday, February 27, 2025 12:25 AM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Cc: intel-xe@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; joonas.lahtinen@linux.intel.com; Zhang, Jianxun <jianxun.zhang@intel.com>; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
> > 
> > On Wed, Feb 26, 2025 at 10:55:56PM +0000, Jonathan Cavitt wrote:
> > > Add support for userspace to get various properties from a specified VM.
> > > The currently supported properties are:
> > > 
> > > - The number of engine resets the VM has observed
> > > - The number of exec queue bans the VM has observed, up to the last 50
> > >   relevant ones, and how many of those were caused by faults.
> > > 
> > > The latter request also includes information on the exec queue bans,
> > > such as the ID of the banned exec queue, whether the ban was caused by a
> > > pagefault or not, and the address and address type of the associated
> > > fault (if one exists).
> > > 
> > 
> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> [...]
> > 
> > > +
> > > +struct drm_xe_ban {
> > > +	/** @exec_queue_id: ID of banned exec queue */
> > > +	__u32 exec_queue_id;
> > 
> > I don't think we can reliably associate a page fault with an
> > exec_queue_id at the moment, given my above statement about having to
> > capture all state at the time of the page fault. Maybe we could with
> > some tricks between the page fault and the IOMMU CAT error G2H?
> > Regardless, let's ask the UMD we are targeting [1] if this information
> > would be helpful. It would seemingly have to be vendor-specific
> > information, not part of the generic Vk information.
> > 
> > Additionally, it might be good to ask what other vendor-specific
> > information, if any, we'd need here based on what the current page fault
> > interface supports.
> > 
> > [1] https://registry.khronos.org/vulkan/specs/latest/man/html/VK_EXT_device_fault.html
> 
> The original request was something along the lines of having a mirror of the
> DRM_IOCTL_I915_GET_RESET_STATS on XeKMD.  Those reset stats contain
> information on the "context" ID, which maps to the exec queue ID on XeKMD.
> 
> Even if we can't reasonably blame a pagefault on a particular exec queue, in
> order to match the request correctly, this information needs to be returned.
> 
> The I915 reset stats also contain information on the number of observed engine
> resets, so that needs to be returned as well.
> 
> @joonas.lahtinen@linux.intel.com can provide more details.  Or maybe
> @Mistat, Tomasz .
> 

You haven't really answered my question here or below where you say see
above. We a need UMD use case posted with any uAPI changes before
merging uAPI changes. I know the above Vk extension is going to be
implemented on top of this series but it is very unclear where the
number of resets requirement / UMD use case is coming from which makes
it impossible to review. 

Again I suggest focusing on the Vk use case first or go talk to our UMD
partners and figure out exactly why something similar to
DRM_IOCTL_I915_GET_RESET_STATS is required in Xe. I have made similar
comments on VLK-69424.

Matt

> > 
> > > +	/** @faulted: Whether or not the ban has an associated pagefault.  0 is no, 1 is yes */
> > > +	__u32 faulted;
> > > +	/** @address: Address of the fault, if relevant */
> > > +	__u64 address;
> > > +	/** @address_type: enum drm_xe_fault_address_type, if relevant */
> > > +	__u32 address_type;
> > 
> > We likely need a fault_size field to support VkDeviceSize
> > addressPrecision; as defined here [2]. I believe we can extract this
> > information from pagefault.fault_level.
> > 
> > [2] https://registry.khronos.org/vulkan/specs/latest/man/html/VkDeviceFaultAddressInfoEXT.html
> 
> I can add this field as a prototype, though it will always return SZ_4K until we
> can have a longer discussion on how to map between the fault_level and the
> fault_size.
> 
> > 
> > > +	/** @pad: MBZ */
> > > +	__u32 pad;
> > > +	/** @reserved: MBZ */
> > > +	__u64 reserved[3];
> > > +};
> > > +
> > > +struct drm_xe_faults {
> > > +	/** @num_faults: Number of faults observed on the VM */
> > > +	__u32 num_faults;
> > > +	/** @num_bans: Number of bans observed on the VM */
> > > +	__u32 num_bans;
> > 
> > I don't think num_bans and num_faults really provide any benefit for
> > supporting [1]. The requirement for [1] is device faults-nothing more.
> > With that in mind, I'd lean toward an array of a single structure
> > (returned in drm_xe_vm_get_property.data, number of faults can be
> > inferred from the returned size) reporting all faults, with each entry
> > containing all the fault information. If another use case arises for
> > reporting all banned queues, we can add a property for that.
> 
> I'm fairly certain the full ban list was directly requested, but I can break
> it into a third query at least.
> 
> Also, the abstraction is done here because that's how copy_from_user
> has historically been used.  I'd rather not experiment with trying to
> copy_from_user a structure array and bungling it, but I guess I can give
> it a try at least...
> 
> > 
> > > +	/** @reserved: MBZ */
> > > +	__u64 reserved[2];
> > > +	/** @list: Dynamic sized array of drm_xe_ban bans */
> > > +	struct drm_xe_ban list[];
> > 
> > list[0] would be the prefered way.
> 
> That is not how dynamic arrays are handled for
> struct drm_xe_query_engines,
> struct drm_xe_query_mem_regions,
> struct drm_xe_query_config,
> struct drm_xe_query_gt_list,
> struct drm_xe_query_topology_mask,
> struct drm_xe_oa_unit,
> or
> struct drm_xe_query_oa_units
> 
> > 
> > > +};
> > > +
> > > +/**
> > > + * struct drm_xe_vm_get_property - Input of &DRM_IOCTL_XE_VM_GET_PROPERTY
> > > + *
> > > + * The user provides a VM ID and a property to query to this ioctl,
> > > + * and the ioctl returns the size of the return value.  Calling the
> > > + * ioctl again with memory reserved for the data will save the
> > > + * requested property data to the data pointer.
> > > + *
> > > + * The valid properties are:
> > > + *  - %DRM_XE_VM_GET_PROPERTY_FAULTS : Property is a drm_xe_faults struct of dynamic size
> > > + *  - %DRM_XE_VM_GET_PROPERTY_NUM_RESETS: Property is a scalar
> > 
> > We need to consider where the number of resets requirement is coming
> > from. As far as I know, the Vk extension [1] we are targeting does not
> > need this information. I'm unsure about the compute UMD requirements at
> > the moment, so let's focus on supporting the Vk extension first.
> > 
> > Any uAPI must also have a UMD component, so focusing on one issue at a
> > time makes sense.
> 
> See first reply.
> -Jonathan Cavitt
> 
> > 
> > > + */
> > > +struct drm_xe_vm_get_property {
> > > +	/** @extensions: Pointer to the first extension struct, if any */
> > > +	__u64 extensions;
> > > +
> > > +	/** @vm_id: The ID of the VM to query the properties of */
> > > +	__u32 vm_id;
> > > +
> > > +#define DRM_XE_VM_GET_PROPERTY_FAULTS		0
> > > +#define DRM_XE_VM_GET_PROPERTY_NUM_RESETS	1
> > > +	/** @property: The property to get */
> > > +	__u32 property;
> > > +
> > > +	/** @size: Size of returned property @data */
> > > +	__u32 size;
> > > +
> > > +	/** @pad: MBZ */
> > > +	__u32 pad;
> > > +
> > > +	/** @reserved: MBZ */
> > > +	__u64 reserved[2];
> > 
> > I'd put the reserved bits at the end.
> > 
> > > +
> > > +	/** @data: Pointer storing return data */
> > > +	__u64 data;
> > 
> > union {
> > 	__u64 data;
> > 	__u64 ptr;
> > };
> > 
> > We would simply return 'data' for properties that fit in a u64 and
> > perform the size=0, return size process for properties that require a
> > user allocation.
> > 
> > This may not be relevant at the moment if we drop
> > DRM_XE_VM_GET_PROPERTY_NUM_RESET.
> > 
> > Matt
> > 
> > > +};
> > > +
> > >  /**
> > >   * struct drm_xe_exec_queue_create - Input of &DRM_IOCTL_XE_EXEC_QUEUE_CREATE
> > >   *
> > > -- 
> > > 2.43.0
> > > 
> > 

✓ CI.Patch_applied: success for drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Patchwork]

== Series Details ==

Series: drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
URL   : https://patchwork.freedesktop.org/series/145529/
State : success

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 8541a64154cd drm-tip: 2025y-02m-26d-23h-59m-28s UTC integration manifest
=== git am output follows ===
Applying: drm/xe/xe_gt_pagefault: Migrate lookup_vma to xe_vm.h
Applying: drm/xe/xe_exec_queue: Add ID param to exec queue struct
Applying: drm/xe/xe_gt_pagefault: Migrate pagefault struct to header
Applying: drm/xe/xe_vm: Add per VM pagefault info
Applying: drm/xe/xe_vm: Add per VM reset stats
Applying: drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

✗ CI.checkpatch: warning for drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Patchwork]

== Series Details ==

Series: drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
URL   : https://patchwork.freedesktop.org/series/145529/
State : warning

== Summary ==

+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
22f9cda3436b4fe965b5c5f31d2f2c1bcb483189
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit ef956f5e0e0f3defc038158f3b241d55e5f9e28d
Author: Jonathan Cavitt <jonathan.cavitt@intel.com>
Date:   Wed Feb 26 22:55:56 2025 +0000

    drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
    
    Add support for userspace to get various properties from a specified VM.
    The currently supported properties are:
    
    - The number of engine resets the VM has observed
    - The number of exec queue bans the VM has observed, up to the last 50
      relevant ones, and how many of those were caused by faults.
    
    The latter request also includes information on the exec queue bans,
    such as the ID of the banned exec queue, whether the ban was caused by a
    pagefault or not, and the address and address type of the associated
    fault (if one exists).
    
    Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
    Suggested-by: Matthew Brost <matthew.brost@intel.com>
+ /mt/dim checkpatch 8541a64154cd8b9c07977dedc1d22883f43279a0 drm-intel
35a2651335bb drm/xe/xe_gt_pagefault: Migrate lookup_vma to xe_vm.h
-:84: CHECK:LINE_SPACING: Please don't use multiple blank lines
#84: FILE: drivers/gpu/drm/xe/xe_vm.h:274:
+
+

total: 0 errors, 0 warnings, 1 checks, 67 lines checked
c27a745e0810 drm/xe/xe_exec_queue: Add ID param to exec queue struct
9716297e102a drm/xe/xe_gt_pagefault: Migrate pagefault struct to header
9839144569cf drm/xe/xe_vm: Add per VM pagefault info
ce0efa900b91 drm/xe/xe_vm: Add per VM reset stats
ef956f5e0e0f drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
-:188: WARNING:LONG_LINE: line length of 129 exceeds 100 columns
#188: FILE: include/uapi/drm/xe_drm.h:122:
+#define DRM_IOCTL_XE_VM_GET_PROPERTY		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_GET_PROPERTY, struct drm_xe_vm_get_property)

total: 0 errors, 1 warnings, 0 checks, 225 lines checked

✗ CI.KUnit: failure for drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Patchwork]

== Series Details ==

Series: drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
URL   : https://patchwork.freedesktop.org/series/145529/
State : failure

== Summary ==

+ trap cleanup EXIT
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
ERROR:root:../lib/iomap.c:156:5: warning: no previous prototype for ‘ioread64_lo_hi’ [-Wmissing-prototypes]
  156 | u64 ioread64_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:163:5: warning: no previous prototype for ‘ioread64_hi_lo’ [-Wmissing-prototypes]
  163 | u64 ioread64_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:170:5: warning: no previous prototype for ‘ioread64be_lo_hi’ [-Wmissing-prototypes]
  170 | u64 ioread64be_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:178:5: warning: no previous prototype for ‘ioread64be_hi_lo’ [-Wmissing-prototypes]
  178 | u64 ioread64be_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:264:6: warning: no previous prototype for ‘iowrite64_lo_hi’ [-Wmissing-prototypes]
  264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:272:6: warning: no previous prototype for ‘iowrite64_hi_lo’ [-Wmissing-prototypes]
  272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:280:6: warning: no previous prototype for ‘iowrite64be_lo_hi’ [-Wmissing-prototypes]
  280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
../lib/iomap.c:288:6: warning: no previous prototype for ‘iowrite64be_hi_lo’ [-Wmissing-prototypes]
  288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
../drivers/gpu/drm/xe/xe_vm.c: In function ‘xe_pagefault_access_type_to_address_type’:
../drivers/gpu/drm/xe/xe_vm.c:3288:9: error: ‘vma’ undeclared (first use in this function); did you mean ‘vm’?
 3288 |         vma = lookup_vma(vm, pf->page_addr);
      |         ^~~
      |         vm
../drivers/gpu/drm/xe/xe_vm.c:3288:9: note: each undeclared identifier is reported only once for each function it appears in
../drivers/gpu/drm/xe/xe_vm.c:3288:15: error: implicit declaration of function ‘lookup_vma’ [-Werror=implicit-function-declaration]
 3288 |         vma = lookup_vma(vm, pf->page_addr);
      |               ^~~~~~~~~~
cc1: some warnings being treated as errors
make[7]: *** [../scripts/Makefile.build:207: drivers/gpu/drm/xe/xe_vm.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [../scripts/Makefile.build:465: drivers/gpu/drm/xe] Error 2
make[5]: *** [../scripts/Makefile.build:465: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:465: drivers/gpu] Error 2
make[3]: *** [../scripts/Makefile.build:465: drivers] Error 2
make[2]: *** [/kernel/Makefile:1989: .] Error 2
make[1]: *** [/kernel/Makefile:251: __sub-make] Error 2
make: *** [Makefile:251: __sub-make] Error 2

[00:12:06] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[00:12:11] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json ARCH=um O=.kunit --jobs=48
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel