Re: KVM: MMU: only write protect mappings at pagetable level

[Avi Kivity <avi@redhat.com>]

On 12/22/2010 03:06 PM, Marcelo Tosatti wrote:
> On Wed, Dec 22, 2010 at 03:01:19PM +0200, Avi Kivity wrote:
> >  On 12/22/2010 01:12 PM, Marcelo Tosatti wrote:
> >  >On Wed, Dec 22, 2010 at 01:07:23PM +0200, Avi Kivity wrote:
> >  >>   On 12/22/2010 01:01 PM, Marcelo Tosatti wrote:
> >  >>   >If a pagetable contains a writeable large spte, all of its sptes will be
> >  >>
> >  >>   non-writeable
> >  >>
> >  >>   >write protected, including non-leaf ones, leading to endless pagefaults.
> >  >>   >
> >  >>   >Do not write protect pages above PT_PAGE_TABLE_LEVEL, as the spte fault
> >  >>   >paths assume non-leaf sptes are writable.
> >  >>   >
> >  >>   >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >  >>   >
> >  >>   >diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> >  >>   >index c3853d5..c716ff8 100644
> >  >>   >--- a/arch/x86/kvm/mmu.c
> >  >>   >+++ b/arch/x86/kvm/mmu.c
> >  >>   >@@ -3442,6 +3442,9 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
> >  >>   >    		if (!test_bit(slot, sp->slot_bitmap))
> >  >>   >    			continue;
> >  >>   >
> >  >>   >+		if (sp->role.level != PT_PAGE_TABLE_LEVEL)
> >  >>   >+			continue;
> >  >>   >+
> >  >>   >    		pt = sp->spt;
> >  >>   >    		for (i = 0; i<    PT64_ENT_PER_PAGE; ++i)
> >  >>   >    			/* avoid RMW */
> >  >>
> >  >>   But what about large leaf sptes?  Don't we want to write protect, or
> >  >>   perhaps drop them?
> >  >>
> >  >>   I think write-protecting leaf sptes and ignoring nonleaf sptes should work.
> >  >
> >  >When dirty logging is enabled large sptes are nuked and creation of new
> >  >ones is not allowed. So i don't see the need?
> >
> >  Where does this nuking happen?
> >
> >  All I see is the call to kvm_mmu_slot_remove_write_access().
>
> set_memory_region:
>
>                  /* destroy any largepage mappings for dirty tracking */
>                  if (old.npages)
>                          flush_shadow = 1;
>

You're right, patch is fine.

We should drop this though, and replace it by an ordinary 
remove_write_access() enhanced to drop large sptes.  Should be done 
independently of the fix, of course.

-- 
error compiling committee.c: too many arguments to function