Re: [CI 1/2] drm/xe/wopcm: Use the size from HW when excluding the wopcm range from GGTT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/xe: guc_bo_ggtt_addr() asserts against hardcoded
  xe_wopcm_size()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] drm/xe: out-of-bounds write in xe_ggtt_set_pte() [1]
--

commit 4a9c3b5b3550bed91fd0e4d956401869552dfe0a
Author: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

drm/xe/wopcm: Use the size from HW when excluding the wopcm range from GGTT

This commit reads the per-tile WOPCM size from the STOLEN_RESERVED
register during tile initialization and uses it to set the GGTT start
offset and stolen memory size instead of relying on hardcoded values.

> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 9ef2ce743770f..cd8b9000046c9 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c

[ ... ]

> @@ -394,11 +394,13 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  {
>  	struct xe_device *xe = tile_to_xe(ggtt->tile);
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +	u64 ggtt_start, ggtt_size;
>  	unsigned int gsm_size;
> -	u64 ggtt_start, wopcm = xe_wopcm_size(xe), ggtt_size;
>  	int err;
>  
>  	if (!IS_SRIOV_VF(xe)) {
> +		u64 wopcm = xe_tile_wopcm_size(ggtt->tile);
> +
>  		if (GRAPHICS_VERx100(xe) >= 1250)
>  			gsm_size = SZ_8M; /* GGTT is expected to be 4GiB */
>  		else

[Severity: Medium]
Does guc_bo_ggtt_addr() also need to be updated to check against
xe_tile_wopcm_size()?

drivers/gpu/drm/xe/xe_guc.c:guc_bo_ggtt_addr() {
    ...
	addr = __xe_bo_ggtt_addr(bo, gt_to_tile(guc_to_gt(guc))->id);

	/* GuC addresses above GUC_GGTT_TOP don't map through the GTT */
	xe_assert(xe, addr >= xe_wopcm_size(guc_to_xe(guc)));
    ...
}

With ggtt_start now set to xe_tile_wopcm_size(ggtt->tile), early GuC BOs
such as guc->log.bo can be placed below the hardcoded xe_wopcm_size(xe)
whenever hardware reports a smaller WOPCM in STOLEN_RESERVED (for example,
2 MiB on DG1/DG2 where xe_wopcm_size() returns 4 MiB), triggering a false
xe_assert() failure during GuC initialization.  Conversely, when the
hardware WOPCM is larger than xe_wopcm_size(xe), the assertion checks
against a stale lower bound.

[ ... ]

> @@ -647,13 +648,15 @@ static void xe_ggtt_invalidate_engine(struct xe_ggtt *ggtt)
>   * This function may be called multiple times during recovery, but if
>   * @new_start is unchanged from the current base, it's a noop.
>   *
> - * @new_start should be a value between xe_wopcm_size() and #GUC_GGTT_TOP.
> + * @new_start should be a value above the wopcm size and below #GUC_GGTT_TOP.
> + * VFs can't access the register with the wopcm size info, so we trust that PF
> + * has done the correct programming for that and we just assert on the upper
> + * bound.
>   */
>  void xe_ggtt_shift_nodes(struct xe_ggtt *ggtt, u64 new_start)
>  {
>  	guard(mutex)(&ggtt->lock);
>  
> -	xe_tile_assert(ggtt->tile, new_start >= xe_wopcm_size(tile_to_xe(ggtt->tile)));
>  	xe_tile_assert(ggtt->tile, new_start + ggtt->size <= GUC_GGTT_TOP);
>  
>  	/* pairs with READ_ONCE in xe_ggtt_node_addr() */

[Severity: Critical]
This problem wasn't introduced by this patch, but can a large new_start
from the PF cause an out-of-bounds write in xe_ggtt_set_pte() [1]?

vf_get_ggtt_info() queries start from GUC_KLV_VF_CFG_GGTT_START_KEY and
passes it directly to xe_ggtt_shift_nodes().  In non-debug builds where
xe_tile_assert() compiles out, an out-of-range new_start is stored into
ggtt->start unchecked, causing xe_ggtt_set_pte() to index past the end of
ggtt->gsm[] when writing PTEs.

[1] https://sashiko.dev/#/bug/linux-eb71dbae-ad75-4245-85dd-767544ad0c44

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113191739.1447747-3-daniele.ceraolospurio@intel.com?part=1