KVM: MMU: fast page fault

Introduce a common function to abstract spte write-protect to cleanup the code

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   60 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 35 insertions(+), 25 deletions(-)

arch/x86/kvm/mmu.c +28 -4

index 4a3cc18..e70ff38 100644
@@ -1041,6 +1041,34 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+/* Return true if the spte is dropped. */
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool large,
+			       bool *flush)
+{
+	u64 spte = *sptep;
+
+	if (!is_writable_pte(spte))
+		return false;
+
+	*flush |= true;
+
+	if (large) {
+		pgprintk("rmap_write_protect(large): spte %p %llx\n",
+			 spte, *spte);
+		BUG_ON(!is_large_pte(spte));
+
+		drop_spte(kvm, sptep);
+		--kvm->stat.lpages;
+		return true;
+	}
+
+	rmap_printk("rmap_write_protect: spte %p %llx\n", spte, *spte);
+	spte = spte & ~PT_WRITABLE_MASK;
+	mmu_spte_update(sptep, spte);
+
+	return false;
+}
+
 static bool
 __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 {

@@ -1050,24 +1078,13 @@ __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
-		rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
-
-		if (!is_writable_pte(*sptep)) {
-			sptep = rmap_get_next(&iter);

It is preferable to remove all large sptes including read-only ones, the current behaviour, then to verify that no read->write transition can occur in fault paths (fault paths which are increasing in number).

Quoted 4 lines · On Fri, 20 Apr 2012 18:33:19 -0300 Marcelo Tosatti <[email protected]> wrote:

It is preferable to remove all large sptes including read-only ones, the current behaviour, then to verify that no read->write transition can occur in fault paths (fault paths which are increasing in number).

I think we should use separate function than spte_write_protect() for the large case.

Quoted 15 lines

static bool
__rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level) {

arch/x86/kvm/mmu.c +0 -8

@@ -1050,24 +1078,13 @@ __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
-		rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
-
-		if (!is_writable_pte(*sptep)) {
-			sptep = rmap_get_next(&iter);
-			continue;
-		}
-
-		if (level == PT_PAGE_TABLE_LEVEL) {

arch/x86/kvm/mmu.c __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level) · line 1066

Quoted 6 lines

arch/x86/kvm/mmu.c +0 -6

-			mmu_spte_update(sptep, *sptep & ~PT_WRITABLE_MASK);
-			sptep = rmap_get_next(&iter);
-		} else {
-			BUG_ON(!is_large_pte(*sptep));
-			drop_spte(kvm, sptep);
-			--kvm->stat.lpages;

It is preferable to remove all large sptes including read-only ones, the

current behaviour, then to verify that no read->write transition can occur in fault paths (fault paths which are increasing in number).

Quoted 15 lines · Marcelo Tosatti

Quoted 15 lines

static bool
__rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level) {

arch/x86/kvm/mmu.c +0 -8

@@ -1050,24 +1078,13 @@ __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
-		rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
-
-		if (!is_writable_pte(*sptep)) {
-			sptep = rmap_get_next(&iter);
-			continue;
-		}
-
-		if (level == PT_PAGE_TABLE_LEVEL) {

Quoted 8 lines

arch/x86/kvm/mmu.c __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level) · line 1066

Quoted 6 lines

arch/x86/kvm/mmu.c +0 -6

-			mmu_spte_update(sptep, *sptep & ~PT_WRITABLE_MASK);
-			sptep = rmap_get_next(&iter);
-		} else {
-			BUG_ON(!is_large_pte(*sptep));
-			drop_spte(kvm, sptep);
-			--kvm->stat.lpages;

It is preferable to remove all large sptes including read-only ones, the

It can cause page faults even if read memory on these large sptse.

Actually, Avi suggested that make large writable spte to be readonly (not dropped) on this path.

Quoted 2 lines

current behaviour, then to verify that no read->write transition can occur in fault paths (fault paths which are increasing in number).

Yes, the small spte also has issue (find a write-protected spte in fault paths). Later, the second part of this patchset will introduce rmap.WRITE_PROTECTED bit, then we can do the fast check before calling fast page fault.

Quoted 28 lines · Xiao Guangrong

Quoted 23 lines · Marcelo Tosatti

Quoted 11 lines

static bool
__rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level) {

arch/x86/kvm/mmu.c +0 -4

@@ -1050,24 +1078,13 @@ __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
-		rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
-
-		if (!is_writable_pte(*sptep)) {
-			sptep = rmap_get_next(&iter);

arch/x86/kvm/mmu.c __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level) · line 1066

Quoted 10 lines

arch/x86/kvm/mmu.c +0 -10

-			continue;
-		}
-
-		if (level == PT_PAGE_TABLE_LEVEL) {
-			mmu_spte_update(sptep, *sptep & ~PT_WRITABLE_MASK);
-			sptep = rmap_get_next(&iter);
-		} else {
-			BUG_ON(!is_large_pte(*sptep));
-			drop_spte(kvm, sptep);
-			--kvm->stat.lpages;

It is preferable to remove all large sptes including read-only ones, the

Quoted 4 lines

It can cause page faults even if read memory on these large sptse.

Actually, Avi suggested that make large writable spte to be readonly (not dropped) on this path.

See commits e49146dce8c3dc6f4485c1904b6587855f393e71, 38187c830cab84daecb41169948467f1f19317e3 for issues with large read-only sptes.

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu.c +2 -1

index e70ff38..dd984b6 100644
@@ -145,7 +145,8 @@ module_param(dbg, bool, 0644);
 #define CREATE_TRACE_POINTS
 #include "mmutrace.h"
 
-#define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

Quoted 4 lines

arch/x86/kvm/mmu.c +0 -0

@@ -1177,9 +1178,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 			new_spte = *sptep & ~PT64_BASE_ADDR_MASK;
 			new_spte |= (u64)new_pfn << PAGE_SHIFT;
 

arch/x86/kvm/mmu.c static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp, · line 1182

Quoted 5 lines

arch/x86/kvm/mmu.c +2 -3

-			new_spte &= ~PT_WRITABLE_MASK;
-			new_spte &= ~SPTE_HOST_WRITEABLE;
-			new_spte &= ~shadow_accessed_mask;
+			new_spte &= ~(PT_WRITABLE_MASK | SPTE_HOST_WRITEABLE |
+				      shadow_accessed_mask | SPTE_ALLOW_WRITE);

Each bit should have a distinct meaning. Here the host pte is being write-protected, which means only the SPTE_HOST_WRITEABLE bit should be cleared.

Quoted 4 lines · Marcelo Tosatti

Quoted 4 lines

arch/x86/kvm/mmu.c +0 -0

@@ -1177,9 +1178,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 			new_spte = *sptep & ~PT64_BASE_ADDR_MASK;
 			new_spte |= (u64)new_pfn << PAGE_SHIFT;
 

Quoted 9 lines

arch/x86/kvm/mmu.c static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp, · line 1182

Quoted 5 lines

arch/x86/kvm/mmu.c +2 -3

-			new_spte &= ~PT_WRITABLE_MASK;
-			new_spte &= ~SPTE_HOST_WRITEABLE;
-			new_spte &= ~shadow_accessed_mask;
+			new_spte &= ~(PT_WRITABLE_MASK | SPTE_HOST_WRITEABLE |
+				      shadow_accessed_mask | SPTE_ALLOW_WRITE);

Each bit should have a distinct meaning. Here the host pte is being write-protected, which means only the SPTE_HOST_WRITEABLE bit should be cleared.

Hmm, it is no problem if SPTE_ALLOW_WRITE is not cleared.

But the meaning of SPTE_ALLOW_WRITE will become strange: we will see a spte with spte.SPTE_ALLOW_WRITE = 1 (means the spte is writable on host and guest) and spte.SPTE_HOST_WRITEABLE = 0 (means the spte is read-only on host).

Quoted 18 lines · Xiao Guangrong

Quoted 4 lines · Marcelo Tosatti

Quoted 4 lines

arch/x86/kvm/mmu.c +0 -0

@@ -1177,9 +1178,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 			new_spte = *sptep & ~PT64_BASE_ADDR_MASK;
 			new_spte |= (u64)new_pfn << PAGE_SHIFT;
 

Quoted 9 lines

arch/x86/kvm/mmu.c static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp, · line 1182

Quoted 5 lines

arch/x86/kvm/mmu.c +2 -3

-			new_spte &= ~PT_WRITABLE_MASK;
-			new_spte &= ~SPTE_HOST_WRITEABLE;
-			new_spte &= ~shadow_accessed_mask;
+			new_spte &= ~(PT_WRITABLE_MASK | SPTE_HOST_WRITEABLE |
+				      shadow_accessed_mask | SPTE_ALLOW_WRITE);

Each bit should have a distinct meaning. Here the host pte is being write-protected, which means only the SPTE_HOST_WRITEABLE bit should be cleared.

Quoted 6 lines

Hmm, it is no problem if SPTE_ALLOW_WRITE is not cleared.

But the meaning of SPTE_ALLOW_WRITE will become strange: we will see a spte with spte.SPTE_ALLOW_WRITE = 1 (means the spte is writable on host and guest) and spte.SPTE_HOST_WRITEABLE = 0 (means the spte is read-only on host).

You are combining gpte writable bit, and host pte writable bit (which are separate and independent of each other) into one bit.

SPTE_HOST_WRITEABLE already indicates whether the host pte is writable or not.

Quoted 18 lines · Xiao Guangrong

Quoted 4 lines · Marcelo Tosatti

Quoted 4 lines

arch/x86/kvm/mmu.c +0 -0

@@ -1177,9 +1178,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 			new_spte = *sptep & ~PT64_BASE_ADDR_MASK;
 			new_spte |= (u64)new_pfn << PAGE_SHIFT;
 

Quoted 9 lines

arch/x86/kvm/mmu.c static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp, · line 1182

Quoted 5 lines

arch/x86/kvm/mmu.c +2 -3

-			new_spte &= ~PT_WRITABLE_MASK;
-			new_spte &= ~SPTE_HOST_WRITEABLE;
-			new_spte &= ~shadow_accessed_mask;
+			new_spte &= ~(PT_WRITABLE_MASK | SPTE_HOST_WRITEABLE |
+				      shadow_accessed_mask | SPTE_ALLOW_WRITE);

Each bit should have a distinct meaning. Here the host pte is being write-protected, which means only the SPTE_HOST_WRITEABLE bit should be cleared.

Quoted 6 lines

Hmm, it is no problem if SPTE_ALLOW_WRITE is not cleared.

But the meaning of SPTE_ALLOW_WRITE will become strange: we will see a spte with spte.SPTE_ALLOW_WRITE = 1 (means the spte is writable on host and guest) and spte.SPTE_HOST_WRITEABLE = 0 (means the spte is read-only on host).

You are combining gpte writable bit, and host pte writable bit (which are separate and independent of each other) into one bit.

SPTE_HOST_WRITEABLE already indicates whether the host pte is writable or not.

Quoted 29 lines · Marcelo Tosatti

Quoted 18 lines · Xiao Guangrong

Quoted 4 lines · Marcelo Tosatti

Quoted 4 lines

arch/x86/kvm/mmu.c +0 -0

@@ -1177,9 +1178,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
 			new_spte = *sptep & ~PT64_BASE_ADDR_MASK;
 			new_spte |= (u64)new_pfn << PAGE_SHIFT;
 

Quoted 9 lines

arch/x86/kvm/mmu.c static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp, · line 1182

Quoted 5 lines

arch/x86/kvm/mmu.c +2 -3

-			new_spte &= ~PT_WRITABLE_MASK;
-			new_spte &= ~SPTE_HOST_WRITEABLE;
-			new_spte &= ~shadow_accessed_mask;
+			new_spte &= ~(PT_WRITABLE_MASK | SPTE_HOST_WRITEABLE |
+				      shadow_accessed_mask | SPTE_ALLOW_WRITE);

Each bit should have a distinct meaning. Here the host pte is being write-protected, which means only the SPTE_HOST_WRITEABLE bit should be cleared.

Quoted 6 lines

Hmm, it is no problem if SPTE_ALLOW_WRITE is not cleared.

But the meaning of SPTE_ALLOW_WRITE will become strange: we will see a spte with spte.SPTE_ALLOW_WRITE = 1 (means the spte is writable on host and guest) and spte.SPTE_HOST_WRITEABLE = 0 (means the spte is read-only on host).

You are combining gpte writable bit, and host pte writable bit (which are separate and independent of each other) into one bit.

Quoted 2 lines

SPTE_HOST_WRITEABLE already indicates whether the host pte is writable or not.

Maybe we should rename SPTE_ALLOW_WRITE to SPTE_NOT_SHADOWED (or SPTE_SHADOWED with the opposite meaning).

Alternatively, SPTE_MMU_WRITEABLE (complements SPTE_HOST_WRITEABLE).

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

BTW,

"introduce SPTE_ALLOW_WRITE bit

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

As soon as guest writes to gpte, information in bit is outdated.

Quoted 42 lines · Marcelo Tosatti

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

BTW,

"introduce SPTE_ALLOW_WRITE bit

Quoted 8 lines

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

As soon as guest writes to gpte, information in bit is outdated.

Ok, i found one example where mmu_lock was expecting sptes not to change:

VCPU0 VCPU1

- read-only gpte
- read-only spte
- write fault
- spte = *sptep
guest write to gpte, set writable bit spte writable
parent page unsync
guest write to gpte writable bit clear guest invlpg updates spte to RO sync_page
enter set_spte from sync_page
- cmpxchg(spte) is now writable
[window where another vcpu can
cache spte with writable bit
set]

				if (is_writable_pte(entry) && !is_writable_pte(*sptep))
					kvm_flush_remote_tlbs(vcpu->kvm);

The flush is not executed because spte was read-only (which is a correct assumption as long as sptes updates are protected by mmu_lock).

So this is an example of implicit assumptions which break if you update spte without mmu_lock. Certainly there are more cases. :(

More importantly than the particular flush TLB case, the point is every piece of code that reads and writes sptes must now be aware that mmu_lock alone does not guarantee stability. Everything must be audited.

Where the bulk of the improvement comes from again? If there is little or no mmu_lock contention (which we have no consistent data to be honest in your testcase) is the bouncing off mmu_lock's cacheline that hurts?

Quoted 2 lines

So this is an example of implicit assumptions which break if you update spte without mmu_lock. Certainly there are more cases. :(

OK, i now see you mentioned a similar case in the document, for rmap_write_protect.

More importantly than the particular flush TLB case, the point is every piece of code that reads and writes sptes must now be aware that mmu_lock alone does not guarantee stability. Everything must be audited.

Quoted 42 lines · Marcelo Tosatti

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

BTW,

"introduce SPTE_ALLOW_WRITE bit

Quoted 8 lines

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

As soon as guest writes to gpte, information in bit is outdated.

Ok, i found one example where mmu_lock was expecting sptes not to change:

VCPU0 VCPU1

- read-only gpte
- read-only spte
- write fault

- spte = *sptep
guest write to gpte, set writable bit spte writable
parent page unsync
guest write to gpte writable bit clear guest invlpg updates spte to RO sync_page
enter set_spte from sync_page
- cmpxchg(spte) is now writable
[window where another vcpu can
cache spte with writable bit
set]

				if (is_writable_pte(entry) && !is_writable_pte(*sptep))
					kvm_flush_remote_tlbs(vcpu->kvm);

The flush is not executed because spte was read-only (which is a correct assumption as long as sptes updates are protected by mmu_lock).

So this is an example of implicit assumptions which break if you update spte without mmu_lock. Certainly there are more cases. :(

Quoted 56 lines · Marcelo Tosatti

Quoted 42 lines · Marcelo Tosatti

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

BTW,

"introduce SPTE_ALLOW_WRITE bit

Quoted 8 lines

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

As soon as guest writes to gpte, information in bit is outdated.

Ok, i found one example where mmu_lock was expecting sptes not to change:

Quoted 5 lines

VCPU0 VCPU1

- read-only gpte
- read-only spte
- write fault

It is not true, gpte is read-only, and it is a write fault, then we should reject the page fault to guest, the fast page fault is not called. :)

Quoted 13 lines

- spte = *sptep
guest write to gpte, set writable bit spte writable
parent page unsync
guest write to gpte writable bit clear guest invlpg updates spte to RO sync_page
enter set_spte from sync_page
- cmpxchg(spte) is now writable
[window where another vcpu can
cache spte with writable bit
set]

Quoted 6 lines

				if (is_writable_pte(entry) && !is_writable_pte(*sptep))
					kvm_flush_remote_tlbs(vcpu->kvm);

The flush is not executed because spte was read-only (which is a correct assumption as long as sptes updates are protected by mmu_lock).

It is also not true, flush tlbs in set_sptes is used to ensure rmap_write_protect work correctly, but rmap_write_protect will flush tlbs even if the spte can be changed by fast page fault.

Quoted 2 lines

So this is an example of implicit assumptions which break if you update spte without mmu_lock. Certainly there are more cases. :(

We only need care the path which is depends on spte.WRITEABLE == 0, since only these spte has chance to be changed out of mmu-lock.

The most trouble is in rmap_write_protect that need flush tlb to protect shadow page table.

I think it is not too hard to check. :)

Quoted 1 line

I think it is not too hard to check. :)

You are minimizing the possible impact these modifications have.

Perhaps you should prepare code under mmu_lock to handle concurrent spte R->W updates first, and then later introduce the concurrent updates. In a way that its clear for somebody reading the code that parallel updates can happen (say read spte once, work on local copy, later re-read spte).

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

BTW,

"introduce SPTE_ALLOW_WRITE bit

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

As soon as guest writes to gpte, information in bit is outdated.

Quoted 34 lines · Marcelo Tosatti

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

Quoted 3 lines

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

It can not happen, rmap_write_protect will set SPTE_WRITE_PROTECT even if the spte is not WRITABLE, please see:

arch/x86/kvm/mmu.c +4 -0

+	if (page_table_protect && spte_wp_by_dirty_log(spte))
+		goto reset_spte;
+
+	return false;

Quoted 8 lines

BTW,

"introduce SPTE_ALLOW_WRITE bit

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Quoted 3 lines

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

Above explanation can ensure the meaning of this bit is accurate? Or it has another case? :)

Quoted 2 lines

As soon as guest writes to gpte, information in bit is outdated.

The bit will be updated when spte is updated.

When the guest write gpte, the spte is not updated immediately, yes, the bit is outdated at that time, but it is ok since tlb is not flushed.

From Intel manual:

After tlb flush, the bit can be coincident with gpte.

Quoted 53 lines · Xiao Guangrong

Quoted 34 lines · Marcelo Tosatti

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

Quoted 3 lines

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

It can not happen, rmap_write_protect will set SPTE_WRITE_PROTECT even if the spte is not WRITABLE, please see:

arch/x86/kvm/mmu.c +7 -0

+	if (page_table_protect && spte_wp_by_dirty_log(spte))
+		goto reset_spte;
+
+	return false;
+
+reset_spte:
 	rmap_printk("rmap_write_protect: spte %p %llx\n", spte, *spte);
 	spte = spte & ~PT_WRITABLE_MASK;
+	if (page_table_protect)

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1087

Quoted 5 lines

arch/x86/kvm/mmu.c +1 -1

+		spte |= SPTE_WRITE_PROTECT;
 	mmu_spte_update(sptep, spte);
-
 	return false;
 }

Right. What about sync path, fault path, prefault path, do they update SPTE_WRITE_PROTECT properly?

Quoted 8 lines

Quoted 8 lines

BTW,

"introduce SPTE_ALLOW_WRITE bit

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Quoted 6 lines

Quoted 3 lines

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

Above explanation can ensure the meaning of this bit is accurate? Or it has another case? :)

