Re: [PATCH v3 08/10] drm/xe/vsec: Crescent Island PMT callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The CRI PMT functionality and VSEC registration lack an explicit IS_SRIOV_VF() guard, allowing Virtual Functions to access PF-only SOC_BASE MMIO registers.
--

--- Patch [8]: [PATCH v3 08/10] drm/xe/vsec: Crescent Island PMT callbacks ---
commit 20efb72ccb405a03098f93ae5977f349cb941f0a
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Crescent Island PMT callbacks

This commit introduces necessary callbacks to read and write status and
control information for Crescent Island (CRI) PMT usage. It also adds the
logic to register the CRI PMT functionality.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c

[ ... ]

> +static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
> +{
> +	struct xe_device *xe = kdev_to_xe_device(dev);
> +	void __iomem *disc_addr = xe->mmio.regs;

[ ... ]

> +	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
> +		CRI_CRASHLOG_PUNIT_DISC_OFFSET : CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
> +	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
> +
> +	xe_pm_runtime_get(xe);

[Severity: High]
Can a Virtual Function (VF) trigger this access?

It looks like disc_addr calculates a SOC_BASE MMIO region offset using
CRI_DISCOVERY_OFFSET, which is typically restricted to Physical Functions
(PFs). 

Accessing PF-only hardware resources from a VF context can cause MMIO
timeouts and hardware failures. 

[ ... ]

> @@ -375,8 +478,17 @@ void xe_vsec_init(struct xe_device *xe)
>  		if (!xe->soc_remapper.set_telem_region)
>  			return;
>  		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
> -		info->priv_data = &xe_pmt_cb;
> +		info->priv_data = &xe_bmg_pmt_cb;
> +		break;
> +
> +	case XE_VSEC_CRI:
> +		if (!xe->soc_remapper.set_telem_region)
> +			return;
> +		xe->pmt.base_offset = CRI_TELEMETRY_OFFSET;
> +		info->priv_data = &xe_cri_pmt_cb;
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>  		break;

[Severity: High]
Should this platform registration block be guarded by IS_SRIOV_VF()?

Since the newly added callbacks interact directly with SOC_BASE MMIO regions,
checking for IS_SRIOV_VF(xe) and returning early could prevent VFs from
registering the PMT functionality and attempting to access restricted hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824162317.2450380-12-michael.j.ruhl@intel.com?part=8