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

Ingo Molnar
2007-01-08T08:42:44Z

* 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;
}
}