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

From: Dongli Zhang <dongli.zhang@oracle.com>
To: Sean Christopherson <seanjc@google.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 19:07:25 -0700	[thread overview]
Message-ID: <1326f47a-45c0-963d-d50f-a9774d932744@oracle.com> (raw)
In-Reply-To: <ZRtzEgnRVZ7FpG3R@google.com>

Hi Sean,

On 10/2/23 18:49, Sean Christopherson wrote:
> 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.

The below is the code.

line 3175: query freq for current *physical* CPU.

line 3211: scale the freq if scaling is involved.

line 3215: compute the view for guest based on new 'tgt_tsc_khz' after scaling.

3146 static int kvm_guest_time_update(struct kvm_vcpu *v)
3147 {
3148         unsigned long flags, tgt_tsc_khz;
3149         unsigned seq;
... ...
3173         /* Keep irq disabled to prevent changes to the clock */
3174         local_irq_save(flags);
3175         tgt_tsc_khz = get_cpu_tsc_khz();
... ...
3210         if (kvm_caps.has_tsc_control)
3211                 tgt_tsc_khz = kvm_scale_tsc(tgt_tsc_khz,
3212                                             v->arch.l1_tsc_scaling_ratio);
3213
3214         if (unlikely(vcpu->hw_tsc_khz != tgt_tsc_khz)) {
3215                 kvm_get_time_scale(NSEC_PER_SEC, tgt_tsc_khz * 1000LL,
3216                                    &vcpu->hv_clock.tsc_shift,
3217                                    &vcpu->hv_clock.tsc_to_system_mul);
3218                 vcpu->hw_tsc_khz = tgt_tsc_khz;
3219                 kvm_xen_update_tsc_info(v);
3220         }

Would you please let me know if the above understanding is incorrect?

Thank you very much!

Dongli Zhang

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