Re: KVM: x86: do not retain disabled or invalid pvclock address

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sun, Sep 09, 2012 at 05:08:31PM +0300, Avi Kivity wrote:
> On 08/23/2012 02:16 PM, Marcelo Tosatti wrote:
> > 
> > In case an invalid or disabled gpa is written to the SYSTEM_TIME
> > MSR, do not retain its value. This is not documented behaviour, 
> > nor should be supported.
> > 
> > Also clear it on system reset. Not doing so can hide bugs.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e00050c..ed4bfb7 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1528,6 +1528,7 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
> >  
> >  static void kvmclock_reset(struct kvm_vcpu *vcpu)
> >  {
> > +	vcpu->arch.time = 0;
> >  	if (vcpu->arch.time_page) {
> >  		kvm_release_page_dirty(vcpu->arch.time_page);
> >  		vcpu->arch.time_page = NULL;
> > @@ -1632,8 +1633,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> >  
> >  		/* we verify if the enable bit is set... */
> > -		if (!(data & 1))
> > +		if (!(data & 1)) {
> > +			vcpu->arch.time = 0;
> 
> Should we not just assign data to vcpu->arch.time?  That's how the real
> MSRs work.
> 
> >  			break;
> > +		}
> >  
> >  		/* ...but clean it before doing the actual write */
> >  		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
> > @@ -1641,8 +1644,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >  		vcpu->arch.time_page =
> >  				gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
> >  
> > -		if (is_error_page(vcpu->arch.time_page))
> > +		if (is_error_page(vcpu->arch.time_page)) {
> >  			vcpu->arch.time_page = NULL;
> > +			vcpu->arch.time = 0;
> > +		}
> >  
> 
> Don't see why.

This would cause problems later with the new msr. But you're right,
there is nothing incorrect in the current behaviour.

Will keep these changes where they have more context and meaning.