Re: [PATCH v13 11/16] drm/xe/vsec: Crescent Island PMT callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe/vsec: xe_pmt_telem_read() leaves CRI discovery window remapped
- [Low] drm/xe/vsec: mixed goto and scoped_guard() cleanup in reg callbacks
--

commit dc4cd399324f5200a61e70ed93f8ba5b2a90435c
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Crescent Island PMT callbacks

This commit introduces Crescent Island PMT register read and write callbacks
to access the discovery status and control areas via the indexed MMIO window
and registers the CRI PMT functionality.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index a814d7151eeae..0f6e3be520c09 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -358,20 +358,120 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  	return ret == 0 ? count : ret;
>  }
>  
> -static struct pmt_callbacks xe_pmt_cb = {
> +/**
> + * xe_pmt_read_reg() - read a crashlog register
> + * @dev: the xe device that registered the callback
> + * @guid: PMT guid of the crashlog instance
> + * @reg: data read from the PMT data structure
> + * @offset: which data to read from the PMT data structure
> + *
> + * Read the requested PMT register based on the pcie device and guid.  The
> + * supported struct is the Crashlog Type1 Version2.
> + *
> + * Currently this is for CRI only.
> + *
> + * Return:
> + * * -ENODEV device is not available (hotplug)
> + * * -EINVAL invalid guid
> + * * 0       success
> + */
> +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 + xe->pmt.base_offset;
> +	int ret = 0;
> +	u32 inst;
> +	int idx;
> +
> +	if (!drm_dev_enter(&xe->drm, &idx))
> +		return -ENODEV;
> +
> +	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
> +	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG) {
> +		ret = -EINVAL;
> +		goto dev_exit;
> +	}
> +
> +	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
> +		CRI_CRASHLOG_PUNIT_DISC_OFFSET : CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
> +	disc_addr += inst + offset;
> +
> +	xe_pm_runtime_get(xe);
> +
> +	scoped_guard(mutex, &xe->pmt.lock) {
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +		*reg = readl(disc_addr);
> +	}
> +
> +	xe_pm_runtime_put(xe);
> +
> +dev_exit:
> +	drm_dev_exit(idx);
> +
> +	return ret;
> +}
> +
> +static int xe_pmt_write_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 + xe->pmt.base_offset;
> +	int ret = 0;
> +	u32 inst;
> +	int idx;
> +
> +	if (!drm_dev_enter(&xe->drm, &idx))
> +		return -ENODEV;
> +
> +	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
> +	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG) {
> +		ret = -EINVAL;
> +		goto dev_exit;
> +	}
> +
> +	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
> +		CRI_CRASHLOG_PUNIT_DISC_OFFSET : CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
> +	disc_addr += inst + offset;
> +
> +	xe_pm_runtime_get(xe);
> +
> +	scoped_guard(mutex, &xe->pmt.lock) {
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +		writel(reg, disc_addr);
> +	}
> +
> +	xe_pm_runtime_put(xe);
> +
> +dev_exit:
> +	drm_dev_exit(idx);
> +
> +	return ret;
> +}

[Severity: Low]
As reported by Sashiko on v11 [1], this isn't a bug, but both
xe_pmt_read_reg() and xe_pmt_write_reg() in drivers/gpu/drm/xe/xe_vsec.c mix
goto-based cleanup (goto dev_exit) with scope-based cleanup
(scoped_guard(mutex, &xe->pmt.lock)) in the same function, and duplicate
nearly identical bodies differing only in readl() vs writel().

Since the guid validation only inspects the u32 guid argument, could that
check be performed before drm_dev_enter() to avoid the dev_exit label (or
convert the remaining cleanup in these functions to a consistent pattern),
and could the shared setup between xe_pmt_read_reg() and xe_pmt_write_reg()
be factored into a common helper?

[ ... ]

> @@ -399,8 +499,15 @@ void xe_vsec_init(struct xe_device *xe)
>  	switch (platform) {
>  	case XE_VSEC_BMG:
>  		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:
> +		xe->pmt.base_offset = CRI_PMT_OFFSET;
> +		info->priv_data = &xe_cri_pmt_cb;
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +		break;

[Severity: Medium]
Can a call to xe_pmt_telem_read() leave the shared CRI_PMT_OFFSET MMIO window
banked to a telemetry or crashlog payload region instead of
CRI_IDX_TELEM_DISCOVERY when PMT discovery headers are read during probe?

On Crescent Island, both the discovery tables and the telemetry/crashlog data
banks share the CRI_PMT_OFFSET window controlled by set_telem_region().
During auxiliary driver probe in pmt_telem_probe() and pmt_crashlog_probe(),
the Intel PMT drivers read the discovery headers directly from the ioremapped
CRI_PMT_OFFSET window without calling read_reg() or holding xe->pmt.lock:

drivers/platform/x86/intel/pmt/class.c:pmt_get_headers() {
    ...
	memset(entry->disc_header, 0, header_bytes);
	memcpy_fromio(entry->disc_header, disc_table,
		      min(header_bytes, resource_size(disc_res)));

	/* Used by crashlog driver */
	entry->disc_table = disc_table;
    ...
}

and:

drivers/platform/x86/intel/pmt/crashlog.c:pmt_crashlog_header_decode() {
    ...
	header->access_type = GET_ACCESS(readl(disc_table));
	header->guid = readl(disc_table + GUID_OFFSET);
	header->base_offset = readl(disc_table + BASE_OFFSET);

	/* Size is measured in DWORDS, but accessor returns bytes */
	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
    ...
}

However, in drivers/gpu/drm/xe/xe_vsec.c, xe_pmt_telem_read() switches the
remapper index to mem_region and does not restore CRI_IDX_TELEM_DISCOVERY
before releasing xe->pmt.lock:

	scoped_guard(mutex, &xe->pmt.lock) {
		/* set SoC re-mapper index register based on GUID memory region */
		xe->soc_remapper.set_telem_region(xe, mem_region);

		memcpy_fromio(data, telem_addr, count);
	}

In addition, xe_vsec_init() sets CRI_IDX_TELEM_DISCOVERY without holding
xe->pmt.lock.

If pmt_telemetry or pmt_crashlog probes asynchronously as a module or is
rebound via sysfs after any xe_pmt_telem_read() call (such as from debugfs
residency reads or an earlier telemetry sysfs read), wouldn't
pmt_get_headers() and pmt_crashlog_header_decode() read telemetry or crashlog
payload data instead of the discovery headers?

[1] https://lore.kernel.org/r/20260923181115.2514193-38-michael.j.ruhl@intel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102221314.119028-18-michael.j.ruhl@intel.com?part=11