Re: [PATCH v11 04/23] KVM: x86: Allow KVM master clock mode when TSCs are offset from each other

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: x86: stale pvclock tsc_timestamp after __kvm_synchronize_tsc()
- [Medium] KVM: x86: missing masterclock update in kvm_track_tsc_matching()
--

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

KVM: x86: Allow KVM master clock mode when TSCs are offset from each other

This commit decouples master clock eligibility from TSC offset matching so
that KVM remains in master clock mode as long as all vCPUs share the same TSC
frequency. Per-vCPU TSC offset matching is tracked separately and used only
to gate PVCLOCK_TSC_STABLE_BIT.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ec8693f4827fc..38fcb1d756611 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1114,30 +1114,63 @@ static inline bool gtod_is_based_on_tsc(int mode)
>  }
>  #endif
>  
> -static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu, bool new_generation)
> +static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu, bool update_mclock)
>  {
>  #ifdef CONFIG_X86_64
>  	struct kvm_arch *ka = &vcpu->kvm->arch;
>  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> +	bool prev_matched_tsc = ka->all_vcpus_matched_tsc;
>  
>  	/*
> -	 * To use the masterclock, the host clocksource must be based on TSC
> -	 * and all vCPUs must have matching TSCs.  Note, the count for matching
> -	 * vCPUs doesn't include the reference vCPU, hence "+1".
> +	 * Track whether all vCPUs have matching TSC offsets (for
> +	 * PVCLOCK_TSC_STABLE_BIT) and matching frequencies (for
> +	 * master clock eligibility).
> +	 */
> +
> +	/*
> +	 * A new vCPU might already have incremented ->online_vcpus
> +	 * and cause a temporary false negative here. But will then
> +	 * call kvm_synchronize_tsc() from kvm_arch_vcpu_postcreate()
> +	 * and finish the job.
> +	 */
> +	int online = atomic_read(&vcpu->kvm->online_vcpus);
> +	/*
> +	 * WRITE_ONCE() pairs with the READ_ONCE() in kvm_guest_time_update(),
> +	 * which samples the flag locklessly (outside both tsc_write_lock and
> +	 * its pvclock seqcount read section); the race is benign (a stale
> +	 * value merely delays the PVCLOCK_TSC_STABLE_BIT broadcast to the
> +	 * masterclock update which is requested below).
>  	 */
> -	ka->all_vcpus_matched_tsc = (ka->nr_vcpus_matched_tsc + 1 ==
> -				     atomic_read(&vcpu->kvm->online_vcpus));
> +	WRITE_ONCE(ka->all_vcpus_matched_tsc,
> +		   ka->nr_vcpus_matched_tsc >= online);
>  
> -	bool use_master_clock = ka->all_vcpus_matched_tsc &&
> +	update_mclock |= (prev_matched_tsc != ka->all_vcpus_matched_tsc);
> +
> +	/*
> +	 * all_vcpus_matched_freq starts true and is cleared when
> +	 * __kvm_synchronize_tsc() detects a frequency mismatch.
> +	 * Re-enable when all vCPUs have synced with matching frequency.
> +	 * If all offsets also match, that implies frequencies match too.
> +	 */
> +	if (ka->all_vcpus_matched_tsc ||
> +	    ka->nr_vcpus_matched_freq >= online)
> +		ka->all_vcpus_matched_freq = true;
> +
> +	/*
> +	 * To use the masterclock, the host clocksource must be based on TSC
> +	 * and all vCPUs must have matching TSC *frequency*. Different offsets
> +	 * are fine — each vCPU's pvclock has its own tsc_timestamp that
> +	 * accounts for its offset.
> +	 */
> +	bool use_master_clock = ka->all_vcpus_matched_freq &&
>  				gtod_is_based_on_tsc(gtod->clock.vclock_mode);
>  
>  	/*
> -	 * Request a masterclock update if the masterclock needs to be toggled
> -	 * on/off, or when starting a new generation and the masterclock is
> -	 * enabled (compute_guest_tsc() requires the masterclock snapshot to be
> -	 * taken _after_ the new generation is created).
> +	 * Request a masterclock update if needed: toggling master clock,
> +	 * new generation requiring a fresh snapshot, or a change in
> +	 * all_vcpus_matched_tsc (to broadcast PVCLOCK_TSC_STABLE_BIT).
>  	 */
> -	if ((ka->use_master_clock && new_generation) ||
> +	if ((ka->use_master_clock && update_mclock) ||
>  	    (ka->use_master_clock != use_master_clock))
>  		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

[Severity: Medium]
In arch/x86/kvm/x86.c, when ka->all_vcpus_matched_freq transitions from false
to true in kvm_track_tsc_matching(), should that also set update_mclock?

Suppose a VM with 3 online vCPUs already has ka->use_master_clock set to true
from vCPU creation, and userspace updates the TSC frequency on all vCPUs
before entering KVM_RUN:

1. vCPU 0 and vCPU 1 call __kvm_synchronize_tsc() at the new frequency with
   divergent offsets (matched == false). ka->all_vcpus_matched_freq becomes
   false and KVM_REQ_MASTERCLOCK_UPDATE is queued on vCPU 0 and vCPU 1, but
   ka->use_master_clock remains true until a vCPU enters the guest and
   services the request.

2. vCPU 2 then calls __kvm_synchronize_tsc() at the new frequency with an
   offset matching vCPU 1 (matched == true). ka->nr_vcpus_matched_freq
   reaches online (3), so ka->all_vcpus_matched_freq transitions from false
   to true and local use_master_clock is true. However, ka->nr_vcpus_matched_tsc
   is 2 (< 3), so ka->all_vcpus_matched_tsc stays false and update_mclock
   remains false.

Because ka->use_master_clock is still true and local use_master_clock is also
true, both (ka->use_master_clock && update_mclock) and
(ka->use_master_clock != use_master_clock) evaluate to false. Could vCPU 2
then enter KVM_RUN without KVM_REQ_MASTERCLOCK_UPDATE set and use stale
master_tsc_scaling_ratio, master_tsc_mul, and master_tsc_shift values from the
old frequency?

[ ... ]

> @@ -1365,17 +1413,18 @@ static void __kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 offset, u64 tsc,
>  		 * exact software computation in compute_guest_tsc()
>  		 */
>  		kvm->arch.cur_tsc_generation++;
> +		WRITE_ONCE(kvm->arch.all_vcpus_matched_tsc, false);
> +		kvm->arch.nr_vcpus_matched_tsc = 0;
>  		kvm->arch.cur_tsc_nsec = ns;
>  		kvm->arch.cur_tsc_write = tsc;
>  		kvm->arch.cur_tsc_offset = offset;
> -		kvm->arch.nr_vcpus_matched_tsc = 0;
> -		kvm->arch.all_vcpus_matched_tsc = false;
> -	} else if (vcpu->arch.this_tsc_generation != kvm->arch.cur_tsc_generation) {
> +	}
> +
> +	if (vcpu->arch.this_tsc_generation != kvm->arch.cur_tsc_generation) {
> +		vcpu->arch.this_tsc_generation = kvm->arch.cur_tsc_generation;
>  		kvm->arch.nr_vcpus_matched_tsc++;
>  	}
>  
> -	/* Keep track of which generation this VCPU has synchronized to */
> -	vcpu->arch.this_tsc_generation = kvm->arch.cur_tsc_generation;
>  	vcpu->arch.this_tsc_nsec = kvm->arch.cur_tsc_nsec;
>  	vcpu->arch.this_tsc_write = kvm->arch.cur_tsc_write;
>  
>  	kvm_track_tsc_matching(vcpu, !matched);
>  }

