Re: [PATCH 0/2] maintaining of kvm_clock stability on guest vCPU hotplug

Re: [PATCH 0/2] maintaining of kvm_clock stability on guest vCPU hotplug

[Denis Plotnikov]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 1124 bytes --]

the series sending repeated

On 05.04.2017 17:23, Radim Krčmář wrote:
> 2017-04-05 11:56+0300, Denis Plotnikov:
>> ping
> Please re-send the series, I don't have it in mailbox and web-archives
> don't show it either,
> 
> thanks.
> 
>> On 23.03.2017 17:28, Denis Plotnikov wrote:
>>> It was found, that after adding a new vCPU to a running VM (CPU hotplug)
>>> kvm_clock lost the stability property in case of using it. This happened
>>> because KVM didn't treated cpu hotplug as a special case of TSC
>>> synchronization.
>>> This patch series adds cpu hotplug to the TSC synchronization cases.
> Right, it looks like an incarnation of the deep problem where the kvm
> clock with masterclock diverges from host (different frequency), so any
> case where the masterclock is synchronized results in time shifts in the
> guest.
> 
>>> Denis Plotnikov (2):
>>>   KVM: x86: remaster kvm_write_tsc code
>>>   KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug
>>>
>>>  arch/x86/kvm/x86.c | 59 ++++++++++++++++++------------------------------------
>>>  1 file changed, 20 insertions(+), 39 deletions(-)
>>>

[PATCH 1/2] KVM: x86: remaster kvm_write_tsc code

[Denis Plotnikov]

Reuse existing code instead of using inline asm.
Make the code more concise and clear in the TSC
synchronization part.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
---
 arch/x86/kvm/x86.c | 51 ++++++++++++---------------------------------------
 1 file changed, 12 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1faf620..4c674eb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1444,10 +1444,10 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	struct kvm *kvm = vcpu->kvm;
 	u64 offset, ns, elapsed;
 	unsigned long flags;
-	s64 usdiff;
 	bool matched;
 	bool already_matched;
 	u64 data = msr->data;
+	bool synchronizing = false;
 
 	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
 	offset = kvm_compute_tsc_offset(vcpu, data);
@@ -1455,51 +1455,24 @@ 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) {
-		int faulted = 0;
-
-		/* n.b - signed multiplication and division required */
-		usdiff = data - kvm->arch.last_tsc_write;
-#ifdef CONFIG_X86_64
-		usdiff = (usdiff * 1000) / vcpu->arch.virtual_tsc_khz;
-#else
-		/* do_div() only does unsigned */
-		asm("1: idivl %[divisor]\n"
-		    "2: xor %%edx, %%edx\n"
-		    "   movl $0, %[faulted]\n"
-		    "3:\n"
-		    ".section .fixup,\"ax\"\n"
-		    "4: movl $1, %[faulted]\n"
-		    "   jmp  3b\n"
-		    ".previous\n"
-
-		_ASM_EXTABLE(1b, 4b)
-
-		: "=A"(usdiff), [faulted] "=r" (faulted)
-		: "A"(usdiff * 1000), [divisor] "rm"(vcpu->arch.virtual_tsc_khz));
-
-#endif
-		do_div(elapsed, 1000);
-		usdiff -= elapsed;
-		if (usdiff < 0)
-			usdiff = -usdiff;
-
-		/* idivl overflow => difference is larger than USEC_PER_SEC */
-		if (faulted)
-			usdiff = USEC_PER_SEC;
-	} else
-		usdiff = USEC_PER_SEC; /* disable TSC match window below */
+		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;
+	}
 
 	/*
-	 * 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.
-         *
 	 * For a reliable TSC, we can match TSC offsets, and for an unstable
 	 * TSC, we add elapsed time in this computation.  We could let the
 	 * compensation code attempt to catch up if we fall behind, but
 	 * it's better to try to match offsets from the beginning.
          */