No, it is out of sync with guest pte.

Quoted 9 lines

Quoted 2 lines

As soon as guest writes to gpte, information in bit is outdated.

The bit will be updated when spte is updated.

When the guest write gpte, the spte is not updated immediately, yes, the bit is outdated at that time, but it is ok since tlb is not flushed.

Page faults cause TLB flushes.

Quoted 1 line

From Intel manual:

"In addition to the instructions identified above, page faults invalidate entries in the TLBs and paging-structure caches. In particular, a page-fault exception resulting from an attempt to use a linear address will invalidate any TLB entries that are for a page number corresponding to that linear address and that are associated with the current PCID."

Quoted 1 line

After tlb flush, the bit can be coincident with gpte.

You must read the gpte before updating from ro->rw, unless you write protect gpte. IIRC you were doing that in previous patches?

Quoted 54 lines · Marcelo Tosatti

Quoted 53 lines · Xiao Guangrong

Quoted 34 lines · Marcelo Tosatti

Quoted 21 lines · Xiao Guangrong

If this bit is set, it means the W bit of the spte is cleared due to shadow page table protection

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   56 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

arch/x86/kvm/mmu.c +1 -0

index dd984b6..eb02fc4 100644
@@ -147,6 +147,7 @@ module_param(dbg, bool, 0644);
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_ALLOW_WRITE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_WRITE_PROTECT	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 2))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1051

