From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH] KVM: x86: fix KVM_SET_CLOCK relative to setting correct
 clock value
Date: Wed, 3 May 2017 15:24:01 -0300
Message-ID: <20170503182358.GA19415@amt.cnet>
References: <20170502213616.GA24837@amt.cnet>
 <2499ef65-1dfe-8460-ec41-661b05cc5023@redhat.com>
 <20170503134057.GA10468@amt.cnet>
 <568f71c4-440a-dcfd-3bd0-544275d104ee@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm-devel <kvm@vger.kernel.org>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:59940 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751315AbdECSY3 (ORCPT <rfc822;kvm@vger.kernel.org>);
        Wed, 3 May 2017 14:24:29 -0400
Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])
        (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
        (No client certificate requested)
        by mx1.redhat.com (Postfix) with ESMTPS id 00E7B448D96
        for <kvm@vger.kernel.org>; Wed,  3 May 2017 18:24:29 +0000 (UTC)
Content-Disposition: inline
In-Reply-To: <568f71c4-440a-dcfd-3bd0-544275d104ee@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Wed, May 03, 2017 at 03:55:36PM +0200, Paolo Bonzini wrote:
> 
> 
> On 03/05/2017 15:40, Marcelo Tosatti wrote:
> >>> In the masterclock enabled case, kvmclock_offset must be adjusted so
> >>> that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the
> >>> value set from KVM_SET_CLOCK is the one visible at system_timestamp).
> >>>
> >>> This way the guest clock:
> >>>
> >>> 	1. Starts counting when KVM_SET_CLOCK executes.
> >>> 	2. With the value provided by userspace.
> >>
> >> So this fixes rounding errors?
> > 
> > No. It just does the correct 
> > 
> > "user_ns.clock = master_kernel_ns + kvmclock_offset = return value of
> > get_kvmclock_ns(kvm) at moment of KVM_SET_CLOCK" 
> > 
> > (that is, when guest_rdtsc() == tsc_timestamp, so guest_rdtsc() - tsc_timestamp = 0, 
> > you want get_kvmclock_ns() to return user_ns.clock).
> 
> Yep, and it currently doesn't match because the current code introduces
> rounding errors?

No. Consider the current code. When get_kvmclock_ns() is called,
ka->masterclock = false.

kvm_gen_update_masterclock() sets it to true (because at the time
KVM_SET_CLOCK is called, the conditions necessary to do so are
fulfilled).

> >>> +			kvm_for_each_vcpu(i, vcpu, kvm)
> >>> +				kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> >> kvm_gen_update_masterclock already does that, why did you move that to
> >> before the assignment?
> > Its after the assignment of kvmclock_offset because
> > KVM_REQ_CLOCK_UPDATES processing by vcpus make use of kvmclock_offset.
> 
> Sure, but why did you move kvm_gen_update_masterclock before?

Explained above.

> If you left kvm_gen_update_masterclock after the update of
> kvm->arch.kvmclock_offset, the duplicate KVM_REQ_CLOCK_UPDATE would not
> be necessary.

No can do.

Feel free to reorganize/rewrite the patch if you feel like it, as long
as the same effects are achieved.

Thanks.

> 
> Paolo
> 
> > So if you change that value, you have to request the vcpus to
> > recalculate their kvmclock areas using the new kvmclock_offset.