Re: [PATCH v6 12/18] drm/xe/vsec: Refactor BattleMage PMT defines

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Uninitialized `xe->pmt.base_offset` causes `xe_pmt_telem_read()` to read from the wrong MMIO address when accessed before `xe_vsec_init()` completes.

Pre-existing issues:
- [High] Crashlog control sysfs attributes perform direct MMIO reads/writes on Xe without waking the device, risking hardware hangs.
--

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

drm/xe/vsec: Refactor BattleMage PMT defines

This commit refactors PMT definitions for Battlemage and introduces a
configurable base offset to support multiple platforms. It modifies the
PMT telemetry read path to use the new offset rather than a hardcoded macro.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index a9baf0bfe572c..031c1c0ae5706 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -140,37 +201,79 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
[ ... ]
>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset,
>  		      u32 count)
>  {
>  	struct xe_device *xe = kdev_to_xe_device(dev);
> -	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
> +	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
> +	void __iomem *telem_addr = xe->mmio.regs + xe->pmt.base_offset;

[Severity: High]
Does this read from the wrong MMIO address during early probe?

During device initialization in xe_device_probe(), xe_hwmon_register() is
called before xe_vsec_init(). The hwmon registration synchronously reads the
initial energy value through this call chain:

xe_hwmon_register()
  xe_hwmon_energy_is_visible()
    xe_hwmon_energy_get()
      xe_pmt_telem_read()

Since xe->pmt.base_offset is not initialized until xe_vsec_init() is called
later in the probe sequence, it will still be 0 here. This would cause an
out-of-bounds MMIO read (xe->mmio.regs + 0 + offset) that misses the
SOC_BASE offset entirely, potentially triggering a machine check exception
or hanging the system.

[ ... ]
> @@ -210,6 +313,9 @@ void xe_vsec_init(struct xe_device *xe)
>  
>  	switch (platform) {
>  	case XE_VSEC_BMG:
> +		if (IS_SRIOV_VF(xe))
> +			return;
> +		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
>  		info->priv_data = &xe_pmt_cb;
>  		break;

[Severity: High]
This is a pre-existing issue, but does the lack of .read_reg and .write_reg
callbacks in xe_pmt_cb risk unclocked MMIO accesses?

The crashlog.c driver implements sysfs attributes (e.g., clear, trigger)
that read and write PMT crashlog control registers. If the parent driver
doesn't provide .read_reg and .write_reg callbacks, pmt_crashlog_read_reg()
and pmt_crashlog_rmw() fall back to direct readl() and writel() on the
mapped MMIO region (entry->disc_table).

Since xe_pmt_cb only provides .read_telem, reading or writing these sysfs
attributes while the Xe device is suspended (D3cold) will trigger unclocked
MMIO accesses without waking the device via runtime PM. This can cause PCIe
Unsupported Request faults or system hangs.

Could .read_reg and .write_reg be implemented for Battlemage to ensure the
device is woken up during these accesses?

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