From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikunj A Dadhania Subject: Re: [PATCH v3 4/8] KVM-HV: Add VCPU running/pre-empted state for guest Date: Sun, 05 Aug 2012 00:03:57 +0530 Message-ID: <8739428q22.fsf@linux.vnet.ibm.com> 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> <20120803173122.GA5905@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain 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: Marcelo Tosatti Return-path: Received: from e28smtp02.in.ibm.com ([122.248.162.2]:47521 "EHLO e28smtp02.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753780Ab2HDSew (ORCPT ); Sat, 4 Aug 2012 14:34:52 -0400 Received: from /spool/local by e28smtp02.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sun, 5 Aug 2012 00:04:48 +0530 Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay02.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q74IYkcU10223726 for ; Sun, 5 Aug 2012 00:04:46 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q74IYjl3024872 for ; Sun, 5 Aug 2012 04:34:45 +1000 In-Reply-To: <20120803173122.GA5905@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, 3 Aug 2012 14:31:22 -0300, Marcelo Tosatti wrote: > 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 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? > Yes, will do that. > The rest is fine, thanks. > Thanks Nikunj