Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically

From: Sean Christopherson <seanjc@google.com>
To: Dongli Zhang <dongli.zhang@oracle.com>
Cc: David Woodhouse <dwmw2@infradead.org>,
	Joe Jin <joe.jin@oracle.com>,
	x86@kernel.org, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, pbonzini@redhat.com,
	tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com
Subject: Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically
Date: Mon, 2 Oct 2023 18:49:06 -0700	[thread overview]
Message-ID: <ZRtzEgnRVZ7FpG3R@google.com> (raw)
In-Reply-To: <afa70110-72dc-cf7d-880f-345a6e8a3995@oracle.com>

On Mon, Oct 02, 2023, Dongli Zhang wrote:
> > @@ -12185,6 +12203,10 @@ int kvm_arch_hardware_enable(void)
> >  	if (ret != 0)
> >  		return ret;
> >  
> > +	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> > +		kvm_get_time_scale(NSEC_PER_SEC, tsc_khz * 1000LL,
> > +				   &host_tsc_shift, &host_tsc_to_system_mul);
> 
> I agree that to use the kvmclock to calculate the ns elapsed when updating the
> master clock.
> 
> Would you take the tsc scaling into consideration?
> 
> While the host_tsc_shift and host_tsc_to_system_mul are pre-computed, how about
> the VM using different TSC frequency?

Heh, I'm pretty sure that's completely broken today.  I don't see anything in KVM
that takes hardware TSC scaling into account.

This code:

	if (unlikely(vcpu->hw_tsc_khz != tgt_tsc_khz)) {
		kvm_get_time_scale(NSEC_PER_SEC, tgt_tsc_khz * 1000LL,
				   &vcpu->hv_clock.tsc_shift,
				   &vcpu->hv_clock.tsc_to_system_mul);
		vcpu->hw_tsc_khz = tgt_tsc_khz;
		kvm_xen_update_tsc_info(v);
	}

is recomputing the multipler+shift for the current *physical* CPU, it's not
related to the guest's TSC in any way.

__get_kvmclock() again shows that quite clearly, there's no scaling for the guest
TSC anywhere in there.