From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH v3 4/8] KVM-HV: Add VCPU running/pre-empted state for
 guest
Date: Fri, 3 Aug 2012 14:31:22 -0300
Message-ID: <20120803173122.GA5905@amt.cnet>
References: <20120731104312.16662.27889.stgit@abhimanyu.in.ibm.com>
 <20120731104838.16662.82021.stgit@abhimanyu.in.ibm.com>
 <20120802195628.GA30673@amt.cnet>
 <87a9yc7c4f.fsf@abhimanyu.in.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: peterz@infradead.org, avi@redhat.com, raghukt@linux.vnet.ibm.com,
	alex.shi@intel.com, mingo@elte.hu, kvm@vger.kernel.org,
	hpa@zytor.com
To: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:35458 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752877Ab2HCSRE (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 3 Aug 2012 14:17:04 -0400
Content-Disposition: inline
In-Reply-To: <87a9yc7c4f.fsf@abhimanyu.in.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Fri, Aug 03, 2012 at 11:25:44AM +0530, Nikunj A Dadhania wrote:
> On Thu, 2 Aug 2012 16:56:28 -0300, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > >  
> > > +	case MSR_KVM_VCPU_STATE:
> > > +		vcpu->arch.v_state.vs_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
> > > +		vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED);
> > 
> > Assign vs_offset after success.
> > 
> > > +
> > > +		if (is_error_page(vcpu->arch.v_state.vs_page)) {
> > > +			kvm_release_page_clean(vcpu->arch.time_page);
> > > +			vcpu->arch.v_state.vs_page = NULL;
> > > +			pr_info("KVM: VCPU_STATE - Unable to pin the page\n");
> > 
> > Missing break or return;
> > 
> > > +		}
> > > +		vcpu->arch.v_state.msr_val = data;
> > > +		break;
> > > +
> > >  	case MSR_IA32_MCG_CTL:
> > 
> > Please verify this code carefully again.
> > 
> > Also leaking the page reference.
> > 
> > >  	vcpu->arch.apf.msr_val = 0;
> > >  	vcpu->arch.st.msr_val = 0;
> > > +	vcpu->arch.v_state.msr_val = 0;
> > 
> > Add a newline and comment (or even better a new helper).
> > >  
> > >  	kvmclock_reset(vcpu);
> > 
> 
> How about something like the below. I have tried to look at time_page
> for reference:
> 
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 580abcf..c82cc12 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1604,6 +1604,16 @@ static void kvm_clear_vcpu_state(struct kvm_vcpu *vcpu)
>  	kunmap_atomic(kaddr);
>  }
>  
> +static void kvm_vcpu_state_reset(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.v_state.msr_val = 0;
> +	vcpu->arch.v_state.vs_offset = 0;
> +	if (vcpu->arch.v_state.vs_page) {
> +		kvm_release_page_dirty(vcpu->arch.v_state.vs_page);
> +		vcpu->arch.v_state.vs_page = NULL;
> +	}
> +}
> +
>  int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  {
>  	bool pr = false;
> @@ -1724,14 +1734,17 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  		break;
>  
>  	case MSR_KVM_VCPU_STATE:
> +		kvm_vcpu_state_reset(vcpu);
> +
>  		vcpu->arch.v_state.vs_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
> -		vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED);

Should also fail if its not enabled (KVM_MSR_ENABLED bit).

What is the point of having non-NULL vs_page pointer if KVM_MSR_ENABLED
bit is not set?

The rest is fine, thanks.