Ingo Molnar wrote:
> * Avi Kivity <[EMAIL PROTECTED]> 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.
>
>
Ok. Some mechanism is then needed to unpin the pages in case they are
recycled by the guest.
>> 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?
No, rmap_write_protect() is called whenever we shadow a new guest page
table (the mechanism used to maintain coherency is write faults on page
tables).
> 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;
> }
> }
>
This will kill svm.
I don't think the tlb flushes are really necessary on today's Intel
machines, as they probably flush the tlb on each vmx switch. But AMD
keeps the TLB, and the flushes are critical there.
--
Do not meddle in the internals of kernels, for they are subtle and quick to
panic.
-------------------------------------------------------------------------
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
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
