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)

Paolo Bonzini
2025-12-22T21:15:32Z

On 12/22/25 20:34, Sean Christopherson wrote:
> On Mon, Dec 22, 2025, Paolo Bonzini wrote:
>> On 12/22/25 10:16, Ankit Soni wrote:
>>> - 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 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()

That's (c). :)

I called irq_set_thread_affinity() sketchy, because it's a core kernel
subsystem that takes a pretty common lock, and directly calls a function
that takes another very common lock. KVM's deadlock scenario seems like
a relatively natural occurrence, though in this case it can be fixed
outside kernel/irq as well.

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

In fact this was actually my first idea, exactly because it makes
svm->ir_list_lock a leaf lock!

I threw it away because it makes amd_ir_set_vcpu_affinity() weird,
passing back the ir_data but not really doing anything else. Basically
its role becomes little more than violate abstractions, which seemed
wrong. On the other hand, drivers/iommu is already very much tied to
the KVM vendor modules (in particular avic.c already calls
amd_iommu_{,de}activate_guest_mode), so who am I to judge what the IOMMU
driver does.

Paolo

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