Re: [RFC PATCH v3 5/6] KVM: SVM: Add support for AMD IOMMU Guest APIC Physical Processor Interrupt (GAPPI)

[Sairaj Kodilkar <sarunkod@amd.com>]

On 7/13/2026 4:41 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Host-userspace-reachable WARN_ON due to uninitialized `svm->gappi_cpu` (-1) when configuring IRQ routing before a vCPU runs.
> - [High] Missing CPU hotplug handling causes blocked vCPUs to hang if their last-run physical CPU goes offline.
> - [High] ABBA deadlock between scheduler `rq_lock` and `gappi_vcpu_wakeup_list_lock` due to manipulating the wakeup list during vCPU preemption.
> - [Low] Mixing scoped `guard()` cleanup with `goto` error handling violates architectural guidelines.
> --
> 
> --- Patch [5]: [RFC PATCH v3 5/6] KVM: SVM: Add support for AMD IOMMU Guest APIC Physical Processor Interrupt (GAPPI) ---
> commit c3bb8845bd944e9a327f36d68b70032d436f727c
> Author: Sairaj Kodilkar <sarunkod@amd.com>
> 
>     KVM: SVM: Add support for AMD IOMMU Guest APIC Physical Processor Interrupt (GAPPI)
>     
>     With AVIC guest-mode interrupt remapping, device interrupts are posted into
>     the guest vAPIC backing page by the IOMMU. When the vCPU is not running
>     (IRTE[IsRun] = 0), KVM must still be notified to schedule it. The legacy
>     path uses the GA log.
> 
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 3b2d92a587098..982a5988532ff 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -886,6 +892,42 @@ int avic_init_vcpu(struct vcpu_svm *svm)
>>  	return ret;
>>  }
>>  
>> +static void avic_add_vcpu_to_gappi_wakeup_list(struct vcpu_svm *svm, int cpu)
>> +{
>> +	struct list_head *wakeup_list;
>> +	raw_spinlock_t *spinlock;
>> +
>> +	if (WARN_ON(unlikely(cpu < 0)))
>> +		return;
> 
> [Severity: High]
> Can an unprivileged user-space process trigger this WARN_ON?
> 
> When a vCPU is created, svm->gappi_cpu is initialized to -1 in
> avic_init_vcpu(). It is only set to a valid CPU ID later during
> __avic_vcpu_load().
> 
> If host user-space configures device interrupt routing via the KVM_IRQFD ioctl
> before the vCPU runs, avic_pi_update_irte() is invoked. This will pass the
> uninitialized svm->gappi_cpu (-1) down to this function, hitting the WARN_ON.
> If the host has panic_on_warn enabled, this allows host userspace to trigger
> a kernel panic.
> 
> [ ... ]
>> @@ -947,6 +993,12 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>>  		 * scheduled out, KVM will update the pCPU info when the vCPU
>>  		 * is awakened and/or scheduled in.  See also avic_vcpu_load().
>>  		 */
>> +		if (amd_iommu_gappi)
>> +			pi_data.ga_tag = POSTED_INTR_WAKEUP_VECTOR;
>> +		else
>> +			pi_data.ga_tag = AVIC_GATAG(to_kvm_svm(kvm)->avic_vm_id,
>> +						    vcpu->vcpu_idx);
>> +
>>  		entry = svm->avic_physical_id_entry;
>>  		if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
>>  			pi_data.apicid = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
>> @@ -955,11 +1007,19 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>>  			pi_data.apicid = -1;
>>  			pi_data.wakeup_intr = entry & AVIC_PHYSICAL_ID_ENTRY_WAKEUP_INTR;
>>  			pi_data.is_running = false;
>> +
>> +			if (amd_iommu_gappi) {
>> +				pi_data.apicid = kvm_cpu_get_apicid(svm->gappi_cpu);
>> +				if (list_empty(&svm->ir_list)) {
>> +					avic_add_vcpu_to_gappi_wakeup_list(svm, svm->gappi_cpu);
>> +					is_vcpu_waiting = true;
>> +				}
>> +			}
>>  		}
>>  
>>  		ret = irq_set_vcpu_affinity(host_irq, &pi_data);
>>  		if (ret)
>> -			return ret;
>> +			goto gappi_err_out;
> 
> [Severity: Low]
> Is it safe to mix goto-based error handling in a function that uses guard()?
> 
> Earlier in avic_pi_update_irte(),
> guard(raw_spinlock_irqsave)(&svm->ir_list_lock) is used. According to
> include/linux/cleanup.h, mixing goto with guard() in the same function creates
> confusing ownership semantics and violates the subsystem's cleanup rules.
> 
>>  
>>  		/*
>>  		 * Revert to legacy mode if the IOMMU didn't provide metadata
>> @@ -968,12 +1028,17 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>>  		 */
>>  		if (WARN_ON_ONCE(!pi_data.ir_data)) {
>>  			irq_set_vcpu_affinity(host_irq, NULL);
>> -			return -EIO;
>> +			ret = -EIO;
>> +			goto gappi_err_out;
>>  		}
>>  
>>  		irqfd->irq_bypass_data = pi_data.ir_data;
>>  		list_add(&irqfd->vcpu_list, &svm->ir_list);
>>  		return 0;
>> +gappi_err_out:
>> +		if (is_vcpu_waiting)
>> +			avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);
>> +		return ret;
>>  	}
>>  	return irq_set_vcpu_affinity(host_irq, NULL);
>>  }
> [ ... ]
>> @@ -1300,6 +1382,21 @@ static bool __init avic_want_avic_enabled(void)
>>  	return true;
>>  }
>>  
>> +static void avic_gappi_wakeup_handler(void)
>> +{
>> +	int cpu = smp_processor_id();
>> +	struct list_head *vcpu_wakeup_list = &per_cpu(gappi_vcpu_wakeup_list, cpu);
>> +	raw_spinlock_t *spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
>> +	struct vcpu_svm *svm;
>> +
>> +	raw_spin_lock(spinlock);
>> +	list_for_each_entry(svm, vcpu_wakeup_list, gappi_vcpu_wakeup_list) {
>> +		if (kvm_lapic_find_highest_irr(&svm->vcpu) >= 0)
>> +			kvm_vcpu_wake_up(&svm->vcpu);
>> +	}
>> +	raw_spin_unlock(spinlock);
>> +}
> 
> [Severity: High]
> Does this lock ordering create an ABBA deadlock?
> 
> When a vCPU is preempted, the scheduler executes kvm_sched_out() while holding
> the runqueue lock (rq_lock). KVM then calls __avic_vcpu_put(), which in turn
> calls avic_add_vcpu_to_gappi_wakeup_list(). This acquires the
> gappi_vcpu_wakeup_list_lock, establishing an ordering of rq_lock ->
> gappi_vcpu_wakeup_list_lock.
> 
> Conversely, when a GAPPI interrupt arrives, avic_gappi_wakeup_handler()
> acquires gappi_vcpu_wakeup_list_lock and calls kvm_vcpu_wake_up(). The wakeup
> path then attempts to acquire the target task's runqueue lock, establishing
> the reverse order: gappi_vcpu_wakeup_list_lock -> rq_lock.
> 
> This inversion will likely trigger a lockdep ABBA deadlock splat and violates
> the scheduler locking hierarchy.

