Re: [RFC PATCH v2 4/5] kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing CPU hotplug teardown logic for `gappi_vcpu_wakeup_list` causes indefinitely stalled vCPUs when a physical CPU is taken offline.
- [High] Host-userspace reachable `WARN_ON(cpu < 0)` via uninitialized `svm->gappi_cpu` when setting up posted interrupts for AVIC-inhibited VMs.
- [High] Unconditional call to `avic_remove_vcpu_from_gappi_wakeup_list()` triggers a spurious `WARN_ON` when GAPPI is disabled.
- [Medium] Mixing `goto`-based error handling with `guard()` scope-based cleanup in `avic_pi_update_irte()` violates subsystem guidelines.
--

--- Patch [4]: [RFC PATCH v2 4/5] kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload ---
commit 943f44c8b99a66a1765989cd046e31b1a7a04469
Author: Sairaj Kodilkar <sarunkod@amd.com>

kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload

When a vCPU is unloaded from a physical CPU enqueue it on that CPU's
GAPPI wakeup list. Remove the vCPU from the wakeup list when it is loaded
on to a CPU again.

Also enqueue from avic_pi_update_irte() when vCPU is not running and
ir_list is still empty.  This handles the condition where vCPU load skips
the per-CPU wakeup-list update when ir_list is empty.

The GAPPI wakeup handler walks this CPU's list and wakes vCPUs that still
have a pending IRR.  Install it with kvm_set_posted_intr_wakeup_handler()
so deliveries on POSTED_INTR_WAKEUP_VECTOR invoke it.

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index e7a4c0e90e7a7..8185841d9b816 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c

[ ... ]

> @@ -899,13 +938,18 @@ static void svm_ir_list_del(struct kvm_kernel_irqfd *irqfd)
>  {
>  	struct kvm_vcpu *vcpu = irqfd->irq_bypass_vcpu;
>  	unsigned long flags;
> +	struct vcpu_svm *svm;
>  
>  	if (!vcpu)
>  		return;
>  
> -	raw_spin_lock_irqsave(&to_svm(vcpu)->ir_list_lock, flags);
> +	svm = to_svm(vcpu);
> +
> +	raw_spin_lock_irqsave(&svm->ir_list_lock, flags);
>  	list_del(&irqfd->vcpu_list);
> -	raw_spin_unlock_irqrestore(&to_svm(vcpu)->ir_list_lock, flags);
> +	if (list_empty(&svm->ir_list))
> +		avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);

[Severity: High]
If GAPPI is globally disabled (amd_iommu_gappi == false), svm->gappi_cpu
remains at its initial value of -1 for the vCPU's entire lifetime.

When the last irqfd is deleted in svm_ir_list_del(), this calls
avic_remove_vcpu_from_gappi_wakeup_list() unconditionally without checking
if GAPPI is actually enabled.

Since cpu is -1, does this trigger the WARN_ON(cpu < 0) when host userspace
detaches an irqfd or shuts down a VM with assigned devices?

> +	raw_spin_unlock_irqrestore(&svm->ir_list_lock, flags);
>  }
>  
>  int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,

[ ... ]

> @@ -958,16 +1003,18 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>  		} else {
>  			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
>  			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;
> -			/* GAPPI is disabled at this point (amd_iommu_gappi is
> -			 * enabled in the following patches) hence keep the
> -			 * apicid as 0.
> -			 */
> -			pi_data.apicid = 0;
> +			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;
> +				}
> +			}
>  		}

[Severity: High]
svm->gappi_cpu is initialized to -1. If AVIC is disabled or inhibited for
the VM, kvm_vcpu_apicv_active() is false and avic_vcpu_load() is never
called, leaving svm->gappi_cpu as -1.

When userspace configures an IRQFD and the global amd_iommu_gappi flag is
true, this passes -1 to avic_add_vcpu_to_gappi_wakeup_list().

Could this trigger the host-userspace reachable WARN_ON(cpu < 0) in
avic_add_vcpu_to_gappi_wakeup_list()?

>  
>  		ret = irq_set_vcpu_affinity(host_irq, &pi_data);
>  		if (ret)
> -			return ret;
> +			goto gappi_err_out;

[Severity: Medium]
This introduces goto-based error handling in avic_pi_update_irte(), which
already uses guard(raw_spinlock_irqsave)(&svm->ir_list_lock) earlier in
the function.

Does mixing goto-based cleanup labels and guard() declarations violate the
subsystem guidelines for cleanup function compatibility?

>  
>  		/*
>  		 * Revert to legacy mode if the IOMMU didn't provide metadata

[ ... ]

> @@ -1310,6 +1376,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]
The per-CPU wakeup list is processed here in avic_gappi_wakeup_handler(),
but this handler will never run for an offline CPU.

If a physical CPU is hot-unplugged, the IOMMU's GAPPI hardware doorbell
interrupts sent to that CPU are dropped.

Since there is no CPU hotplug teardown logic registered (e.g., via
cpuhp_setup_state() in avic_hardware_setup()) to drain the list or migrate
the vCPUs to an online CPU, will any vCPUs left on its list miss their
wakeup events and stall indefinitely?

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