Re: [PATCH 6/6] drm/xe: capture L3 node status registers in devcoredump

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Capturing MCR registers in the devcoredump snapshot triggers a kernel assertion failure (BUG/WARN) when executed in an SR-IOV Virtual Function (VF) environment.
- [Medium] MCR registers are incorrectly defined as regular `XE_REG` instead of `XE_REG_MCR`, bypassing the subsystem's compile-time type safety.
--

--- Patch [6]: [PATCH 6/6] drm/xe: capture L3 node status registers in devcoredump ---
commit a45fb5f7ed9065191b925af139e27c7fca72cb39
Author: Nareshkumar Gollakoti <[email protected]>

drm/xe: capture L3 node status registers in devcoredump

This commit adds L3_NODE_IDLE and L3STAT_LBCF_GFX register definitions
and includes them in the devcoredump node status section. It also
extends the register descriptor to mark MCR registers, allowing the
snapshot code to read them using xe_gt_mcr_unicast_read_any().

> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 8c55d766a5a63..fdd0817919617 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -430,6 +430,9 @@
>  #define   L3_ESC_MASK				REG_BIT(0)
>  #define   L3_ESC(value)				REG_FIELD_PREP(L3_ESC_MASK, value)
>  
> +#define L3_NODE_IDLE				XE_REG(0xb0b0)
> +#define L3STAT_LBCF_GFX				XE_REG(0xb128)
> +

[Severity: Medium]
Should these registers be defined using XE_REG_MCR instead of XE_REG?

Defining them with XE_REG bypasses the compile-time type safety intended
for multicast/replicated registers.

>  #define XEHP_L3NODEARBCFG			XE_REG_MCR(0xb0b4)
>  #define   XEHP_LNESPARE				REG_BIT(19)

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index 6cc4e3969417e..ab8cd7d589646 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c

[ ... ]

> @@ -83,6 +84,8 @@ struct xe_reg_desc {
>  	struct xe_reg reg;
>  	/** @name: Name of the register */
>  	const char *name;
> +	/** @mcr: mcr type registers */
> +	bool mcr;
>  };

[Severity: Medium]
Does adding this boolean flag subvert the existing type safety architecture?

The struct xe_reg already contains an mcr bitfield which is automatically
populated when registers are correctly defined with XE_REG_MCR. Adding a
separate flag forces the code to manually cast the address back into an MCR
register at runtime, losing the intended compiler type checks.

[ ... ]

> @@ -156,7 +164,12 @@ xe_capture_reg_desc_list(struct xe_gt *gt,
>  
>  	for (i = 0; i < count; i++) {
>  		dst[i].reg = src[i].reg;
> -		dst[i].value = xe_mmio_read32(&gt->mmio, src[i].reg);
> +		if (src[i].mcr)
> +			dst[i].value = xe_gt_mcr_unicast_read_any(gt,
> +								  XE_REG_MCR
> +								  (src[i].reg.addr));
> +		else
> +			dst[i].value = xe_mmio_read32(&gt->mmio, src[i].reg);
>  		dst[i].name = src[i].name;
>  	}
>  }

[Severity: High]
Can this path trigger a kernel assertion failure in an SR-IOV Virtual Function
environment?

Looking at xe_gt_mcr_unicast_read_any, it contains an explicit guard against
execution in a VF:

xe_gt_mcr_unicast_read_any(struct xe_gt *gt, struct xe_reg_mcr reg_mcr)
{
    ...
    xe_gt_assert(gt, !IS_SRIOV_VF(gt_to_xe(gt)));
    ...
}

Because devcoredumps can be triggered by unprivileged VF workloads causing a
GPU hang, executing this read could lead to unauthorized attempts to access
privileged steering registers, potentially causing MMIO timeouts and undefined
driver behavior.

[ ... ]

> @@ -266,6 +279,19 @@ static struct xe_dbg_reg_snapshot *xe_gam_pf_report_capture(struct xe_gt *gt)
>  	return xe_dbg_reg_snapshot_capture(gt, &reg_list);
>  }
>  
> +static struct xe_dbg_reg_snapshot *xe_node_status_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_l3_node_reglist;
> +	reg_list.num_regs = ARRAY_SIZE(xe3p_l3_node_reglist);
> +
> +	return xe_dbg_reg_snapshot_capture(gt, &reg_list);
> +}

[Severity: High]
Should this capture function include an early return guard for VFs?

Since capturing MCR registers is a PF-only operation, omitting an
IS_SRIOV_VF check here allows the snapshot code to unconditionally attempt
MCR reads, leading to the assertion failure mentioned above.

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