Re: [PATCH v5 26/34] KVM: x86: Avoid redundant masterclock updates from multiple vCPUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] O(N) vCPU iteration loops moved inside a raw spinlock and seqcount writer critical section, causing severe O(N^2) latency spikes.
- [High] Unilateral clearing of KVM_REQ_MCLOCK_INPROGRESS in the bailout path creates a race condition allowing premature guest entries.
--

commit c617628fa6cacec7b404452ff34e7359195be099
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Fix redundant pvclock updates

When a masterclock update is triggered, KVM_REQ_MASTERCLOCK_UPDATE is set on
all vCPUs. This patch adds a check in __kvm_start_pvclock_update() to prevent
redundant updates by having it bail out if another vCPU already processed the
request. It also clears the request on all other vCPUs once the lock is
acquired.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3288,10 +3288,39 @@ static void kvm_make_mclock_inprogress_request(struct kvm *kvm)
>  	kvm_make_all_cpus_request(kvm, KVM_REQ_MCLOCK_INPROGRESS);
>  }
>  
> -static void __kvm_start_pvclock_update(struct kvm *kvm)
> +static void kvm_clear_mclock_inprogress_request(struct kvm *kvm)
>  {
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
> +}
> +
> +static bool __kvm_start_pvclock_update(struct kvm *kvm, struct kvm_vcpu *requesting_vcpu)
> +{
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
>  	raw_spin_lock_irq(&kvm->arch.tsc_write_lock);
> +
> +	/*
> +	 * If another vCPU already did the update while we were waiting
> +	 * for the lock, our request will have been cleared. Bail out.
> +	 */
> +	if (requesting_vcpu &&
> +	    !kvm_test_request(KVM_REQ_MASTERCLOCK_UPDATE, requesting_vcpu)) {
> +		kvm_clear_mclock_inprogress_request(kvm);
> +		raw_spin_unlock_irq(&kvm->arch.tsc_write_lock);
> +		return false;
> +	}

[Severity: High]
Does this bailout path risk prematurely clearing the protection set by
another concurrent update?

If a concurrent masterclock update is initiated (for example, by
kvm_arch_vcpu_postcreate() calling kvm_update_masterclock() with a NULL
requesting_vcpu), that thread will set KVM_REQ_MCLOCK_INPROGRESS to
prevent guest entries.

If the bailing vCPU holds the lock, sees its bailout condition met, and
calls kvm_clear_mclock_inprogress_request(), could this clear the newly
set protection flag globally? This might leave the system without
protection and allow other vCPUs to enter the guest without
KVM_REQ_CLOCK_UPDATE being set, causing them to observe an inconsistent
clock state.

[Severity: High]
Is it safe to execute this O(N) vCPU iteration loop inside the
tsc_write_lock raw spinlock?

Since kvm_clear_mclock_inprogress_request() iterates over all vCPUs, and
up to N vCPUs might sequentially take this bailout path, could this lead
to O(N^2) lock hold times with IRQs disabled? In large VMs, this might
result in significant latency spikes and potential soft lockups.

> +
> +	/* The update is VM-wide; prevent other vCPUs from redoing it. */
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		kvm_clear_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

[Severity: High]
Similarly, does adding this O(N) vCPU iteration inside the raw spinlock
create a risk of latency spikes on the fast path for large VMs?

[ ... ]

>  static void kvm_end_pvclock_update(struct kvm *kvm)
>  {
>  	struct kvm_arch *ka = &kvm->arch;
>  	struct kvm_vcpu *vcpu;
>  	unsigned long i;
>  
> -	write_seqcount_end(&ka->pvclock_sc);
> -	raw_spin_unlock_irq(&ka->tsc_write_lock);
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>  
>  	/* guest entries allowed */
> -	kvm_for_each_vcpu(i, vcpu, kvm)
> -		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
> +	kvm_clear_mclock_inprogress_request(kvm);
> +
> +	write_seqcount_end(&ka->pvclock_sc);
> +	raw_spin_unlock_irq(&ka->tsc_write_lock);
>  }

[Severity: High]
Does moving write_seqcount_end() and raw_spin_unlock_irq() after the two
O(N) vCPU iteration loops risk stalling seqcount readers indefinitely?

Performing O(N) atomic bit operations inside the seqcount writer critical
section and with IRQs disabled could cause severe latency spikes, RT
constraint violations, and potentially stall readers spinning on the
seqcount in NMI context.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608145455.89187-1-dwmw2@infradead.org?part=26