Re: [PATCH v5 5/7] KVM: VMX: Factor out wakeup list handling code to KVM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: uninitialized return value from vcpu_irq_pending due to KVM_X86_OP_OPTIONAL
--

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

KVM: VMX: Factor out wakeup list handling code to KVM

This commit moves the per-pCPU list of blocked vCPUs and the wakeup
handler from VMX to common KVM x86 code to support AMD. It introduces
a new optional vcpu_irq_pending callback so Intel and AMD can
determine if a queued vCPU has a notification pending.

> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index e213c9ae3e301..5d337f9336c5d 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -30,6 +30,7 @@ KVM_X86_OP(vcpu_reset)
>  KVM_X86_OP(prepare_switch_to_guest)
>  KVM_X86_OP(vcpu_load)
>  KVM_X86_OP(vcpu_put)
> +KVM_X86_OP_OPTIONAL(vcpu_irq_pending)

[Severity: Medium]
Since vcpu_irq_pending() returns a boolean, should this be declared with
KVM_X86_OP_OPTIONAL_RET0 instead?

For optional KVM operations, if the active vendor module does not implement
the callback, KVM patches the static call site to a dummy trampoline. Because
KVM_X86_OP_OPTIONAL is used here, the trampoline does not clear the return
register.

[ ... ]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index aad065d035fbb..0c637a18bcfa4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -2688,6 +2709,79 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)

[ ... ]

> +/*
> + * Handler for POSTED_INTERRUPT_WAKEUP_VECTOR.
> + */
> +static void kvm_pi_wakeup_handler(void)
> +{
> +	int cpu = smp_processor_id();
> +	struct list_head *wakeup_list = &per_cpu(wakeup_vcpus_on_cpu, cpu);
> +	raw_spinlock_t *spinlock = &per_cpu(wakeup_vcpus_on_cpu_lock, cpu);
> +	struct kvm_vcpu *vcpu;
> +
> +	raw_spin_lock(spinlock);
> +	list_for_each_entry(vcpu, wakeup_list, pi_wakeup_list) {
> +		if (kvm_x86_call(vcpu_irq_pending)(vcpu))

[Severity: Medium]
Could this evaluate uninitialized garbage if the vendor omits the callback?

Because KVM_X86_OP_OPTIONAL does not clear the return register, this check
will read whatever was left in the register. This could lead to unpredictable
behavior, such as spurious vCPU wakeups or missed wakeups on platforms
lacking the implementation.

> +			kvm_vcpu_wake_up(vcpu);
> +	}
> +	raw_spin_unlock(spinlock);
> +}

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