From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH] x86/kvm: fix condition to update kvm master clocks
Date: Mon, 20 Jun 2016 18:29:10 -0300
Message-ID: <20160620212908.GA17813@amt.cnet>
References: <1464274195-31296-1-git-send-email-rkagan@virtuozzo.com>
 <20160529233844.GA14374@amt.cnet>
 <20160609032710.GA13318@amt.cnet>
 <20160609120945.GB2570@rkaganb.sw.ru>
 <20160609182501.GA24024@amt.cnet>
 <20160613171929.exgvqiw5hmnza2rp@rkaganb.sw.ru>
 <20160617222119.GA15883@amt.cnet>
 <20160620172249.3mo5aazrehdzttpa@rkaganb.sw.ru>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
To: Roman Kagan <rkagan@virtuozzo.com>, kvm@vger.kernel.org,
	"Denis V. Lunev" <den@openvz.org>, Owen Hofmann <osh@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:58321 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752127AbcFTViN (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 20 Jun 2016 17:38:13 -0400
Content-Disposition: inline
In-Reply-To: <20160620172249.3mo5aazrehdzttpa@rkaganb.sw.ru>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Mon, Jun 20, 2016 at 08:22:49PM +0300, Roman Kagan wrote:
> On Fri, Jun 17, 2016 at 07:21:21PM -0300, Marcelo Tosatti wrote:
> > On Mon, Jun 13, 2016 at 08:19:30PM +0300, Roman Kagan wrote:
> > > While we're at this:
> > > 
> > > According to the comments in the code, the purpose of the masterclock
> > > scheme is to prevent any vCPU from seeing an outdated hv_clock of
> > > another vCPU.
> > 
> > It prevents two vcpus from using different hv_clocks:
> > 
> > "
> >  * To avoid that problem, do not allow visibility of distinct
> >  * system_timestamp/tsc_timestamp values simultaneously: use a master
> >  * copy of host monotonic time values. Update that master copy
> >  * in lockstep.
> > "
> > 
> > > However I'm missing how that is achieved.  AFAICS the guest entry is
> > > allowed as soon as all vCPUs are kicked from guest with
> > > KVM_REQ_CLOCK_UPDATE set; what stops one vCPU from processing it and
> > > entering the guest before another vCPU even started updating its
> > > hv_clock?
> > 
> > static void kvm_gen_update_masterclock(struct kvm *kvm)
> > {
> > #ifdef CONFIG_X86_64
> >         int i;
> >         struct kvm_vcpu *vcpu;
> >         struct kvm_arch *ka = &kvm->arch;
> > 
> >         spin_lock(&ka->pvclock_gtod_sync_lock);
> >         kvm_make_mclock_inprogress_request(kvm);
> >         /* no guest entries from this point */
> >         pvclock_update_vm_gtod_copy(kvm);
> > 
> >         kvm_for_each_vcpu(i, vcpu, kvm)
> >                 kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> > 
> >         /* guest entries allowed */
> >         kvm_for_each_vcpu(i, vcpu, kvm)
> >                 clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
> > 
> >         spin_unlock(&ka->pvclock_gtod_sync_lock);
> > #endif
> > }
> 
> Unless I'm missing something obvious again:
> 
> The per-vcpu hv_clock is updated when the vcpu processes
> KVM_REQ_CLOCK_UPDATE request.

Yes.

> Once kvm_gen_update_masterclock() sets KVM_REQ_CLOCK_UPDATE and
> clears KVM_REQ_MCLOCK_INPROGRESS for all vcpus, one vcpu can process the
> requests, enter the guest, and read another vcpu's hv_clock, before that
> other vcpu had a chance to process its KVM_REQ_CLOCK_UPDATE request.

Yes. But guest code should be reading its local kvmclock area:

                /*
                 * Test we're still on the cpu as well as the version.
                 * We could have been migrated just after the first
                 * vgetcpu but before fetching the version, so we
                 * wouldn't notice a version change.
                 */
                cpu1 = __getcpu() & VGETCPU_CPU_MASK;

(vclock_gettime.c)

> Is there anything that prevents this?

Guest code confirming both version and cpu do not change across 
a kvmclock read. Other than this, no.
> 
> Thanks,
> Roman.