Skip to contentSkip to conversation

KVM: MMU: only write protect mappings at pagetable level

PATCH Marcelo Tosatti 3 participants 7 messages Dec 22, 2010

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

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 <[email protected]>
arch/x86/kvm/mmu.c +3 -0
index c3853d5..c716ff8 100644
@@ -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 */
#1 lore raw
Quoted 1 line · Marcelo Tosatti

If a pagetable contains a writeable large spte, all of its sptes will be

non-writeable

Quoted 12 lines

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<[email protected]>
arch/x86/kvm/mmu.c +0 -0
index c3853d5..c716ff8 100644

arch/x86/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 3450

Quoted 9 lines
arch/x86/kvm/mmu.c +3 -0
@@ -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.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#2 lore raw
Quoted 30 lines · Avi Kivity
Quoted 1 line · Marcelo Tosatti

If a pagetable contains a writeable large spte, all of its sptes will be

non-writeable

Quoted 12 lines

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<[email protected]>
arch/x86/kvm/mmu.c +0 -0
index c3853d5..c716ff8 100644

arch/x86/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 3450

Quoted 9 lines
arch/x86/kvm/mmu.c +3 -0
@@ -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?

Quoted 1 line

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?

#3 lore raw
Quoted 28 lines · Marcelo Tosatti
Quoted 27 lines · Avi Kivity
Quoted 1 line · Marcelo Tosatti

If a pagetable contains a writeable large spte, all of its sptes will be

non-writeable

Quoted 21 lines

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<[email protected]>
arch/x86/kvm/mmu.c +3 -0
index c3853d5..c716ff8 100644
@@ -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 */
Quoted 7 lines
Quoted 4 lines

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

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#4 lore raw
Quoted 32 lines · Avi Kivity
Quoted 31 lines · Marcelo Tosatti
Quoted 30 lines · Avi Kivity
Quoted 1 line · Marcelo Tosatti

If a pagetable contains a writeable large spte, all of its sptes will be

non-writeable

Quoted 12 lines

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<[email protected]>
arch/x86/kvm/mmu.c +0 -0
index c3853d5..c716ff8 100644

arch/x86/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 3450

Quoted 9 lines
arch/x86/kvm/mmu.c +3 -0
@@ -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?

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

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;
#5 lore raw
Quoted 40 lines · Marcelo Tosatti
Quoted 39 lines · Avi Kivity
Quoted 28 lines · Marcelo Tosatti
Quoted 27 lines · Avi Kivity
Quoted 1 line · Marcelo Tosatti

If a pagetable contains a writeable large spte, all of its sptes will be

non-writeable

Quoted 21 lines

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<[email protected]>
arch/x86/kvm/mmu.c +3 -0
index c3853d5..c716ff8 100644
@@ -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 */
Quoted 7 lines
Quoted 4 lines

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?

Quoted 8 lines
Quoted 1 line

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.

Signature · 1 line
error compiling committee.c: too many arguments to function
Tested-by
#6 lore raw
In reply to Marcelo Tosatti
Quoted 13 lines · 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 <[email protected]>
arch/x86/kvm/mmu.c +0 -0
index c3853d5..c716ff8 100644

arch/x86/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 3450

Quoted 9 lines
arch/x86/kvm/mmu.c +3 -0
@@ -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 <[email protected]>