Re: [PATCH v3 29/36] drm/xe: Move queue init before LRC creation

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Oct 02, 2025 at 02:44:47AM +0200, Lis, Tomasz wrote:
> 
> On 9/29/2025 4:55 AM, Matthew Brost wrote:
> > A queue must be in the submission backend's tracking state before the
> > LRC is created to avoid a race condition where the LRC's GGTT addresses
> > are not properly fixed up during VF post-migration recovery.
> > 
> > Move the queue initialization—which adds the queue to the submission
> > backend's tracking state—before LRC creation.
> > 
> > v2:
> >   - Wait on VF GGTT fixes before creating LRC (testing)
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_exec_queue.c        | 43 +++++++++++++++++------
> >   drivers/gpu/drm/xe/xe_execlist.c          |  2 +-
> >   drivers/gpu/drm/xe/xe_gt_sriov_vf.c       | 39 +++++++++++++++++++-
> >   drivers/gpu/drm/xe/xe_gt_sriov_vf.h       |  2 ++
> >   drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h |  5 +++
> >   drivers/gpu/drm/xe/xe_guc_submit.c        |  2 +-
> >   drivers/gpu/drm/xe/xe_lrc.h               | 10 ++++++
> >   7 files changed, 90 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index 81f707d2c388..3db8e64d9d13 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -15,6 +15,7 @@
> >   #include "xe_dep_scheduler.h"
> >   #include "xe_device.h"
> >   #include "xe_gt.h"
> > +#include "xe_gt_sriov_vf.h"
> >   #include "xe_hw_engine_class_sysfs.h"
> >   #include "xe_hw_engine_group.h"
> >   #include "xe_hw_fence.h"
> > @@ -179,17 +180,32 @@ static int __xe_exec_queue_init(struct xe_exec_queue *q)
> >   			flags |= XE_LRC_CREATE_RUNALONE;
> >   	}
> > +	err = q->ops->init(q);
> > +	if (err)
> > +		return err;
> > +
> > +	/*
> > +	 * This must occur after q->ops->init to avoid race conditions during VF
> > +	 * post-migration recovery, as the fixups for the LRC GGTT addresses
> > +	 * depend on the queue being present in the backend tracking structure.
> > +	 *
> > +	 * In addition to above, we must wait on inflight GGTT changes to
> > +	 * avoid writing out stale values here.
> > +	 */
> > +	xe_gt_sriov_vf_wait_valid_ggtt(q->gt);
> 
> So to avoid locks, we rely on the VF knowing it got migrated from the first
> moment after vCPU starts.
> 
> On `qemu`, we do have it this way - when vCPU starts the 'MIGRATED' memirq
> is already filled.
> 
> But what about other VM managers? What about future support of platforms
> without memirq?
> 
> I don't think the availability of information that we've got migrated from
> the first vCPU cycle is guaranteed by any specification.
> 

It is guarnetted byt the design of Xe.

