Re: [PATCH v8 3/5] drm/xe/ggtt: Split GGTT into usable and shareable pools

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/xe/ggtt: top-down BO placement in __xe_ggtt_insert_bo_at()
  overflows BAR2
- [Medium] drm/xe/ggtt: VF GGTT provisioning in pf_provision_vf_ggtt()
  uses usable pool
--

commit f8827204c76aa40b1a2f516ba70dd829ec78e86b
Author: Piotr Piórkowski <piotr.piorkowski@intel.com>

drm/xe/ggtt: Split GGTT into usable and shareable pools

This commit splits the GGTT into usable and shareable pools for driver-owned
allocations and SR-IOV VF provisioning, adding xe_ggtt_insert_node_shareable()
and adjusting insertion direction based on whether a shareable pool is
configured.

> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 73c45eecae2be..607578e4080e2 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c

[ ... ]

> @@ -447,7 +476,8 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  	if (!ggtt->wq)
>  		return -ENOMEM;
>  
> -	__xe_ggtt_init_early(ggtt, ggtt_start, ggtt_size);
> +	ggtt_init_ranges(ggtt, ggtt_start, ggtt_size, ggtt_size,
> +			 IS_SRIOV_PF(xe) ? ggtt_size : 0);

[ ... ]

> +/*
> + * When the shareable range is present, allocations start from the bottom
> + * to leave space at the top; otherwise they start from the top.
> + */
> +static u32 ggtt_usable_insert_flags(struct xe_ggtt *ggtt)
> +{
> +#ifdef CONFIG_PCI_IOV
> +	if (ggtt->shareable_size > 0)
> +		return DRM_MM_INSERT_LOW;
> +#endif
> +	return DRM_MM_INSERT_HIGH;
> +}
> +
> +static int xe_ggtt_insert_node_locked(struct xe_ggtt_node *node, u32 size, u32 align)
> +{
> +	struct xe_ggtt *ggtt = node->ggtt;
> +
> +	return ggtt_insert_node_in_range_locked(node, size, align, 0,
> +						ggtt->size, ggtt_usable_insert_flags(ggtt));
> +}

[ ... ]

> @@ -703,8 +777,45 @@ struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 ali
>  		return node;
>  
>  	guard(mutex)(&ggtt->lock);
> -	ret = xe_ggtt_insert_node_locked(node, size, align,
> -					 DRM_MM_INSERT_HIGH);
> +
> +	ret = xe_ggtt_insert_node_locked(node, size, align);
> +	if (ret) {
> +		ggtt_node_fini(node);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return node;
> +}

[Severity: Medium]
Does switching xe_ggtt_insert_node() to ggtt_usable_insert_flags() invert VF
GGTT allocation order on SR-IOV PF?

On SR-IOV PF, ggtt->shareable_size is non-zero, so ggtt_usable_insert_flags()
returns DRM_MM_INSERT_LOW instead of DRM_MM_INSERT_HIGH.  However,
pf_provision_vf_ggtt() in drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c still
calls xe_ggtt_insert_node() rather than xe_ggtt_insert_node_shareable(),
which causes VF GGTT regions on PF to be allocated bottom-up from the usable
pool alongside PF driver BOs instead of top-down from the shareable pool.

[ ... ]

> @@ -882,10 +995,14 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  	else
>  		end = 0;
>  
> -	xe_tile_assert(ggtt->tile, end >= start + xe_bo_size(bo));
> +	end = min(end, ggtt->size);
>  
> -	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node[tile_id]->base,
> -					  xe_bo_size(bo), alignment, 0, start, end, 0);
> +	if (end < start + xe_bo_size(bo))
> +		err = -ENOSPC;
> +	else
> +		err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node[tile_id]->base,
> +						  xe_bo_size(bo), alignment, 0, start, end,
> +						  ggtt_usable_insert_flags(ggtt));

[Severity: High]
Does passing ggtt_usable_insert_flags(ggtt) here place stolen memory BOs past
the end of PCI BAR 2 on pre-12.70 integrated platforms?

When ggtt->shareable_size is 0, ggtt_usable_insert_flags() returns
DRM_MM_INSERT_HIGH, changing __xe_ggtt_insert_bo_at() from bottom-up (mode 0)
to top-down allocation near GUC_GGTT_TOP (~4 GiB).  On integrated platforms
with graphics version < 12.70 where xe_ttm_stolen_cpu_access_needs_ggtt() is
true, CPU access to stolen BOs (such as the initial framebuffer in
initial_plane_bo()) is mapped through PCI BAR 2 (GMADR) at mgr->io_base +
xe_bo_ggtt_addr(bo):

drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c:__xe_ttm_stolen_io_mem_reserve_stolen() {
	...
	/* GGTT is always contiguously mapped */
	mem->bus.offset = xe_bo_ggtt_addr(bo) + mgr->io_base;
	...
}

Because PCI BAR 2 only covers the low mappable GGTT aperture (typically
256 MiB), placing stolen BOs at the top of the 4 GiB GGTT causes
__xe_ttm_stolen_io_mem_reserve_stolen() and xe_ttm_stolen_io_offset() to
compute physical addresses gigabytes beyond BAR 2 and ioremap or mmap
unrelated host physical address space.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112115147.2329912-1-piotr.piorkowski@intel.com?part=3