Claude review: drm/xe/pagefault: Add SRCID to pagefault struct

Claude review: drm/xe/pagefault: Add SRCID to pagefault struct

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

The `PFD_SRC_ID` field is `GENMASK(10, 3)` — an 8-bit value that fits cleanly in the new `u8 srcid` field.

The `reserved` field change from `u64 reserved` to `u8 reserved[7]` correctly maintains the 8-byte footprint at the same offset (16), keeping the struct at its documented 64-byte size:

```c
/** @consumer.srcid: ID of hardware unit producing fault */
u8 srcid;
/** @consumer.reserved: reserved bits for future expansion */
u8 reserved[7];
```

The print format addition is correct — `0x%02x` for a `u8` value, and the argument ordering matches the format string.

No issues with this patch.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v2 0/2] drm/xe/pagefault: Add SRCID to pagefault reporting

[Jonathan Cavitt]

Add SRCID to the xe_pagefault struct, which reports the ID of the
faulting hardware unit.  This will be passed on to the
xe_vm_get_property_ioctl for reading per-vm faults and will assist in
diagnosing pagefaults.

v2:
- Readd pad check, as the pad in the ioctl struct was not changed
  (jcavitt)

Jonathan Cavitt (2):
  drm/xe/pagefault: Add SRCID to pagefault struct
  drm/xe/vm: Add srcid to xe_vm_get_property_ioctl fault report

 drivers/gpu/drm/xe/xe_guc_pagefault.c   | 1 +
 drivers/gpu/drm/xe/xe_pagefault.c       | 6 ++++--
 drivers/gpu/drm/xe/xe_pagefault_types.h | 4 +++-
 drivers/gpu/drm/xe/xe_vm.c              | 8 ++++++++
 drivers/gpu/drm/xe/xe_vm_types.h        | 2 ++
 include/uapi/drm/xe_drm.h               | 4 ++--
 6 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] drm/xe/pagefault: Add SRCID to pagefault struct

[Jonathan Cavitt]

Add SRCID information to pagefault struct for the purpose of reporting
the hardware unit that resulted in the pagefault.

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
---
 drivers/gpu/drm/xe/xe_guc_pagefault.c   | 1 +
 drivers/gpu/drm/xe/xe_pagefault.c       | 6 ++++--
 drivers/gpu/drm/xe/xe_pagefault_types.h | 4 +++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_pagefault.c b/drivers/gpu/drm/xe/xe_guc_pagefault.c
index 607e32392f46..17cb7f359261 100644
--- a/drivers/gpu/drm/xe/xe_guc_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_guc_pagefault.c
@@ -91,6 +91,7 @@ int xe_guc_pagefault_handler(struct xe_guc *guc, u32 *msg, u32 len)
 				   FIELD_GET(PFD_FAULT_TYPE, msg[2]));
 	pf.consumer.engine_class = FIELD_GET(PFD_ENG_CLASS, msg[0]);
 	pf.consumer.engine_instance = FIELD_GET(PFD_ENG_INSTANCE, msg[0]);
+	pf.consumer.srcid = FIELD_GET(PFD_SRC_ID, msg[0]);
 
 	pf.producer.private = guc;
 	pf.producer.ops = &guc_pagefault_ops;
diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
index dd3c068e1a39..42b682ba5f9b 100644
--- a/drivers/gpu/drm/xe/xe_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_pagefault.c
@@ -248,7 +248,8 @@ static void xe_pagefault_print(struct xe_pagefault *pf)
 		   "\tAccessType: %lu\n"
 		   "\tFaultLevel: %lu\n"
 		   "\tEngineClass: %d %s\n"
-		   "\tEngineInstance: %d\n",
+		   "\tEngineInstance: %d\n"
+		   "\tSRCID: 0x%02x\n",
 		   pf->consumer.asid,
 		   upper_32_bits(pf->consumer.page_addr),
 		   lower_32_bits(pf->consumer.page_addr),
