KVM: x86: update masterclock values on TSC writes

PATCH In review Marcelo Tosatti 2 participants 6 messages Nov 4, 2014 – Nov 5, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2014-11-04 23:30 UTC #0 lore raw

PATCH v2 1/1 KVM: x86: update masterclock values on TSC writes

When the guest writes to the TSC, the masterclock TSC copy must be updated as well along with the TSC_OFFSET update, otherwise a negative tsc_timestamp is calculated at kvm_guest_time_update.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +9 -9

index 0033df3..50b6f56 100644
@@ -1237,21 +1237,21 @@ void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
 {
 #ifdef CONFIG_X86_64
 	bool vcpus_matched;
-	bool do_request = false;
 	struct kvm_arch *ka = &vcpu->kvm->arch;
 	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
 
 	vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
 			 atomic_read(&vcpu->kvm->online_vcpus));
 
-	if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC)
-		if (!ka->use_master_clock)
-			do_request = 1;
-
-	if (!vcpus_matched && ka->use_master_clock)
-			do_request = 1;
-
-	if (do_request)
+	/*
+	 * If the vcpus have matched TSCs and host clocksource is TSC,
+	 * perform request to enable masterclock.
+	 *
+	 * If the masterclock is enabled, perform request to update
+	 * masterclock values.
+	 */
+	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
+	     ka->use_master_clock)
 		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
 
 	trace_kvm_track_tsc(vcpu->vcpu_id, ka->nr_vcpus_matched_tsc,

Paolo Bonzini

maintainer

2014-11-05 09:36 UTC #1 lore raw

arch/x86/kvm/x86.c void kvm_track_tsc_matching(struct kvm_vcpu *vcpu) · line 1254

Quoted 9 lines · Marcelo Tosatti

arch/x86/kvm/x86.c +9 -0

+	/*
+	 * If the vcpus have matched TSCs and host clocksource is TSC,
+	 * perform request to enable masterclock.
+	 *
+	 * If the masterclock is enabled, perform request to update
+	 * masterclock values.
+	 */
+	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
+	     ka->use_master_clock)

This is not an explanation, it is a literal translation from C to English. :) Can you also explain the why, especially for the first half of the condition?

Paolo

Marcelo Tosatti

author

2014-11-05 18:19 UTC #2 lore raw

Quoted 15 lines · Paolo Bonzini

arch/x86/kvm/x86.c void kvm_track_tsc_matching(struct kvm_vcpu *vcpu) · line 1254
Quoted 9 lines · Marcelo Tosatti
arch/x86/kvm/x86.c +9 -0
+	/*
+	 * If the vcpus have matched TSCs and host clocksource is TSC,
+	 * perform request to enable masterclock.
+	 *
+	 * If the masterclock is enabled, perform request to update
+	 * masterclock values.
+	 */
+	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
+	     ka->use_master_clock)
This is not an explanation, it is a literal translation from C to English. :) Can you also explain the why, especially for the first half of the condition?

Quoted 1 line

Paolo

The comment on top of pvclock_update_vm_gtod_copy says:

"Rely on synchronization of host TSCs and guest TSCs for monotonicity."

Then with this patch

"If the vcpus have matched TSCs and host clocksource is TSC"

Is it sufficient to add

"Masterclock requires synchronized guest TSC and host clocksource TSC" to this patch?

Paolo Bonzini

maintainer

2014-11-05 19:47 UTC #3 lore raw

Quoted 18 lines · Marcelo Tosatti

Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
arch/x86/kvm/x86.c void kvm_track_tsc_matching(struct kvm_vcpu *vcpu) · line 1254
Quoted 8 lines
arch/x86/kvm/x86.c +8 -0
+	 * If the vcpus have matched TSCs and host clocksource is TSC,
+	 * perform request to enable masterclock.
+	 *
+	 * If the masterclock is enabled, perform request to update
+	 * masterclock values.
+	 */
+	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
+	     ka->use_master_clock)
This is not an explanation, it is a literal translation from C to English. :) Can you also explain the why, especially for the first half of the condition?
Quoted 1 line
Quoted 1 line
Paolo
The comment on top of pvclock_update_vm_gtod_copy says:
"Rely on synchronization of host TSCs and guest TSCs for monotonicity."

