Re: [PATCH v6 12/36] KVM: x86: Restructure get_kvmclock()

[Sean Christopherson <seanjc@google.com>]

On Fri, Jul 03, 2026, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Simplify the use_master_clock condition: the open-coded CONSTANT_TSC ||
> cpu_tsc_khz check is unnecessary since use_master_clock can only be true
> when the host clocksource is TSC based, which in turn requires a stable,
> constant and synchronised TSC across all CPUs.
> 
> Given that, the get_cpu()/put_cpu() pinning is not needed either: both
> the TSC read and get_cpu_tsc_khz() are CPU-independent when the master
> clock is in use, so drop them.
> 
> Wrap the entire use_master_clock block in #ifdef CONFIG_X86_64, since
> use_master_clock is never true on 32-bit (host_tsc_clocksource is only
> set under CONFIG_X86_64), and declare hv_clock inside the block so it is
> not left as an unused variable on 32-bit.
> 
> Use 'continue' on the master-clock success path so the non-master-clock
> computation becomes the common tail, avoiding a goto and label. When the
> clock read fails (e.g. clocksource transitioning away from TSC), fall
> back to that path rather than proceeding with uninitialised data or
> spinning in the seqcount loop.

Please split this up.  The changelog suggests there are at least three logical
changes here.  Yeah, the series is big, but smaller patches helps with review,
even if it results in more total patches.

> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  arch/x86/kvm/x86.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 50f088570dab..37b1f8192842 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3203,40 +3203,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);
>  
> -		/* both __this_cpu_read() and rdtsc() should be on the same cpu */
> -		get_cpu();
> -
>  		data->flags = 0;
> -		if (ka->use_master_clock &&
> -		    (static_cpu_has(X86_FEATURE_CONSTANT_TSC) || __this_cpu_read(cpu_tsc_khz))) {
>  #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->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
> -		} else {
> -			data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
> -		}
> +				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);
> +				data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
> +				continue;
> +			}
>  
> -		put_cpu();
> +			/*
> +			 * 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));
>  }
>  
> -- 
> 2.54.0
>