Re: [patch] kvm: make cr3 loading more robust

[Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>]

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> 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