Quoted 9 lines

arch/x86/kvm/mmu.c +6 -0

@@ -1042,36 +1043,51 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_wp_by_dirty_log(u64 spte)
+{
+	WARN_ON(is_writable_pte(spte));
+
+	return (spte & SPTE_ALLOW_WRITE) && !(spte & SPTE_WRITE_PROTECT);
+}

Is the information accurate? Say:

Quoted 3 lines

- dirty log write protect, set SPTE_ALLOW_WRITE, clear WRITABLE.
- shadow gfn, rmap_write_protect finds page not WRITABLE.
- spte points to shadow gfn, but SPTE_WRITE_PROTECT is not set.

It can not happen, rmap_write_protect will set SPTE_WRITE_PROTECT even if the spte is not WRITABLE, please see:

arch/x86/kvm/mmu.c +7 -0

+	if (page_table_protect && spte_wp_by_dirty_log(spte))
+		goto reset_spte;
+
+	return false;
+
+reset_spte:
 	rmap_printk("rmap_write_protect: spte %p %llx\n", spte, *spte);
 	spte = spte & ~PT_WRITABLE_MASK;
+	if (page_table_protect)

Quoted 9 lines

arch/x86/kvm/mmu.c static void drop_spte(struct kvm *kvm, u64 *sptep) · line 1087

