Re: [PATCH v3 03/10] drm/xe/vsec: Protect against missing config

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Aug 24, 2026 at 04:36:35PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Subsystem guidelines violation: The patch uses the absence of `soc_remapper.set_telem_region` as a proxy check to prevent execution on Virtual Functions (VF) instead of an explicit `IS_SRIOV_VF()` guard.

Sashiko has a point here.

If the goal is to check for VF as commit message states, the IS_SRIOV_VF is the
right check.

If we need to really ensure the set_telem_region is there we need to add
a function in the xe_soc_remmaper component, rather than traversing the
component and checking it internals, what is a xe component violation.

Please create a helper with a better commit message or use the IS_SRIOV_VF
macro.

> 
> Pre-existing issues:
> - [High] The `xe_pmt_cb` struct is missing the `.read_reg` and `.write_reg` callbacks required for safe PMT Crashlog control register access on discrete GPUs.
> --
> 
> --- Patch [3]: [PATCH v3 03/10] drm/xe/vsec: Protect against missing config ---
> commit 6d90522f74020b6e17f1f98588ed1740977adfb4
> Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
> 
> drm/xe/vsec: Protect against missing config
> 
> This commit prevents PMT registration and telemetry read access when the
> soc-remapper callbacks are not configured, specifically targeting Virtual
> Function (VF) init paths.
> 
> > 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
> [ ... ]
> > @@ -210,6 +210,8 @@ void xe_vsec_init(struct xe_device *xe)
> >  
> >  	switch (platform) {
> >  	case XE_VSEC_BMG:
> > +		if (!xe->soc_remapper.set_telem_region)
> > +			return;
> 
> [Severity: Medium]
> The commit message mentions that some init paths (VF) will not provide
> soc-remapper callbacks. Is it safe to use this feature presence as a proxy
> check to prevent execution on Virtual Functions?
> 
> According to the SR-IOV guidelines for the xe driver, VFs must be explicitly
> blocked from PF-only resource registration using an IS_SRIOV_VF() guard to
> prevent hardware failures and MMIO timeouts.
> 
> Should this code explicitly check for the VF context instead of relying on
> the absence of the set_telem_region callback?
> 
> >  		info->priv_data = &xe_pmt_cb;
> 
> [Severity: High]
> This is a pre-existing issue, but does assigning xe_pmt_cb here expose us to
> unprotected MMIO accesses during suspend?
> 
> Since BMG exposes VSEC_CAP_CRASHLOG in xe_vsec_info, the crashlog driver
> will probe and create sysfs entries. However, the xe_pmt_cb structure lacks
> the .read_reg and .write_reg callbacks.
> 
> When userspace accesses crashlog sysfs attributes, the driver will fall back
> to using direct readl()/writel() on entry->disc_table.
> 
> If the GPU is in D3cold, will these direct accesses bypass runtime PM checks
> and attempt to read or write powered-off MMIO, potentially triggering MCE or
> AER kernel panics?
> 
> >  		break;
> >  	default:
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260824162317.2450380-12-michael.j.ruhl@intel.com?part=3