Re: [RFC PATCH v3 3/6] iommu/amd: KVM: SVM: Add explicit vCPU running state to IOMMU interface

[Sairaj Kodilkar <sarunkod@amd.com>]

On 7/13/2026 4:38 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] KVM continues to infer the vCPU running state via `is_running = apicid >= 0`, completely defeating the purpose of the newly decoupled IOMMU API.
> --
> 
> --- Patch [3]: [RFC PATCH v3 3/6] iommu/amd: KVM: SVM: Add explicit vCPU running state to IOMMU interface ---
> commit bbb2175fc26e0d5c105e610aeee4bcf1fe796a2d
> Author: Sairaj Kodilkar <sarunkod@amd.com>
> iommu/amd: KVM: SVM: Add explicit vCPU running state to IOMMU interface
> 
> This commit adds an explicit is_running parameter to the AMD IOMMU guest
> mode interface. It states that this decoupling is necessary for GAPPI to
> pass a valid destination apicid when a vCPU is not running. The change
> intends to allow KVM to pass the vCPU running state explicitly rather than
> inferring it from the apicid value.
> 
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 8e87a7f2f64f7..3b2d92a587098 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
> [ ... ]
>> @@ -1008,6 +1010,7 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
>>  					    enum avic_vcpu_action action)
>>  {
>>  	bool wakeup_intr = (action & AVIC_START_BLOCKING);
>> +	bool is_running = apicid >= 0;
> 
> [Severity: Medium]
> Does this assignment defeat the purpose of the API decoupling described
> in the commit message?
> 
> The commit message states that KVM must supply a valid apicid even when
> the vCPU is not running, and that we can no longer derive the vCPU running
> state from the apicid alone.
> 
> However, by defining is_running as apicid >= 0 here in
> avic_update_iommu_vcpu_affinity(), aren't we continuing to infer the
> running state directly from the apicid?
> 
> If a caller attempts to pass a valid GAPPI destination APIC ID for a
> scheduled-out vCPU, won't this helper erroneously force is_running to true?
> Because of this hardcoded logic, other KVM paths handling scheduled-out
> vCPUs would be forced to continue passing -1 for the APIC ID rather than
> a valid GAPPI destination.
> 

No — this does not defeat the API decoupling. The change is that the
IOMMU driver no longer infers vCPU running state from apicid.
Previously, __amd_iommu_update_ga() used apicid >= 0 to decide
IRTE[IsRun] and destination programming. That breaks with GAPPI, where a
scheduled-out vCPU still needs a valid IRTE[Destination] while
IRTE[IsRun] is clear. The new API takes an explicit is_running parameter
so the IOMMU can program both fields independently.

The is_running = apicid >= 0 assignment in
avic_update_iommu_vcpu_affinity() is internal SVM logic, not part of the
IOMMU API. That helper is private to avic.c and only called from
__avic_vcpu_load() (running pCPU APIC ID) and __avic_vcpu_put() (apicid
= -1). Within SVM, apicid == -1 still means the vCPU is not running.
When GAPPI is enabled on that path, SVM derives the GAPPI destination
from gappi_cpu, substitutes it into apicid, and calls the IOMMU with
is_running = false.

Scheduled-out callers pass -1 to this helper by convention; SVM
translates that into (valid GAPPI apicid, is_running = false) before
reaching the IOMMU. The inference moved from the IOMMU layer to SVM,
where vCPU lifecycle is already known. avic_pi_update_irte() follows the
same model: it sets is_running explicitly and fills GAPPI apicid
separately when the vCPU is not running.

Thanks
Sairaj

>>  	struct vcpu_svm *svm = to_svm(vcpu);
>>  	struct kvm_kernel_irqfd *irqfd;
>>  
> [ ... ]
>> @@ -1024,9 +1027,10 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
>>  		void *data = irqfd->irq_bypass_data;
>>  
>>  		if (!(action & AVIC_TOGGLE_ON_OFF))
>> -			WARN_ON_ONCE(amd_iommu_update_ga(data, apicid, wakeup_intr));
>> -		else if (apicid >= 0)
>> -			WARN_ON_ONCE(amd_iommu_activate_guest_mode(data, apicid, wakeup_intr));
>> +			WARN_ON_ONCE(amd_iommu_update_ga(data, apicid, wakeup_intr, is_running));
>> +		else if (is_running)
>> +			WARN_ON_ONCE(amd_iommu_activate_guest_mode(data, apicid, wakeup_intr,
>> +								   is_running));
>>  		else
>>  			WARN_ON_ONCE(amd_iommu_deactivate_guest_mode(data));
>>  	}
>