Re: [PATCH 2/6] drm/xe/devcoredump: Add GuC register snapshot to devcoredump

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Devcoredump captures GuC registers using the wrong GT's MMIO and forcewake context, leading to duplicated media GT captures, missing primary GT captures, and potential hardware timeouts.
- [High] Unconditional reading of restricted GuC registers during devcoredump capture can cause MMIO timeouts or hardware failures on SR-IOV Virtual Functions.
- [High] Devcoredump fails to capture the primary GT fuse registers when a hang occurs on the media GT, and accesses media fuses without forcewake when a hang occurs on the primary GT.
--

--- Patch [2]: [PATCH 2/6] drm/xe/devcoredump: Add GuC register snapshot to devcoredump ---
commit ec31bf15e0d277f5f7b46d37e62dd0788024eecc
Author: Nareshkumar Gollakoti <[email protected]>

drm/xe/devcoredump: Add GuC register snapshot to devcoredump

Add support for capturing GuC register state as part of the xe
devcoredump snapshot.

> diff --git a/drivers/gpu/drm/xe/regs/xe_guc_regs.h b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
> --- a/drivers/gpu/drm/xe/regs/xe_guc_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
> @@ -150,6 +150,19 @@
>  #define GUC_INTR_SW_INT_1			REG_BIT(1)
>  #define GUC_INTR_SW_INT_0			REG_BIT(0)
>  
> +#define GUC_DEVICEID				XE_REG(0xc008)
> +#define GUC_SHIM_ERR_TRAP			XE_REG(0xc070)
> +#define GUC_HW_FATL_ERR				XE_REG(0xc598)
> +#define GUC_HW_NOTIFY_ERR			XE_REG(0xc59c)
> +
> +#define GUC_STATUS_M				XE_REG(0x38c000)

[Severity: High]
By defining hardcoded media GT register addresses like GUC_STATUS_M above
the adj_limit, won't they bypass MMIO adjustment? This could cause them
to behave differently than primary registers when passed a media GT.

> +#define GUC_WOPCM_SIZE_M			XE_REG(0x38c050)
> +#define GUC_DEVICEID_M				XE_REG(0x38c008)
> +#define DMA_CTRL_M				XE_REG(0x38c314)
> +#define GUC_HW_FATL_ERR_M			XE_REG(0x38c598)
> +#define GUC_HW_NOTIFY_ERR_M			XE_REG(0x38c59c)
> +#define GUC_SHIM_ERR_TRAP_M			XE_REG(0x38c070)
> +

> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
[ ... ]
> @@ -201,6 +222,32 @@ static struct xe_dbg_reg_snapshot *xe_media_gt_fuse_snapshot_capture(struct xe_g
>  	return xe_dbg_reg_snapshot_capture(gt, &reg_list);
>  }
>  
> +static struct xe_dbg_reg_snapshot *xe_gt_guc_reg_snapshot_capture(struct xe_gt *gt)
> +{
> +	struct xe_reg_desc_list reg_list;
> +
> +	if (!xe_dbg_reg_snapshot_is_supported(gt_to_xe(gt)))
> +		return NULL;

[Severity: High]
Does this code need an IS_SRIOV_VF() check before capturing GuC registers?

According to the driver pattern for other hardware controllers, accessing
PF-only hardware resources from an SR-IOV Virtual Function causes MMIO
timeouts. If a GPU hang on an SR-IOV VF triggers devcoredump_snapshot(),
this unconditional read might cause a timeout.

> +
> +	reg_list.regs = xe_gt_guc_reg_list;
> +	reg_list.num_regs = ARRAY_SIZE(xe_gt_guc_reg_list);
> +
> +	return xe_dbg_reg_snapshot_capture(gt, &reg_list);
> +}
> +
> +static struct xe_dbg_reg_snapshot *xe_media_gt_guc_reg_snapshot_capture(struct xe_gt *gt)
> +{
> +	struct xe_reg_desc_list reg_list;
> +
> +	if (!xe_dbg_reg_snapshot_is_supported(gt_to_xe(gt)))
> +		return NULL;

[Severity: High]
Does the media GT GuC capture also need a VF check to prevent MMIO timeouts
when running as a guest VM user?

> +
> +	reg_list.regs = xe_media_gt_guc_reg_list;
> +	reg_list.num_regs = ARRAY_SIZE(xe_media_gt_guc_reg_list);
> +
> +	return xe_dbg_reg_snapshot_capture(gt, &reg_list);
> +}
[ ... ]
> @@ -495,6 +553,8 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>  	ss->gt_fuse = xe_gt_fuse_snapshot_capture(q->gt);
>  	ss->media_fuse = xe_media_gt_fuse_snapshot_capture(q->gt);

[Severity: High]
This isn't a bug introduced by this patch, but does capturing both primary
and media GT fuses using the same faulting queue GT (q->gt) cause MMIO
aliasing and missing forcewake?

When the hang is on the primary GT, reading media fuses could happen without
holding the media GT's forcewake domain, risking an MMIO timeout. When on
the media GT, primary fuse reads might be translated by the media GT's
adj_offset, failing to capture the primary GT fuses.

> +
> +	ss->gt_guc_reg = xe_gt_guc_reg_snapshot_capture(q->gt);
> +	ss->media_gt_guc_reg = xe_media_gt_guc_reg_snapshot_capture(q->gt);

[Severity: High]
Will passing the faulting queue's GT (q->gt) to both primary and media GuC
captures cause similar MMIO aliasing and missing forcewake issues?

If the hang is on the primary GT, reading media GuC registers could occur
without acquiring the media GT's forcewake domain, which may trigger MMIO
timeouts. If the hang is on the media GT, reading primary GuC registers
would be shifted by the media GT's adj_offset (0x380000), causing it to
erroneously read the media GuC registers again and completely lose the
primary GuC state.

>  	ss->guc.log = xe_guc_log_snapshot_capture(&guc->log, true);
>  	ss->guc.ct = xe_guc_ct_snapshot_capture(&guc->ct);
>  	ss->ge = xe_guc_exec_queue_snapshot_capture(q);

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