KVM: MMU: lazily drop large spte

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

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

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

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

Can you measure an improvement with this change?

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

So i wonder why is this part from your patch

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

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

Quoted 35 lines · Marcelo Tosatti

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 8 lines

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

This case is removed now, the code when e49146dce8c3dc6f44 was applied is:

|
|                pt = sp->spt;
|                for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
|                        /* avoid RMW */
|                        if (is_writable_pte(pt[i]))
|                                update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
|        }

The real problem in this code is it would write-protect the spte even if it is not a last spte that caused the middle-level shadow page table was write-protected. So e49146dce8c3dc6f44 added this code:

|                if (sp->role.level != PT_PAGE_TABLE_LEVEL)
|                        continue;
|

was good to fix this problem.

Now, the current code is:

|		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
|			if (!is_shadow_present_pte(pt[i]) ||
|			      !is_last_spte(pt[i], sp->role.level))
|				continue;
|
|			spte_write_protect(kvm, &pt[i], &flush, false);
|		}

It only write-protect the last spte. So, it allows large spte existent. (the large spte can be broken by drop_large_spte() on the page-fault path.)

Quoted 10 lines

So i wonder why is this part from your patch

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

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

This is safe, we change these code to:

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

-		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		if ((level > PT_PAGE_TABLE_LEVEL &&
+		   has_wrprotected_page(vcpu->kvm, gfn, level)) ||
+		      mmu_need_write_protect(vcpu, gfn, can_unsync)) {
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;

The spte become read-only which can ensure the shadow gfn can not be changed.

Btw, the origin code allows to create readonly spte under this case if !(pte_access & WRITEABBLE)

Quoted 94 lines · Xiao Guangrong

Quoted 35 lines · Marcelo Tosatti

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 8 lines

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

This case is removed now, the code when e49146dce8c3dc6f44 was applied is:

|
|                pt = sp->spt;
|                for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
|                        /* avoid RMW */
|                        if (is_writable_pte(pt[i]))
|                                update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
|        }

The real problem in this code is it would write-protect the spte even if it is not a last spte that caused the middle-level shadow page table was write-protected. So e49146dce8c3dc6f44 added this code:

|                if (sp->role.level != PT_PAGE_TABLE_LEVEL)
|                        continue;
|

was good to fix this problem.

Now, the current code is:

|		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
|			if (!is_shadow_present_pte(pt[i]) ||
|			      !is_last_spte(pt[i], sp->role.level))
|				continue;
|
|			spte_write_protect(kvm, &pt[i], &flush, false);
|		}

It only write-protect the last spte. So, it allows large spte existent. (the large spte can be broken by drop_large_spte() on the page-fault path.)

Quoted 10 lines

So i wonder why is this part from your patch

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

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

This is safe, we change these code to:

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

-		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		if ((level > PT_PAGE_TABLE_LEVEL &&
+		   has_wrprotected_page(vcpu->kvm, gfn, level)) ||
+		      mmu_need_write_protect(vcpu, gfn, can_unsync)) {
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;

Quoted 3 lines

The spte become read-only which can ensure the shadow gfn can not be changed.

Btw, the origin code allows to create readonly spte under this case if !(pte_access & WRITEABBLE)

Regarding shadow: it should be fine as long as fault path always deletes large mappings, when shadowed pages are present in the region.

Ah, unshadowing from reexecute_instruction does not handle large pages. I suppose that is what "simplification" refers to.

Quoted 95 lines · Marcelo Tosatti

Quoted 94 lines · Xiao Guangrong

Quoted 35 lines · Marcelo Tosatti

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 8 lines

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

This case is removed now, the code when e49146dce8c3dc6f44 was applied is:

|
|                pt = sp->spt;
|                for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
|                        /* avoid RMW */
|                        if (is_writable_pte(pt[i]))
|                                update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
|        }

The real problem in this code is it would write-protect the spte even if it is not a last spte that caused the middle-level shadow page table was write-protected. So e49146dce8c3dc6f44 added this code:

