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

["Lis, Tomasz" <tomasz.lis@intel.com>]

On 10/2/2025 9:36 AM, Matthew Brost wrote:
> 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.

You mean by the PF part? Because generally, Xe cannot make guarantees 
for how VMM works.

When state is restored, one of the chunks is GuC state. After the GuC 
state is restored, GuC is expected to send MIGRATED irq to the VM. It 
sends the interrupt around the same time it answers to PF that state 
restore completed. The vCPU is not started at that point. When it 
finally starts, on qemu we see the interrupt set from the start. And 
this should always be the case for memory-based IRQs - because the 
interrupt data is stored in a VRAM buffer. However, in general, this 
depends on how qemu implements the interrupt model. I wonder if there 
are situations where that would become a problem.


>
>>>    	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.

Ok, if you can find a lockless solution again, that would be beneficial.

-Tomasz

>
> 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);