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

[sashiko-bot@kernel.org]

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.

>  	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));
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713105033.15405-1-sarunkod@amd.com?part=3