Re: [PATCH 2/2] KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug

["Radim Krčmář" <rkrcmar@redhat.com>]

2017-04-06 11:08+0300, Denis Plotnikov:
> VCPU TSC synchronization is perfromed in kvm_write_tsc() when the TSC
> value being set is within 1 second from the expected, as obtained by
> extrapolating of the TSC in already synchronized VCPUs.
> 
> This is naturally achieved on all VCPUs at VM start and resume;
> however on VCPU hotplug it is not: the newly added VCPU is created
> with TSC == 0 while others are well ahead.
> 
> To compensate for that, consider host-initiated kvm_write_tsc() with
> TSC == 0 a special case requiring synchronization regardless of the
> current TSC on other VCPUs.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> ---

The idea goes well with current kvm clock.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -1455,15 +1455,23 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	elapsed = ns - kvm->arch.last_tsc_nsec;
>  
>  	if (vcpu->arch.virtual_tsc_khz) {
> -		u64 tsc_exp = kvm->arch.last_tsc_write +
> -					nsec_to_cycles(vcpu, elapsed);
> -		u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
> -		/*
> -		 * Special case: TSC write with a small delta (1 second) of virtual
> -		 * cycle time against real time is interpreted as an attempt to
> -		 * synchronize the CPU.
> -		 */
> -		synchronizing = data < tsc_exp + tsc_hz && data > tsc_exp - tsc_hz;
> +		if ((data == 0) && msr->host_initiated) {
> +			/*
> +			* detection of vcpu initialization -- need to sync with other vCPUs
> +			* particularly helps to keep kvm_clock stable after CPU hotplug
> +			*/
> +			synchronizing = true;
> +		} else {
> +			u64 tsc_exp = kvm->arch.last_tsc_write +
> +						nsec_to_cycles(vcpu, elapsed);
> +			u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
> +			/*
> +			 * Special case: TSC write with a small delta (1 second) of virtual
> +			 * cycle time against real time is interpreted as an attempt to
> +			 * synchronize the CPU.
> +			 */
> +			synchronizing = data < tsc_exp + tsc_hz && data > tsc_exp - tsc_hz;

I would have proposed to fix the bug in [1/2] myself if there weren't
too many overlong lines here.  Please wrap it, so the last element
doesn't start after 80 characters.  (And the first comment can also use
spaces before stars.)

Thanks.