Re: [PATCH v7 23/36] KVM: x86: Re-synchronize TSC after KVM_SET_TSC_KHZ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM_SET_TSC_KHZ unconditionally sets user_set_tsc = true during vCPU creation, breaking the 1-second slop hack workaround for live migrating young VMs.
- [High] Lockless TOCTOU race when synchronizing the guest TSC causes guest clock stalls or backwards jumps during preemption.
--

--- Patch [23]: [PATCH v7 23/36] KVM: x86: Re-synchronize TSC after KVM_SET_TSC_KHZ ---
commit 7a6fbf4fe1dbd0e60f9a3418fa5cc8be64f86841
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Re-synchronize TSC after KVM_SET_TSC_KHZ

KVM_SET_TSC_KHZ changes the vCPU's TSC scaling ratio but does not
update the VM-wide cur_tsc_scaling_ratio used by get_kvmclock().
This commit calls kvm_synchronize_tsc() after changing the TSC
frequency to ensure the TSC value is continuous and updates the scaling ratio.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1942ccadb7a50..57494812cc839 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1061,7 +1061,17 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
>  			 user_tsc_khz, thresh_lo, thresh_hi);
>  		use_scaling = 1;
>  	}
> -	return set_tsc_khz(vcpu, user_tsc_khz, use_scaling);
> +	/*
> +	 * Read the guest TSC before changing the ratio, so we can
> +	 * re-synchronize to preserve continuity across the change.
> +	 */
> +	tsc = kvm_read_l1_tsc(vcpu, rdtsc());

[Severity: High]
Can this lockless read cause guest clock stalls or backwards jumps?

If the thread is preempted between this lockless rdtsc() read and the 
lock acquisition in kvm_synchronize_tsc(), the delay is lost.

kvm_synchronize_tsc() takes a new rdtsc() reading under tsc_write_lock:

arch/x86/kvm/x86.c:__kvm_synchronize_tsc() {
    ...
    raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
    ...
    host_tsc = rdtsc();
    ...
}

Will this effectively stall the guest TSC for the duration of any preemption
delay, shifting it backwards relative to other vCPUs and the host clock?

> +
> +	if (set_tsc_khz(vcpu, user_tsc_khz, use_scaling))
> +		return -1;
> +
> +	kvm_synchronize_tsc(vcpu, &tsc);

[Severity: High]
Does passing a non-NULL pointer to kvm_synchronize_tsc() here inadvertently 
break the 1-second slop hack workaround for live migrating young VMs?

Since kvm_set_tsc_khz() is called unconditionally during vCPU creation via
kvm_arch_vcpu_create()->kvm_set_tsc_khz(), passing &tsc causes 
kvm_synchronize_tsc() to treat this as a user-initiated write and globally 
set user_set_tsc = true for every new VM:

arch/x86/kvm/x86.c:__kvm_synchronize_tsc() {
    ...
    if (user_set_tsc)
        vcpu->kvm->arch.user_set_tsc = true;
    ...
}

Would this defeat the flag's purpose, causing the 1-second slop hack to 
incorrectly trigger during migration and overwrite an explicitly restored 
guest TSC with a value synced to KVM's dummy creation time?

> +	return 0;
>  }
>  
>  static u64 compute_guest_tsc(struct kvm_vcpu *vcpu, s64 kernel_ns)

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