From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [patch] kvm: make cr3 loading more robust Date: Wed, 3 Jan 2007 13:21:14 +0100 Message-ID: <20070103122114.GD2786@elte.hu> References: <20070103021057.GA11316@elte.hu> <459B695C.5090004@qumranet.com> <20070103115911.GA2786@elte.hu> <459B9C5C.9060008@qumranet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel Return-path: To: Avi Kivity Content-Disposition: inline In-Reply-To: <459B9C5C.9060008-atKUWr5tajBWk0Htik3J/w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org * Avi Kivity wrote: > >hm, where do we ensure that? kvm_mmu_get_page() calls > >kvm_mmu_alloc_page(), which might return NULL: > > > > static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, > > u64 *parent_pte) > > { > > struct kvm_mmu_page *page; > > > > if (list_empty(&vcpu->free_pages)) > > return NULL; > > > >and i dont see where we ensure that ->free_pages is not empty. The > >relevant callchain would be kvm_vmx_return() -> handle_cr() -> > >set_cr3() -> paging_new_cr3() -> mmu_alloc_roots() -> > >kvm_mmu_get_page(). > > > >is there some accounting that makes a NULL pointer return impossible? > >The only one i can see is the direct quering of vcpu->free_pages or > >the querying of n_free_mmu_pages, but neither seems to be done in > >this codepath. (but i might have missed it) > > > > > > The page fault path does this: > > static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva, > u32 error_code) > { > if (unlikely(vcpu->kvm->n_free_mmu_pages < KVM_MIN_FREE_MMU_PAGES)) > kvm_mmu_free_some_pages(vcpu); > return vcpu->mmu.page_fault(vcpu, gva, error_code); > } ah. This hid in an include file. (Please dont put real program logic into include files - although i guess this is a justified exception :-) > The context switch paths need to do the same. yeah. Although, as this example has shown it, such implicit assumptions carried into code tends to be volatile. Couldnt the kvm_mmu_zap_page() be done implicitly within kvm_mmu_alloc_page()? As long as the list is LRU, and the number of allocations done within a logical operation doesnt exceed the size of the LRU list (which it doesnt), this should be doable. Ingo ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV