Re: [RFC PATCH v3 5/6] 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 4 potential issue(s) to consider:
- [High] Host-userspace-reachable WARN_ON due to uninitialized `svm->gappi_cpu` (-1) when configuring IRQ routing before a vCPU runs.
- [High] Missing CPU hotplug handling causes blocked vCPUs to hang if their last-run physical CPU goes offline.
- [High] ABBA deadlock between scheduler `rq_lock` and `gappi_vcpu_wakeup_list_lock` due to manipulating the wakeup list during vCPU preemption.
- [Low] Mixing scoped `guard()` cleanup with `goto` error handling violates architectural guidelines.
--

--- Patch [5]: [RFC PATCH v3 5/6] KVM: SVM: Add support for AMD IOMMU Guest APIC Physical Processor Interrupt (GAPPI) ---
commit c3bb8845bd944e9a327f36d68b70032d436f727c
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 3b2d92a587098..982a5988532ff 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -886,6 +892,42 @@ int avic_init_vcpu(struct vcpu_svm *svm)
>  	return ret;
>  }
>  
> +static void avic_add_vcpu_to_gappi_wakeup_list(struct vcpu_svm *svm, int cpu)
> +{
> +	struct list_head *wakeup_list;
> +	raw_spinlock_t *spinlock;
> +
> +	if (WARN_ON(unlikely(cpu < 0)))
> +		return;

[Severity: High]
Can an unprivileged user-space process trigger this WARN_ON?

When a vCPU is created, svm->gappi_cpu is initialized to -1 in
avic_init_vcpu(). It is only set to a valid CPU ID later during
__avic_vcpu_load().

If host user-space configures device interrupt routing via the KVM_IRQFD ioctl
before the vCPU runs, avic_pi_update_irte() is invoked. This will pass the
uninitialized svm->gappi_cpu (-1) down to this function, hitting the WARN_ON.
If the host has panic_on_warn enabled, this allows host userspace to trigger
a kernel panic.

[ ... ]
> @@ -947,6 +993,12 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>  		 * scheduled out, KVM will update the pCPU info when the vCPU
>  		 * is awakened and/or scheduled in.  See also avic_vcpu_load().
>  		 */
> +		if (amd_iommu_gappi)
> +			pi_data.ga_tag = POSTED_INTR_WAKEUP_VECTOR;
> +		else
> +			pi_data.ga_tag = AVIC_GATAG(to_kvm_svm(kvm)->avic_vm_id,
> +						    vcpu->vcpu_idx);
> +
>  		entry = svm->avic_physical_id_entry;
>  		if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
>  			pi_data.apicid = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> @@ -955,11 +1007,19 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>  			pi_data.apicid = -1;
>  			pi_data.wakeup_intr = entry & AVIC_PHYSICAL_ID_ENTRY_WAKEUP_INTR;
>  			pi_data.is_running = false;
> +
> +			if (amd_iommu_gappi) {
> +				pi_data.apicid = kvm_cpu_get_apicid(svm->gappi_cpu);
> +				if (list_empty(&svm->ir_list)) {
> +					avic_add_vcpu_to_gappi_wakeup_list(svm, svm->gappi_cpu);
> +					is_vcpu_waiting = true;
> +				}
> +			}
>  		}
>  
>  		ret = irq_set_vcpu_affinity(host_irq, &pi_data);
>  		if (ret)
> -			return ret;
> +			goto gappi_err_out;

[Severity: Low]
Is it safe to mix goto-based error handling in a function that uses guard()?

Earlier in avic_pi_update_irte(),
guard(raw_spinlock_irqsave)(&svm->ir_list_lock) is used. According to
include/linux/cleanup.h, mixing goto with guard() in the same function creates
confusing ownership semantics and violates the subsystem's cleanup rules.

>  
>  		/*
>  		 * Revert to legacy mode if the IOMMU didn't provide metadata
> @@ -968,12 +1028,17 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>  		 */
>  		if (WARN_ON_ONCE(!pi_data.ir_data)) {
>  			irq_set_vcpu_affinity(host_irq, NULL);
> -			return -EIO;
> +			ret = -EIO;
> +			goto gappi_err_out;
>  		}
>  
>  		irqfd->irq_bypass_data = pi_data.ir_data;
>  		list_add(&irqfd->vcpu_list, &svm->ir_list);
>  		return 0;
> +gappi_err_out:
> +		if (is_vcpu_waiting)
> +			avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);
> +		return ret;
>  	}
>  	return irq_set_vcpu_affinity(host_irq, NULL);
>  }
[ ... ]
> @@ -1300,6 +1382,21 @@ static bool __init avic_want_avic_enabled(void)
>  	return true;
>  }
>  
> +static void avic_gappi_wakeup_handler(void)
> +{
> +	int cpu = smp_processor_id();
> +	struct list_head *vcpu_wakeup_list = &per_cpu(gappi_vcpu_wakeup_list, cpu);
> +	raw_spinlock_t *spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
> +	struct vcpu_svm *svm;
> +
> +	raw_spin_lock(spinlock);
> +	list_for_each_entry(svm, vcpu_wakeup_list, gappi_vcpu_wakeup_list) {
> +		if (kvm_lapic_find_highest_irr(&svm->vcpu) >= 0)
> +			kvm_vcpu_wake_up(&svm->vcpu);
> +	}
> +	raw_spin_unlock(spinlock);
> +}

[Severity: High]
Does this lock ordering create an ABBA deadlock?

When a vCPU is preempted, the scheduler executes kvm_sched_out() while holding
the runqueue lock (rq_lock). KVM then calls __avic_vcpu_put(), which in turn
calls avic_add_vcpu_to_gappi_wakeup_list(). This acquires the
gappi_vcpu_wakeup_list_lock, establishing an ordering of rq_lock ->
gappi_vcpu_wakeup_list_lock.

Conversely, when a GAPPI interrupt arrives, avic_gappi_wakeup_handler()
acquires gappi_vcpu_wakeup_list_lock and calls kvm_vcpu_wake_up(). The wakeup
path then attempts to acquire the target task's runqueue lock, establishing
the reverse order: gappi_vcpu_wakeup_list_lock -> rq_lock.

This inversion will likely trigger a lockdep ABBA deadlock splat and violates
the scheduler locking hierarchy.

[ ... ]
> @@ -1308,12 +1405,18 @@ static bool __init avic_want_avic_enabled(void)
>   */
>  bool __init avic_hardware_setup(void)
>  {
> +	int cpu;
>  	avic = avic_want_avic_enabled();
>  	if (!avic)
>  		return false;
>  
>  	pr_info("AVIC enabled\n");
>  
> +	for_each_possible_cpu(cpu) {
> +		INIT_LIST_HEAD(&per_cpu(gappi_vcpu_wakeup_list, cpu));
> +		raw_spin_lock_init(&per_cpu(gappi_vcpu_wakeup_list_lock, cpu));
> +	}

[Severity: High]
Are blocked vCPUs left stranded if their physical CPU is taken offline?

The per-CPU wakeup lists queue blocked vCPUs on the physical CPU they last ran
on. However, there are no CPU hotplug callbacks registered. If a physical CPU
is taken offline, the IOMMU hardware will not deliver GAPPI interrupts for
that offlined CPU's APIC ID.

Since there is no mechanism to migrate the stranded vCPUs to an online CPU's
wakeup list during the offline event, these vCPUs could hang indefinitely.

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