KVM: x86: update masterclock on TSC writes

PATCH Marcelo Tosatti 2 participants 3 messages Nov 3, 2014 – Nov 4, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2014-11-03 21:16 UTC #0 lore raw

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 +1 -1

index 0033df3..f52a887 100644
@@ -1248,7 +1248,7 @@ void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
 		if (!ka->use_master_clock)
 			do_request = 1;
 
-	if (!vcpus_matched && ka->use_master_clock)
+	if (ka->use_master_clock)
 			do_request = 1;
 
 	if (do_request)

Paolo Bonzini

maintainer

2014-11-04 17:10 UTC #1 lore raw

Extending the context we have:

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

Quoted 10 lines

        if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC)

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

 		if (!ka->use_master_clock)
 			do_request = 1;
 
-	if (!vcpus_matched && ka->use_master_clock)
+	if (ka->use_master_clock)
 			do_request = 1;
 
        if (do_request)
                kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

The patch also makes the previous "if (!ka->use_master_clock)" redundant. If you enter the first "if", do_request will be 1 independent of ka->use_master_clock. So you should also drop that one, and possibly rewrite it simply like this:

	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
	    ka->use_master_clock)
		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

But this brings the question: what is vclock_mode in your case? If it is VCLOCK_TSC, are you sure that the bug is fixed because you modified the second "if", or could it be fixed also by removing instead the "if (!ka->use_master_clock)"? This would leave the optimization in the case "!vcpus_matched && ka->use_master_clock". Or is the optimization always invalid?

A different way to state the same question: can you explain the resulting condition

	((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
	 ka->use_master_clock)

? Please add a comment to kvm_track_tsc_matching that clarifies this logic.

Paolo

Marcelo Tosatti

author

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

Quoted 21 lines · Paolo Bonzini

Extending the context we have:
arch/x86/kvm/x86.c void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
Quoted 10 lines
        if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC)
arch/x86/kvm/x86.c +1 -1
 		if (!ka->use_master_clock)
 			do_request = 1;
 
-	if (!vcpus_matched && ka->use_master_clock)
+	if (ka->use_master_clock)
 			do_request = 1;
 
        if (do_request)
                kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
The patch also makes the previous "if (!ka->use_master_clock)" redundant. If you enter the first "if", do_request will be 1 independent of ka->use_master_clock. So you should also drop that one, and possibly rewrite it simply like this:
	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
	    ka->use_master_clock)
		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

Quoted 6 lines

But this brings the question: what is vclock_mode in your case? If it is VCLOCK_TSC, are you sure that the bug is fixed because you modified the second "if", or could it be fixed also by removing instead the "if (!ka->use_master_clock)"? This would leave the optimization in the case "!vcpus_matched && ka->use_master_clock". Or is the optimization always invalid?

The bug is fixed by always updating masterclock values, when it is enabled.

Quoted 10 lines

A different way to state the same question: can you explain the resulting condition
	((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
	 ka->use_master_clock)
? Please add a comment to kvm_track_tsc_matching that clarifies this logic.
Paolo

Sure, sending v2.