Skip to contentSkip to conversation

KVM: MMU: fix forgot reserved bits check in speculative path

PATCH In review Xiao Guangrong 2 participants 12 messages Jul 16, 2010 – Jul 25, 2010

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

Xiao Guangrong

author
#0 lore raw

PATCH v2 1/6 KVM: MMU: fix forgot reserved bits check in speculative path

In the speculative path, we should check guest pte's reserved bits just as the real processor does

Reported-by Marcelo Tosatti <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c         |    9 ++++++++-
 arch/x86/kvm/paging_tmpl.h |    5 +++--
 2 files changed, 11 insertions(+), 3 deletions(-)
arch/x86/kvm/mmu.c +8 -1
index d16efbe..1a4b42e 100644
@@ -2693,6 +2693,9 @@ static void mmu_pte_write_new_pte(struct kvm_vcpu *vcpu,
 		return;
         }
 
+	if (is_rsvd_bits_set(vcpu, *(u64 *)new, PT_PAGE_TABLE_LEVEL))
+		return;
+
 	++vcpu->kvm->stat.mmu_pte_updated;
 	if (!sp->role.cr4_pae)
 		paging32_update_pte(vcpu, sp, spte, new);

@@ -2771,6 +2774,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       bool guest_initiated)
 {
 	gfn_t gfn = gpa >> PAGE_SHIFT;
+	union kvm_mmu_page_role mask = { .word = 0 };
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node;
 	LIST_HEAD(invalid_list);

@@ -2845,6 +2849,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		}
 	}
 
