Re: [PATCH v7 26/36] KVM: x86: Cache host vclock_mode for masterclock eligibility checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch leaves behind unused static functions (`do_kvmclock_base`, `do_monotonic`, `do_realtime`, `read_tsc`, `vgettsc`) and a useless data structure update (`update_pvclock_gtod` updating `pvclock_gtod_data`). This causes a build failure under `CONFIG_WERROR` due to `-Wunused-function` and introduces unnecessary performance overhead on every timekeeper tick.
- [Medium] Lockless accesses to `kvm_host_vclock_mode` lack `READ_ONCE()` and `WRITE_ONCE()`, which can lead to compiler-optimized double-loads and spurious state transitions.
--

--- Patch [26]: [PATCH v7 26/36] KVM: x86: Cache host vclock_mode for masterclock eligibility checks ---
commit d71eb6f562c183153e68671d8422238c00275229
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Cache host vclock_mode for masterclock eligibility checks
    
This commit changes KVM to cache the host clocksource mode in a new
kvm_host_vclock_mode variable. It removes the remaining readers of
pvclock_gtod_data, opting to use gtod_is_based_on_tsc() with the cached
value instead.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 32f0550c5c0dd..3f660bdea8a28 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1155,7 +1156,7 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu, bool update_mclock)
>  	 * accounts for its offset.
>  	 */
>  	bool use_master_clock = kvm_use_master_clock(vcpu->kvm) &&
> -				gtod_is_based_on_tsc(gtod->clock.vclock_mode);
> +				gtod_is_based_on_tsc(kvm_host_vclock_mode);

[Severity: Medium]
Should this lockless read of kvm_host_vclock_mode use READ_ONCE()?

Because gtod_is_based_on_tsc() is inline and evaluates the mode multiple
times:

	return mode == VDSO_CLOCKMODE_TSC || mode == VDSO_CLOCKMODE_HVCLOCK;

If the compiler inlines the function and reads the global variable twice,
could a concurrent update during a clocksource change cause both checks
to evaluate to false?

This could cause the code to spuriously drop masterclock usage.

[ ... ]

> @@ -7055,12 +7055,15 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
>  	update_pvclock_gtod(tk);

[Severity: High]
Does this commit leave behind unused static functions?

With pvclock_gtod_data no longer being read by KVM, it appears
update_pvclock_gtod() is now updating a useless data structure on every
timekeeper tick, which adds unnecessary overhead.

Additionally, does this leave behind static functions like do_kvmclock_base(),
do_monotonic(), do_realtime(), read_tsc(), and vgettsc() which are no longer
referenced?

Could this cause a build failure due to -Wunused-function when compiling with
CONFIG_WERROR?

>  
>  #ifdef CONFIG_X86_64
> +	kvm_host_vclock_mode =
> +		tk->tkr_mono.clock->vdso_clock_mode;

[Severity: Medium]
Should this update to kvm_host_vclock_mode use WRITE_ONCE()?

Since readers access this global variable locklessly, missing WRITE_ONCE()
could lead to compiler-optimized tearing and trigger KCSAN data race warnings.

> +
>  	/*
>  	 * Disable master clock if host does not trust, or does not use,
>  	 * TSC based clocksource. Delegate queue_work() to irq_work as
>  	 * this is invoked with tk_core.seq write held.
>  	 */
> -	if (!gtod_is_based_on_tsc(pvclock_gtod_data.clock.vclock_mode) &&
> +	if (!gtod_is_based_on_tsc(kvm_host_vclock_mode) &&
>  	    atomic_read(&kvm_guest_has_master_clock) != 0)
>  		irq_work_queue(&pvclock_irq_work);
>  #endif

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