Re: possible deadlock due to irq_set_thread_affinity() calling into the scheduler (was Re: [PATCH v3 38/62] KVM: SVM: Take and hold ir_list_lock across IRTE updates in IOMMU)

Sean Christopherson
2025-12-22T19:34:34Z

On Mon, Dec 22, 2025, Paolo Bonzini wrote:
> On 12/22/25 10:16, Ankit Soni wrote:
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 6.19.0-rc2 #20 Tainted: G E
> > ------------------------------------------------------
> > CPU 58/KVM/28597 is trying to acquire lock:
> > ff12c47d4b1f34c0 (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> >
> > but task is already holding lock:
> > ff12c49b28552110 (&svm->ir_list_lock){....}-{2:2}, at: avic_pi_update_irte+0x147/0x270 [kvm_amd]
> >
> > which lock already depends on the new lock.
> >
> > Chain exists of:
> > &irq_desc_lock_class --> &rq->__lock --> &svm->ir_list_lock
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(&svm->ir_list_lock);
> > lock(&rq->__lock);
> > lock(&svm->ir_list_lock);
> > lock(&irq_desc_lock_class);
> >
> > *** DEADLOCK ***
> >
> > So lockdep sees:
> >
> > &irq_desc_lock_class -> &rq->__lock -> &svm->ir_list_lock
> >
> > while avic_pi_update_irte() currently holds svm->ir_list_lock and then
> > takes irq_desc_lock via irq_set_vcpu_affinity(), which creates the
> > potential inversion.
> >
> > - Is this lockdep warning expected/benign in this code path, or does it
> > indicate a real potential deadlock between svm->ir_list_lock and
> > irq_desc_lock with AVIC + irq_bypass + VFIO?
>
> I'd treat it as a potential (if unlikely) deadlock:
>
> (a) irq_set_thread_affinity triggers the scheduler via wake_up_process,
> while irq_desc->lock is taken
>
> (b) the scheduler calls into KVM with rq_lock taken, and KVM uses
> ir_list_lock within __avic_vcpu_load/__avic_vcpu_put
>
> (c) KVM wants to block scheduling for a while and uses ir_list_lock for
> that purpose, but then takes irq_set_vcpu_affinity takes irq_desc->lock.
>
> I don't think there's an alternative choice of lock for (c); and there's
> no easy way to pull the irq_desc->lock out of the IRQ subsystem--in fact
> the stickiness of the situation comes from rq->rq_lock and
> irq_desc->lock being both internal and not leaf.
>
> Of the three, the most sketchy is (a);

Maybe the most unnecessary, but I think there's a pretty strong argument that
(d) is the most sketchy:

(d) KVM takes svm->ir_list_lock around the call to irq_set_vcpu_affinity()

> notably, __setup_irq() calls wake_up_process outside desc->lock. Therefore
> I'd like so much to treat it as a kernel/irq/ bug; and the simplest (perhaps
> too simple...) fix is to drop the wake_up_process(). The only cost is extra
> latency on the next interrupt after an affinity change.

Alternatively, what if we rework the KVM<=>IOMMU exchange to decouple updating
the IRTE from binding the metadata to the vCPU? KVM already has the necessary
exports to do "out-of-band" updates due to the AVIC architecture requiring IRTE
updates on scheduling changes.

It's a bit wonky (and not yet tested), but I like the idea of making
svm->ir_list_lock a leaf lock so that we don't end up with a game of whack-a-mole,
e.g. if something in the IRQ subsystem changes in the future.

---
arch/x86/include/asm/irq_remapping.h | 3 --
arch/x86/kvm/svm/avic.c | 78 ++++++++++++++++++----------
drivers/iommu/amd/iommu.c | 24 +++------
3 files changed, 57 insertions(+), 48 deletions(-)

diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index 4e55d1755846..1426ecd09943 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -35,9 +35,6 @@ struct amd_iommu_pi_data {
u64 vapic_addr; /* Physical address of the vCPU's vAPIC. */
u32 ga_tag;
u32 vector; /* Guest vector of the interrupt */
- int cpu;
- bool ga_log_intr;
- bool is_guest_mode;
void *ir_data;
};

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 6b77b2033208..0f4f353c7db6 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -868,6 +868,51 @@ static void svm_ir_list_del(struct kvm_kernel_irqfd *irqfd)
raw_spin_unlock_irqrestore(&to_svm(vcpu)->ir_list_lock, flags);
}

