KVM: MMU: only write protect mappings at pagetable level

KVM: MMU: only write protect mappings at pagetable level

[Marcelo Tosatti]

If a pagetable contains a writeable large spte, all of its sptes will be
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 */

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

[Avi Kivity]

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.

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

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

[Marcelo Tosatti]

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? 

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

[Avi Kivity]

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().

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

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

[Marcelo Tosatti]

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;

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

[Avi Kivity]

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

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

[Alex Williamson]

On Wed, 2010-12-22 at 09:01 -0200, Marcelo Tosatti wrote:
> If a pagetable contains a writeable large spte, all of its sptes will be
> 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 */

Works for me.

Tested-by: Alex Williamson <alex.williamson@redhat.com>