Re: [PATCH v5 33/34] KVM: x86: Replace pvclock_gtod_data vclock_mode with boolean

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `kvm_check_tsc_unstable()` incorrectly masks TSC instability when the host uses a native TSC clocksource, breaking backward TSC compensation.
- [Medium] Tracepoints expect a clock mode enum but now receive a collapsed boolean value, changing trace output and breaking observability tooling.
--

commit e7585c139257b84c3968a9648d71f5c5478115f9
Author: David Woodhouse <dwmw@amazon.co.uk>
Subject: KVM: x86: Replace vclock_mode checks with kvm_host_has_tsc_clocksource

This commit replaces remaining users of pvclock_gtod_data with a simple
kvm_host_has_tsc_clocksource boolean. This removes the need for tracking
the exact vclock_mode when determining if the host clocksource is
TSC-based.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c18947c5b63f..93a428c37847 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -2726,7 +2727,7 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu, bool new_generation)
>  
>  	trace_kvm_track_tsc(vcpu->vcpu_id, ka->nr_vcpus_matched_tsc,
>  			    atomic_read(&vcpu->kvm->online_vcpus),
> -		            ka->use_master_clock, gtod->clock.vclock_mode);
> +		            ka->use_master_clock, kvm_host_has_tsc_clocksource);

[Severity: Medium]
Will this change break ABI for observability tooling that expects the
tracepoint to log the original vclock_mode integer enum?

Since kvm_host_has_tsc_clocksource is a boolean, a true value will
implicitly cast to 1 (VDSO_CLOCKMODE_TSC). If the actual mode is
VDSO_CLOCKMODE_HVCLOCK (3), the boolean still evaluates to 1, causing the
trace output to erroneously log 'tsc' instead of the expected integer 3.

[ ... ]
> @@ -2850,7 +2851,7 @@ static inline bool kvm_check_tsc_unstable(void)
>  	 * TSC is marked unstable when we're running on Hyper-V,
>  	 * 'TSC page' clocksource is good.
>  	 */
> -	if (pvclock_gtod_data.clock.vclock_mode == VDSO_CLOCKMODE_HVCLOCK)
> +	if (kvm_host_has_tsc_clocksource)
>  		return false;

[Severity: High]
Does this change unintentionally mask TSC instability when the host uses a
native TSC clocksource?

Previously, this check bypassed check_tsc_unstable() only when the clocksource
was VDSO_CLOCKMODE_HVCLOCK. Now, it evaluates to true for both HVCLOCK and
native TSC clocksources.

When using a native TSC clocksource, KVM detects backwards TSCs in
kvm_arch_vcpu_load() and synchronously calls mark_tsc_unstable(). However,
because kvm_host_has_tsc_clocksource remains true until the timekeeper
notifier fires, the subsequent call to kvm_check_tsc_unstable() will
immediately return false.

Will this completely bypass the TSC offset catchup logic, directly exposing
the backwards TSC jump to the guest?

[ ... ]
> @@ -3315,7 +3316,7 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
>  			ka->use_master_clock = false;
>  	}
>  
> -	vclock_mode = pvclock_gtod_data.clock.vclock_mode;
> +	vclock_mode = kvm_host_has_tsc_clocksource;
>  	trace_kvm_update_master_clock(ka->use_master_clock, vclock_mode,
>  					ka->all_vcpus_matched_freq);
>  #endif

[Severity: Medium]
Similar to trace_kvm_track_tsc above, does passing a boolean here break
tools expecting the original vclock_mode enum for
trace_kvm_update_master_clock?

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