Re: [PATCH v2 1/4] drm/xe/guc: Split GuC ID manager into usable and shareable pools

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Flawed quota check in `idm_reserve_chunk_shareable_locked()` allows VFs to bypass the PF's spare ID limits due to free-space fragmentation.
--

--- Patch [1]: [PATCH v2 1/4] drm/xe/guc: Split GuC ID manager into usable and shareable pools ---
commit 8c49ff8930e2fc8ce16d38b0a4e9813aa986a134
Author: Piotr Piórkowski <[email protected]>

drm/xe/guc: Split GuC ID manager into usable and shareable pools

This commit modifies the GuC identifier manager to separate usable GuC IDs
(for submissions) from shareable ones (for VFs). Both pools are mapped to
the same underlying bitmap and can overlap, with new helpers added to
manage the separate capacities and quotas.

> diff --git a/drivers/gpu/drm/xe/xe_guc_id_mgr.c b/drivers/gpu/drm/xe/xe_guc_id_mgr.c
> index e845425d670be..08e72af2735ff 100644
> --- a/drivers/gpu/drm/xe/xe_guc_id_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_guc_id_mgr.c

[ ... ]

> +static int idm_reserve_chunk_shareable_locked(struct xe_guc_id_mgr *idm, unsigned int count,
> +					      unsigned int spare)
> +{
> +	unsigned int shareable_start = idm_shareable_start(idm);
> +	unsigned int total = idm_total(idm);
> +	int id;
> +
> +	idm_assert(idm, count);
> +	lockdep_assert_held(idm_mutex(idm));
> +
> +	if (!idm->shareable)
> +		return -ENODATA;
> +
> +	if (shareable_start < idm->usable) {
> +		/*
> +		 * Spare is meaningful only for IDs that are shared between usable and
> +		 * shareable pools. For non-overlapping pools, shareable reservations do
> +		 * not reduce PF usable capacity, so there is nothing to retain.
> +		 */
> +		if (spare) {
> +			unsigned int used_overlap = bitmap_weight(idm->bitmap, idm->usable) -
> +						    bitmap_weight(idm->bitmap, shareable_start);
> +			unsigned int shared_tail = total - idm->usable;
> +			unsigned int used_tail = bitmap_weight(idm->bitmap, total) -
> +						 bitmap_weight(idm->bitmap, idm->usable);
> +			unsigned int free_tail = shared_tail - used_tail;
> +			unsigned int need_overlap = count > free_tail ? count - free_tail : 0;
> +			unsigned int overlap = idm->usable - shareable_start;
> +
> +			if (used_overlap + need_overlap + spare > overlap)
> +				return -EDQUOT;
> +		}
> +
> +		/* For overlapping pools, prefer allocating from the end. */
> +		id = idm_find_last_zero_area_in_range(idm->bitmap, shareable_start, total, count);

[Severity: Medium]
Does this quota check correctly account for fragmentation in the tail?

It looks like free_tail is a raw count of unset bits. Since the allocator
idm_find_last_zero_area_in_range() searches for contiguous bits, couldn't it
skip a fragmented tail and allocate directly from the shared overlap region?

If this happens, it seems the allocation would bypass the need_overlap quota
assumption and potentially allow VFs to consume GuC IDs beyond the PF's spare
limits, starving the PF of usable contexts.

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