Quoted 8 lines

Then with this patch
"If the vcpus have matched TSCs and host clocksource is TSC"
Is it sufficient to add
"Masterclock requires synchronized guest TSC and host clocksource TSC" to this patch?

Got it now, thanks. I think I misread one "enable" as "update" in your comment, sorry. I'll apply v2 tomorrow.

Paolo

Paolo Bonzini

maintainer

2014-11-05 19:49 UTC #4 lore raw

Quoted 25 lines · Paolo Bonzini

Quoted 22 lines · Marcelo Tosatti
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
arch/x86/kvm/x86.c void kvm_track_tsc_matching(struct kvm_vcpu *vcpu) · line 1254
Quoted 8 lines
arch/x86/kvm/x86.c +8 -0
+	 * If the vcpus have matched TSCs and host clocksource is TSC,
+	 * perform request to enable masterclock.
+	 *
+	 * If the masterclock is enabled, perform request to update
+	 * masterclock values.
+	 */
+	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
+	     ka->use_master_clock)
This is not an explanation, it is a literal translation from C to English. :) Can you also explain the why, especially for the first half of the condition?
Quoted 1 line
Quoted 1 line
Paolo
The comment on top of pvclock_update_vm_gtod_copy says:
"Rely on synchronization of host TSCs and guest TSCs for monotonicity."
Then with this patch
"If the vcpus have matched TSCs and host clocksource is TSC"

Quoted 7 lines

Quoted 4 lines
Is it sufficient to add
"Masterclock requires synchronized guest TSC and host clocksource TSC" to this patch?
Got it now, thanks. I think I misread one "enable" as "update" in your comment, sorry. I'll apply v2 tomorrow.

Slightly reworded:

        /*
         * Once the masterclock is enabled, always perform request in
         * order to update it.
         *
         * Until then, if the vcpus have matched TSCs and host clocksource
         * is TSC, perform request to enable masterclock.
         */
        if (ka->use_master_clock ||
            (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC))
                kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

Paolo

Marcelo Tosatti

author

2014-11-05 20:58 UTC #5 lore raw

Quoted 49 lines · Paolo Bonzini

Quoted 25 lines · Paolo Bonzini
Quoted 22 lines · Marcelo Tosatti
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
arch/x86/kvm/x86.c void kvm_track_tsc_matching(struct kvm_vcpu *vcpu) · line 1254
Quoted 8 lines
arch/x86/kvm/x86.c +8 -0
+	 * If the vcpus have matched TSCs and host clocksource is TSC,
+	 * perform request to enable masterclock.
+	 *
+	 * If the masterclock is enabled, perform request to update
+	 * masterclock values.
+	 */
+	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
+	     ka->use_master_clock)
This is not an explanation, it is a literal translation from C to English. :) Can you also explain the why, especially for the first half of the condition?
Quoted 1 line
Quoted 1 line
Paolo
The comment on top of pvclock_update_vm_gtod_copy says:
"Rely on synchronization of host TSCs and guest TSCs for monotonicity."
Then with this patch
"If the vcpus have matched TSCs and host clocksource is TSC"
Quoted 7 lines
Quoted 4 lines
Is it sufficient to add
"Masterclock requires synchronized guest TSC and host clocksource TSC" to this patch?
Got it now, thanks. I think I misread one "enable" as "update" in your comment, sorry. I'll apply v2 tomorrow.
Slightly reworded:
        /*
         * Once the masterclock is enabled, always perform request in
         * order to update it.
         *
         * Until then, if the vcpus have matched TSCs and host clocksource
         * is TSC, perform request to enable masterclock.
         */
        if (ka->use_master_clock ||
            (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC))
                kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

Quoted 1 line

Paolo

Looks good, thanks.