|                if (sp->role.level != PT_PAGE_TABLE_LEVEL)
|                        continue;
|

was good to fix this problem.

Now, the current code is:

|		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
|			if (!is_shadow_present_pte(pt[i]) ||
|			      !is_last_spte(pt[i], sp->role.level))
|				continue;
|
|			spte_write_protect(kvm, &pt[i], &flush, false);
|		}

It only write-protect the last spte. So, it allows large spte existent. (the large spte can be broken by drop_large_spte() on the page-fault path.)

Quoted 10 lines

So i wonder why is this part from your patch

(quoted diff) +0 -6

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

This is safe, we change these code to:

(quoted diff) +3 -1

-		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		if ((level > PT_PAGE_TABLE_LEVEL &&
+		   has_wrprotected_page(vcpu->kvm, gfn, level)) ||
+		      mmu_need_write_protect(vcpu, gfn, can_unsync)) {
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;

Quoted 6 lines

Quoted 3 lines

The spte become read-only which can ensure the shadow gfn can not be changed.

Btw, the origin code allows to create readonly spte under this case if !(pte_access & WRITEABBLE)

Regarding shadow: it should be fine as long as fault path always deletes large mappings, when shadowed pages are present in the region.

For hard mmu is also safe, in this patch i added these code:

(quoted diff) +2 -0

@@ -2635,6 +2617,8 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			break;
 		}
 
+		drop_large_spte(vcpu, iterator.sptep);
+
 

It can delete large mappings like soft mmu does.

Anything i missed?

Quoted 1 line

Quoted 3 lines

Ah, unshadowing from reexecute_instruction does not handle large pages. I suppose that is what "simplification" refers to.

reexecute_instruction did not directly handle last spte, it just removes all shadow pages, then let cpu retry the instruction, the page can become writable when encounter #PF again, large spte is fine under this case.

(Out of this thread: I notice reexecute_instruction allows to retry
instruct only if tdp_enabled == 0, but on nested npt, it also has page write-protected by shadow pages. Maybe we need to improve this restriction.
)

Quoted 117 lines · Xiao Guangrong

Quoted 95 lines · Marcelo Tosatti

Quoted 94 lines · Xiao Guangrong

Quoted 35 lines · Marcelo Tosatti

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 8 lines

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

This case is removed now, the code when e49146dce8c3dc6f44 was applied is:

|
|                pt = sp->spt;
|                for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
|                        /* avoid RMW */
|                        if (is_writable_pte(pt[i]))
|                                update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
|        }

The real problem in this code is it would write-protect the spte even if it is not a last spte that caused the middle-level shadow page table was write-protected. So e49146dce8c3dc6f44 added this code:

|                if (sp->role.level != PT_PAGE_TABLE_LEVEL)
|                        continue;
|

was good to fix this problem.

Now, the current code is:

|		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
|			if (!is_shadow_present_pte(pt[i]) ||
|			      !is_last_spte(pt[i], sp->role.level))
|				continue;
|
|			spte_write_protect(kvm, &pt[i], &flush, false);
|		}

It only write-protect the last spte. So, it allows large spte existent. (the large spte can be broken by drop_large_spte() on the page-fault path.)

Quoted 10 lines

So i wonder why is this part from your patch

(quoted diff) +0 -6

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

This is safe, we change these code to:

(quoted diff) +3 -1

-		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		if ((level > PT_PAGE_TABLE_LEVEL &&
+		   has_wrprotected_page(vcpu->kvm, gfn, level)) ||
+		      mmu_need_write_protect(vcpu, gfn, can_unsync)) {
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;

Quoted 6 lines

Quoted 3 lines

The spte become read-only which can ensure the shadow gfn can not be changed.

Btw, the origin code allows to create readonly spte under this case if !(pte_access & WRITEABBLE)

Regarding shadow: it should be fine as long as fault path always deletes large mappings, when shadowed pages are present in the region.

For hard mmu is also safe, in this patch i added these code:

(quoted diff) +2 -0

@@ -2635,6 +2617,8 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			break;
 		}
 
