Re: [PATCH] KVM: x86: Refine calculation of guest wall clock to use a single TSC read

[Sean Christopherson <seanjc@google.com>]

On Sun, Oct 01, 2023, David Woodhouse wrote:
> +uint64_t kvm_get_wall_clock_epoch(struct kvm *kvm)
> +{
> +	/*
> +	 * The guest calculates current wall clock time by adding
> +	 * system time (updated by kvm_guest_time_update below) to the
> +	 * wall clock specified here.  We do the reverse here.

I would much rather this be a function comment that first explains what "epoch"
means in this context.  "epoch" is a perfect fit, but I suspect it won't be all
that intuitive for many readers (definitely wasn't for me).

> +	 */
> +#ifdef CONFIG_X86_64
> +	struct pvclock_vcpu_time_info hv_clock;
> +	struct kvm_arch *ka = &kvm->arch;
> +	unsigned long seq, local_tsc_khz = 0;
> +	struct timespec64 ts;
> +	uint64_t host_tsc;
> +
> +	do {
> +		seq = read_seqcount_begin(&ka->pvclock_sc);
> +
> +		if (!ka->use_master_clock)
> +			break;

This needs to zero local_tsc_khz, no?  E.g. read everything on loop 1, but the
pvclock_sc changes because use_master_clock is disabled, and so loop 2 will bail
and the code below will consume garbage TSC/masterclock information from loop 1.

> +		/* It all has to happen on the same CPU */

Define "it all", e.g. explain exactly why the cutoff for reenabling preemption is
after reading master_kernel_ns and not before, or not after kvm_get_time_scale().

> +		get_cpu();
> +
> +		local_tsc_khz = get_cpu_tsc_khz();
> +
> +		if (local_tsc_khz &&
> +		    !kvm_get_walltime_and_clockread(&ts, &host_tsc))
> +			local_tsc_khz = 0; /* Fall back to old method */
> +
> +		hv_clock.tsc_timestamp = ka->master_cycle_now;
> +		hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
> +
> +		put_cpu();
> +	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
> +
> +	/*
> +	 * If the conditions were right, and obtaining the wallclock+TSC was
> +	 * successful, calculate the KVM clock at the corresponding time and
> +	 * subtract one from the other to get the epoch in nanoseconds.
> +	 */
> +	if (local_tsc_khz) {
> +		kvm_get_time_scale(NSEC_PER_SEC, local_tsc_khz * 1000LL,

s/1000LL/NSEC_PER_USEC?

> +				   &hv_clock.tsc_shift,
> +				   &hv_clock.tsc_to_system_mul);
> +		return ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec -
> +			__pvclock_read_cycles(&hv_clock, host_tsc);
> +	}
> +#endif