Re: [PATCH v2 4/4] drm/xe/pf: Explicitly use shareable GuC IDs for VFs provisioning

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `spare` quota enforcement in `idm_reserve_chunk_shareable_locked` mathematically miscalculates quota and incorrectly rejects allocations.
- [Medium] The `check_quota` kunit test was modified to use non-overlapping pools, completely bypassing the quota enforcement logic it is supposed to test.
--

--- Patch [4]: [PATCH v2 4/4] drm/xe/pf: Explicitly use shareable GuC IDs for VFs provisioning ---
commit 249a1c056e1b8b61fd7f66cd23baf1408b302ed8
Author: Piotr Piórkowski <[email protected]>

drm/xe/pf: Explicitly use shareable GuC IDs for VFs provisioning

Switch PF VF-context provisioning to explicitly allocate IDs from
the dedicated shareable ID pool. Also, lets remove the legacy GuC ID
reservation API now that all VFs provisioning paths use the shareable
allocation helpers.

> diff --git a/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c b/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c
> index f21d3341b5033..3e6d289f7d7a0 100644
> --- a/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c
> +++ b/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c
[ ... ]
> @@ -153,26 +160,31 @@ static void check_quota(struct kunit *test)
>  {
>  	struct xe_guc_id_mgr *idm = test->priv;
>  	unsigned int n;
> +	unsigned int max;
>  
> -	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, 2, 0));
> +	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, 2, 2));
> +	max = xe_guc_id_mgr_max_shareable(idm);
>  
>  	mutex_lock(idm_mutex(idm));
>  
> -	for (n = 0; n < idm_total(idm) - 1; n++) {
> +	for (n = 0; n < max - 1; n++) {
>  		kunit_info(test, "n=%u", n);
> -		KUNIT_EXPECT_EQ(test, idm_reserve_chunk_locked(idm, 1, idm_total(idm)), -EDQUOT);
> +		KUNIT_EXPECT_GE(test,
> +				xe_guc_id_mgr_reserve_shareable_locked(idm, 1, max), 0);

[Severity: Medium]
Does changing the initialization to idm_init(idm, 2, 2) bypass the quota
enforcement logic this function is supposed to test?

Because usable (2) + shareable (2) is less than GUC_ID_MAX, these pools are
configured as non-overlapping. Non-overlapping pools intentionally bypass the
spare quota check.

With the test now receiving successful allocations instead of -EDQUOT, it
appears the assertions were inverted (expecting >= 0 instead of -EDQUOT),
which removes the test's coverage for the quota enforcement feature entirely.

> +		KUNIT_EXPECT_GE(test,
> +				xe_guc_id_mgr_reserve_shareable_locked(idm, 1, max - n), 0);
>  		KUNIT_EXPECT_EQ(test,
> -				idm_reserve_chunk_locked(idm, 1, idm_total(idm) - n),
> -				-EDQUOT);
> +				xe_guc_id_mgr_reserve_shareable_locked(idm, max - n, 1),
> +				-ENOSPC);
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> index b4f4a7aa21c78..79535f7478e96 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
[ ... ]
> @@ -889,7 +889,7 @@ static int pf_reserve_ctxs(struct xe_gt *gt, u32 num)
>  	struct xe_guc_id_mgr *idm = &gt->uc.guc.submission_state.idm;
>  	unsigned int spare = pf_get_spare_ctxs(gt);
>  
> -	return xe_guc_id_mgr_reserve(idm, num, spare);
> +	return xe_guc_id_mgr_reserve_shareable(idm, num, spare);

[Severity: Medium]
Will this switch to xe_guc_id_mgr_reserve_shareable() expose a mathematical
error in the spare quota calculation?

Looking at idm_reserve_chunk_shareable_locked() in xe_guc_id_mgr.c:

	if (spare) {
		unsigned int used_overlap = bitmap_weight(idm->bitmap, idm->usable) -
					    bitmap_weight(idm->bitmap, shareable_start);
		...
		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;
	}

This logic calculates quota based entirely on the overlap region, effectively
ignoring free IDs in the PF-exclusive region.

While this may be masked in production because the PF currently initializes
the manager with full overlap (GUC_ID_MAX, GUC_ID_MAX), any partial overlap
configuration (e.g., usable = 65536, shareable = 2) will incorrectly fail
allocations with -EDQUOT whenever spare > overlap, regardless of the actual
number of free usable IDs available.

>  }
>  
>  static void pf_release_ctxs(struct xe_gt *gt, u32 start, u32 num)

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