For this condition to exist, Following two conditions must be true.
1. avic_vcpu_put() and gappi interrupt handler() must be on same
physical CPU.
2. GAPPI must preempt the __avic_vcpu_put() once it has acquired
gappi_vcpu_wakeup_list_lock().

The second condition is not possible since __avic_vcpu_put() disables
the interrupt while aquiring the ir_list lock.

Hence ABBA deadlock is not possible

Thanks
Sairaj

> 
> [ ... ]
>> @@ -1308,12 +1405,18 @@ static bool __init avic_want_avic_enabled(void)
>>   */
>>  bool __init avic_hardware_setup(void)
>>  {
>> +	int cpu;
>>  	avic = avic_want_avic_enabled();
>>  	if (!avic)
>>  		return false;
>>  
>>  	pr_info("AVIC enabled\n");
>>  
>> +	for_each_possible_cpu(cpu) {
>> +		INIT_LIST_HEAD(&per_cpu(gappi_vcpu_wakeup_list, cpu));
>> +		raw_spin_lock_init(&per_cpu(gappi_vcpu_wakeup_list_lock, cpu));
>> +	}
> 
> [Severity: High]
> Are blocked vCPUs left stranded if their physical CPU is taken offline?
> 
> The per-CPU wakeup lists queue blocked vCPUs on the physical CPU they last ran
> on. However, there are no CPU hotplug callbacks registered. If a physical CPU
> is taken offline, the IOMMU hardware will not deliver GAPPI interrupts for
> that offlined CPU's APIC ID.
> 
> Since there is no mechanism to migrate the stranded vCPUs to an online CPU's
> wakeup list during the offline event, these vCPUs could hang indefinitely.
>