> 
> >   	for (i = 0; i < q->width; ++i) {
> > -		q->lrc[i] = xe_lrc_create(q->hwe, q->vm, SZ_16K, q->msix_vec, flags);
> > -		if (IS_ERR(q->lrc[i])) {
> > -			err = PTR_ERR(q->lrc[i]);
> > +		struct xe_lrc *lrc;
> > +
> > +		lrc = xe_lrc_create(q->hwe, q->vm, xe_lrc_ring_size(),
> > +				    q->msix_vec, flags);
> 
> If migration happened at this place, it is still possible to create a
> context with wrong GGTT references in the one LRC which was already filled
> but not integrated into the queue yet.
> 
> I don't think we can avoid races without a lock.
> 

There might be a small race here, let me think about this. I will say
this change xe_exec_threads --r threads-many-queues though. Locking is
definitely not the way solve this though - reclaim rules are in play
here which make locking difficult and convoluted cross layer locks will
always get nacked by myself and others.

Matt

> > +		if (IS_ERR(lrc)) {
> > +			err = PTR_ERR(lrc);
> >   			goto err_lrc;
> >   		}
> > -	}
> > -	err = q->ops->init(q);
> > -	if (err)
> > -		goto err_lrc;
> > +		/* Pairs with READ_ONCE to xe_exec_queue_contexts_hwsp_rebase */
> > +		WRITE_ONCE(q->lrc[i], lrc);
> > +	}
> >   	return 0;
> > @@ -1095,9 +1111,16 @@ int xe_exec_queue_contexts_hwsp_rebase(struct xe_exec_queue *q, void *scratch)
> >   	int err = 0;
> >   	for (i = 0; i < q->width; ++i) {
> > -		xe_lrc_update_memirq_regs_with_address(q->lrc[i], q->hwe, scratch);
> > -		xe_lrc_update_hwctx_regs_with_address(q->lrc[i]);
> > -		err = xe_lrc_setup_wa_bb_with_scratch(q->lrc[i], q->hwe, scratch);
> > +		struct xe_lrc *lrc;
> > +
> > +		/* Pairs with WRITE_ONCE in __xe_exec_queue_init  */
> > +		lrc = READ_ONCE(q->lrc[i]);
> > +		if (!lrc)
> > +			continue;
> > +
> > +		xe_lrc_update_memirq_regs_with_address(lrc, q->hwe, scratch);
> > +		xe_lrc_update_hwctx_regs_with_address(lrc);
> > +		err = xe_lrc_setup_wa_bb_with_scratch(lrc, q->hwe, scratch);
> >   		if (err)
> >   			break;
> >   	}
> > diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
> > index f83d421ac9d3..769d05517f93 100644
> > --- a/drivers/gpu/drm/xe/xe_execlist.c
> > +++ b/drivers/gpu/drm/xe/xe_execlist.c
> > @@ -339,7 +339,7 @@ static int execlist_exec_queue_init(struct xe_exec_queue *q)
> >   	const struct drm_sched_init_args args = {
> >   		.ops = &drm_sched_ops,
> >   		.num_rqs = 1,
> > -		.credit_limit = q->lrc[0]->ring.size / MAX_JOB_SIZE_BYTES,
> > +		.credit_limit = xe_lrc_ring_size() / MAX_JOB_SIZE_BYTES,
> >   		.hang_limit = XE_SCHED_HANG_LIMIT,
> >   		.timeout = XE_SCHED_JOB_TIMEOUT,
> >   		.name = q->hwe->name,
> > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > index 0d94867dce8e..42f9fd43b436 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > @@ -482,6 +482,11 @@ static int vf_get_ggtt_info(struct xe_gt *gt, bool recovery)
> >   				 shift, config->ggtt_base);
> >   		xe_tile_sriov_vf_fixup_ggtt_nodes(gt_to_tile(gt), shift);
> >   	}
> > +
> > +	WRITE_ONCE(gt->sriov.vf.migration.ggtt_need_fixes, false);
> > +	smp_wmb();	/* Ensure above write visible before wake */
> > +	wake_up_all(&gt->sriov.vf.migration.wq);
> > +
> >   out:
> >   	up_write(config->lock);
> >   	return err;
> > @@ -820,7 +825,8 @@ static void vf_start_migration_recovery(struct xe_gt *gt)
> >   	    !gt->sriov.vf.migration.recovery_teardown) {
> >   		gt->sriov.vf.migration.recovery_queued = true;
> >   		WRITE_ONCE(gt->sriov.vf.migration.recovery_inprogress, true);
> > -		smp_wmb();	/* Ensure above write visable before wake */
> > +		WRITE_ONCE(gt->sriov.vf.migration.ggtt_need_fixes, true);
> > +		smp_wmb();	/* Ensure above writes visable before wake */
> typo in patch "Wakeup in GuC backend on VF post migration recovery"
> >   		wake_up_all(&gt->uc.guc.ct.wq);
> > @@ -1344,6 +1350,7 @@ int xe_gt_sriov_vf_init_early(struct xe_gt *gt)
> >   		&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);
> > +	init_waitqueue_head(&gt->sriov.vf.migration.wq);
> >   	return 0;
> >   }
> > @@ -1387,3 +1394,33 @@ bool xe_gt_sriov_vf_recovery_inprogress(struct xe_gt *gt)
> >   	return (xe_memirq_sw_int_0_irq_pending(memirq, &gt->uc.guc) ||
> >   		READ_ONCE(gt->sriov.vf.migration.recovery_inprogress));
> >   }
> > +
> > +static bool vf_valid_ggtt(struct xe_gt *gt)
> > +{
> > +	struct xe_memirq *memirq = &gt_to_tile(gt)->memirq;
> > +
> > +	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
> > +
> > +	if (xe_memirq_sw_int_0_irq_pending(memirq, &gt->uc.guc) ||
> > +	    READ_ONCE(gt->sriov.vf.migration.ggtt_need_fixes))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +/**
> > + * xe_gt_sriov_vf_wait_valid_ggtt() - VF wait for valid GGTT addresses
> > + * @gt: the &xe_gt
> > + */
> > +void xe_gt_sriov_vf_wait_valid_ggtt(struct xe_gt *gt)
> > +{
> > +	int ret;
> > +
> > +	if (!IS_SRIOV_VF(gt_to_xe(gt)))
> > +		return;
> > +
> > +	ret = wait_event_interruptible_timeout(gt->sriov.vf.migration.wq,
> > +					       vf_valid_ggtt(gt),
> > +					       HZ * 5);
> > +	XE_WARN_ON(!ret);
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> > index 71e1d566da81..20cc0c4c32e3 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> > @@ -40,4 +40,6 @@ void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p);
> >   void xe_gt_sriov_vf_print_runtime(struct xe_gt *gt, struct drm_printer *p);
> >   void xe_gt_sriov_vf_print_version(struct xe_gt *gt, struct drm_printer *p);
> > +void xe_gt_sriov_vf_wait_valid_ggtt(struct xe_gt *gt);
> > +
> >   #endif
> > 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 e135018cba1e..3c3e415199d1 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
> > @@ -8,6 +8,7 @@
> >   #include <linux/rwsem.h>
> >   #include <linux/types.h>
> > +#include <linux/wait.h>
> >   #include <linux/workqueue.h>
> >   #include "xe_uc_fw_types.h"
> > @@ -61,6 +62,8 @@ struct xe_gt_sriov_vf_migration {
> >   	struct work_struct worker;
> >   	/** @lock: Protects recovery_queued, teardown */
> >   	spinlock_t lock;
> > +	/** @wq: wait queue for migration fixes */
> > +	wait_queue_head_t wq;
> >   	/** @scratch: Scratch memory for VF recovery */
> >   	void *scratch;
> >   	/** @recovery_teardown: VF post migration recovery is being torn down */
> > @@ -69,6 +72,8 @@ struct xe_gt_sriov_vf_migration {
> >   	bool recovery_queued;
> >   	/** @recovery_inprogress: VF post migration recovery in progress */
> >   	bool recovery_inprogress;
> > +	/** @ggtt_need_fixes: VF GGTT needs fixes */
> > +	bool ggtt_need_fixes;
> >   };
> >   /**
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 497a736c23c3..7fe3fb07e35e 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -1943,7 +1943,7 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
> >   	timeout = (q->vm && xe_vm_in_lr_mode(q->vm)) ? MAX_SCHEDULE_TIMEOUT :
> >   		  msecs_to_jiffies(q->sched_props.job_timeout_ms);
> >   	err = xe_sched_init(&ge->sched, &drm_sched_ops, &xe_sched_ops,
> > -			    NULL, q->lrc[0]->ring.size / MAX_JOB_SIZE_BYTES, 64,
> > +			    NULL, xe_lrc_ring_size() / MAX_JOB_SIZE_BYTES, 64,
> >   			    timeout, guc_to_gt(guc)->ordered_wq, NULL,
> >   			    q->name, gt_to_xe(q->gt)->drm.dev);
> >   	if (err)
> > diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h
> > index 188565465779..5fb6c74bdab5 100644
> > --- a/drivers/gpu/drm/xe/xe_lrc.h
> > +++ b/drivers/gpu/drm/xe/xe_lrc.h
> > @@ -74,6 +74,16 @@ static inline void xe_lrc_put(struct xe_lrc *lrc)
> >   	kref_put(&lrc->refcount, xe_lrc_destroy);
> >   }
> > +/**
> > + * xe_lrc_ring_size() - Xe LRC ring size
> > + *
> > + * Return: Size of LRC size
> 
> Size of LRC ring buffer
> -Tomasz
> 
> > + */
> > +static inline size_t xe_lrc_ring_size(void)
> > +{
> > +	return SZ_16K;
> > +}
> > +
> >   size_t xe_gt_lrc_size(struct xe_gt *gt, enum xe_engine_class class);
> >   u32 xe_lrc_pphwsp_offset(struct xe_lrc *lrc);
> >   u32 xe_lrc_regs_offset(struct xe_lrc *lrc);