-	if (usdiff < USEC_PER_SEC &&
+	if (synchronizing &&
 	    vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
 		if (!check_tsc_unstable()) {
 			offset = kvm->arch.cur_tsc_offset;
-- 
1.8.3.1

Re: [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code

[Radim Krčmář]

2017-04-06 11:08+0300, Denis Plotnikov:
> Reuse existing code instead of using inline asm.
> Make the code more concise and clear in the TSC
> synchronization part.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  arch/x86/kvm/x86.c | 51 ++++++++++++---------------------------------------
>  1 file changed, 12 insertions(+), 39 deletions(-)

I like this patch a lot.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -1455,51 +1455,24 @@ 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) {
> -		int faulted = 0;
> -
> -		/* n.b - signed multiplication and division required */
> -		usdiff = data - kvm->arch.last_tsc_write;
> -#ifdef CONFIG_X86_64
> -		usdiff = (usdiff * 1000) / vcpu->arch.virtual_tsc_khz;
> -#else
> -		/* do_div() only does unsigned */
> -		asm("1: idivl %[divisor]\n"
> -		    "2: xor %%edx, %%edx\n"
> -		    "   movl $0, %[faulted]\n"
> -		    "3:\n"
> -		    ".section .fixup,\"ax\"\n"
> -		    "4: movl $1, %[faulted]\n"
> -		    "   jmp  3b\n"
> -		    ".previous\n"
> -
> -		_ASM_EXTABLE(1b, 4b)
> -
> -		: "=A"(usdiff), [faulted] "=r" (faulted)
> -		: "A"(usdiff * 1000), [divisor] "rm"(vcpu->arch.virtual_tsc_khz));
> -
> -#endif
> -		do_div(elapsed, 1000);

Oh, this is actually fixing a bug, because we later consider elapsed in
nanoseconds, but this one converts it to microsends ...

> -		usdiff -= elapsed;
> -		if (usdiff < 0)
> -			usdiff = -usdiff;
> -
> -		/* idivl overflow => difference is larger than USEC_PER_SEC */
> -		if (faulted)
> -			usdiff = USEC_PER_SEC;
> -	} else
> -		usdiff = USEC_PER_SEC; /* disable TSC match window below */
> +		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;

This condition is wrong -- if tsc_exp < tsc_hz, then it will wraparound
and resolve as false.  It should read:

   synchronizing = data < tsc_exp + tsc_hz && data + tsc_hz > tsc_exp;

> +	}
>  
>  	/*
> -	 * 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.
> -         *
>  	 * For a reliable TSC, we can match TSC offsets, and for an unstable
>  	 * TSC, we add elapsed time in this computation.  We could let the
>  	 * compensation code attempt to catch up if we fall behind, but
>  	 * it's better to try to match offsets from the beginning.
>           */
> -	if (usdiff < USEC_PER_SEC &&
> +	if (synchronizing &&
>  	    vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
>  		if (!check_tsc_unstable()) {
>  			offset = kvm->arch.cur_tsc_offset;
> -- 
> 1.8.3.1
> 

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

[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>
---
 arch/x86/kvm/x86.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4c674eb..a4091ee 100644
--- 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;
+		}
 	}
 
 	/*
-- 
1.8.3.1

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

[Radim Krčmář]

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.

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

[Radim Krčmář]

2017-04-06 19:11+0200, Radim Krčmář:
> 2017-04-06 11:08+0300, Denis Plotnikov:
>> 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) {

And while I'm nitpicking ... the code would also be nicer without the
parentheses around data.

[PATCH v2 0/2] maintaining of kvm_clock stability on guest vCPU hotplug

[Denis Plotnikov]

new in v2:
  - fix possible wraparound on the synchronization detection
  - lines exceeding 80 chars are re-formatted
  - excess parentheses are removed

======================

It was found, that after adding a new vCPU to a running VM (CPU hotplug)
kvm_clock lost the stability property in case of using it. This happened
because KVM didn't treated cpu hotplug as a special case of TSC
synchronization.
This patch series adds cpu hotplug to the TSC synchronization cases.

Denis Plotnikov (2):
  KVM: x86: remaster kvm_write_tsc code
  KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug

 arch/x86/kvm/x86.c | 61 ++++++++++++++++++++----------------------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] KVM: x86: remaster kvm_write_tsc code

[Denis Plotnikov]

Reuse existing code instead of using inline asm.
Make the code more concise and clear in the TSC
synchronization part.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
---
 arch/x86/kvm/x86.c | 52 +++++++++++++---------------------------------------
 1 file changed, 13 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1faf620..c59e38f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1444,10 +1444,10 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	struct kvm *kvm = vcpu->kvm;
 	u64 offset, ns, elapsed;
 	unsigned long flags;
-	s64 usdiff;
 	bool matched;
 	bool already_matched;
 	u64 data = msr->data;
+	bool synchronizing = false;
 
 	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
 	offset = kvm_compute_tsc_offset(vcpu, data);
@@ -1455,51 +1455,25 @@ 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) {
-		int faulted = 0;
-
-		/* n.b - signed multiplication and division required */
-		usdiff = data - kvm->arch.last_tsc_write;
-#ifdef CONFIG_X86_64
-		usdiff = (usdiff * 1000) / vcpu->arch.virtual_tsc_khz;
-#else
-		/* do_div() only does unsigned */
-		asm("1: idivl %[divisor]\n"
-		    "2: xor %%edx, %%edx\n"
-		    "   movl $0, %[faulted]\n"
-		    "3:\n"
-		    ".section .fixup,\"ax\"\n"
-		    "4: movl $1, %[faulted]\n"
-		    "   jmp  3b\n"
-		    ".previous\n"
-
-		_ASM_EXTABLE(1b, 4b)
-
-		: "=A"(usdiff), [faulted] "=r" (faulted)
-		: "A"(usdiff * 1000), [divisor] "rm"(vcpu->arch.virtual_tsc_khz));
-
-#endif
-		do_div(elapsed, 1000);
-		usdiff -= elapsed;
-		if (usdiff < 0)
-			usdiff = -usdiff;
-
-		/* idivl overflow => difference is larger than USEC_PER_SEC */
-		if (faulted)
-			usdiff = USEC_PER_SEC;
-	} else
-		usdiff = USEC_PER_SEC; /* disable TSC match window below */
+		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_hz > tsc_exp;
+	}
 
 	/*
-	 * 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.
-         *
 	 * For a reliable TSC, we can match TSC offsets, and for an unstable
 	 * TSC, we add elapsed time in this computation.  We could let the
 	 * compensation code attempt to catch up if we fall behind, but
 	 * it's better to try to match offsets from the beginning.
          */
-	if (usdiff < USEC_PER_SEC &&
+	if (synchronizing &&
 	    vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
 		if (!check_tsc_unstable()) {
 			offset = kvm->arch.cur_tsc_offset;
-- 
1.8.3.1