Re: [PATCH v3 16/36] drm/xe/vf: Close multi-GT GGTT shift race

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 9/29/2025 4:55 AM, Matthew Brost wrote:
> As multi-GT VF post-migration recovery can run in parallel on different
> workqueues, but both GTs point to the same GGTT, only one GT needs to
> shift the GGTT. However, both GTs need to know when this step has
> completed. To coordinate this, share the VF config lock among all GTs
> that share a GGTT, and perform the GGTT shift under this lock. With
> shift being done under the lock, storing the shift value becomes
> unnecessary.

maybe better (and more natural) option would be to move VF GGTT config
from GT (xe_gt_sriov_vf_config) to Tile (xe_tile_sriov_vf_config) ? 

and protect it there with single lock also defined there ?

I'm doing similar changes on the PF provisioning side...

> 
> v3:
>  - Update commmit message (Tomasz)
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_vf.c       | 95 +++++++++--------------
>  drivers/gpu/drm/xe/xe_gt_sriov_vf.h       |  3 +-
>  drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h | 11 ++-
>  drivers/gpu/drm/xe/xe_guc.c               |  2 +-
>  drivers/gpu/drm/xe/xe_tile_sriov_vf.c     |  6 +-
>  drivers/gpu/drm/xe/xe_tile_sriov_vf.h     |  1 -
>  6 files changed, 51 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> index 6f15619efe01..ad1d63b5b8d1 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> @@ -436,16 +436,19 @@ u32 xe_gt_sriov_vf_gmdid(struct xe_gt *gt)
>  	return value;
>  }
>  
> -static int vf_get_ggtt_info(struct xe_gt *gt)
> +static int vf_get_ggtt_info(struct xe_gt *gt, bool recovery)
>  {
>  	struct xe_gt_sriov_vf_selfconfig *config = &gt->sriov.vf.self_config;
> +	struct xe_gt_sriov_vf_selfconfig *primary_config =
> +		&gt_to_tile(gt)->primary_gt->sriov.vf.self_config;
>  	struct xe_guc *guc = &gt->uc.guc;
>  	u64 start, size;
> +	s64 shift;
>  	int err;
>  
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  
> -	down_write(&config->lock);
> +	down_write(config->lock);
>  
>  	err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_GGTT_START_KEY, &start);
>  	if (unlikely(err))
> @@ -465,13 +468,17 @@ static int vf_get_ggtt_info(struct xe_gt *gt)
>  	xe_gt_sriov_dbg_verbose(gt, "GGTT %#llx-%#llx = %lluK\n",
>  				start, start + size - 1, size / SZ_1K);
>  
> -	config->ggtt_shift = start - (s64)config->ggtt_base;
> +	shift = start - (s64)primary_config->ggtt_base;
>  	config->ggtt_base = start;
>  	config->ggtt_size = size;
> +	if (recovery)
> +		primary_config->ggtt_base = start;
>  	err = config->ggtt_size ? 0 : -ENODATA;
>  
> +	if (!err && shift && recovery)
> +		xe_tile_sriov_vf_fixup_ggtt_nodes(gt_to_tile(gt), shift);
>  out:
> -	up_write(&config->lock);
> +	up_write(config->lock);
>  	return err;
>  }
>  
> @@ -485,7 +492,7 @@ static int vf_get_lmem_info(struct xe_gt *gt)
>  
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  
> -	down_write(&config->lock);
> +	down_write(config->lock);
>  
>  	err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_LMEM_SIZE_KEY, &size);
>  	if (unlikely(err))
> @@ -505,7 +512,7 @@ static int vf_get_lmem_info(struct xe_gt *gt)
>  	err = config->lmem_size ? 0 : -ENODATA;
>  
>  out:
> -	up_write(&config->lock);
> +	up_write(config->lock);
>  	return err;
>  }
>  
> @@ -518,7 +525,7 @@ static int vf_get_submission_cfg(struct xe_gt *gt)
>  
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  
> -	down_write(&config->lock);
> +	down_write(config->lock);
>  
>  	err = guc_action_query_single_klv32(guc, GUC_KLV_VF_CFG_NUM_CONTEXTS_KEY, &num_ctxs);
>  	if (unlikely(err))
> @@ -549,7 +556,7 @@ static int vf_get_submission_cfg(struct xe_gt *gt)
>  	err = config->num_ctxs ? 0 : -ENODATA;
>  
>  out:
> -	up_write(&config->lock);
> +	up_write(config->lock);
>  	return err;
>  }
>  
> @@ -564,17 +571,18 @@ static void vf_cache_gmdid(struct xe_gt *gt)
>  /**
>   * xe_gt_sriov_vf_query_config - Query SR-IOV config data over MMIO.
>   * @gt: the &xe_gt
> + * @recovery: VF post migration recovery path
>   *
>   * This function is for VF use only.
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
> -int xe_gt_sriov_vf_query_config(struct xe_gt *gt)
> +int xe_gt_sriov_vf_query_config(struct xe_gt *gt, bool recovery)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
>  	int err;
>  
> -	err = vf_get_ggtt_info(gt);
> +	err = vf_get_ggtt_info(gt, recovery);
>  	if (unlikely(err))
>  		return err;
>  
> @@ -610,10 +618,10 @@ u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt)
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  	xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
>  
> -	down_read(&config->lock);
> +	down_read(config->lock);
>  	xe_gt_assert(gt, config->num_ctxs);
>  	val = config->num_ctxs;
> -	up_read(&config->lock);
> +	up_read(config->lock);
>  
>  	return val;
>  }
> @@ -634,10 +642,10 @@ u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt)
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  	xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
>  
> -	down_read(&config->lock);
> +	down_read(config->lock);
>  	xe_gt_assert(gt, config->lmem_size);
>  	val = config->lmem_size;
> -	up_read(&config->lock);
> +	up_read(config->lock);
>  
>  	return val;
>  }
> @@ -656,11 +664,9 @@ u64 xe_gt_sriov_vf_ggtt(struct xe_gt *gt)
>  	u64 val;
>  
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
> -	xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
> +	lockdep_assert_held(config->lock);
>  
> -	down_read(&config->lock);
>  	val = config->ggtt_size;
> -	up_read(&config->lock);
>  
>  	return val;
>  }
> @@ -680,34 +686,10 @@ u64 xe_gt_sriov_vf_ggtt_base(struct xe_gt *gt)
>  
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  	xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
> -
> -	down_read(&config->lock);
>  	xe_gt_assert(gt, config->ggtt_size);
> -	val = config->ggtt_base;
> -	up_read(&config->lock);
> -
> -	return val;
> -}
> +	lockdep_assert_held(config->lock);
>  
> -/**
> - * xe_gt_sriov_vf_ggtt_shift - Return shift in GGTT range due to VF migration
> - * @gt: the &xe_gt struct instance
> - *
> - * This function is for VF use only.
> - *
> - * Return: The shift value; could be negative
> - */
> -s64 xe_gt_sriov_vf_ggtt_shift(struct xe_gt *gt)
> -{
> -	struct xe_gt_sriov_vf_selfconfig *config = &gt->sriov.vf.self_config;
> -	s64 val;
> -
> -	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
> -	xe_gt_assert(gt, xe_gt_is_main_type(gt));
> -
> -	down_read(&config->lock);
> -	val = config->ggtt_shift;
> -	up_read(&config->lock);
> +	val = config->ggtt_base;
>  
>  	return val;
>  }
> @@ -1115,7 +1097,7 @@ void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p)
>  
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  
> -	down_read(&config->lock);
> +	down_read(config->lock);
>  	drm_printf(p, "GGTT range:\t%#llx-%#llx\n",
>  		   config->ggtt_base,
>  		   config->ggtt_base + config->ggtt_size - 1);
> @@ -1123,8 +1105,6 @@ void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p)
>  	string_get_size(config->ggtt_size, 1, STRING_UNITS_2, buf, sizeof(buf));
>  	drm_printf(p, "GGTT size:\t%llu (%s)\n", config->ggtt_size, buf);
>  
> -	drm_printf(p, "GGTT shift on last restore:\t%lld\n", config->ggtt_shift);
> -
>  	if (IS_DGFX(xe) && xe_gt_is_main_type(gt)) {
>  		string_get_size(config->lmem_size, 1, STRING_UNITS_2, buf, sizeof(buf));
>  		drm_printf(p, "LMEM size:\t%llu (%s)\n", config->lmem_size, buf);
> @@ -1132,7 +1112,7 @@ void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p)
>  
>  	drm_printf(p, "GuC contexts:\t%u\n", config->num_ctxs);
>  	drm_printf(p, "GuC doorbells:\t%u\n", config->num_dbs);
> -	up_read(&config->lock);
> +	up_read(config->lock);
>  }
>  
>  /**
> @@ -1215,21 +1195,16 @@ static size_t post_migration_scratch_size(struct xe_device *xe)
>  static int vf_post_migration_fixups(struct xe_gt *gt)
>  {
>  	void *buf = gt->sriov.vf.migration.scratch;
> -	s64 shift;
>  	int err;
>  
> -	err = xe_gt_sriov_vf_query_config(gt);
> +	err = xe_gt_sriov_vf_query_config(gt, true);
>  	if (err)
>  		return err;
>  
> -	shift = xe_gt_sriov_vf_ggtt_shift(gt);
> -	if (shift) {
> -		xe_tile_sriov_vf_fixup_ggtt_nodes(gt_to_tile(gt), shift);
> -		xe_gt_sriov_vf_default_lrcs_hwsp_rebase(gt);
> -		err = xe_guc_contexts_hwsp_rebase(&gt->uc.guc, buf);
> -		if (err)
> -			return err;
> -	}
> +	xe_gt_sriov_vf_default_lrcs_hwsp_rebase(gt);
> +	err = xe_guc_contexts_hwsp_rebase(&gt->uc.guc, buf);
> +	if (err)
> +		return err;
>  
>  	return 0;
>  }
> @@ -1316,6 +1291,7 @@ static void migration_worker_func(struct work_struct *w)
>   */
>  int xe_gt_sriov_vf_init_early(struct xe_gt *gt)
>  {
> +	struct xe_tile *tile = gt_to_tile(gt);
>  	void *buf;
>  
>  	if (!xe_sriov_vf_migration_supported(gt_to_xe(gt)))
> @@ -1328,7 +1304,10 @@ int xe_gt_sriov_vf_init_early(struct xe_gt *gt)
>  		return -ENOMEM;
>  
>  	gt->sriov.vf.migration.scratch = buf;
> -	init_rwsem(&gt->sriov.vf.self_config.lock);
> +	if (xe_gt_is_main_type(gt))
> +		init_rwsem(&gt->sriov.vf.self_config.__lock);
> +	gt->sriov.vf.self_config.lock =
> +		&tile->primary_gt->sriov.vf.self_config.__lock;
>  	spin_lock_init(&gt->sriov.vf.migration.lock);
>  	INIT_WORK(&gt->sriov.vf.migration.worker, migration_worker_func);
>  
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> index 0b0f2a30e67c..ff3a0ce608cd 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> @@ -18,7 +18,7 @@ int xe_gt_sriov_vf_bootstrap(struct xe_gt *gt);
>  void xe_gt_sriov_vf_guc_versions(struct xe_gt *gt,
>  				 struct xe_uc_fw_version *wanted,
>  				 struct xe_uc_fw_version *found);
> -int xe_gt_sriov_vf_query_config(struct xe_gt *gt);
> +int xe_gt_sriov_vf_query_config(struct xe_gt *gt, bool recovery);
>  int xe_gt_sriov_vf_connect(struct xe_gt *gt);
>  int xe_gt_sriov_vf_query_runtime(struct xe_gt *gt);
>  void xe_gt_sriov_vf_migrated_event_handler(struct xe_gt *gt);
> @@ -31,7 +31,6 @@ u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt);
>  u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt);
>  u64 xe_gt_sriov_vf_ggtt(struct xe_gt *gt);
>  u64 xe_gt_sriov_vf_ggtt_base(struct xe_gt *gt);
> -s64 xe_gt_sriov_vf_ggtt_shift(struct xe_gt *gt);
>  
>  u32 xe_gt_sriov_vf_read32(struct xe_gt *gt, struct xe_reg reg);
>  void xe_gt_sriov_vf_write32(struct xe_gt *gt, struct xe_reg reg, u32 val);
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> index a63b6004b0b7..6cbf8291a5ab 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> @@ -19,16 +19,19 @@ struct xe_gt_sriov_vf_selfconfig {
>  	u64 ggtt_base;
>  	/** @ggtt_size: assigned size of the GGTT region. */
>  	u64 ggtt_size;
> -	/** @ggtt_shift: difference in ggtt_base on last migration */
> -	s64 ggtt_shift;
>  	/** @lmem_size: assigned size of the LMEM. */
>  	u64 lmem_size;
>  	/** @num_ctxs: assigned number of GuC submission context IDs. */
>  	u16 num_ctxs;
>  	/** @num_dbs: assigned number of GuC doorbells IDs. */
>  	u16 num_dbs;
> -	/** @lock: lock for protecting access to all selfconfig fields. */
> -	struct rw_semaphore lock;
> +	/** @__lock: lock for protecting access to all selfconfig fields. */
> +	struct rw_semaphore __lock;
> +	/**
> +	 * @lock: pointer to lock for protecting access to all selfconfig
> +	 * fields, all GTs point to primary GT.
> +	 */
> +	struct rw_semaphore *lock;

this could be placed in tile.sriov.vf

>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index d5adbbb013ec..c016a11b6ab1 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -713,7 +713,7 @@ static int vf_guc_init_noalloc(struct xe_guc *guc)
>  	if (err)
>  		return err;
>  
> -	err = xe_gt_sriov_vf_query_config(gt);
> +	err = xe_gt_sriov_vf_query_config(gt, false);
>  	if (err)
>  		return err;
>  
> diff --git a/drivers/gpu/drm/xe/xe_tile_sriov_vf.c b/drivers/gpu/drm/xe/xe_tile_sriov_vf.c
> index f221dbed16f0..dc6221fc0520 100644
> --- a/drivers/gpu/drm/xe/xe_tile_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_tile_sriov_vf.c
> @@ -40,7 +40,7 @@ static int vf_init_ggtt_balloons(struct xe_tile *tile)
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
> -int xe_tile_sriov_vf_balloon_ggtt_locked(struct xe_tile *tile)
> +static int xe_tile_sriov_vf_balloon_ggtt_locked(struct xe_tile *tile)
>  {
>  	u64 ggtt_base = xe_gt_sriov_vf_ggtt_base(tile->primary_gt);
>  	u64 ggtt_size = xe_gt_sriov_vf_ggtt(tile->primary_gt);
> @@ -100,12 +100,16 @@ int xe_tile_sriov_vf_balloon_ggtt_locked(struct xe_tile *tile)
>  
>  static int vf_balloon_ggtt(struct xe_tile *tile)
>  {
> +	struct xe_gt_sriov_vf_selfconfig *config =
> +		&tile->primary_gt->sriov.vf.self_config;

with GGTT (and its lock) stored at tile level we will not be forced
to look at the primary-gt any more


>  	struct xe_ggtt *ggtt = tile->mem.ggtt;
>  	int err;
>  
> +	down_read(config->lock);
>  	mutex_lock(&ggtt->lock);
>  	err = xe_tile_sriov_vf_balloon_ggtt_locked(tile);
>  	mutex_unlock(&ggtt->lock);
> +	up_read(config->lock);
>  
>  	return err;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_tile_sriov_vf.h b/drivers/gpu/drm/xe/xe_tile_sriov_vf.h
> index 93eb043171e8..4ee68d1fb28e 100644
> --- a/drivers/gpu/drm/xe/xe_tile_sriov_vf.h
> +++ b/drivers/gpu/drm/xe/xe_tile_sriov_vf.h
> @@ -11,7 +11,6 @@
>  struct xe_tile;
>  
>  int xe_tile_sriov_vf_prepare_ggtt(struct xe_tile *tile);
> -int xe_tile_sriov_vf_balloon_ggtt_locked(struct xe_tile *tile);
>  void xe_tile_sriov_vf_deballoon_ggtt_locked(struct xe_tile *tile);
>  void xe_tile_sriov_vf_fixup_ggtt_nodes(struct xe_tile *tile, s64 shift);
>