Re: [patch 01/16] KVM: x86: retain pvclock guest stopped bit in guest memory

[Marcelo Tosatti <mtosatti@redhat.com>]

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 <mtosatti@redhat.com>
> > 
> Reviewed-by: Gleb Natapov <gleb@redhat.com>
> 
> 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.