+	mask.cr0_wp = mask.cr4_pae = mask.nxe = 1;
 	for_each_gfn_indirect_valid_sp(vcpu->kvm, sp, gfn, node) {
 		pte_size = sp->role.cr4_pae ? 8 : 4;
 		misaligned = (offset ^ (offset + bytes - 1)) & ~(pte_size - 1);

@@ -2892,7 +2897,9 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		while (npte--) {
 			entry = *spte;
 			mmu_pte_write_zap_pte(vcpu, sp, spte);
-			if (gentry)
+			if (gentry &&
+			      !(sp->role.word ^ vcpu->arch.mmu.base_role.word)
+			      & mask.word)
 				mmu_pte_write_new_pte(vcpu, sp, spte, &gentry);
 			if (!remote_flush && need_remote_flush(entry, *spte))
 				remote_flush = true;
arch/x86/kvm/paging_tmpl.h +3 -2
index a09e04c..231fce1 100644
@@ -638,8 +638,9 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 			return -EINVAL;
 
 		gfn = gpte_to_gfn(gpte);
-		if (gfn != sp->gfns[i] ||
-		      !is_present_gpte(gpte) || !(gpte & PT_ACCESSED_MASK)) {
+		if (is_rsvd_bits_set(vcpu, gpte, PT_PAGE_TABLE_LEVEL)
+		      || gfn != sp->gfns[i] || !is_present_gpte(gpte)
+		      || !(gpte & PT_ACCESSED_MASK)) {
 			u64 nonpresent;
 
 			if (is_present_gpte(gpte) || !clear_unsync)
Signature · 1 line
1.6.1.2
author
#1 lore raw

PATCH v2 2/6 KVM: MMU: fix page accessed tracking lost if ept is enabled

In current code, if ept is enabled(shadow_accessed_mask = 0), the page accessed tracking is lost

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
arch/x86/kvm/mmu.c +3 -2
index 1a4b42e..5937054 100644
@@ -687,7 +687,7 @@ static void drop_spte(struct kvm *kvm, u64 *sptep, u64 new_spte)
 	if (!is_rmap_spte(old_spte))
 		return;
 	pfn = spte_to_pfn(old_spte);
-	if (old_spte & shadow_accessed_mask)
+	if (!shadow_accessed_mask || old_spte & shadow_accessed_mask)
 		kvm_set_pfn_accessed(pfn);
 	if (is_writable_pte(old_spte))
 		kvm_set_pfn_dirty(pfn);

@@ -815,7 +815,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 				kvm_set_pfn_dirty(spte_to_pfn(*spte));
 			old_spte = __xchg_spte(spte, new_spte);
 			if (is_shadow_present_pte(old_spte)
-			    && (old_spte & shadow_accessed_mask))
+			    && (!shadow_accessed_mask ||
+			    old_spte & shadow_accessed_mask))
 				mark_page_accessed(pfn_to_page(spte_to_pfn(old_spte)));
 			spte = rmap_next(kvm, rmapp, spte);
 		}
Signature · 1 line
1.6.1.2
author
#2 lore raw

PATCH v2 3/6 KVM: MMU: fix page dirty tracking lost while sync page

In reply to Xiao Guangrong

In sync-page path, if spte.writable is changed, it will lose page dirty tracking, for example:

assume spte.writable = 0 in a unsync-page, when it's synced, it map spte to writable(that is spte.writable = 1), later guest write spte.gfn, it means spte.gfn is dirty, then guest changed this mapping to read-only, after it's synced, spte.writable = 0

So, when host release the spte, it detect spte.writable = 0 and not mark page dirty

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
arch/x86/kvm/mmu.c +3 -7
index 5937054..b3896bf 100644
@@ -1981,6 +1981,8 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 		mark_page_dirty(vcpu->kvm, gfn);
 
 set_pte:
+	if (is_writable_pte(*sptep) && !is_writable_pte(spte))
+		kvm_set_pfn_dirty(pfn);
 	update_spte(sptep, spte);
 done:
 	return ret;

@@ -1994,7 +1996,6 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			 bool reset_host_protection)
 {
 	int was_rmapped = 0;
-	int was_writable = is_writable_pte(*sptep);
 	int rmap_count;
 
 	pgprintk("%s: spte %llx access %x write_fault %d"

@@ -2044,15 +2045,10 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	page_header_update_slot(vcpu->kvm, sptep, gfn);
 	if (!was_rmapped) {
 		rmap_count = rmap_add(vcpu, sptep, gfn);
-		kvm_release_pfn_clean(pfn);
 		if (rmap_count > RMAP_RECYCLE_THRESHOLD)
 			rmap_recycle(vcpu, sptep, gfn);
-	} else {
-		if (was_writable)
-			kvm_release_pfn_dirty(pfn);
-		else
-			kvm_release_pfn_clean(pfn);
 	}
+	kvm_release_pfn_clean(pfn);
 	if (speculative) {
 		vcpu->arch.last_pte_updated = sptep;
 		vcpu->arch.last_pte_gfn = gfn;
Signature · 1 line
1.6.1.2
author
#3 lore raw

PATCH v2 4/6 KVM: MMU: don't atomicly set spte if it's not present

If the old mapping is not present, the spte.a is not lost, so no need atomic operation to set it

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
arch/x86/kvm/mmu.c +3 -2
index b3896bf..4123e8e 100644
@@ -307,9 +307,10 @@ static void update_spte(u64 *sptep, u64 new_spte)
 {
 	u64 old_spte;
 
-	if (!shadow_accessed_mask || (new_spte & shadow_accessed_mask)) {
+	if (!shadow_accessed_mask || (new_spte & shadow_accessed_mask) ||
+	      !is_rmap_spte(*sptep))
 		__set_spte(sptep, new_spte);
-	} else {
+	else {
 		old_spte = __xchg_spte(sptep, new_spte);
 		if (old_spte & shadow_accessed_mask)
 			mark_page_accessed(pfn_to_page(spte_to_pfn(old_spte)));
Signature · 1 line
1.6.1.2
#4 lore raw

Re: [PATCH v2 3/6] KVM: MMU: fix page dirty tracking lost while sync page

In reply to Xiao Guangrong
Quoted 12 lines · Xiao Guangrong

In sync-page path, if spte.writable is changed, it will lose page dirty tracking, for example:

assume spte.writable = 0 in a unsync-page, when it's synced, it map spte to writable(that is spte.writable = 1), later guest write spte.gfn, it means spte.gfn is dirty, then guest changed this mapping to read-only, after it's synced, spte.writable = 0

So, when host release the spte, it detect spte.writable = 0 and not mark page dirty

Subtle, good catch.

arch/x86/kvm/mmu.c static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, · line 1986

Quoted 5 lines

set_pte:

arch/x86/kvm/mmu.c +2 -0
+	if (is_writable_pte(*sptep)&&  !is_writable_pte(spte))
+		kvm_set_pfn_dirty(pfn);
  	update_spte(sptep, spte);

I think this has to be done after the tlb flush, otherwise we have

set_pfn_dirty
(some other cpu) write out page, mark as clean (some other vcpu writes through stale tlb entry) update_spte
tlb flush

but perhaps mmu notifiers protect us here, if the cleaner wants to write out the page it has to clear the dirty bit in sptes as well, and that will block on mmu_lock.

Later on we can use the dirty bit instead of writeable bit, except on EPT. But let's start with your fix.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
author
#5 lore raw

Re: [PATCH v2 3/6] KVM: MMU: fix page dirty tracking lost while sync page

Quoted 3 lines · Avi Kivity

Later on we can use the dirty bit instead of writeable bit, except on EPT. But let's start with your fix.

I'm doing it, will post it... :-)

author
#6 lore raw

PATCH v2 5/6 KVM: MMU: cleanup spte set and accssed/dirty tracking

In reply to Xiao Guangrong

Introduce set_spte_track_bits() to cleanup current code

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)
arch/x86/kvm/mmu.c +8 -9
index 4123e8e..fb3ae54 100644
@@ -679,7 +679,7 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 	}
 }
 
-static void drop_spte(struct kvm *kvm, u64 *sptep, u64 new_spte)
+static void set_spte_track_bits(u64 *sptep, u64 new_spte)
 {
 	pfn_t pfn;
 	u64 old_spte;

@@ -692,6 +692,11 @@ static void drop_spte(struct kvm *kvm, u64 *sptep, u64 new_spte)
 		kvm_set_pfn_accessed(pfn);
 	if (is_writable_pte(old_spte))
 		kvm_set_pfn_dirty(pfn);
+}
+
+static void drop_spte(struct kvm *kvm, u64 *sptep, u64 new_spte)
+{
+	set_spte_track_bits(sptep, new_spte);
 	rmap_remove(kvm, sptep);
 }
 

@@ -791,7 +796,7 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 			     unsigned long data)
 {
 	int need_flush = 0;
-	u64 *spte, new_spte, old_spte;
+	u64 *spte, new_spte;
 	pte_t *ptep = (pte_t *)data;
 	pfn_t new_pfn;
 

@@ -812,13 +817,7 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 			new_spte &= ~PT_WRITABLE_MASK;
 			new_spte &= ~SPTE_HOST_WRITEABLE;
 			new_spte &= ~shadow_accessed_mask;
-			if (is_writable_pte(*spte))
-				kvm_set_pfn_dirty(spte_to_pfn(*spte));
-			old_spte = __xchg_spte(spte, new_spte);
-			if (is_shadow_present_pte(old_spte)
-			    && (!shadow_accessed_mask ||
-			    old_spte & shadow_accessed_mask))
-				mark_page_accessed(pfn_to_page(spte_to_pfn(old_spte)));
+			set_spte_track_bits(spte, new_spte);
 			spte = rmap_next(kvm, rmapp, spte);
 		}
 	}
Signature · 1 line
1.6.1.2
author
#7 lore raw

PATCH v2 6/6 KVM: MMU: using __xchg_spte more smarter

Sometimes, atomically set spte is not needed, this patch call __xchg_spte() more smartly

Note: if the old mapping's access bit is already set, we no need atomic operation since the access bit is not lost

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
arch/x86/kvm/mmu.c +7 -2
index fb3ae54..71e9268 100644
@@ -682,9 +682,14 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 static void set_spte_track_bits(u64 *sptep, u64 new_spte)
 {
 	pfn_t pfn;
-	u64 old_spte;
+	u64 old_spte = *sptep;
+
+	if (!shadow_accessed_mask || !is_shadow_present_pte(old_spte) ||
+	      old_spte & shadow_accessed_mask) {
+		__set_spte(sptep, new_spte);
+	} else
+		old_spte = __xchg_spte(sptep, new_spte);
 
-	old_spte = __xchg_spte(sptep, new_spte);
 	if (!is_rmap_spte(old_spte))
 		return;
 	pfn = spte_to_pfn(old_spte);
Signature · 1 line
1.6.1.2
#9 lore raw
Quoted 9 lines · Xiao Guangrong
Quoted 4 lines · Xiao Guangrong

In the speculative path, we should check guest pte's reserved bits just as the real processor does

Ping......?

Sorry. All looks good. I think no change is needed to patch 3, if we agree on this I'll apply it as is.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
author
#10 lore raw
Quoted 9 lines · Avi Kivity
Quoted 8 lines · Xiao Guangrong
Quoted 5 lines · Xiao Guangrong

In the speculative path, we should check guest pte's reserved bits just as
the real processor does

Quoted 5 lines
Quoted 2 lines

Ping......?

Sorry. All looks good. I think no change is needed to patch 3, if we agree on this I'll apply it as is.

Yeah, i agree, thanks, Avi.

#11 lore raw
Quoted 10 lines · Xiao Guangrong
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Xiao Guangrong
Quoted 5 lines · Xiao Guangrong

In the speculative path, we should check guest pte's reserved bits just as
the real processor does

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Ping......?

Sorry. All looks good. I think no change is needed to patch 3, if we agree on this I'll apply it as is.

Yeah, i agree, thanks, Avi.

Now applied.

Signature · 1 line
error compiling committee.c: too many arguments to function