Re: [PATCH 1/6] drm/xe/devcoredump: Capture GT fuse registers in devcoredump

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect GT context passed to fuse capture functions causes MMIO offset corruption and missing forcewake.
- [Medium] Attempting to read PF-only physical fuse registers from a Virtual Function (VF) causes kernel warnings.
- [Medium] Unconditional capture of Media GT fuse registers without verifying if the hardware actually possesses a Media GT.
--

--- Patch [1]: [PATCH 1/6] drm/xe/devcoredump: Capture GT fuse registers in devcoredump ---
commit 83b02d67e48ca9a916843ba3c9656f4f78b755ae
Author: Nareshkumar Gollakoti <[email protected]>
drm/xe/devcoredump: Capture GT fuse registers in devcoredump

This commit captures fuse-related GT register state in the Xe devcoredump to
preserve additional hardware configuration information. It adds generic
register snapshot storage and helpers for capturing and printing MMIO dumps.

> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index 5f2b90b18f97b..35d8101f2a192 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
[ ... ]
> +static struct xe_dbg_reg_snapshot *xe_gt_fuse_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;
> +
> +	reg_list.regs = xe3p_gt_fuse_reglist;
> +	reg_list.num_regs = ARRAY_SIZE(xe3p_gt_fuse_reglist);
> +
> +	return xe_dbg_reg_snapshot_capture(gt, &reg_list);
> +}

[Severity: Medium]
Will this trigger kernel warnings when running as a Virtual Function (VF)?

If a GPU hang occurs in an SR-IOV VF environment,
xe_gt_fuse_snapshot_capture() calls xe_mmio_read32(), which routes to
xe_gt_sriov_vf_read32(). Since physical fuse registers like RPM_GCD are
PF-only and not on the VF MMIO whitelist, the access is rejected,
potentially polluting the kernel log with warnings for every register.

Should there be an IS_SRIOV_VF() guard before attempting to read PF-only
physical fuse registers?

[ ... ]
> @@ -349,6 +492,9 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>  
>  	cookie = dma_fence_begin_signalling();
>  
> +	ss->gt_fuse = xe_gt_fuse_snapshot_capture(q->gt);
> +	ss->media_fuse = xe_media_gt_fuse_snapshot_capture(q->gt);

[Severity: High]
Does this pass the wrong GT context to the capture functions when the fault is
on the Media GT?

The faulting q->gt is passed unconditionally to both captures. xe_mmio_read32()
automatically adds a 0x380000 offset to register addresses when invoked on the
Media GT. If q->gt is the Media GT, xe_gt_fuse_snapshot_capture() will
erroneously capture Media GT registers instead of Primary GT registers.

Additionally, devcoredump only holds forcewake for the faulting q->gt. If
q->gt is the Primary GT, xe_media_gt_fuse_snapshot_capture() will attempt
to read Media GT registers while the Media GT is asleep, causing Unclaimed
Register errors and returning garbage data.

Should it capture using the specific primary and media GTs with appropriate
forcewake?

[Severity: Medium]
What happens on hardware configurations that do not possess a Media GT?

The unconditional call to xe_media_gt_fuse_snapshot_capture() attempts to
read registers at offset 0x380000. If the Media GT is physically absent or
fused off on the SKU, would this result in Unclaimed Register hardware errors
and system log spam?

Should there be a check for the existence of a Media GT before attempting
the capture?

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

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