@@ -260,7 +261,8 @@ static void xe_pagefault_print(struct xe_pagefault *pf)
 			     pf->consumer.fault_type_level),
 		   pf->consumer.engine_class,
 		   xe_hw_engine_class_to_str(pf->consumer.engine_class),
-		   pf->consumer.engine_instance);
+		   pf->consumer.engine_instance,
+		   pf->consumer.srcid);
 }
 
 static void xe_pagefault_save_to_vm(struct xe_device *xe, struct xe_pagefault *pf)
diff --git a/drivers/gpu/drm/xe/xe_pagefault_types.h b/drivers/gpu/drm/xe/xe_pagefault_types.h
index c4ee625b93dd..60cc269aeea3 100644
--- a/drivers/gpu/drm/xe/xe_pagefault_types.h
+++ b/drivers/gpu/drm/xe/xe_pagefault_types.h
@@ -86,8 +86,10 @@ struct xe_pagefault {
 		u8 engine_class;
 		/** @consumer.engine_instance: engine instance */
 		u8 engine_instance;
+		/** @consumer.srcid: ID of hardware unit producing fault */
+		u8 srcid;
 		/** @consumer.reserved: reserved bits for future expansion */
-		u64 reserved;
+		u8 reserved[7];
 	} consumer;
 	/**
 	 * @producer: State for the producer (i.e., HW/FW interface). Populated
-- 
2.53.0

[PATCH v2 2/2] drm/xe/vm: Add srcid to xe_vm_get_property_ioctl fault report

[Jonathan Cavitt]

Add the SRCID of the faulting hardware unit to the return of the
xe_vm_get_property_ioctl fault report.

v2:
- Readd pad check, as the pad in the ioctl struct was not changed
  (jcavitt)

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/xe/xe_vm.c       | 8 ++++++++
 drivers/gpu/drm/xe/xe_vm_types.h | 2 ++
 include/uapi/drm/xe_drm.h        | 4 ++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 080c2fff0e95..7e084c05f10e 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -636,6 +636,7 @@ void xe_vm_add_fault_entry_pf(struct xe_vm *vm, struct xe_pagefault *pf)
 				  pf->consumer.fault_type_level);
 	e->fault_level = FIELD_GET(XE_PAGEFAULT_LEVEL_MASK,
 				   pf->consumer.fault_type_level);
+	e->srcid = pf->consumer.srcid;
 
 	list_add_tail(&e->list, &vm->faults.list);
 	vm->faults.len++;
@@ -4108,6 +4109,11 @@ static u8 xe_to_user_fault_level(u8 fault_level)
 	return fault_level;
 }
 
+static u8 xe_to_user_srcid(u8 srcid)
+{
+	return srcid;
+}
+
 static int fill_faults(struct xe_vm *vm,
 		       struct drm_xe_vm_get_property *args)
 {
@@ -4135,6 +4141,8 @@ static int fill_faults(struct xe_vm *vm,
 		fault_entry.fault_type = xe_to_user_fault_type(entry->fault_type);
 		fault_entry.fault_level = xe_to_user_fault_level(entry->fault_level);
 
+		fault_entry.srcid = xe_to_user_srcid(entry->srcid);
+
 		memcpy(&fault_list[i], &fault_entry, entry_size);
 
 		i++;
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 635ed29b9a69..e8aea75341cc 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -196,6 +196,7 @@ struct xe_device;
  * @access_type: type of address access that resulted in fault
  * @fault_type: type of fault reported
  * @fault_level: fault level of the fault
+ * @srcid: ID of the faulting hardware unit
  */
 struct xe_vm_fault_entry {
 	struct list_head list;
@@ -204,6 +205,7 @@ struct xe_vm_fault_entry {
 	u8 access_type;
 	u8 fault_type;
 	u8 fault_level;
+	u8 srcid;
 };
 
 struct xe_vm {
diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index 48e9f1fdb78d..2b45b691a032 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -1294,8 +1294,8 @@ struct xe_vm_fault {
 #define FAULT_LEVEL_PML4	3
 #define FAULT_LEVEL_PML5	4
 	__u8 fault_level;
-	/** @pad: MBZ */
-	__u8 pad;
+	/** @srcid: ID of the faulting hardware unit */
+	__u8 srcid;
 	/** @reserved: MBZ */
 	__u64 reserved[4];
 };
-- 
2.53.0

Claude review: drm/xe/pagefault: Add SRCID to pagefault reporting

[Claude Code Review Bot]

Overall Series Review

Subject: drm/xe/pagefault: Add SRCID to pagefault reporting
Author: Jonathan Cavitt <jonathan.cavitt@intel.com>
Patches: 3
Reviewed: 2026-06-04T11:40:32.542120

---

This is a clean, small 2-patch series that adds the SRCID (source ID of the faulting hardware unit) to the xe pagefault reporting path. The data flows correctly from the GuC G2H message through the internal pagefault struct, into the per-VM fault entry, and out to userspace via the `xe_vm_get_property_ioctl`. The UAPI change reuses the existing `pad` byte in `struct xe_vm_fault`, which is correct since userspace was already required to pass it as zero (MBZ).

The series is straightforward and the implementation is consistent with the existing patterns for the other fault fields (access_type, fault_type, fault_level). One minor concern and one nit below.

**Verdict: Looks good with minor comments.**

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/xe/pagefault: Add SRCID to pagefault struct

[Claude Code Review Bot]

Patch Review

**Correctness: Good.**

The SRCID is extracted from `msg[0]` using `PFD_SRC_ID` which is `GENMASK(10, 3)` — an 8-bit field, so storing it in a `u8` is correct.

The `reserved` field change from `u64` to `u8 reserved[7]` preserves the struct size. The original consumer sub-struct layout was:
- `u64 page_addr` (8) + `u32 asid` (4) + `u8 access_type` (1) + `u8 fault_type_level` (1) + `u8 engine_class` (1) + `u8 engine_instance` (1) + `u64 reserved` (8) = 24 bytes

After the patch:
- Same prefix (16) + `u8 srcid` (1) + `u8 reserved[7]` (7) = 24 bytes

Size is preserved, maintaining the 64-byte struct invariant mentioned in the comment on `struct xe_pagefault`.

**Minor concern:** The `pf` in `xe_guc_pagefault_handler` is stack-allocated and not zero-initialized. The `reserved` bytes in the consumer struct are not explicitly zeroed. While the current consumer side doesn't inspect `reserved`, if a future reader relies on it being zero, this could be a latent issue. The existing code has the same pattern for the old `u64 reserved` though, so this is pre-existing.

The debug print addition is fine — `0x%02x` is appropriate for the u8 srcid value.

No issues with this patch.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/xe/vm: Add srcid to xe_vm_get_property_ioctl fault report

[Claude Code Review Bot]

Patch Review

**UAPI change:** The `__u8 pad` in `struct xe_vm_fault` is renamed to `__u8 srcid`. Since `pad` was MBZ and occupied the same offset, this is ABI-compatible — old userspace that set pad=0 will just read srcid=0 (or whatever value is there), and old kernels receiving new userspace won't break because the pad check was on the `drm_xe_vm_get_property` struct, not on the `xe_vm_fault` struct itself.

Looking at `xe_vm.c:4200-4202`, the pad check is on `args->pad` (the `drm_xe_vm_get_property` struct's pad, not `xe_vm_fault`'s pad), so this is indeed unaffected — the cover letter's "Readd pad check" note correctly identifies that the `drm_xe_vm_get_property.pad` check should remain.

**Nit:** `xe_to_user_srcid()` is a trivial identity function:
```c
static u8 xe_to_user_srcid(u8 srcid)
{
	return srcid;
}
```
This follows the pattern of `xe_to_user_fault_type()` and `xe_to_user_fault_level()` which are also identity functions. I understand these exist as placeholders for potential future remapping (similar to the XXX comment about enum matching in `xe_guc_pagefault.c:74-76`), so this is consistent with the codebase style.

The `xe_vm_fault_entry` struct addition and the `xe_vm_add_fault_entry_pf` plumbing are both correct and straightforward.

No blocking issues with this patch.

---
Generated by Claude Code Patch Reviewer