Re: [RFC PATCH 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] `avic_remove_vcpu_from_gappi_wakeup_list` and `avic_add_vcpu_to_gappi_wakeup_list` acquire a raw spinlock without disabling interrupts, leading to a hard deadlock with the GAPPI IPI hardirq handler.
- [High] Uninitialized `svm->gappi_cpu` (-1) is unconditionally passed to `kvm_cpu_get_apicid()` and `avic_add_vcpu_to_gappi_wakeup_list()`, causing an out-of-bounds array read and a userspace-triggerable `WARN_ON`.
- [High] The GAPPI wakeup handler is registered with the core kernel but never unregistered, leading to a dangling pointer if the KVM module is unloaded.
- [Medium] The per-CPU GAPPI wakeup lists lack CPU hotplug support, resulting in permanently lost device interrupts and hanging vCPUs if a CPU is offlined.
--

commit 3af48ba2bd8326c04b2aa47ac47a9f53e0cd61a3
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.

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index e7a4c0e90e7a7..d238f65a8172b 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -889,6 +892,44 @@ 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(cpu < 0))
> +		return;
> +
> +	wakeup_list = &per_cpu(gappi_vcpu_wakeup_list, cpu);
> +	spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
> +	raw_spin_lock(spinlock);
> +	if (list_empty(&svm->gappi_vcpu_wakeup_list))
> +		list_add_tail(&svm->gappi_vcpu_wakeup_list, wakeup_list);
> +	raw_spin_unlock(spinlock);
> +}
> +
> +static void avic_remove_vcpu_from_gappi_wakeup_list(struct vcpu_svm *svm, int cpu)
> +{
> +	raw_spinlock_t *spinlock;
> +
> +	if (WARN_ON(cpu < 0))
> +		return;
> +
> +	spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
> +	raw_spin_lock(spinlock);
> +	if (!list_empty(&svm->gappi_vcpu_wakeup_list))
> +		list_del_init(&svm->gappi_vcpu_wakeup_list);
> +	raw_spin_unlock(spinlock);
> +}

[Severity: High]
Can this cause a hard deadlock with the GAPPI IPI hardirq handler?

When a user process destroys a VM, avic_destroy_vcpu() is called from
svm_vcpu_free() in normal thread context with interrupts enabled. If the
POSTED_INTR_WAKEUP_VECTOR IPI fires on the same CPU while this thread holds
the gappi_vcpu_wakeup_list_lock non-recursive raw spinlock, the interrupt
handler will spin forever attempting to acquire it.

Should these functions use raw_spin_lock_irqsave() to disable interrupts
during lock acquisition?

[ ... ]

> @@ -958,16 +1005,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]
Is it possible for svm->gappi_cpu to still be -1 here?

If userspace creates a VM but inhibits AVIC (for example, via an invalid
APIC ID), avic_vcpu_load() bails out early leaving svm->gappi_cpu as -1.
If userspace later assigns a device interrupt via KVM_IRQFD,
avic_pi_update_irte() will pass -1 to kvm_cpu_get_apicid().

This triggers a negative index array read yielding a garbage APIC ID, and
also hits the WARN_ON(cpu < 0) inside avic_add_vcpu_to_gappi_wakeup_list().

[ ... ]

> @@ -1353,6 +1436,7 @@ bool __init avic_hardware_setup(void)
>  		enable_ipiv = false;
>  
>  	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> +	kvm_set_posted_intr_wakeup_handler(&avic_gappi_wakeup_handler);
>  
>  	return true;
>  }

[Severity: Medium]
Does the per-CPU GAPPI wakeup list need CPU hotplug coordination?

If a physical CPU is taken offline while a VM is running, any blocked
vCPUs queued on that offlined CPU's gappi_vcpu_wakeup_list might permanently
miss their GAPPI wakeups. Without a hotplug state notifier to migrate these
vCPUs, will they hang indefinitely?

[Severity: High]
Is there a missing module unload teardown for the wakeup handler?

This registers avic_gappi_wakeup_handler with the core kernel, but it
doesn't seem to be unregistered (via kvm_set_posted_intr_wakeup_handler(NULL))
during module teardown in avic_hardware_unsetup(). If the kvm-amd module
is unloaded, could a subsequent POSTED_INTR_WAKEUP_VECTOR jump to unmapped
memory and cause a kernel panic?

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