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: Fri, 03 Aug 2012 10:47:23 +0530 Message-ID: <87fw847dwc.fsf@abhimanyu.in.ibm.com> References: <20120731104312.16662.27889.stgit@abhimanyu.in.ibm.com> <20120731104838.16662.82021.stgit@abhimanyu.in.ibm.com> <20120802195628.GA30673@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 e23smtp03.au.ibm.com ([202.81.31.145]:40251 "EHLO e23smtp03.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751122Ab2HCFSO (ORCPT ); Fri, 3 Aug 2012 01:18:14 -0400 Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 3 Aug 2012 15:17:46 +1000 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7359n4A23330978 for ; Fri, 3 Aug 2012 15:09:49 +1000 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q735I7rH013523 for ; Fri, 3 Aug 2012 15:18:08 +1000 In-Reply-To: <20120802195628.GA30673@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, 2 Aug 2012 16:56:28 -0300, Marcelo Tosatti wrote: > On Tue, Jul 31, 2012 at 04:18:41PM +0530, Nikunj A. Dadhania wrote: > > From: Nikunj A. Dadhania > > > > Hypervisor code to indicate guest running/pre-empteded status through > > msr. The page is now pinned during MSR write time and use > > kmap_atomic/kunmap_atomic to access the shared area vcpu_state area. > > > > Suggested-by: Marcelo Tosatti > > Signed-off-by: Nikunj A. Dadhania > > --- > > arch/x86/include/asm/kvm_host.h | 7 ++++ > > arch/x86/kvm/cpuid.c | 1 + > > arch/x86/kvm/x86.c | 71 ++++++++++++++++++++++++++++++++++++++- > > 3 files changed, 77 insertions(+), 2 deletions(-) [...] > > +static void kvm_set_atomic(u64 *addr, u64 old, u64 new) > > +{ > > + int loop = 1000000; > > + while (1) { > > + if (cmpxchg(addr, old, new) == old) > > + break; > > + loop--; > > + if (!loop) { > > + pr_info("atomic cur: %lx old: %lx new: %lx\n", > > + *addr, old, new); > > + break; > > + } > > + } > > +} > > A generic "kvm_set_atomic" would need that loop, but in the particular > TLB flush case we know that the only information being transmitted is > a TLB flush. > yes, so the next patch gets rid of this in a neater way. > So this idea should work: > > old = *addr; > if (cmpxchg(addr, old, IN_GUEST_MODE) == FAILURE) > kvm_x86_ops->tlb_flush() > atomic_set(addr, IN_GUEST_MODE); > } else if { > if (old & TLB_SHOULD_FLUSH) > kvm_x86_ops->tlb_flush() > } > > (the actual pseucode above is pretty ugly and > mus be improved but it should be enough to transmit > the idea). > > Of course as long as you make sure the atomic_set does not > overwrite information. > > > > + char *kaddr; > > + > > + if (!(vcpu->arch.v_state.msr_val & KVM_MSR_ENABLED) || > > + !vcpu->arch.v_state.vs_page) > > + return; > > If its not enabled vs_page should be NULL? > Yes, it should be: if (!(enabled && vs_page)) return; > > + > > + kaddr = kmap_atomic(vcpu->arch.v_state.vs_page); > > + kaddr += vcpu->arch.v_state.vs_offset; > > + vs = kaddr; > > + kvm_set_atomic(&vs->state, 0, 1 << KVM_VCPU_STATE_IN_GUEST_MODE); > > + kunmap_atomic(kaddr); > > +} > > + > > +static void kvm_clear_vcpu_state(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_vcpu_state *vs; > > + char *kaddr; > > + > > + if (!(vcpu->arch.v_state.msr_val & KVM_MSR_ENABLED) || > > + !vcpu->arch.v_state.vs_page) > > + return; > > Like above. > > > + kaddr = kmap_atomic(vcpu->arch.v_state.vs_page); > > + kaddr += vcpu->arch.v_state.vs_offset; > > + vs = kaddr; > > + kvm_set_atomic(&vs->state, 1 << KVM_VCPU_STATE_IN_GUEST_MODE, 0); > > + kunmap_atomic(kaddr); > > +} > > + > > int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > { > > bool pr = false; > > @@ -1676,6 +1723,18 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > return 1; > > break; > > > > + 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. > Will do that. > > + > > + if (is_error_page(vcpu->arch.v_state.vs_page)) { > > + kvm_release_page_clean(vcpu->arch.time_page); C&P error :( kvm_release_page_clean(vcpu->arch.v_state.vs_page); > > + vcpu->arch.v_state.vs_page = NULL; > > + pr_info("KVM: VCPU_STATE - Unable to pin the page\n"); > > Missing break or return; > Right > > + } > > + 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). > Will do. Thanks for the detailed review. Nikunj