[PATCH v2] KVM: x86: update masterclock values on TSC writes

[PATCH v2] KVM: x86: update masterclock values 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..50b6f56 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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,

Re: [PATCH v2] KVM: x86: update masterclock values on TSC writes

[Paolo Bonzini]

On 05/11/2014 00:30, Marcelo Tosatti wrote:
> +	/*
> +	 * 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

Re: [PATCH v2] KVM: x86: update masterclock values on TSC writes

[Marcelo Tosatti]

On Wed, Nov 05, 2014 at 10:36:05AM +0100, Paolo Bonzini wrote:
> On 05/11/2014 00:30, Marcelo Tosatti wrote:
> > +	/*
> > +	 * 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

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?

Re: [PATCH v2] KVM: x86: update masterclock values on TSC writes

[Paolo Bonzini]

On 05/11/2014 19:19, Marcelo Tosatti wrote:
>>> > > +	 * 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
> 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?

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

Paolo

Re: [PATCH v2] KVM: x86: update masterclock values on TSC writes

[Paolo Bonzini]

On 05/11/2014 20:47, Paolo Bonzini wrote:
> 
> 
> On 05/11/2014 19:19, Marcelo Tosatti wrote:
>>>>>> +	 * 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
>> 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?
> 
> 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

Re: [PATCH v2] KVM: x86: update masterclock values on TSC writes

[Marcelo Tosatti]

On Wed, Nov 05, 2014 at 08:49:38PM +0100, Paolo Bonzini wrote:
> 
> 
> On 05/11/2014 20:47, Paolo Bonzini wrote:
> > 
> > 
> > On 05/11/2014 19:19, Marcelo Tosatti wrote:
> >>>>>> +	 * 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
> >> 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?
> > 
> > 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

Looks good, thanks.