From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [patch 01/16] KVM: x86: retain pvclock guest stopped bit in guest memory Date: Thu, 1 Nov 2012 18:51:08 -0200 Message-ID: <20121101205108.GA14888@amt.cnet> References: <20121031224656.417434866@redhat.com> <20121031224823.865958085@redhat.com> <20121101103930.GP25650@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, johnstul@us.ibm.com, jeremy@goop.org, glommer@parallels.com, zamsden@gmail.com, avi@redhat.com, pbonzini@redhat.com To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:53855 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759940Ab2KAVHK (ORCPT ); Thu, 1 Nov 2012 17:07:10 -0400 Content-Disposition: inline In-Reply-To: <20121101103930.GP25650@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Nov 01, 2012 at 12:39:30PM +0200, Gleb Natapov wrote: > On Wed, Oct 31, 2012 at 08:46:57PM -0200, Marcelo Tosatti wrote: > > Otherwise its possible for an unrelated KVM_REQ_UPDATE_CLOCK (such as due to CPU > > migration) to clear the bit. > > > > Noticed by Paolo Bonzini. > > > > Signed-off-by: Marcelo Tosatti > > > Reviewed-by: Gleb Natapov > > Small nitpick bellow. > > Also do we really need to call kvm_guest_time_update() on a guest pause? > Wouldn't separate request bit, which only sets the flag, suffice? Management of vcpu clock area better be isolated in a single place. > > Index: vsyscall/arch/x86/kvm/x86.c > > =================================================================== > > --- vsyscall.orig/arch/x86/kvm/x86.c > > +++ vsyscall/arch/x86/kvm/x86.c > > @@ -1143,6 +1143,7 @@ static int kvm_guest_time_update(struct > > unsigned long this_tsc_khz; > > s64 kernel_ns, max_kernel_ns; > > u64 tsc_timestamp; > > + struct pvclock_vcpu_time_info *guest_hv_clock; > > u8 pvclock_flags; > > > > /* Keep irq disabled to prevent changes to the clock */ > > @@ -1226,13 +1227,6 @@ static int kvm_guest_time_update(struct > > vcpu->last_kernel_ns = kernel_ns; > > vcpu->last_guest_tsc = tsc_timestamp; > > > > - pvclock_flags = 0; > > - if (vcpu->pvclock_set_guest_stopped_request) { > > - pvclock_flags |= PVCLOCK_GUEST_STOPPED; > > - vcpu->pvclock_set_guest_stopped_request = false; > > - } > > - > > - vcpu->hv_clock.flags = pvclock_flags; > > > > /* > > * The interface expects us to write an even number signaling that the > > @@ -1243,6 +1237,18 @@ static int kvm_guest_time_update(struct > > > > shared_kaddr = kmap_atomic(vcpu->time_page); > > > > + guest_hv_clock = shared_kaddr + vcpu->time_offset; > > + > > + /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ > > + pvclock_flags = (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); > > + > > + if (vcpu->pvclock_set_guest_stopped_request) { > > + pvclock_flags |= PVCLOCK_GUEST_STOPPED; > > + vcpu->pvclock_set_guest_stopped_request = false; > > + } > > + > > + vcpu->hv_clock.flags = pvclock_flags; > > + > > memcpy(shared_kaddr + vcpu->time_offset, &vcpu->hv_clock, > We can use guest_hv_clock here now. > > > sizeof(vcpu->hv_clock)); > > > > > > -- > Gleb.