+static int avic_pi_add_irte(struct kvm_kernel_irqfd *irqfd, void *ir_data,
+ struct kvm_vcpu *vcpu)
+{
+ struct vcpu_svm *svm = to_svm(vcpu);
+ int r;
+
+ /*
+ * Prevent the vCPU from being scheduled out or migrated until the IRTE
+ * is updated and its metadata has been added to the list of IRQs being
+ * posted to the vCPU, to ensure the IRTE isn't programmed with stale
+ * pCPU/IsRunning information.
+ */
+ guard(raw_spinlock_irqsave)(&svm->ir_list_lock);
+
+ if (kvm_vcpu_apicv_active(vcpu)) {
+ u64 entry = svm->avic_physical_id_entry;
+ bool ga_log_intr;
+ int cpu;
+
+ /*
+ * Update the target pCPU for IOMMU doorbells if the vCPU is
+ * running. If the vCPU is NOT running, i.e. is blocking or
+ * scheduled out, KVM will update the pCPU info when the vCPU
+ * is awakened and/or scheduled in. See also avic_vcpu_load().
+ */
+ if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
+ cpu = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
+ ga_log_intr = false;
+ } else {
+ cpu = -1;
+ ga_log_intr = entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR;
+ }
+ r = amd_iommu_activate_guest_mode(ir_data, cpu, ga_log_intr);
+ } else {
+ r = amd_iommu_deactivate_guest_mode(ir_data);
+ }
+
+ if (r)
+ return r;
+
+ irqfd->irq_bypass_data = ir_data;
+ list_add(&irqfd->vcpu_list, &svm->ir_list);
+ return 0;
+}
+
int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
unsigned int host_irq, uint32_t guest_irq,
struct kvm_vcpu *vcpu, u32 vector)
@@ -888,36 +933,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
struct amd_iommu_pi_data pi_data = {
.ga_tag = AVIC_GATAG(to_kvm_svm(kvm)->avic_vm_id,
vcpu->vcpu_idx),
- .is_guest_mode = kvm_vcpu_apicv_active(vcpu),
.vapic_addr = avic_get_backing_page_address(to_svm(vcpu)),
.vector = vector,
};
- struct vcpu_svm *svm = to_svm(vcpu);
- u64 entry;
int ret;

- /*
- * Prevent the vCPU from being scheduled out or migrated until
- * the IRTE is updated and its metadata has been added to the
- * list of IRQs being posted to the vCPU, to ensure the IRTE
- * isn't programmed with stale pCPU/IsRunning information.
- */
- guard(raw_spinlock_irqsave)(&svm->ir_list_lock);
-
- /*
- * Update the target pCPU for IOMMU doorbells if the vCPU is
- * running. If the vCPU is NOT running, i.e. is blocking or
- * scheduled out, KVM will update the pCPU info when the vCPU
- * is awakened and/or scheduled in. See also avic_vcpu_load().
- */
- entry = svm->avic_physical_id_entry;
- if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
- pi_data.cpu = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
- } else {
- pi_data.cpu = -1;
- pi_data.ga_log_intr = entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR;
- }
-
ret = irq_set_vcpu_affinity(host_irq, &pi_data);
if (ret)
return ret;
@@ -932,9 +952,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
return -EIO;
}

- irqfd->irq_bypass_data = pi_data.ir_data;
- list_add(&irqfd->vcpu_list, &svm->ir_list);
- return 0;
+ ret = avic_pi_add_irte(irqfd, pi_data.ir_data, vcpu);
+ if (WARN_ON_ONCE(ret))
+ irq_set_vcpu_affinity(host_irq, NULL);
+
+ return ret;
}
return irq_set_vcpu_affinity(host_irq, NULL);
}
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 5d45795c367a..855c6309900c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3970,7 +3970,6 @@ EXPORT_SYMBOL(amd_iommu_deactivate_guest_mode);

static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *info)
{
- int ret;
struct amd_iommu_pi_data *pi_data = info;
struct amd_ir_data *ir_data = data->chip_data;
struct irq_2_irte *irte_info = &ir_data->irq_2_irte;
@@ -3993,25 +3992,16 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *info)

ir_data->cfg = irqd_cfg(data);

- if (pi_data) {
- pi_data->ir_data = ir_data;
+ if (!pi_data)
+ return amd_iommu_deactivate_guest_mode(ir_data);

- ir_data->ga_root_ptr = (pi_data->vapic_addr >> 12);
- ir_data->ga_vector = pi_data->vector;
- ir_data->ga_tag = pi_data->ga_tag;
- if (pi_data->is_guest_mode)
- ret = amd_iommu_activate_guest_mode(ir_data, pi_data->cpu,
- pi_data->ga_log_intr);
- else
- ret = amd_iommu_deactivate_guest_mode(ir_data);
- } else {
- ret = amd_iommu_deactivate_guest_mode(ir_data);
- }
-
- return ret;
+ pi_data->ir_data = ir_data;
+ ir_data->ga_root_ptr = (pi_data->vapic_addr >> 12);
+ ir_data->ga_vector = pi_data->vector;
+ ir_data->ga_tag = pi_data->ga_tag;
+ return 0;
}

-
static void amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
struct amd_ir_data *ir_data,
struct irq_2_irte *irte_info,

base-commit: 9448598b22c50c8a5bb77a9103e2d49f134c9578
--