From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH] x86/kvm: fix condition to update kvm master clocks Date: Tue, 21 Jun 2016 18:28:45 -0300 Message-ID: <20160621212845.GA14318@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> <20160620212908.GA17813@amt.cnet> <20160621144032.pmwc7baeo6247rzq@rkaganb.sw.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii To: Roman Kagan , kvm@vger.kernel.org, "Denis V. Lunev" , Owen Hofmann , Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55032 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751427AbcFUV31 (ORCPT ); Tue, 21 Jun 2016 17:29:27 -0400 Content-Disposition: inline In-Reply-To: <20160621144032.pmwc7baeo6247rzq@rkaganb.sw.ru> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Jun 21, 2016 at 05:40:32PM +0300, Roman Kagan wrote: > On Mon, Jun 20, 2016 at 06:29:10PM -0300, Marcelo Tosatti wrote: > > On Mon, Jun 20, 2016 at 08:22:49PM +0300, Roman Kagan wrote: > > > 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) > > This code is from an older version. The latest always reads the clock > of the CPU #0: > > /* > * Note: The kernel and hypervisor must guarantee that cpu ID > * number maps 1:1 to per-CPU pvclock time info. > * > * Because the hypervisor is entirely unaware of guest userspace > * preemption, it cannot guarantee that per-CPU pvclock time > * info is updated if the underlying CPU changes or that that > * version is increased whenever underlying CPU changes. > * > * On KVM, we are guaranteed that pvti updates for any vCPU are > * atomic as seen by *all* vCPUs. This is an even stronger > * guarantee than we get with a normal seqlock. > * > * On Xen, we don't appear to have that guarantee, but Xen still > * supplies a valid seqlock using the version field. > * > * We only do pvclock vdso timing at all if > * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to > * mean that all vCPUs have matching pvti and that the TSC is > * synced, so we can just look at vCPU 0's pvti. > */ In that case vCPU-N (that has its local kvmclock updated), will read vCPU-0's (which does not have its kvmclock area updated). vCPU-0 kvmclock area either contains the old copy of kvmclock values, or the new copy. That reading of vCPU-0 is protected by the version check, therefore it is safe. > > > Is there anything that prevents this? > > > > Guest code confirming both version and cpu do not change across > > a kvmclock read. Other than this, no. > > So is the code reading another vcpu's hv_clock wrong? Its fine. What you can't do is to: at vcpu-3: read kvmclock area of local vcpu. read kvmclock area of remote vcpu. And compare the values. What you can do is: at vcpu-1: read kvmclock area of local vcpu. at vcpu-3: read kvmclock area of local vcpu. And compare these reads. They should be monotonic. Or only read from a single vcpu, thats also monotonic.