+		drop_large_spte(vcpu, iterator.sptep);
+
 

It can delete large mappings like soft mmu does.

Anything i missed?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Ah, unshadowing from reexecute_instruction does not handle large pages. I suppose that is what "simplification" refers to.

reexecute_instruction did not directly handle last spte, it just removes all shadow pages, then let cpu retry the instruction, the page can become writable when encounter #PF again, large spte is fine under this case.

While searching for a given "gpa", you don't find large gfn which is mapping it, right? (that is, searching for gfn 4 fails to find large read-only "gfn 0"). Unshadowing gfn 4 will keep large read-only mapping present.

1. large read-write spte to gfn 0
2. shadow gfn 4
3. write-protect large spte pointing to gfn 0
4. write to gfn 4
5. instruction emulation fails
5. unshadow gfn 4
6. refault, do not drop large spte because no pages shadowed

Quoted 132 lines · Marcelo Tosatti

Quoted 117 lines · Xiao Guangrong

Quoted 95 lines · Marcelo Tosatti

Quoted 94 lines · Xiao Guangrong

Quoted 35 lines · Marcelo Tosatti

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 8 lines

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

This case is removed now, the code when e49146dce8c3dc6f44 was applied is:

|
|                pt = sp->spt;
|                for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
|                        /* avoid RMW */
|                        if (is_writable_pte(pt[i]))
|                                update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
|        }

The real problem in this code is it would write-protect the spte even if it is not a last spte that caused the middle-level shadow page table was write-protected. So e49146dce8c3dc6f44 added this code:

|                if (sp->role.level != PT_PAGE_TABLE_LEVEL)
|                        continue;
|

was good to fix this problem.

Now, the current code is:

|		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
|			if (!is_shadow_present_pte(pt[i]) ||
|			      !is_last_spte(pt[i], sp->role.level))
|				continue;
|
|			spte_write_protect(kvm, &pt[i], &flush, false);
|		}

It only write-protect the last spte. So, it allows large spte existent. (the large spte can be broken by drop_large_spte() on the page-fault path.)

Quoted 10 lines

So i wonder why is this part from your patch

(quoted diff) +0 -6

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

This is safe, we change these code to:

(quoted diff) +3 -1

-		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		if ((level > PT_PAGE_TABLE_LEVEL &&
+		   has_wrprotected_page(vcpu->kvm, gfn, level)) ||
+		      mmu_need_write_protect(vcpu, gfn, can_unsync)) {
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;

Quoted 6 lines

Quoted 3 lines

The spte become read-only which can ensure the shadow gfn can not be changed.

Btw, the origin code allows to create readonly spte under this case if !(pte_access & WRITEABBLE)

Regarding shadow: it should be fine as long as fault path always deletes large mappings, when shadowed pages are present in the region.

For hard mmu is also safe, in this patch i added these code:

(quoted diff) +2 -0

@@ -2635,6 +2617,8 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			break;
 		}
 
+		drop_large_spte(vcpu, iterator.sptep);
+
 

It can delete large mappings like soft mmu does.

Anything i missed?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Ah, unshadowing from reexecute_instruction does not handle large pages. I suppose that is what "simplification" refers to.

reexecute_instruction did not directly handle last spte, it just removes all shadow pages, then let cpu retry the instruction, the page can become writable when encounter #PF again, large spte is fine under this case.

While searching for a given "gpa", you don't find large gfn which is mapping it, right? (that is, searching for gfn 4 fails to find large read-only "gfn 0"). Unshadowing gfn 4 will keep large read-only mapping present.

Quoted 7 lines

1. large read-write spte to gfn 0
2. shadow gfn 4
3. write-protect large spte pointing to gfn 0
4. write to gfn 4
5. instruction emulation fails
5. unshadow gfn 4
6. refault, do not drop large spte because no pages shadowed

Hmm, it is not true. :)

The large spte can become writable since 'no pages adhadoes' (that means has_wrprotected_page() can return 0 for this case). No?

