Re: [PATCH v7 07/36] KVM: x86: Restructure get_kvmclock()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removal of `get_cpu()` reintroduces a race condition between `rdtsc` and `cpu_tsc_khz`, and triggers a kernel warning for preemptible `__this_cpu_read`.
--

--- Patch [7]: [PATCH v7 07/36] KVM: x86: Restructure get_kvmclock() ---
commit c5c131ac2510f7641714a694b1cf0192a9da9749
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Restructure get_kvmclock()

This commit wraps the use_master_clock block in an ifdef for CONFIG_X86_64
to resolve 32-bit build issues with unused variables and host_tsc_clocksource.
It also refactors the master-clock success path to use continue, allowing
the fallback computation to become the common tail.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ee115dc6df6e1..d68d00cdbea73 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -1636,34 +1636,38 @@ static unsigned long get_cpu_tsc_khz(void)
>  static void get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
>  {
>  	struct kvm_arch *ka = &kvm->arch;
> -	struct pvclock_vcpu_time_info hv_clock;
>  	unsigned int seq;
>  
>  	do {
>  		seq = read_seqcount_begin(&ka->pvclock_sc);
>  
>  		data->flags = 0;
> -		if (ka->use_master_clock) {
>  #ifdef CONFIG_X86_64
> +		if (ka->use_master_clock) {
> +			struct pvclock_vcpu_time_info hv_clock;
>  			struct timespec64 ts;
>  
>  			if (kvm_get_walltime_and_clockread(&ts, &data->host_tsc)) {
>  				data->realtime = ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec;
> -				data->flags |= KVM_CLOCK_REALTIME | KVM_CLOCK_HOST_TSC;
> -			} else
> -#endif
> -			data->host_tsc = rdtsc();
> -
> -			data->flags |= KVM_CLOCK_TSC_STABLE;
> -			hv_clock.tsc_timestamp = ka->master_cycle_now;
> -			hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
> -			kvm_get_time_scale(NSEC_PER_SEC, get_cpu_tsc_khz() * 1000LL,
> -					   &hv_clock.tsc_shift,
> -					   &hv_clock.tsc_to_system_mul);
> +				data->flags |= KVM_CLOCK_REALTIME | KVM_CLOCK_HOST_TSC | KVM_CLOCK_TSC_STABLE;
> +
> +				hv_clock.tsc_timestamp = ka->master_cycle_now;
> +				hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
> +				kvm_get_time_scale(NSEC_PER_SEC, get_cpu_tsc_khz() * 1000LL,
> +						   &hv_clock.tsc_shift,
> +						   &hv_clock.tsc_to_system_mul);

[Severity: High]
Does this code trigger a preemptible __this_cpu_read() warning?

If the host lacks X86_FEATURE_CONSTANT_TSC, get_cpu_tsc_khz() will call
__this_cpu_read(cpu_tsc_khz). Since previous changes dropped the get_cpu()
pinning protection in get_kvmclock(), this read happens with preemption
enabled, which violates __this_cpu_read constraints and triggers the debug
warning.

Can this also result in using a mismatched TSC frequency if the thread is
preempted and migrated to a CPU with a different frequency while servicing
ioctl(KVM_GET_CLOCK)?

> +				data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
> +				continue;
> +			}
> +
> +			/*
> +			 * Clock read failed (e.g. clocksource is transitioning
> +			 * away from TSC). Fall back to the non-master-clock path
> +			 * rather than spinning.
> +			 */
>  		}
> +#endif
> +		data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
>  	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
>  }

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