kvm: rework remove-write-access for a slot
Subsystems (4)
PATCH 1/1 kvm: rework remove-write-access for a slot
Current code uses slot_bitmap to find ptes who map a page from the memory slot, it is not precise: some ptes in the shadow page are not map any page from the memory slot.
This patch uses rmap to find the ptes precisely, and remove the unused slot_bitmap.
---
Documentation/kvm/mmu.txt +0 -7
@@ -183,13 +183,6 @@ Shadow pages contain the following information: perform a reverse map from a pte to a gfn. When role.direct is set, any element of this array can be calculated from the gfn field when used, in this case, the array of gfns is not allocated. See role.direct and gfn. - slot_bitmap: - A bitmap containing one bit per memory slot. If the page contains a pte - mapping a page from memory slot n, then bit n of slot_bitmap will be set - (if a page is aliased among several slots, then it is not guaranteed that - all slots will be marked). - Used during dirty logging to avoid scanning a shadow page if none if its - pages need tracking. root_count: A counter keeping track of how many hardware registers (guest cr3 or pdptrs) are now pointing at the page. While this counter is nonzero, the
arch/x86/include/asm/kvm_host.h +0 -5
@@ -197,11 +197,6 @@ struct kvm_mmu_page {u64 *spt;/* hold the gfn of each spte inside spt */ gfn_t *gfns; -/* -* One bit set per slot which has memory -* in this shadow page. -*/ -DECLARE_BITMAP (slot_bitmap,KVM_MEMORY_SLOTS +KVM_PRIVATE_MEM_SLOTS );bool multimapped;/* More than one parent_pte? */ bool unsync;int root_count;/* Currently serving as active root */
arch/x86/kvm/mmu.c +27 -21
77 lines not on this page · raw message
Quoted 8 lines · Lai Jiangshan
Current code uses
slot_bitmapto find ptes who map a page from the memory slot, it is not precise: some ptes in the shadow page are not map any page from the memory slot.This patch uses rmap to find the ptes precisely, and remove the unused
slot_bitmap.
Patch looks good; a couple of comments:
- We might see a slowdown with !tdp, since we no longer have locality. Each page will map to an spte in a different page. However, it's still worth it in my opinion.
- I thought of a different approach to write protection: write protect the L4 sptes, on write fault add write permission to the L4 spte and write protect the L3 sptes that it points to, etc. This method can use the slot bitmap to reduce the number of write faults. However we can reintroduce the slot bitmap if/when we use the method, this shouldn't block the patch.
arch/x86/kvm/mmu.c void kvm_mmu_destroy(struct kvm_vcpu *vcpu) · line 2975
Quoted 10 lines
+static void
rmapp_remove_write_access(struct kvm *kvm, unsigned long *rmapp)arch/x86/kvm/mmu.c +7
-0+{ +u64 *spte =rmap_next (kvm, rmapp,NULL ); + +while (spte) { +/* avoid RMW */ +if (is_writable_pte (*spte)) + *spte &= ~PT_WRITABLE_MASK ;
Must use an atomic operation here to avoid losing dirty or accessed bit.
Quoted 3 lines
arch/x86/kvm/mmu.c +3
-0+ spte =rmap_next (kvm, rmapp, spte); + } +}
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 10 lines · Avi Kivity
Quoted 8 lines · Lai Jiangshan
Current code uses
slot_bitmapto find ptes who map a page from the memory slot, it is not precise: some ptes in the shadow page are not map any page from the memory slot.This patch uses rmap to find the ptes precisely, and remove the unused
slot_bitmap.
Quoted 5 lines
Patch looks good; a couple of comments:
- We might see a slowdown with !tdp, since we no longer have locality. Each page will map to an spte in a different page. However, it's still worth it in my opinion.
Yes, this patch hurts the cache since we no longer have locality. And if most pages of the slot are not mapped(rmap_next(kvm, rmapp, NULL)==NULL), this patch will worse than old method I think.
This patch do things straightly, precisely.
Quoted 6 lines
- I thought of a different approach to write protection: write protect the L4 sptes, on write fault add write permission to the L4 spte and write protect the L3 sptes that it points to, etc. This method can use the slot bitmap to reduce the number of write faults. However we can reintroduce the slot bitmap if/when we use the method, this shouldn't block the patch.
It is very a good approach and it is blazing fast.
I have no time to implement it currently,
could you update it into the TODO list?
Quoted 14 lines
arch/x86/kvm/mmu.c void kvm_mmu_destroy(struct kvm_vcpu *vcpu) · line 2975
Quoted 11 lines
+static void
rmapp_remove_write_access(struct kvm *kvm, unsigned long *rmapp)arch/x86/kvm/mmu.c +7
-0+{ +u64 *spte =rmap_next (kvm, rmapp,NULL ); + +while (spte) { +/* avoid RMW */ +if (is_writable_pte (*spte)) + *spte &= ~PT_WRITABLE_MASK ;Must use an atomic operation here to avoid losing dirty or accessed bit.
Atomic operation is too expensive, I retained the comment "/* avoid RMW */" and wait someone take a good approach for it.
Quoted 8 lines · Lai Jiangshan
Quoted 7 lines · Avi Kivity
Quoted 6 lines · Lai Jiangshan
Current code uses
slot_bitmapto find ptes who map a page from the memory slot, it is not precise: some ptes in the shadow page are not map any page from the memory slot.This patch uses rmap to find the ptes precisely, and remove the unused
slot_bitmap.
Note that the current code is precise: memslot_id does unalias_gfn.
Quoted 9 lines
Quoted 5 lines
Patch looks good; a couple of comments:
- We might see a slowdown with !tdp, since we no longer have locality. Each page will map to an spte in a different page. However, it's still worth it in my opinion.
Yes, this patch hurts the cache since we no longer have locality. And if most pages of the slot are not
mapped(rmap_next(kvm, rmapp, NULL)==NULL), this patch will worse than old method I think.
Can you get some numbers before/after patch, with/without lots of shadow pages instantiated? Better with large amount of memory for the guest.
Because shrinking kvm_mmu_page is good.
Quoted 9 lines · Lai Jiangshan
Quoted 9 lines
- I thought of a different approach to write protection: write protect the L4 sptes, on write fault add write permission to the L4 spte and write protect the L3 sptes that it points to, etc. This method can use the slot bitmap to reduce the number of write faults. However we can reintroduce the slot bitmap if/when we use the method, this shouldn't block the patch.
Quoted 5 lines
It is very a good approach and it is blazing fast.
I have no time to implement it currently,
could you update it into the TODO list?
Done.
Quoted 13 lines
Quoted 13 lines
arch/x86/kvm/mmu.c void kvm_mmu_destroy(struct kvm_vcpu *vcpu)
Quoted 10 lines
+static void
rmapp_remove_write_access(struct kvm *kvm, unsigned long *rmapp)arch/x86/kvm/mmu.c +7
-0+{ +u64 *spte =rmap_next (kvm, rmapp,NULL ); + +while (spte) { +/* avoid RMW */ +if (is_writable_pte (*spte)) + *spte&= ~PT_WRITABLE_MASK ;Must use an atomic operation here to avoid losing dirty or accessed bit.
Quoted 3 lines
Atomic operation is too expensive, I retained the comment "/* avoid RMW */" and wait someone take a good approach for it.
You are right, it is an existing problem. I just posted a patchset which fixes the problem, when it's merged please rebase on top.
Signature · 1 line
error compiling committee.c: too many arguments to function