KVM: x86: update masterclock on TSC writes

KVM: x86: update masterclock on TSC writes

[Marcelo Tosatti]

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 <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0033df3..f52a887 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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)

Re: KVM: x86: update masterclock on TSC writes

[Paolo Bonzini]

Extending the context we have:
>         if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC)
>  		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

Re: KVM: x86: update masterclock on TSC writes

[Marcelo Tosatti]

On Tue, Nov 04, 2014 at 06:10:34PM +0100, Paolo Bonzini wrote:
> Extending the context we have:
> >         if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC)
> >  		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?

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

> 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.