Re: [PATCH v11 11/23] KVM: x86: Activate master clock from kvm_arch_init_vm()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] KVM: x86: unreachable scaling check in kvm_synchronize_tsc()
- [Medium] KVM: x86: premature TSC sync in kvm_arch_vcpu_create() corrupts freq match
- [Medium] KVM: x86: unserviced masterclock request in kvm_arch_vcpu_postcreate()
--

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

KVM: x86: Activate master clock from kvm_arch_init_vm()

This commit initializes the initial TSC generation and master clock state in
kvm_arch_init_vm() from an atomic ktime/TSC snapshot, and updates
kvm_synchronize_tsc() and kvm_arch_vcpu_postcreate() to synchronize vCPUs and
refresh the master clock against that initial generation.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9401685287215..6dbdb60962fe7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1500,11 +1500,25 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *value, bool user_initiated)
>  
>  		data = kvm->arch.cur_tsc_write + delta;
>  
> -		if (!kvm_check_tsc_unstable())
> -			offset = kvm->arch.cur_tsc_offset;
> -		else
> +		/*
> +		 * The generation's offset can only be joined by a vCPU which
> +		 * scales the TSC by the same ratio it was recorded with; the
> +		 * initial generation's offset is recorded at VM creation
> +		 * with the default ratio, before any vCPU exists. With a
> +		 * different ratio, open a new generation (recording this
> +		 * vCPU's own offset) rather than leave the generation's
> +		 * ratio and offset describing different scaling domains.
> +		 */
> +		if (vcpu->arch.l1_tsc_scaling_ratio !=
> +		    kvm->arch.cur_tsc_scaling_ratio) {
>  			offset = kvm_compute_l1_tsc_offset(vcpu, host_tsc, data);
> -		matched = true;
> +		} else {
> +			if (!kvm_check_tsc_unstable())
> +				offset = kvm->arch.cur_tsc_offset;
> +			else
> +				offset = kvm_compute_l1_tsc_offset(vcpu, host_tsc, data);
> +			matched = true;
> +		}
>  	}
>  
>  	__kvm_synchronize_tsc(vcpu, offset, data, ns, matched, user_initiated);

[Severity: Medium]
Can this scaling ratio check in kvm_synchronize_tsc() ever be reached?

