Re: [announce] [patch] KVM paravirtualization for Linux

[Ingo Molnar <mingo@elte.hu>]

* Avi Kivity <avi@qumranet.com> wrote:

> > the cache is zapped upon pagefaults anyway, so unpinning ought to be 
> > possible. Which one would you prefer?
> 
> It's zapped by the equivalent of mmu_free_roots(), right?  That's 
> effectively unpinning it (by zeroing ->root_count).

no, right now only the guest-visible cache is zapped - the roots are 
zapped by natural rotation. I guess they should be zapped in 
kvm_cr3_cache_clear() - but i wanted to keep that function an invariant 
to the other MMU state, to make it easier to call it from whatever mmu 
codepath.

> However, kvm takes pagefaults even for silly things like setting (in 
> hardware) or clearing (in software) the dirty bit.

yeah. I think it also does some TLB flushes that are not needed. For 
example in rmap_write_protect() we do this:

                rmap_remove(vcpu, spte);
                kvm_arch_ops->tlb_flush(vcpu);

but AFAICS rmap_write_protect() is only ever called if we write a new 
cr3 - hence a TLB flush will happen anyway, because we do a 
vmcs_writel(GUEST_CR3, new_cr3). Am i missing something? I didnt want to 
remove it as part of the cr3 patches (to keep things simpler), but that 
flush looks quite unnecessary to me. The patch below seems to work in 
light testing.

	Ingo

Index: linux/drivers/kvm/mmu.c
===================================================================
--- linux.orig/drivers/kvm/mmu.c
+++ linux/drivers/kvm/mmu.c
@@ -404,7 +404,11 @@ static void rmap_write_protect(struct kv
 		BUG_ON(!(*spte & PT_WRITABLE_MASK));
 		rmap_printk("rmap_write_protect: spte %p %llx\n", spte, *spte);
 		rmap_remove(vcpu, spte);
-		kvm_arch_ops->tlb_flush(vcpu);
+		/*
+		 * While we removed a mapping there's no need to explicitly
+		 * flush the TLB here, because this codepath only triggers
+		 * if we write a new cr3 - which will flush the TLB anyway.
+		 */
 		*spte &= ~(u64)PT_WRITABLE_MASK;
 	}
 }