Quoted 5 lines

arch/x86/kvm/mmu.c +1 -1

+		spte |= SPTE_WRITE_PROTECT;
 	mmu_spte_update(sptep, spte);
-
 	return false;
 }

Right. What about sync path, fault path, prefault path, do they update SPTE_WRITE_PROTECT properly?

All of these case can call set_spte() to update the spte,

arch/x86/kvm/mmu.c +1 -0

@@ -2346,6 +2363,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			ret = 1;
 			pte_access &= ~ACC_WRITE_MASK;
 			spte &= ~PT_WRITABLE_MASK;
+			spte |= SPTE_WRITE_PROTECT;
 

SPTE_WRITE_PROTECT bit is set if the page is write-protected.

Quoted 29 lines

Quoted 8 lines

Quoted 8 lines

BTW,

"introduce SPTE_ALLOW_WRITE bit

This bit indicates whether the spte is allow to be writable that means the gpte of this spte is writable and the pfn pointed by this spte is writable on host"

Quoted 6 lines

Quoted 3 lines

Other than the fact that each bit should have one meaning, how can this bit be accurate without write protection of the gpte?

Above explanation can ensure the meaning of this bit is accurate? Or it has another case? :)

No, it is out of sync with guest pte.

Quoted 9 lines

Quoted 2 lines

