Re: [PATCH v12 03/24] KVM: x86: Replace nr_vcpus_matched_tsc count with all_vcpus_matched_tsc bool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] KVM: x86: incomplete TSC match invalidation in
  kvm_arch_vcpu_postcreate()
- [Medium] KVM: x86: stale cur_tsc_scaling_ratio for scaled
  protected-TSC guests
--

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

KVM: x86: Replace nr_vcpus_matched_tsc count with all_vcpus_matched_tsc bool

This commit replaces the read-time comparison of nr_vcpus_matched_tsc
against kvm->online_vcpus with a cached all_vcpus_matched_tsc boolean
updated under tsc_write_lock and initialized to true at VM creation.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 20b84b2e44243..d29e5bc75f7a5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -9552,8 +9554,23 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  {
> -	if (mutex_lock_killable(&vcpu->mutex))
> +	if (mutex_lock_killable(&vcpu->mutex)) {
> +		/*
> +		 * The new vCPU is already counted in online_vcpus but has
> +		 * not been synchronized. Nothing recomputes the cached
> +		 * matched state against the raised count until the next
> +		 * synchronization, so clear it here rather than leave the
> +		 * clock advertised as matched while an unmatched vCPU is
> +		 * runnable. It heals when this vCPU first synchronizes.
> +		 */
> +		struct kvm_arch *ka = &vcpu->kvm->arch;
> +		unsigned long flags;
> +
> +		raw_spin_lock_irqsave(&ka->tsc_write_lock, flags);
> +		WRITE_ONCE(ka->all_vcpus_matched_tsc, false);
> +		raw_spin_unlock_irqrestore(&ka->tsc_write_lock, flags);
>  		return;
> +	}

[Severity: Medium]
Does clearing ka->all_vcpus_matched_tsc here without calling
kvm_update_masterclock() leave ka->use_master_clock stale?

ka->all_vcpus_matched_tsc is consumed by pvclock_update_vm_gtod_copy(), so
returning without triggering a masterclock update leaves
ka->use_master_clock enabled (initially set in kvm_arch_init_vm()) and does
not request KVM_REQ_CLOCK_UPDATE on existing vCPUs.  Even after commit "KVM:
x86: Allow KVM master clock mode when TSCs are offset from each other"
checks ka->all_vcpus_matched_tsc in kvm_guest_time_update() and clears
ka->all_vcpus_matched_freq here, ka->use_master_clock remains true and
KVM_GET_CLOCK still reports KVM_CLOCK_TSC_STABLE.

Also, __kvm_synchronize_tsc() returns early when
vcpu->arch.guest_tsc_protected is true, and KVM_RUN does not call
kvm_synchronize_tsc(), so clearing the flag on this path will not heal for
protected-TSC guests or on normal vCPU entry; conversely, if the thread
holding vcpu->mutex already synchronized the vCPU via a host-initiated TSC
write, this error path overwrites the matched state back to false.

>  	vcpu_load(vcpu);
>  	kvm_synchronize_tsc(vcpu, NULL);
>  	if (kvm_check_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu))
> @@ -10029,6 +10046,20 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	ratelimit_set_flags(&kvm->arch.kvmclock_update_rs, RATELIMIT_MSG_ON_RELEASE);
>  	kvm->arch.kvmclock_offset = -get_kvmclock_base_ns();
>  
> +	/*
> +	 * Start with all (zero) vCPUs considered matched, so that a VM whose
> +	 * vCPUs never reach kvm_track_tsc_matching() — notably when the guest
> +	 * TSC is protected (TDX, SNP Secure TSC) and __kvm_synchronize_tsc()
> +	 * returns early — is still eligible for PVCLOCK_TSC_STABLE_BIT. The
> +	 * scaling ratio must then also be initialized: with vcpus_matched
> +	 * now true here, pvclock_update_vm_gtod_copy() reaches the scaling
> +	 * path, and a zero ratio would zero tsc_hz and turn the master
> +	 * clock straight back off (permanently so for protected-TSC guests,
> +	 * which never write the ratio in __kvm_synchronize_tsc()).
> +	 */
> +	kvm->arch.all_vcpus_matched_tsc = true;
> +	kvm->arch.cur_tsc_scaling_ratio = kvm_caps.default_tsc_scaling_ratio;
> +
>  	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
>  	pvclock_update_vm_gtod_copy(kvm);
>  	raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);

[Severity: Medium]
Does initializing kvm->arch.cur_tsc_scaling_ratio to
kvm_caps.default_tsc_scaling_ratio here leave the masterclock unscaled for
protected-TSC guests configured with a scaled TSC frequency?

Protected-TSC guests configure their TSC frequency before vCPU creation via
the VM-level KVM_SET_TSC_KHZ ioctl, which only updates
kvm->arch.default_tsc_khz.  When kvm_arch_vcpu_create() later calls
kvm_set_tsc_khz(), the scaled ratio is stored in
vcpu->arch.l1_tsc_scaling_ratio, but __kvm_synchronize_tsc() returns early
on vcpu->arch.guest_tsc_protected before copying l1_tsc_scaling_ratio into
kvm->arch.cur_tsc_scaling_ratio or requesting KVM_REQ_MASTERCLOCK_UPDATE:

arch/x86/kvm/x86.c:__kvm_synchronize_tsc() {
	if (vcpu->arch.guest_tsc_protected)
		return;
...
	kvm->arch.cur_tsc_khz = vcpu->arch.virtual_tsc_khz;
	kvm->arch.cur_tsc_scaling_ratio = vcpu->arch.l1_tsc_scaling_ratio;
}

As a result, kvm->arch.master_tsc_scaling_ratio, master_tsc_shift, and
master_tsc_mul remain computed for the 1:1 host TSC frequency, so
__get_kvmclock_master_clock() (and KVM_GET_CLOCK_GUEST in commit "KVM: x86:
Add KVM_[GS]ET_CLOCK_GUEST for accurate KVM clock migration") computes
kvmclock with unscaled host parameters that drift from the scaled pvclock
time info exposed to the guest.

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