Quoted 133 lines · Xiao Guangrong

Quoted 132 lines · Marcelo Tosatti

Quoted 117 lines · Xiao Guangrong

Quoted 95 lines · Marcelo Tosatti

Quoted 94 lines · Xiao Guangrong

Quoted 35 lines · Marcelo Tosatti

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 8 lines

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

This case is removed now, the code when e49146dce8c3dc6f44 was applied is:

|
|                pt = sp->spt;
|                for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
|                        /* avoid RMW */
|                        if (is_writable_pte(pt[i]))
|                                update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
|        }

The real problem in this code is it would write-protect the spte even if it is not a last spte that caused the middle-level shadow page table was write-protected. So e49146dce8c3dc6f44 added this code:

|                if (sp->role.level != PT_PAGE_TABLE_LEVEL)
|                        continue;
|

was good to fix this problem.

Now, the current code is:

|		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
|			if (!is_shadow_present_pte(pt[i]) ||
|			      !is_last_spte(pt[i], sp->role.level))
|				continue;
|
|			spte_write_protect(kvm, &pt[i], &flush, false);
|		}

It only write-protect the last spte. So, it allows large spte existent. (the large spte can be broken by drop_large_spte() on the page-fault path.)

Quoted 10 lines

So i wonder why is this part from your patch

(quoted diff) +0 -6

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

This is safe, we change these code to:

(quoted diff) +3 -1

-		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		if ((level > PT_PAGE_TABLE_LEVEL &&
+		   has_wrprotected_page(vcpu->kvm, gfn, level)) ||
+		      mmu_need_write_protect(vcpu, gfn, can_unsync)) {
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;

Quoted 6 lines

Quoted 3 lines

The spte become read-only which can ensure the shadow gfn can not be changed.

Btw, the origin code allows to create readonly spte under this case if !(pte_access & WRITEABBLE)

Regarding shadow: it should be fine as long as fault path always deletes large mappings, when shadowed pages are present in the region.

For hard mmu is also safe, in this patch i added these code:

(quoted diff) +2 -0

@@ -2635,6 +2617,8 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			break;
 		}
 
+		drop_large_spte(vcpu, iterator.sptep);
+
 

It can delete large mappings like soft mmu does.

Anything i missed?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Ah, unshadowing from reexecute_instruction does not handle large pages. I suppose that is what "simplification" refers to.

reexecute_instruction did not directly handle last spte, it just removes all shadow pages, then let cpu retry the instruction, the page can become writable when encounter #PF again, large spte is fine under this case.

While searching for a given "gpa", you don't find large gfn which is mapping it, right? (that is, searching for gfn 4 fails to find large read-only "gfn 0"). Unshadowing gfn 4 will keep large read-only mapping present.

Quoted 7 lines

Quoted 7 lines

1. large read-write spte to gfn 0
2. shadow gfn 4
3. write-protect large spte pointing to gfn 0
4. write to gfn 4
5. instruction emulation fails
5. unshadow gfn 4
6. refault, do not drop large spte because no pages shadowed

7. refault, then goto 2 (as part of write to gfn 4)

Quoted 1 line

Quoted 4 lines

Hmm, it is not true. :)

The large spte can become writable since 'no pages adhadoes' (that means has_wrprotected_page() can return 0 for this case). No?

What if gfn 4 is a pagetable part of the pagedirectory chain used to map gfn 4? See corrected step 7 above.

Quoted 145 lines · Marcelo Tosatti

Quoted 133 lines · Xiao Guangrong

Quoted 132 lines · Marcelo Tosatti

Quoted 117 lines · Xiao Guangrong

Quoted 95 lines · Marcelo Tosatti

Quoted 94 lines · Xiao Guangrong

Quoted 35 lines · Marcelo Tosatti

Quoted 25 lines · Xiao Guangrong

Hi Marcelo,

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

Quoted 9 lines

Quoted 4 lines

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

It needs a page fault to install a pte even if it is the read access. After the change, the page fault can be avoided.

