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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] VF-exclusive ID allocations incorrectly fail with -EDQUOT when the PF exceeds its usable spare quota.
- [Low] Redundant and unreachable bounds check in `idm_reserve_chunk_shareable_locked`.
--

commit 3e38a9d8d7cef76aa3a1600296b4b451c5ffa3ff
Author: Piotr Piórkowski <piotr.piorkowski@intel.com>
Subject: drm/xe/guc: Split GuC ID manager into usable and shareable pools

Modify the GuC identifier manager so that it distinguishes between usable GuC
IDs allocated for GuC submissions, and shareable those that can be shared
with VFs. Both pools are represented as a single bitmap and may overlap. For
now, let's keep the current allocation behaviour by mapping both pools onto
the same GuC ID space.

> diff --git a/drivers/gpu/drm/xe/xe_guc_id_mgr.c b/drivers/gpu/drm/xe/xe_guc_id_mgr.c
> index e845425d670be..2c181d2805525 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.
> +		 */
> +		id = idm_find_last_zero_area_in_range(idm->bitmap, shareable_start, total, count);
> +		if (id >= total)
> +			return -ENOSPC;
> +
> +		if (spare) {
> +			unsigned int used_usable = bitmap_weight(idm->bitmap, idm->usable);
> +			unsigned int overlap_start = max_t(int, id, shareable_start);
> +			unsigned int overlap_end = min(id + count, idm->usable);
> +			unsigned int need_overlap = overlap_end > overlap_start ?
> +						    overlap_end - overlap_start : 0;
> +
> +			if (used_usable + need_overlap + spare > idm->usable)
> +				return -EDQUOT;

[Severity: High]
Can this logic inadvertently deny VF provisioning when the PF exceeds its
usable spare quota, even if the requested IDs fall entirely in the
non-overlapping pool?

If need_overlap is 0, the allocation does not consume any usable IDs.
However, if used_usable + spare > idm->usable due to PF load, this check
will fail the VF allocation with -EDQUOT.

> +		}
> +	} else {
> +		id = bitmap_find_next_zero_area(idm->bitmap, total, shareable_start, count, 0);
> +		if (id + count > total)
> +			return -ENOSPC;
> +		if (id < shareable_start)
> +			return -ENOSPC;

[Severity: Low]
Is this check for id < shareable_start necessary?

Since bitmap_find_next_zero_area() is called with shareable_start as the
start parameter, the returned id should be mathematically guaranteed to be
>= shareable_start, making this bounds check redundant.

> +	}
> +
> +	bitmap_set(idm->bitmap, id, count);
> +
> +	return id;
> +}

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