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

On 7/13/2026 4:41 PM, [email protected] 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 <[email protected]>
> 
>     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.

This is a valid concern.

If host userspace attaches a bypass IRQ targeting a vCPU that has never
been loaded. Functionally, there is nothing to do in that window. A vCPU
that has never been loaded cannot be blocking, so no GAPPI wakeup is
required. The IOMMU still posts the interrupt into the vAPIC backing
page, and the pending IRR is evaluated at the first VMRUN after
avic_vcpu_load(), which is also where the IRTE gets a valid Destination
and IsRun = 1.

This can be resolved by assigning a arbitrary gappi destination, without
actually updating the gappi wakeup list of that CPU.

Thanks
Sairaj

> 
> [ ... ]
>> @@ -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.
> 

Valid concern

Will remove all the gotos in this function.

Thanks
Sairaj

>>  
>>  		/*
>>  		 * 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);
>> +}
> 

> [ ... ]
>> @@ -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.
>