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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional initialization of the shareable pool size causes guaranteed assertion failures on VFs and incorrectly alters the native allocation strategy.
- [Medium] Clamping `end` to `ggtt->size` turns a graceful `-ENOSPC` failure into a kernel WARNING for oversized BOs.
--

--- Patch [1]: [PATCH v3 1/3] drm/xe/ggtt: Split GGTT into usable and shareable pools ---
commit 1bffe9ae3d9e5639133437250d4cd685f998d972
Author: Piotr Piórkowski <[email protected]>
Subject: drm/xe/ggtt: Split GGTT into usable and shareable pools

This commit splits the driver-owned GGTT allocations and VF provisioning into
a usable pool for driver-owned allocations and a shareable pool for VF
provisioning. Separate insertion APIs are added for both pools.

> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 8ec23862477fc..c7e1cb1b1ccbf 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
[ ... ]
> @@ -438,7 +466,7 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  	if (!ggtt->wq)
>  		return -ENOMEM;
>  
> -	__xe_ggtt_init_early(ggtt, ggtt_start, ggtt_size);
> +	__xe_ggtt_init_early(ggtt, ggtt_start, ggtt_size, ggtt_size);

[Severity: High]
Does this unconditionally initialize the shareable pool size on all
configurations?

By passing ggtt_size as the shareable_size here, ggtt->shareable.size will
always be greater than 0. This seems to alter the default allocation behavior
on bare-metal setups, as ggtt_usable_insert_flags() will incorrectly return
DRM_MM_INSERT_LOW.

This also seems to cause an assertion failure later on VFs:

[ ... ]
> @@ -610,16 +638,55 @@ void xe_ggtt_shift_nodes(struct xe_ggtt *ggtt, u64 new_start)
>  
>  	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);
> +#ifdef CONFIG_PCI_IOV
> +	xe_tile_assert(ggtt->tile, ggtt->shareable.size == 0);
> +#endif

Will this assertion unconditionally trip on VFs during driver initialization
or recovery?

Since xe_ggtt_init_early() initializes shareable.size to ggtt_size, it is
never 0.

>  
>  	/* pairs with READ_ONCE in xe_ggtt_node_addr() */
>  	WRITE_ONCE(ggtt->start, new_start);
[ ... ]
> @@ -832,10 +939,13 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  	else
>  		end = 0;
>  
> +	end = min(end, ggtt->size);
> +
>  	xe_tile_assert(ggtt->tile, end >= start + xe_bo_size(bo));

[Severity: Medium]
Does clamping end to ggtt->size here convert a graceful allocation failure
into a kernel warning?

If an oversized buffer object is submitted for mapping, clamping end will
force this assertion to fail. Could this be triggered by userspace submitting
a large framebuffer, resulting in a kernel warning instead of gracefully
returning -ENOSPC from the subsequent drm_mm_insert_node_in_range() call?

>  	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node[tile_id]->base,
> -					  xe_bo_size(bo), alignment, 0, start, end, 0);
> +					  xe_bo_size(bo), alignment, 0, start, end,
> +					  ggtt_usable_insert_flags(ggtt));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1