The enclosing block in kvm_synchronize_tsc() is gated on matching
virtual_tsc_khz:

	if (synchronizing &&
	    vcpu->arch.virtual_tsc_khz == kvm->arch.cur_tsc_khz) {
		u64 delta = nsec_to_cycles(vcpu, elapsed);

		data = kvm->arch.cur_tsc_write + delta;
		...
		if (vcpu->arch.l1_tsc_scaling_ratio !=
		    kvm->arch.cur_tsc_scaling_ratio) {

In set_tsc_khz(), vcpu->arch.l1_tsc_scaling_ratio is derived deterministically
from vcpu->arch.virtual_tsc_khz, and __kvm_synchronize_tsc() updates
kvm->arch.cur_tsc_khz and kvm->arch.cur_tsc_scaling_ratio together.

Meanwhile, kvm_arch_init_vm() initializes:

	kvm->arch.cur_tsc_scaling_ratio = kvm_caps.default_tsc_scaling_ratio;
	kvm->arch.cur_tsc_khz = kvm->arch.default_tsc_khz;

When userspace configures a non-default guest TSC frequency via the VM-scoped
KVM_SET_TSC_KHZ ioctl in kvm_arch_vm_ioctl():

	mutex_lock(&kvm->lock);
	if (!kvm->created_vcpus) {
		WRITE_ONCE(kvm->arch.default_tsc_khz, user_tsc_khz);
		r = 0;
	}
	mutex_unlock(&kvm->lock);

only kvm->arch.default_tsc_khz is updated, leaving kvm->arch.cur_tsc_khz at
the initial host frequency. Or, if userspace instead invokes the vCPU-scoped
KVM_SET_TSC_KHZ ioctl after creating the first vCPU, virtual_tsc_khz also
differs from kvm->arch.cur_tsc_khz.

In both cases, vcpu->arch.virtual_tsc_khz == kvm->arch.cur_tsc_khz is false in
kvm_synchronize_tsc(), so the outer if block is skipped entirely. That leaves
data at 0 instead of extrapolating data = kvm->arch.cur_tsc_write + delta from
the VM-creation epoch (while kvmclock_offset remains anchored at VM creation
time), and on VMs where vcpu->arch.guest_tsc_protected causes
__kvm_synchronize_tsc() to return early, kvm->arch.cur_tsc_scaling_ratio and
kvm->arch.master_tsc_scaling_ratio remain stuck at
kvm_caps.default_tsc_scaling_ratio.

> @@ -9645,8 +9659,15 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  		return;
>  	vcpu_load(vcpu);
>  	kvm_synchronize_tsc(vcpu, NULL, false);
> -	if (kvm_check_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu))
> +
> +	/*
> +	 * Refresh the VM-creation masterclock snapshot (see
> +	 * kvm_arch_init_vm), synchronously so that it is valid even for
> +	 * userspace which queries the clock before ever running a vCPU.
> +	 */
> +	if (vcpu->vcpu_idx == 0)
>  		kvm_update_masterclock(vcpu->kvm, NULL);
> +
>  	vcpu_put(vcpu);
>  
>  	/* poll control enabled by default */

[Severity: Medium]
What happens if creating a secondary vCPU (vcpu->vcpu_idx > 0) queues
KVM_REQ_MASTERCLOCK_UPDATE on that vCPU?

During vCPU creation, kvm_synchronize_tsc() -> __kvm_synchronize_tsc() ->
kvm_track_tsc_matching() checks whether a masterclock update is needed:

	if ((ka->use_master_clock && update_mclock) ||
	    (ka->use_master_clock != use_master_clock))
		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

When vcpu->vcpu_idx > 0 and its synchronization opens a new TSC generation,
changes ka->all_vcpus_matched_tsc, or changes masterclock eligibility,
KVM_REQ_MASTERCLOCK_UPDATE is queued only on the newly created vCPU.

With kvm_arch_vcpu_postcreate() only calling kvm_update_masterclock() when
vcpu->vcpu_idx == 0, won't that request remain unserviced until the secondary
vCPU enters vcpu_enter_guest(), leaving ka->use_master_clock and the
masterclock state stale for host-side KVM_GET_CLOCK / KVM_GET_CLOCK_GUEST /
KVM_SET_CLOCK_GUEST calls and delaying KVM_REQ_CLOCK_UPDATE for already
running sibling vCPUs?

> @@ -10106,16 +10133,41 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	 * returns early — is still eligible for PVCLOCK_TSC_STABLE_BIT.
>  	 */
>  	kvm->arch.all_vcpus_matched_tsc = true;
> +	kvm->arch.cur_tsc_scaling_ratio = kvm_caps.default_tsc_scaling_ratio;
> +	kvm->arch.cur_tsc_khz = kvm->arch.default_tsc_khz;
>  
>  	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
> +
> +	/*
> +	 * Establish the initial TSC generation for synchronization logic.
> +	 * The first vCPU will sync to this, starting its guest TSC at the
> +	 * number of cycles elapsed since VM creation rather than at zero.
> +	 * This ensures coherence with the kvmclock epoch (also set here),
> +	 * and the master clock snapshot taken below is immediately valid.
> +	 * It is refreshed again when the first vCPU is created, in case
> +	 * the host clocksource changed (and changed back) in between:
> +	 * pvclock_gtod_update_fn() does not notify a VM with no vCPUs.
> +	 */
> +#ifdef CONFIG_X86_64
> +	if (!kvm_get_time_and_clockread(&kvmclock_ns, &kvmclock_host_tsc))
> +#endif
> +	{
> +		/*
> +		 * If not simultaneous, we want the TSC to be zero slightly
> +		 * *after* the kvmclock is zero.
> +		 */
> +		kvmclock_ns = get_kvmclock_base_ns();
> +		kvmclock_host_tsc = rdtsc();
> +	}
> +	kvm->arch.cur_tsc_write = 0;
> +	kvm->arch.cur_tsc_offset = -(s64)kvmclock_host_tsc;
> +	kvm->arch.cur_tsc_nsec = kvmclock_ns;
> +	kvm->arch.cur_tsc_generation = 1;
> +	kvm->arch.kvmclock_offset = -kvmclock_ns;

[Severity: Medium]
Now that cur_tsc_khz and cur_tsc_generation are initialized from
kvm_arch_init_vm() with all_vcpus_matched_freq = true and
all_vcpus_matched_tsc = true, does the initial kvm_set_tsc_khz() call in
kvm_arch_vcpu_create() corrupt frequency and TSC matching when userspace uses
per-vCPU KVM_SET_TSC_KHZ?

In kvm_vm_ioctl_create_vcpu(), kvm_arch_vcpu_create() runs before
atomic_inc(&kvm->online_vcpus):

	kvm_arch_vcpu_create(vcpu)
		kvm_set_tsc_khz(vcpu, vcpu->kvm->arch.default_tsc_khz)
			set_tsc_khz(vcpu, user_tsc_khz, false)
				kvm_synchronize_tsc(vcpu, NULL, false)

Consider creating vCPU 0 and setting its frequency to F != default_tsc_khz via
the vCPU-scoped KVM_SET_TSC_KHZ ioctl (so cur_tsc_khz = F,
cur_tsc_freq_generation = 1, and nr_vcpus_matched_freq = 1), and then creating
vCPU 1:

1. In kvm_arch_vcpu_create(vcpu 1), kvm_set_tsc_khz() syncs vCPU 1 at
   default_tsc_khz while online_vcpus is still 1. Because default_tsc_khz != F,
   __kvm_synchronize_tsc() increments cur_tsc_freq_generation to 2, clears
   all_vcpus_matched_freq, sets nr_vcpus_matched_freq = 1, and overwrites
   cur_tsc_khz = default_tsc_khz.

2. Immediately afterwards, kvm_track_tsc_matching() reads online = 1, sees
   nr_vcpus_matched_freq (1) >= online (1), and sets
   ka->all_vcpus_matched_freq = true.

3. After online_vcpus is incremented to 2, kvm_arch_vcpu_postcreate(vcpu 1)
   calls kvm_synchronize_tsc(vcpu 1, NULL, false). Because vCPU 1's
   virtual_tsc_khz already matches cur_tsc_khz (default_tsc_khz),
   __kvm_synchronize_tsc() does not clear all_vcpus_matched_freq.

If vCPU 1 is left at default_tsc_khz, all_vcpus_matched_freq remains true even
though vCPU 0 is at F and vCPU 1 is at default_tsc_khz. Conversely, if
userspace then sets vCPU 1's frequency to F via KVM_SET_TSC_KHZ, the
intermediate sync at default_tsc_khz has already bumped
cur_tsc_freq_generation and cur_tsc_generation, so setting vCPU 1 to F bumps
both generations again with nr_vcpus_matched_freq = 1 and
nr_vcpus_matched_tsc = 1 (for 2 online vCPUs), leaving all_vcpus_matched_freq
and all_vcpus_matched_tsc permanently false even though both vCPUs are
configured to F.

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