Re: large page support for kvm

[Marcelo Tosatti <marcelo@kvack.org>]

On Sun, Feb 17, 2008 at 11:38:51AM +0200, Avi Kivity wrote:
> >+ * Return the pointer to the largepage write count for a given
> >+ * gfn, handling slots that are not large page aligned.
> >+ */
> >+static int *slot_largepage_idx(gfn_t gfn, struct kvm_memory_slot *slot)
> >+{
> >+	unsigned long idx;
> >+
> >+	idx = (gfn - slot->base_gfn) + hpage_align_diff(slot->base_gfn);
> >+	idx /= KVM_PAGES_PER_HPAGE;
> >+	return &slot->lpage_info[idx].write_count;
> >+}
> >  
> 
> Can be further simplified to (gfn / KVM_PAGES_PER_HPAGE) - 
> (slot->base_gfn / KVM_PAGES_PER_HPAGE).  Sorry for not noticing earlier.

Right.

> >+	/* guest has 4M pages, host 2M */
> >+	if (!is_pae(vcpu) && HPAGE_SHIFT == 21)
> >+		return 0;
> >  
> 
> Is this check necessary?  I think that if we remove it things will just 
> work.  A 4MB page will be have either one or two 2MB sptes (which may 
> even belong to different slots).

You mentioned that before, I agree its not necessary.

> >+			/*
> >+ 			 * Largepage creation is susceptible to a upper-level
> >+ 			 * table to be shadowed and write-protected in the
> >+ 			 * area being mapped. If that is the case, invalidate
> >+ 			 * the entry and let the instruction fault again
> >+ 			 * and use 4K mappings.
> >+ 			 */
> >+			if (largepage) {
> >+				spte = shadow_trap_nonpresent_pte;
> >+				kvm_x86_ops->tlb_flush(vcpu);
> >+				goto unshadowed;
> >+			}
> >  
> 
> Would it not repeat exactly the same code path?  Or is this just for the 
> case of the pte_update path?

The problem is if the instruction writing to one of the roots can't be
emulated.

kvm_mmu_unprotect_page() does not know about largepages, so it will zap
a gfn inside the large page frame, but not the large translation itself.

And zapping the gfn brings the shadowed page count in large area to
zero, allowing has_wrprotected_page() to succeed. Endless unfixable
write faults.

> >-	page = gfn_to_page(vcpu->kvm, gpa >> PAGE_SHIFT);
> >+	if (is_largepage_backed(vcpu, gfn & ~(KVM_PAGES_PER_HPAGE-1))
> >+	    && is_physical_memory(vcpu->kvm, gfn)) {
> >+		gfn &= ~(KVM_PAGES_PER_HPAGE-1);
> >+		largepage = 1;
> >+	}
> >  
> 
> Doesn't is_largepage_backed() imply is_physical_memory?

Hum. I'll verify... it seems that now that the ends of the slots have
write_count set to 1 that should be true.

> > 
> >Index: kvm.largepages/arch/x86/kvm/x86.c
> >===================================================================
> >--- kvm.largepages.orig/arch/x86/kvm/x86.c
> >+++ kvm.largepages/arch/x86/kvm/x86.c
> >@@ -86,6 +86,7 @@ struct kvm_stats_debugfs_item debugfs_en
> > 	{ "mmu_recycled", VM_STAT(mmu_recycled) },
> > 	{ "mmu_cache_miss", VM_STAT(mmu_cache_miss) },
> > 	{ "remote_tlb_flush", VM_STAT(remote_tlb_flush) },
> >+	{ "lpages", VM_STAT(lpages) },
> > 	{ NULL }
> > };
> >  
> 
> s/lpages/largepages/, this is user visible.

OK.

> >+		new.lpage_info = vmalloc(largepages * 
> >sizeof(*new.lpage_info));
> >+
> >+		if (!new.lpage_info)
> >+			goto out_free;
> >+
> >+		memset(new.lpage_info, 0, largepages * 
> >sizeof(*new.lpage_info));
> >+		/* large page crosses memslot boundary */
> >+		if (npages % KVM_PAGES_PER_HPAGE) {
> >+			new.lpage_info[0].write_count = 1;
> >  
> 
> This seems wrong, say a 3MB slot at 1GB, you kill the first largepage 
> which is good.
> 
> >+			new.lpage_info[largepages-1].write_count = 1;
> >  
> 
> OTOH, a 3MB slot at 3MB, the last page is fine.  The check needs to be 
> against base_gfn and base_gfn + npages, not the number of pages.

Right, will fix. Will post an uptodated patch soon.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/