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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] KVM: SVM: avic_update_iommu_vcpu_affinity() sets IRTE destination to -1
- [High] KVM: SVM: avic_update_iommu_vcpu_affinity() skips gappi_cpu on activate
- [High] KVM: SVM: circular lock dependency in __avic_vcpu_load() with GAPPI
--

commit eadb6986c75d5c64b26f6497306bb267961e5f78
Author: Sairaj Kodilkar <sarunkod@amd.com>

KVM: SVM: Add support for AMD IOMMU Guest APIC Physical Processor Interrupt (GAPPI)

This commit adds KVM SVM support for AMD IOMMU Guest APIC Physical Processor
Interrupt (GAPPI) as an alternative to the legacy GA log mechanism for waking
non-running vCPUs when device interrupts are posted to the guest vAPIC
backing page. When a vCPU blocks, SVM places it on the previous pCPU's posted
interrupt wakeup list and programs the IOMMU IRTE destination with that
pCPU's physical APIC ID and POSTED_INTR_WAKEUP_VECTOR.

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index b975f08825ce2..baeca0ad8dfee 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,25 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
>  
>  	lockdep_assert_held(&svm->ir_list_lock);
>  
> +	if (action & AVIC_TOGGLE_ON_OFF)
> +		/*
> +		 * When AVIC is disabled, IOMMU switches to legacy IRTE and
> +		 * does not genrate wakeup notification. Hence there is no
> +		 * need to add vCPU on wakeup list. This reduces the number of
> +		 * vCPUs on the list, effectively reducing interrupt handler
> +		 * time and avoiding spurious wakeup to the vCPU.
> +		 */
> +		goto skip_wakeup_handler;

[Severity: High]
When AVIC is activated on a vCPU via avic_refresh_apicv_exec_ctrl() ->
__avic_vcpu_load(vcpu, vcpu->cpu, AVIC_ACTIVATE), action is AVIC_ACTIVATE
(which is aliased to AVIC_TOGGLE_ON_OFF). Won't jumping to
skip_wakeup_handler here skip updating svm->gappi_cpu = cpu?

While APICv is inactive or temporarily inhibited (for example at vCPU startup
before the guest LAPIC is enabled, or due to nested virtualization or IRQ
window inhibits), svm_vcpu_load() in arch/x86/kvm/svm/svm.c does not call
avic_vcpu_load() when the vCPU migrates to another pCPU:

	if (kvm_vcpu_apicv_active(vcpu))
		avic_vcpu_load(vcpu, cpu);

When APICv is later enabled or uninhibited on the current pCPU,
avic_refresh_apicv_exec_ctrl() calls __avic_vcpu_load(vcpu, vcpu->cpu,
AVIC_ACTIVATE). Because AVIC_TOGGLE_ON_OFF jumps over svm->gappi_cpu = cpu,
svm->gappi_cpu remains at its previous value (or 0 if the vCPU started on a
non-zero pCPU).

If the vCPU then blocks via avic_vcpu_blocking() ->
__avic_vcpu_put(vcpu, AVIC_START_BLOCKING) before another schedule-out and
schedule-in cycle, or if avic_pi_update_irte() runs while it is not running,
won't KVM enqueue the vCPU on the wrong pCPU's wakeup_vcpus_on_cpu list and
program IRTE[Destination] with the stale pCPU's physical APIC ID (which may
even belong to an offline CPU if the previous pCPU was offlined while APICv
was inhibited)?

> +
> +	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 && wakeup_intr) {
> +		apicid = cpu_physical_id(svm->gappi_cpu);
> +		kvm_pi_enable_wakeup_handler(vcpu, svm->gappi_cpu);
> +	}

[Severity: High]
What happens when a running vCPU with an assigned device is scheduled out
without blocking (for example, preempted via kvm_sched_out() ->
kvm_arch_vcpu_put() -> svm_vcpu_put() -> avic_vcpu_put(), or put via
vcpu_put() on exit to userspace)?

In that path, avic_vcpu_put() calls __avic_vcpu_put(vcpu, AVIC_STOP_RUNNING),
which invokes avic_update_iommu_vcpu_affinity(vcpu, -1, -1,
AVIC_STOP_RUNNING). Both is_running (apicid >= 0) and wakeup_intr
(action & AVIC_START_BLOCKING) are false, so apicid is not updated to
cpu_physical_id(svm->gappi_cpu) and remains -1 (unlike avic_pi_update_irte(),
which sets pi_data.apicid = cpu_physical_id(svm->gappi_cpu) whenever
!is_running && amd_iommu_gappi).