As soon as guest writes to gpte, information in bit is outdated.

The bit will be updated when spte is updated.

When the guest write gpte, the spte is not updated immediately, yes, the bit is outdated at that time, but it is ok since tlb is not flushed.

Page faults cause TLB flushes.

Quoted 9 lines

Quoted 1 line

From Intel manual:

"In addition to the instructions identified above, page faults invalidate entries in the TLBs and paging-structure caches. In particular, a page-fault exception resulting from an attempt to use a linear address will invalidate any TLB entries that are for a page number corresponding to that linear address and that are associated with the current PCID."

Yes, the fault tlb entries is removed _after_ page fault. On page fault path, the spte is correctly updated (clear SPTE_ALLOW_WRITABLE bit), and the fast page fault path is fail to update the spte (Since SPTE_ALLOW_WRITABLE is not set or cmpxchg is fail.)

There is windows that is between guest write and shadow page page fault, it this windows, fast page fault can make the spte to be writable, it is ok, since the guest write instruction is not completed.

Quoted 5 lines

Quoted 1 line

After tlb flush, the bit can be coincident with gpte.

You must read the gpte before updating from ro->rw, unless you write protect gpte. IIRC you were doing that in previous patches?

Not need. Please see the below sequence:

gpte.W = 1
spte is the shadow page entry of gpte.
spte.W = 0

VCPU 0 VCPU 1
guest write gpte.W = 0
guest write memory through gpte fast page fault:
cmpxchg spte + W

SPTE.SPTE_ALLOW_WRITE = 0 when
host emulate the write or sync shadow pages
(spte is zapped or read-only)

flush_tlb

return to guest
the guest write operation is completed.

It does not break anything.

Marcelo, i guess you missed "gpte to be written" and "access through gpte", yes? A write operation changes the page which the pte points to, not change the pte.

Changlog:

- split the long series for better review. This is the core implementation of fast page fault.
- document fast page fault in locking.txt