Re: [PATCH v7 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
issues to consider:
- [Medium] KVM: SVM: circular lock dependency in __avic_vcpu_load() with
  GAPPI
- [Medium] KVM: SVM: non-blocking vCPUs queued on wakeup list on
  AVIC_STOP_RUNNING
- [Low] KVM: SVM: stale commit message and cleanup churn in
  avic_hardware_unsetup()
--

commit 569af3da1a9a9cddca4d1455d68973d185c0555b
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) to wake blocked vCPUs via POSTED_INTR_WAKEUP_VECTOR instead
of the legacy GA log mechanism.

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index b975f08825ce2..ebfd74adc4cbd 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,36 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
>  
>  	lockdep_assert_held(&svm->ir_list_lock);
>  
> +	if (!amd_iommu_gappi)
> +		goto skip_wakeup_handler;
> +
> +	if (action & AVIC_TOGGLE_ON_OFF) {
> +		/*
> +		 * When AVIC is disabled, IOMMU switches to legacy IRTE and
> +		 * does not generate 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.
> +		 */
> +		if (is_running)
> +			/*
> +			 * Store the CPU so that next call with
> +			 * AVIC_STOP_RUNNING can add the vCPU on to
> +			 * appropriate wakeup list.
> +			 */
> +			svm->gappi_cpu = cpu;
> +		goto skip_wakeup_handler;
> +	}
> +
> +	if (is_running) {
> +		kvm_pi_disable_wakeup_handler(vcpu, svm->gappi_cpu);
> +		svm->gappi_cpu = cpu;

[Severity: Medium]
Does calling kvm_pi_disable_wakeup_handler() while holding svm->ir_list_lock
introduce a lockdep circular locking dependency with the scheduler runqueue
lock?

CPU0 (<hardirq>)        CPU1                     CPU2
----                    ----                     ----
lock(wakeup_vcpus_on_cpu_lock)
                        lock(&rq->__lock)
                        lock(&svm->ir_list_lock)
                                                 lock(&svm->ir_list_lock)
                                                 lock(wakeup_vcpus_on_cpu_lock)
lock(&rq->__lock)

 *** DEADLOCK ***

CPU0: kvm_pi_wakeup_handler() -> kvm_vcpu_wake_up() -> try_to_wake_up()
CPU1: schedule() -> kvm_sched_out() -> ... -> __avic_vcpu_put()
CPU2: __avic_vcpu_load() -> ... -> kvm_pi_disable_wakeup_handler()

kvm_pi_enable_wakeup_handler() uses PI_LOCK_SCHED_OUT to avoid a cycle when
called under rq->__lock during kvm_sched_out(), while
kvm_pi_disable_wakeup_handler() acquires wakeup_vcpus_on_cpu_lock at
subclass 0 before acquiring PI_LOCK_SCHED_OUT.  Because __avic_vcpu_put()
acquires svm->ir_list_lock under rq->__lock when a running vCPU is preempted,
calling kvm_pi_disable_wakeup_handler() under svm->ir_list_lock in
__avic_vcpu_load() bridges rq->__lock to wakeup_vcpus_on_cpu_lock at
subclass 0 and triggers a lockdep splat on CONFIG_PROVE_LOCKING kernels.

> +	} else {
> +		apicid = cpu_physical_id(svm->gappi_cpu);
> +		kvm_pi_enable_wakeup_handler(vcpu, svm->gappi_cpu);
> +	}

[Severity: Medium]
Should kvm_pi_enable_wakeup_handler() only be called when wakeup_intr is true
(action & AVIC_START_BLOCKING)?

When a vCPU is preempted or exits to userspace without blocking,
avic_vcpu_put() calls __avic_vcpu_put(vcpu, AVIC_STOP_RUNNING), where
is_running is false and wakeup_intr is false.  Enqueuing the vCPU here puts
non-blocking vCPUs onto per_cpu(wakeup_vcpus_on_cpu, svm->gappi_cpu) on every
vcpu_put(), causing kvm_pi_wakeup_handler() to scan their vAPIC IRR bitmaps on
every wakeup interrupt and requiring every subsequent __avic_vcpu_load() to
take wakeup_vcpus_on_cpu_lock to remove them.

[ ... ]

> @@ -1308,6 +1358,13 @@ static bool __init avic_want_avic_enabled(void)
>  	return true;
>  }
>  
> +bool avic_vcpu_irq_pending(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	return kvm_lapic_find_highest_irr(&svm->vcpu) >= 0;
> +}
> +
>  /*
>   * Note:
>   * - The module param avic enable both xAPIC and x2APIC mode.
> @@ -1351,6 +1408,8 @@ bool __init avic_hardware_setup(void)
>  
>  void avic_hardware_unsetup(void)
>  {
> -	if (avic)
> -		amd_iommu_register_ga_log_notifier(NULL);
> +	if (!avic)
> +		return;
> +
> +	amd_iommu_register_ga_log_notifier(NULL);
>  }

[Severity: Low]
This isn't a bug, but the change in avic_hardware_unsetup() looks like
leftover churn from when wakeup handler unregistration lived in SVM rather
than common x86 code.  Along the same lines, the commit message states that
SVM registers the GAPPI handler via kvm_set_posted_intr_wakeup_handler() and
walks the local vCPU list, which was moved to common x86 code in earlier
commit "KVM: VMX: Factor out wakeup list handling code to KVM".  Also,
avic_vcpu_irq_pending() can pass vcpu directly to kvm_lapic_find_highest_irr()
without converting through to_svm(vcpu).

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