From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: KVM: x86: do not retain disabled or invalid pvclock address
Date: Sun, 9 Sep 2012 20:37:03 -0300
Message-ID: <20120909233703.GA32724@amt.cnet>
References: <20120823111654.GA18057@amt.cnet>
 <504CA2DF.9070104@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm <kvm@vger.kernel.org>
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:2032 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1755702Ab2IIXhX (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 9 Sep 2012 19:37:23 -0400
Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22])
	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q89NbM0w014949
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <kvm@vger.kernel.org>; Sun, 9 Sep 2012 19:37:23 -0400
Content-Disposition: inline
In-Reply-To: <504CA2DF.9070104@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.