avic_update_iommu_vcpu_affinity() then passes apicid = -1 to
amd_iommu_update_ga(data, -1, false, false), where __amd_iommu_update_ga() in
drivers/iommu/amd/iommu.c unconditionally writes apicid into
IRTE[Destination]:

	if (amd_iommu_gappi) {
		entry->lo.fields_vapic.gappi_dis = !wakeup_intr &&
					check_feature2(FEATURE_GAPPIDISSUP);
		entry->lo.fields_vapic.is_run = false;
		entry->lo.fields_vapic.destination =
					APICID_TO_IRTE_DEST_LO(apicid);
		entry->hi.fields.destination =
					APICID_TO_IRTE_DEST_HI(apicid);
	}

Doesn't this program the live guest-mode IRTE destination fields to
0xffffffff (the broadcast APIC ID), causing the IOMMU to deliver broadcast
POSTED_INTR_WAKEUP_VECTOR interrupts across host CPUs on hardware that
supports GAPPI without FEATURE_GAPPIDISSUP?

> +
> +skip_wakeup_handler:
>  	/*
>  	 * Here, we go through the per-vcpu ir_list to update all existing
>  	 * interrupt remapping table entry targeting this vcpu.
> @@ -1093,7 +1132,7 @@ static void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu,
>  
>  	WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], entry);
>  
> -	avic_update_iommu_vcpu_affinity(vcpu, h_physical_id, action);
> +	avic_update_iommu_vcpu_affinity(vcpu, h_physical_id, cpu, action);
>  
>  	raw_spin_unlock_irqrestore(&svm->ir_list_lock, flags);
>  }
> @@ -1135,7 +1174,7 @@ static void __avic_vcpu_put(struct kvm_vcpu *vcpu, enum avic_vcpu_action action)
>  	 */
>  	raw_spin_lock_irqsave(&svm->ir_list_lock, flags);
>  
> -	avic_update_iommu_vcpu_affinity(vcpu, -1, action);
> +	avic_update_iommu_vcpu_affinity(vcpu, -1, -1, action);

[Severity: High]
Can calling kvm_pi_disable_wakeup_handler() inside
avic_update_iommu_vcpu_affinity() while holding svm->ir_list_lock in
__avic_vcpu_load() trigger a circular lock dependency with the scheduler and
posted-interrupt wakeup locks?

1. In the POSTED_INTR_WAKEUP_VECTOR interrupt handler,
   kvm_pi_wakeup_handler() in arch/x86/kvm/x86.c acquires
   wakeup_vcpus_on_cpu_lock at subclass 0 and calls kvm_vcpu_wake_up() ->
   rcuwait_wake_up() -> wake_up_process() -> try_to_wake_up(), which acquires
   p->pi_lock and rq->__lock:

   wakeup_vcpus_on_cpu_lock(0) -> p->pi_lock -> rq->__lock

2. When a vCPU is preempted during schedule(), prepare_task_switch() holds
   rq->__lock and invokes fire_sched_out_preempt_notifiers() ->
   kvm_sched_out() -> kvm_arch_vcpu_put() -> svm_vcpu_put() ->
   avic_vcpu_put() -> __avic_vcpu_put(), which acquires svm->ir_list_lock:

   rq->__lock -> svm->ir_list_lock

3. In __avic_vcpu_load() (called from svm_vcpu_load() and
   avic_vcpu_unblocking()), svm->ir_list_lock is held when
   avic_update_iommu_vcpu_affinity() calls
   kvm_pi_disable_wakeup_handler(vcpu, svm->gappi_cpu), which in
   arch/x86/kvm/x86.c acquires wakeup_vcpus_on_cpu_lock at subclass 0 before
   PI_LOCK_SCHED_OUT:

   svm->ir_list_lock -> wakeup_vcpus_on_cpu_lock(0)

Holding svm->ir_list_lock across kvm_pi_disable_wakeup_handler() bridges
rq->__lock to wakeup_vcpus_on_cpu_lock(0), defeating the PI_LOCK_SCHED_OUT
subclass separation and completing the lockdep cycle.

[ ... ]

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