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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 10/15/2026 12:35 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] kvm: svm: vCPU leaked on GAPPI pi_wakeup_list when AVIC deactivated
> --
> 
> commit cd7fa3c00904984a2127bee934d4e48326ead7a8
> 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 8b1fa5bded3db..054b89c3cb0f9 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
> [ ... ]
>> @@ -1018,7 +1038,7 @@ enum avic_vcpu_action {
>>  };
>>  
>>  static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
>> -					    enum avic_vcpu_action action)
>> +					    int cpu, enum avic_vcpu_action action)
>>  {
>>  	bool wakeup_intr = (action & AVIC_START_BLOCKING);
>>  	bool is_running = apicid >= 0;
>> @@ -1027,6 +1047,14 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
>>  
>>  	lockdep_assert_held(&svm->ir_list_lock);
>>  
>> +	if (amd_iommu_gappi && is_running) {
>> +		kvm_pi_disable_wakeup_handler(vcpu, svm->gappi_cpu);
>> +		svm->gappi_cpu = cpu; /* Store cpu number as target for GAPPI */
>> +	} else if (amd_iommu_gappi) {
>> +		apicid = cpu_physical_id(svm->gappi_cpu);
>> +		kvm_pi_enable_wakeup_handler(vcpu, svm->gappi_cpu);
>> +	}
>> +
> 
> [Severity: Medium]
> Does this unconditionally add the vCPU to the pi_wakeup_list when is_running is
> false, even during AVIC_DEACTIVATE?
> 
> When AVIC is deactivated via avic_refresh_apicv_exec_ctrl(), it calls
> __avic_vcpu_put(AVIC_DEACTIVATE), which in turn calls
> avic_update_iommu_vcpu_affinity() with a target apicid of -1 (so is_running
> is false). This causes KVM to add the vCPU to the pi_wakeup_list of
> svm->gappi_cpu.
> 
> Since AVIC is deactivated, the IOMMU falls back to legacy mode later in this
> function:
> 
> 	if (!(action & AVIC_TOGGLE_ON_OFF))
> 		WARN_ON_ONCE(amd_iommu_update_ga(...));
> 	else if (is_running)
> 		WARN_ON_ONCE(amd_iommu_activate_guest_mode(...));
> 	else
> 		WARN_ON_ONCE(amd_iommu_deactivate_guest_mode(data));
> 
> Because the IOMMU is reverted to legacy mode, it will not send GAPPI interrupts
> for this vCPU.
> 
> However, the vCPU remains indefinitely on the per-CPU pi_wakeup_list because
> subsequent context switches (via svm_vcpu_load()) ignore deactivated vCPUs and
> will never call kvm_pi_disable_wakeup_handler().
> 
> Could this cause KVM to unnecessarily traverse the leaked vCPU on every GAPPI
> interrupt targeted at that pCPU from other active VMs, wasting CPU cycles and
> risking spurious wakeups?
> 

You are right about this, it can cause spurious wakeups. We can simply
skip list operations during activate and deactivate.

Thanks
Sairaj