Quoted 8 lines

Quoted 1 line

Can you measure an improvement with this change?

I have a test case to measure the read time which has been attached. It maps 4k pages at first (dirt-loggged), then switch to large sptes (stop dirt-logging), at the last, measure the read access time after write protect sptes.

Before: 23314111 ns After: 11404197 ns

Quoted 8 lines

Ok, i'm concerned about cases similar to e49146dce8c3dc6f44 (with shadow), that is:

- large page must be destroyed when write protecting due to shadowed page.
- with shadow, it does not make sense to write protect large sptes as mentioned earlier.

This case is removed now, the code when e49146dce8c3dc6f44 was applied is:

|
|                pt = sp->spt;
|                for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
|                        /* avoid RMW */
|                        if (is_writable_pte(pt[i]))
|                                update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
|        }

The real problem in this code is it would write-protect the spte even if it is not a last spte that caused the middle-level shadow page table was write-protected. So e49146dce8c3dc6f44 added this code:

|                if (sp->role.level != PT_PAGE_TABLE_LEVEL)
|                        continue;
|

was good to fix this problem.

Now, the current code is:

|		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
|			if (!is_shadow_present_pte(pt[i]) ||
|			      !is_last_spte(pt[i], sp->role.level))
|				continue;
|
|			spte_write_protect(kvm, &pt[i], &flush, false);
|		}

It only write-protect the last spte. So, it allows large spte existent. (the large spte can be broken by drop_large_spte() on the page-fault path.)

Quoted 10 lines

So i wonder why is this part from your patch

(quoted diff) +0 -6

-               if (level > PT_PAGE_TABLE_LEVEL &&
-                   has_wrprotected_page(vcpu->kvm, gfn, level)) {
-                       ret = 1;
-                       drop_spte(vcpu->kvm, sptep);
-                       goto done;
-               }

necessary (assuming EPT is in use).

This is safe, we change these code to:

(quoted diff) +3 -1

-		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		if ((level > PT_PAGE_TABLE_LEVEL &&
+		   has_wrprotected_page(vcpu->kvm, gfn, level)) ||
+		      mmu_need_write_protect(vcpu, gfn, can_unsync)) {
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;

Quoted 6 lines

Quoted 3 lines

The spte become read-only which can ensure the shadow gfn can not be changed.

Btw, the origin code allows to create readonly spte under this case if !(pte_access & WRITEABBLE)

Regarding shadow: it should be fine as long as fault path always deletes large mappings, when shadowed pages are present in the region.

For hard mmu is also safe, in this patch i added these code:

(quoted diff) +2 -0

@@ -2635,6 +2617,8 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			break;
 		}
 
+		drop_large_spte(vcpu, iterator.sptep);
+
 

It can delete large mappings like soft mmu does.

Anything i missed?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Ah, unshadowing from reexecute_instruction does not handle large pages. I suppose that is what "simplification" refers to.

reexecute_instruction did not directly handle last spte, it just removes all shadow pages, then let cpu retry the instruction, the page can become writable when encounter #PF again, large spte is fine under this case.

While searching for a given "gpa", you don't find large gfn which is mapping it, right? (that is, searching for gfn 4 fails to find large read-only "gfn 0"). Unshadowing gfn 4 will keep large read-only mapping present.

Quoted 7 lines

Quoted 7 lines

1. large read-write spte to gfn 0
2. shadow gfn 4
3. write-protect large spte pointing to gfn 0
4. write to gfn 4
5. instruction emulation fails
5. unshadow gfn 4
6. refault, do not drop large spte because no pages shadowed

7. refault, then goto 2 (as part of write to gfn 4)

Quoted 3 lines

Hmm, it is not true. :)

Quoted 5 lines

Quoted 2 lines

The large spte can become writable since 'no pages adhadoes' (that means has_wrprotected_page() can return 0 for this case). No?

What if gfn 4 is a pagetable part of the pagedirectory chain used to map gfn 4? See corrected step 7 above.