[Severity: High]
Can __kvm_synchronize_tsc() in arch/x86/kvm/x86.c leave a vCPU's guest
pvclock tsc_timestamp stale when matched is true?

In kvm_guest_time_update(), master clock mode populates each vCPU's
hv_clock.tsc_timestamp using its own l1_tsc_offset:

	if (use_master_clock) {
		hv_clock.tsc_timestamp = kvm_read_l1_tsc(v, master_tsc);
		hv_clock.system_time = master_ns + v->kvm->arch.kvmclock_offset;
	}

When __kvm_synchronize_tsc() is called via kvm_synchronize_tsc() or
kvm_arch_tsc_set_attr(), it updates vcpu->arch.l1_tsc_offset through
kvm_vcpu_write_tsc_offset(vcpu, offset) without requesting
KVM_REQ_CLOCK_UPDATE on vcpu, and then calls:

	kvm_track_tsc_matching(vcpu, !matched);

With this change, ka->use_master_clock remains true even when vCPUs have
different TSC offsets. If matched is true and ka->all_vcpus_matched_tsc does
not change its boolean value in kvm_track_tsc_matching(), update_mclock stays
false and neither KVM_REQ_MASTERCLOCK_UPDATE nor KVM_REQ_CLOCK_UPDATE is
requested.

For example, this can happen when:

1. A VM has 3 or more online vCPUs with divergent offsets: vCPU 0 sets a new
   offset (matched == false), vCPU 1 synchronizes to vCPU 0's offset
   (matched == true, nr_vcpus_matched_tsc becomes 2 < 3 so
   all_vcpus_matched_tsc remains false), while vCPU 2 stays at a different
   offset.

2. A vCPU whose l1_tsc_offset was modified by the guest via
   MSR_IA32_TSC_ADJUST or MSR_IA32_TSC (which do not clear
   ka->all_vcpus_matched_tsc) is later synchronized back to cur_tsc_offset by
   host userspace (matched == true, while all_vcpus_matched_tsc was already
   true).

Since kvm_arch_vcpu_load() also skips requesting KVM_REQ_CLOCK_UPDATE when
ka->use_master_clock is true and vcpu->cpu != -1, wouldn't the vCPU re-enter
the guest with the new hardware l1_tsc_offset in rdtsc while its pvclock
struct in guest memory still holds the old tsc_timestamp?

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