Ah, this is a real bug, and unfortunately, it exists in current code. I will make a separate patchset to fix it. Thank you, Marcelo!

Quoted 3 lines

Ah, this is a real bug, and unfortunately, it exists in current code. I will make a separate patchset to fix it. Thank you, Marcelo!

Is it? Hum..

Anyway, it would be great if you can write a testcase (should be similar in size to rmap_chain).

On 11/18/2012 11:00 AM, Marcelo Tosatti wrote:
map gfn 4? See corrected step 7 above.

Quoted 4 lines

Quoted 3 lines

Ah, this is a real bug, and unfortunately, it exists in current code. I will make a separate patchset to fix it. Thank you, Marcelo!

Quoted 4 lines

Is it? Hum..

Anyway, it would be great if you can write a testcase (should be similar in size to rmap_chain).

Marcelo, is this patch acceptable?

Quoted 4 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

Quoted 8 lines

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

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

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

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

Can you measure an improvement with this change?

Ccing live migration developers who should be interested in this work,

Quoted 14 lines · On Mon, 12 Nov 2012 21:10:32 -0200 Marcelo Tosatti <[email protected]> wrote:

Quoted 4 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

Quoted 8 lines

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

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

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

Quoted 5 lines

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

Can you measure an improvement with this change?

What we discussed at KVM Forum last week was about the jitter we could measure right after starting live migration: both Isaku and Chegu reported such jitter.

So if this patch reduces such jitter for some real workloads, by lazily dropping largepage mappings and saving read faults until that point, that would be very nice!

But sadly, what they measured included interactions with the outside of the guest, and the main cause was due to the big QEMU lock problem, they guessed. The order is so different that an improvement by a kernel side effort may not be seen easily.

FWIW: I am now changing the initial write protection by kvm_mmu_slot_remove_write_access() to rmap based as I proposed at KVM Forum. ftrace said that 1ms was improved to 250-350us by the change for 10GB guest. My code still drops largepage mappings, so the initial write protection time itself may not be a such big issue here, I think.

Again, if we can eliminate read faults to such an extent that guests can see measurable improvement, that should be very nice!

Any thoughts?

Thanks,
Takuya

Quoted 47 lines · Takuya Yoshikawa

Ccing live migration developers who should be interested in this work,

Quoted 14 lines · On Mon, 12 Nov 2012 21:10:32 -0200 Marcelo Tosatti <[email protected]> wrote:

Quoted 4 lines · Xiao Guangrong

Do not drop large spte until it can be insteaded by small pages so that the guest can happliy read memory through it

The idea is from Avi:

Quoted 8 lines

| As I mentioned before, write-protecting a large spte is a good idea,
| since it moves some work from protect-time to fault-time, so it reduces
| jitter.  This removes the need for the return value.

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

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

Quoted 5 lines

Its likely that other 4k pages are mapped read-write in the 2mb range covered by a read-only 2mb map. Therefore its not entirely useful to map read-only.

Can you measure an improvement with this change?

What we discussed at KVM Forum last week was about the jitter we could measure right after starting live migration: both Isaku and Chegu reported such jitter.

So if this patch reduces such jitter for some real workloads, by lazily dropping largepage mappings and saving read faults until that point, that would be very nice!

But sadly, what they measured included interactions with the outside of the guest, and the main cause was due to the big QEMU lock problem, they guessed. The order is so different that an improvement by a kernel side effort may not be seen easily.

FWIW: I am now changing the initial write protection by kvm_mmu_slot_remove_write_access() to rmap based as I proposed at KVM Forum. ftrace said that 1ms was improved to 250-350us by the change for 10GB guest. My code still drops largepage mappings, so the initial write protection time itself may not be a such big issue here, I think.

Again, if we can eliminate read faults to such an extent that guests can see measurable improvement, that should be very nice!

Quoted 4 lines

Any thoughts?

Thanks,
Takuya

OK, makes sense. I'm worried about shadow / oos interactions with large read-only mappings (trying to remember what was the case exactly, it might be non-existant now).