KVM: MMU: fast page fault

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

* Advantage
- it is really fast
it fixes page fault out of mmu-lock, and uses a very light way to avoid the race with other pathes. Also, it fixes page fault in the front of gfn_to_pfn, it means no host page table walking.

- we can get lots of page fault with PFEC.P = 1 in KVM:
- in the case of ept/npt
　after shadow page become stable (all gfn is mapped in shadow page table, it is a short stage since only one shadow page table is used and only a few of page is needed), almost all page fault is caused by write-protect (frame-buffer under Xwindow, migration), the other small part is caused by page merge/COW under KSM/THP.

We do not hope it can fix the page fault caused by the read-only host page of KSM, since after COW, all the spte pointing to the gfn will be unmapped.

- in the case of soft mmu
- many spurious page fault due to tlb lazily flushed
- lots of write-protect page fault (dirty bit track for guest pte, shadow page table write-protected, frame-buffer under Xwindow, migration, ...)

* Implementation
We can freely walk the page between walk_shadow_page_lockless_begin and walk_shadow_page_lockless_end, it can ensure all the shadow page is valid.

In the most case, cmpxchg is fair enough to change the access bit of spte, but the write-protect path on softmmu/nested mmu is a especial case: it is a read-check-modify path: read spte, check W bit, then clear W bit.

In order
to avoid marking spte writable after/during page write-protect, we do the trick like below:

fast page fault path:
lock RCU
set identification in the spte

smp_mb()
if (!rmap.PTE_LIST_WRITE_PROTECT)
cmpxchg + w - vcpu-id
unlock RCU

write protect path:
lock mmu-lock
set rmap.PTE_LIST_WRITE_PROTECT
smp_mb()
if (spte.w || spte has identification)
clear w bit and identification
unlock mmu-lock

Setting identification in the spte is used to notify page-protect path to modify the spte, then we can see the change in the cmpxchg.

Setting identification is also a trick: it only set the last bit of spte that does not change the mapping and lose cpu status bits.

The identification should be unique to avoid the below race:

VCPU 0 VCPU 1 VCPU 2
lock RCU
spte + identification
check conditions
do write-protect, clear
identification
lock RCU
set identification
cmpxchg + w - identification
OOPS!!!

We choose the vcpu id as the unique value, currently, 254 vcpus on VMX and 127 vcpus on softmmu can be fast. Keep it simply firtsly. :)

* Performance
It introduces a full memory barrier on the page write-protect path, i have done the test of kernbench in the text mode which does not generate write-protect page fault by frame-buffer avoiding the optimization introduced by this patch, it shows no regression.

And there is the result tested by x11perf and migration on autotest:

x11perf (x11perf -repeat 10 -comppixwin500):
(Host: Intel(R) Core(TM) i5-2540M CPU @ 2.60GHz * 4 + 4G
Guest: 4 vcpus + 1G)

- For ept:

$ x11perfcomp baseline-hard optimaze-hard
1: baseline-hard
2: optimaze-hard

     1         2    Operation
--------  --------  ---------
  7060.0    7150.0  Composite 500x500 from pixmap to window

- For shadow mmu:

$ x11perfcomp baseline-soft optimaze-soft
1: baseline-soft
2: optimaze-soft

     1         2    Operation
--------  --------  ---------
  6980.0    7490.0  Composite 500x500 from pixmap to window

( It is interesting that after this patch, the performance of x11perf on
softmmu is better than it on hardmmu, i have tested it for many times, it is really true. :) )

autotest migration:
(Host: Intel(R) Xeon(R) CPU X5690 @ 3.47GHz * 12 + 32G)

- For ept:

Before:

                    smp2.Fedora.16.64.migrate
Times   .unix      .with_autotest.dbench.unix     total
 1       102           204                         309
 2       68            203                         275
 3       67            218                         289

After:

                    smp2.Fedora.16.64.migrate
Times   .unix      .with_autotest.dbench.unix     total
 1       103           189                         295
 2       67            188                         259
 3       64            202                         271

- For shadow mmu:

Before:

                    smp2.Fedora.16.64.migrate
Times   .unix      .with_autotest.dbench.unix     total
 1       102           262                         368
 2       68            220                         292
 3       68            234                         307

After:

                    smp2.Fedora.16.64.migrate
Times   .unix      .with_autotest.dbench.unix     total
 1       104           231                         341
 2       68            218                         289
 3       66            205                         275

Any comments are welcome. :)

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 |   57 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 33 insertions(+), 24 deletions(-)

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

index c759e4f..ad40647 100644

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

+{
+	u64 spte = *sptep;
+
+	if (!is_writable_pte(spte))
+		return false;
+
+	*flush |= true;

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

+
+	rmap_printk("rmap_write_protect: spte %p %llx\n", spte, *spte);
+	spte = spte & ~PT_WRITABLE_MASK;
+	mmu_spte_update(sptep, spte);
+
+	return false;
+}
+
 

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 3 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

Wow!

Looks like interesting times are back in mmu-land.

Comments below are before review of actual patches, so maybe they're already answered there, or maybe they're just nonsense.

Quoted 7 lines

* Implementation
We can freely walk the page between walk_shadow_page_lockless_begin and walk_shadow_page_lockless_end, it can ensure all the shadow page is valid.

In the most case, cmpxchg is fair enough to change the access bit of spte, but the write-protect path on softmmu/nested mmu is a especial case: it is a read-check-modify path: read spte, check W bit, then clear W bit.

We also set gpte.D and gpte.A, no? How do you handle that?

Quoted 7 lines

In order
to avoid marking spte writable after/during page write-protect, we do the trick like below:

fast page fault path:
lock RCU
set identification in the spte

What if you can't (already taken)? Spin? Slow path?

Quoted 12 lines

The identification should be unique to avoid the below race:

VCPU 0 VCPU 1 VCPU 2
lock RCU
spte + identification
check conditions
do write-protect, clear
identification
lock RCU
set identification
cmpxchg + w - identification
OOPS!!!

Is it not sufficient to use just two bits?

pf_lock - taken by page fault path
wp_lock - taken by write protect path

pf cmpxchg checks both bits.

Quoted 18 lines

- For ept:

$ x11perfcomp baseline-hard optimaze-hard
1: baseline-hard
2: optimaze-hard

     1         2    Operation
--------  --------  ---------
  7060.0    7150.0  Composite 500x500 from pixmap to window

- For shadow mmu:

$ x11perfcomp baseline-soft optimaze-soft
1: baseline-soft
2: optimaze-soft

     1         2    Operation
--------  --------  ---------
  6980.0    7490.0  Composite 500x500 from pixmap to window

Quoted 3 lines

( It is interesting that after this patch, the performance of x11perf on
softmmu is better than it on hardmmu, i have tested it for many times, it is really true. :) )

It could be because you cannot use THP with dirty logging, so you pay the overhead of TDP.

Quoted 2 lines

Any comments are welcome. :)

Very impressive. Now to review the patches (will take me some time).

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

Quoted 3 lines

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

+/* Return true if the spte is dropped. */
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool large,
+			       int *flush)

bool *flush

Quoted 7 lines

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

+{
+	u64 spte = *sptep;
+
+	if (!is_writable_pte(spte))
+		return false;
+
+	*flush |= true;

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

Quoted 10 lines

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

+
+	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;
+	}

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.

It may also be a good idea to populate the lower level instead of dropping the spte.

All outside this patch set of course. I'd add those ideas to the wiki but it won't let me edit at the moment.

Quoted 8 lines

Quoted 7 lines

* Implementation
We can freely walk the page between walk_shadow_page_lockless_begin and walk_shadow_page_lockless_end, it can ensure all the shadow page is valid.

In the most case, cmpxchg is fair enough to change the access bit of spte, but the write-protect path on softmmu/nested mmu is a especial case: it is a read-check-modify path: read spte, check W bit, then clear W bit.

Quoted 2 lines

We also set gpte.D and gpte.A, no? How do you handle that?

We still need walk gust page table before fast page fault to check whether the access is valid.

Quoted 10 lines

Quoted 8 lines

In order
to avoid marking spte writable after/during page write-protect, we do the trick like below:

fast page fault path:
lock RCU
set identification in the spte

What if you can't (already taken)? Spin? Slow path?

In this patch, it allows to concurrently access on the same spte:
it freely set its identification on the spte, because i did not want to introduce other atomic operations.

You remind me that there may be a risk: if many vcpu fault on the same spte, it will retry the spte forever. Hmm, how about fix it like this:

if ( spte.identification = 0) {
set spte.identification = vcpu.id
goto cmpxchg-path
}

if (spte.identification == vcpu.id)
goto cmpxchg-path

return to guest and retry the address again;

cmpxchg-path:
do checks and cmpxchg

It can ensure the spte can be updated.

Quoted 19 lines

Quoted 13 lines

The identification should be unique to avoid the below race:

VCPU 0 VCPU 1 VCPU 2
lock RCU
spte + identification
check conditions
do write-protect, clear
identification
lock RCU
set identification
cmpxchg + w - identification
OOPS!!!

Is it not sufficient to use just two bits?

pf_lock - taken by page fault path
wp_lock - taken by write protect path

Quoted 2 lines

pf cmpxchg checks both bits.

If we just use two byte as identification, it has to use atomic operations to maintain these bits? or i misunderstood?

Split FNAME(prefetch_invalid_gpte) to check gpte independently which will be used in the later patch

arch/x86/kvm/paging_tmpl.h +2 -4

-static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu,
-				    struct kvm_mmu_page *sp, u64 *spte,
-				    pt_element_t gpte)
+static bool FNAME(invalid_gpte)(struct kvm_vcpu *vcpu, pt_element_t gpte)
 {
 	if (is_rsvd_bits_set(&vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL))
-		goto no_present;
+		return true;

+}

arch/x86/kvm/paging_tmpl.h +4 -0

+
+	return gfn_to_gpa(sp->gfn) + offset;
+}
+
 /*
  * Page fault handler.  There are several causes for a page fault:
  *   - there is no shadow pte for the guest pte

@@ -659,18 +669,6 @@ out_unlock:
 	return 0;
 

Do not use the last byte (bit 56 ~ bit 63) in shadow_mmio_mask, the late patch will store vcpu id in the last byte

Quoted 2 lines · Xiao Guangrong

Do not use the last byte (bit 56 ~ bit 63) in shadow_mmio_mask, the late patch will store vcpu id in the last byte

Bit 63 is PT64_NX_MASK.

Using PTE_LIST_WRITE_PROTECT bit in rmap store the write-protect status to avoid unnecessary shadow page walking

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Xiao Guangrong

Do not use the last byte (bit 56 ~ bit 63) in shadow_mmio_mask, the late patch will store vcpu id in the last byte

Bit 63 is PT64_NX_MASK.

Oh, you did say that with softmmu we're limited to 127 cpus. But what if spte.nx is changed while we update the identity?

Quoted 17 lines · Xiao Guangrong

Split FNAME(prefetch_invalid_gpte) to check gpte independently which will be used in the later patch

arch/x86/kvm/paging_tmpl.h +3 -5

-static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu,
-				    struct kvm_mmu_page *sp, u64 *spte,
-				    pt_element_t gpte)
+static bool FNAME(invalid_gpte)(struct kvm_vcpu *vcpu, pt_element_t gpte)
 {
 	if (is_rsvd_bits_set(&vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL))
-		goto no_present;
+		return true;
 
 	if (!is_present_gpte(gpte))
-		goto no_present;
+		return true;

arch/x86/kvm/paging_tmpl.h static int FNAME(walk_addr_nested)(struct guest_walker *walker, · line 351

Quoted 5 lines

arch/x86/kvm/paging_tmpl.h +1 -1

 	if (!(gpte & PT_ACCESSED_MASK))
-		goto no_present;
+		return true;
 
 	return false;

A comment (or a name change) is needed to indicate that we're not checking for an invalid gpte, just invalid for caching in an spte.

arch/x86/kvm/paging_tmpl.h out_gpte_changed: · line 562

Quoted 6 lines · Xiao Guangrong

It can calculate the base gpa of the specified shadow page on any level, let it instead of FNAME(get_level1_sp_gpa)

arch/x86/kvm/paging_tmpl.h +2 -0

+static gpa_t FNAME(get_sp_gpa)(struct kvm_mmu_page *sp)
+{

Maybe worth adding a pte index and calling it get_spte_gpa(), depends on the next patches I guess.

arch/x86/kvm/paging_tmpl.h out_gpte_changed: · line 566

Quoted 4 lines

arch/x86/kvm/paging_tmpl.h +4 -0

+	int offset, shift;
+
+	shift = PAGE_SHIFT - (PT_LEVEL_BITS - PT64_LEVEL_BITS) * sp->role.level;
+	offset = sp->role.quadrant << shift;

if you add '& ~PAGE_MASK' the compiler may be smart enough to drop the whole thing for 64-bit ptes.

Quoted 2 lines · Xiao Guangrong

Using PTE_LIST_WRITE_PROTECT bit in rmap store the write-protect status to avoid unnecessary shadow page walking

Does kvm_set_pte_rmapp() need adjustment?

arch/x86/kvm/mmu.c static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn, · line 2276

Quoted 6 lines

Also if no shadow page is indirect, the page is write-free

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

+	if (!vcpu->kvm->arch.indirect_shadow_pages)
+		return 0;
+

Best in its own little patch.

Quoted 12 lines · Xiao Guangrong

Quoted 10 lines

Quoted 7 lines

* Implementation
We can freely walk the page between walk_shadow_page_lockless_begin and walk_shadow_page_lockless_end, it can ensure all the shadow page is valid.

In the most case, cmpxchg is fair enough to change the access bit of spte, but the write-protect path on softmmu/nested mmu is a especial case: it is a read-check-modify path: read spte, check W bit, then clear W bit.

We also set gpte.D and gpte.A, no? How do you handle that?

Quoted 2 lines

We still need walk gust page table before fast page fault to check whether the access is valid.

Ok. But that's outside the mmu lock.

We can also skip that: if !sp->unsync, copy access rights and gpte.A/D into spare bits in the spte, and use that to check.

Quoted 28 lines

Quoted 10 lines

Quoted 8 lines

In order
to avoid marking spte writable after/during page write-protect, we do the trick like below:

fast page fault path:
lock RCU
set identification in the spte

What if you can't (already taken)? Spin? Slow path?

In this patch, it allows to concurrently access on the same spte:
it freely set its identification on the spte, because i did not want to introduce other atomic operations.

You remind me that there may be a risk: if many vcpu fault on the same spte, it will retry the spte forever. Hmm, how about fix it like this:

if ( spte.identification = 0) {
set spte.identification = vcpu.id
goto cmpxchg-path
}

if (spte.identification == vcpu.id)
goto cmpxchg-path

Quoted 6 lines

return to guest and retry the address again;

cmpxchg-path:
do checks and cmpxchg

It can ensure the spte can be updated.

What's the difference between this and

if test_and_set_bit(spte.lock)
return_to_guest
else
do checks and cmpxchg

?

Quoted 23 lines

Quoted 21 lines

Quoted 13 lines

The identification should be unique to avoid the below race:

VCPU 0 VCPU 1 VCPU 2
lock RCU
spte + identification
check conditions
do write-protect, clear
identification
lock RCU
set identification
cmpxchg + w - identification
OOPS!!!

Is it not sufficient to use just two bits?

pf_lock - taken by page fault path
wp_lock - taken by write protect path

pf cmpxchg checks both bits.

Quoted 2 lines

If we just use two byte as identification, it has to use atomic operations to maintain these bits? or i misunderstood?

Ah, you're using byte writes to set the identification? remember I didn't read the patches yet.

Please check the optimization manual, I remember there are warnings there about using different sized accesses for the same location, particularly when locked ops are involved.

Maybe bitops are cheaper.
If the cost is similar, they're at least more similar to the rest of the kernel.

arch/x86/kvm/paging_tmpl.h +18 -0

+static bool
+FNAME(fast_pf_fetch_indirect_spte)(struct kvm_vcpu *vcpu, u64 *sptep,
+				   u64 *new_spte, gfn_t gfn,
+				   u32 expect_access, u64 spte)
+
+{
+	struct kvm_mmu_page *sp = page_header(__pa(sptep));
+	pt_element_t gpte;
+	gpa_t pte_gpa;
+	unsigned pte_access;
+
+	if (sp->role.direct)
+		return fast_pf_fetch_direct_spte(vcpu, sptep, new_spte,
+						 gfn, expect_access, spte);
+
+	pte_gpa = FNAME(get_sp_gpa)(sp);
+	pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
+

Quoted 13 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

Quoted 10 lines

Quoted 7 lines

* Implementation
We can freely walk the page between walk_shadow_page_lockless_begin and walk_shadow_page_lockless_end, it can ensure all the shadow page is valid.

In the most case, cmpxchg is fair enough to change the access bit of spte, but the write-protect path on softmmu/nested mmu is a especial case: it is a read-check-modify path: read spte, check W bit, then clear W bit.

We also set gpte.D and gpte.A, no? How do you handle that?

Quoted 8 lines

Quoted 2 lines

We still need walk gust page table before fast page fault to check whether the access is valid.

Ok. But that's outside the mmu lock.

We can also skip that: if !sp->unsync, copy access rights and gpte.A/D into spare bits in the spte, and use that to check.

Great!

gpte.A need not be copied into spte since EFEC.P = 1 means the shadow page table is present, gpte.A must be set in this case.

And, we do not need to cache gpte access rights into spte, instead of it, we can atomicly read gpte to get these information (anyway, we need read gpte to get the gfn.)

Quoted 11 lines · Avi Kivity

arch/x86/kvm/paging_tmpl.h out_gpte_changed: · line 562

Quoted 6 lines · Xiao Guangrong

It can calculate the base gpa of the specified shadow page on any level, let it instead of FNAME(get_level1_sp_gpa)

arch/x86/kvm/paging_tmpl.h +2 -0

+static gpa_t FNAME(get_sp_gpa)(struct kvm_mmu_page *sp)
+{

Maybe worth adding a pte index and calling it get_spte_gpa(), depends on the next patches I guess.

Yes.

But FNAME(get_sp_gpa) is still needed by FNAME(sync_page) path which sync all sptes. I will introduce get_spte_gpa() as you mentioned which will be used in invlpg path and in the later patch.

Quoted 8 lines

arch/x86/kvm/paging_tmpl.h out_gpte_changed: · line 566

Quoted 4 lines

arch/x86/kvm/paging_tmpl.h +4 -0

+	int offset, shift;
+
+	shift = PAGE_SHIFT - (PT_LEVEL_BITS - PT64_LEVEL_BITS) * sp->role.level;
+	offset = sp->role.quadrant << shift;

if you add '& ~PAGE_MASK' the compiler may be smart enough to drop the whole thing for 64-bit ptes.

Sorry. how to do that?

But if you want to remove the unnecessary workload, i will change it like this:

static gpa_t FNAME(get_sp_gpa)(struct kvm_mmu_page *sp)

{
	int offset = 0;

#if PTTYPE == 32
	int shift;

	shift = PAGE_SHIFT - (PT_LEVEL_BITS - PT64_LEVEL_BITS) * sp->role.level;
	offset = sp->role.quadrant << shift;
#endif
	return gfn_to_gpa(sp->gfn) + offset;
}

Quoted 13 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

Quoted 10 lines

Quoted 7 lines

* Implementation
We can freely walk the page between walk_shadow_page_lockless_begin and walk_shadow_page_lockless_end, it can ensure all the shadow page is valid.

In the most case, cmpxchg is fair enough to change the access bit of spte, but the write-protect path on softmmu/nested mmu is a especial case: it is a read-check-modify path: read spte, check W bit, then clear W bit.

We also set gpte.D and gpte.A, no? How do you handle that?

Quoted 8 lines

Quoted 2 lines

We still need walk gust page table before fast page fault to check whether the access is valid.

Ok. But that's outside the mmu lock.

We can also skip that: if !sp->unsync, copy access rights and gpte.A/D into spare bits in the spte, and use that to check.

Great!

gpte.A need not be copied into spte since EFEC.P = 1 means the shadow page table is present, gpte.A must be set in this case.

And, we do not need to cache gpte access rights into spte, instead of it, we can atomicly read gpte to get these information (anyway, we need read gpte to get the gfn.)

I will do it in the next version.

Quoted 43 lines

Quoted 28 lines

Quoted 10 lines

Quoted 8 lines

In order
to avoid marking spte writable after/during page write-protect, we do the trick like below:

fast page fault path:
lock RCU
set identification in the spte

What if you can't (already taken)? Spin? Slow path?

In this patch, it allows to concurrently access on the same spte:
it freely set its identification on the spte, because i did not want to introduce other atomic operations.

You remind me that there may be a risk: if many vcpu fault on the same spte, it will retry the spte forever. Hmm, how about fix it like this:

if ( spte.identification = 0) {
set spte.identification = vcpu.id
goto cmpxchg-path
}

if (spte.identification == vcpu.id)
goto cmpxchg-path

Quoted 6 lines

return to guest and retry the address again;

cmpxchg-path:
do checks and cmpxchg

It can ensure the spte can be updated.

What's the difference between this and

if test_and_set_bit(spte.lock)
return_to_guest
else
do checks and cmpxchg

Quoted 2 lines

?

test_and_set_bit is a atomic operation that is i want to avoid.

Quoted 24 lines

Quoted 24 lines

Quoted 22 lines

Quoted 4 lines

The identification should be unique to avoid the below race:

Quoted 10 lines

VCPU 0 VCPU 1 VCPU 2
lock RCU
spte + identification
check conditions
do write-protect, clear
identification
lock RCU
set identification
cmpxchg + w - identification
OOPS!!!

Is it not sufficient to use just two bits?

pf_lock - taken by page fault path
wp_lock - taken by write protect path

pf cmpxchg checks both bits.

Quoted 6 lines

Quoted 2 lines

If we just use two byte as identification, it has to use atomic operations to maintain these bits? or i misunderstood?

Ah, you're using byte writes to set the identification? remember I didn't read the patches yet.

Yes.

Quoted 3 lines

Please check the optimization manual, I remember there are warnings there about using different sized accesses for the same location, particularly when locked ops are involved.

"using different sized accesses for the same location" is OK i guess, anyway, we can have the data struct of this style:

union {
	struct	{
		char byte1,
		char byte2,
		char byte3,
		char byte4
      	} foo;

	int bar;
}

Access .bar and .foo.byte1 is allowed.

And for the LOCK operation, i find these descriptions in the manual:

"Locked operations are atomic with respect to all other memory operations and all externally visible events"

And there is a warning:
Software should access semaphores (shared memory used for signalling between multiple processors) using identical addresses and operand lengths. For example, if one processor accesses a semaphore using a word access, other processors should not access the semaphore using a byte access.

It is the warning you remember?

In our case, we do not treat it as "semaphores": the "byte writes" does not need be atomic.

Quoted 4 lines

Maybe bitops are cheaper.
If the cost is similar, they're at least more similar to the rest of the kernel.

The "bitops" you mentioned is the atomic-style bitops? It is cheaper that byte writes?

Using PTE_LIST_WRITE_PROTECT bit in rmap store the write-protect status to avoid unnecessary shadow page walking

Also if no shadow page is indirect, the page is write-free

It depends on PTE_LIST_WRITE_PROTECT bit in rmap which let us quickly know whether the page is writable out of mmu-lock

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

---
 arch/x86/kvm/mmu.c         |   17 +++++++++++++----
 arch/x86/kvm/paging_tmpl.h |    2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

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

index 3887a07..c029185 100644

If the the present bit of page fault error code is set, it indicates the shadow page is populated on all levels, it means what we do is only modify the access bit which can be done out of mmu-lock

The tricks in this patch is avoiding the race between fast page fault path and write-protect path, write-protect path is a read-check-modify path:
read spte, check W bit, then clear W bit. What we do is populating a identification in spte, if write-protect meets it, it modify the spte even if the spte is readonly. See the comment in the code to get more information

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

+
+static bool page_fault_can_be_fast(struct kvm_vcpu *vcpu, gfn_t gfn,
+				   u32 error_code)
+{
+	unsigned long *rmap;
+	bool write = error_code & PFERR_WRITE_MASK;
+
+	/*
+	 * #PF can be fast only if the shadow page table is present, that

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

+
+	/* Quickly check the page can be writable. */
+	if (write && (ACCESS_ONCE(*rmap) & PTE_LIST_WRITE_PROTECT))
+		return false;
+
+	return true;
+}
+

Quoted 28 lines · Xiao Guangrong

If the the present bit of page fault error code is set, it indicates the shadow page is populated on all levels, it means what we do is only modify the access bit which can be done out of mmu-lock

The tricks in this patch is avoiding the race between fast page fault path and write-protect path, write-protect path is a read-check-modify path:
read spte, check W bit, then clear W bit. What we do is populating a identification in spte, if write-protect meets it, it modify the spte even if the spte is readonly. See the comment in the code to get more information

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

+
+static bool page_fault_can_be_fast(struct kvm_vcpu *vcpu, gfn_t gfn,
+				   u32 error_code)
+{
+	unsigned long *rmap;
+	bool write = error_code & PFERR_WRITE_MASK;
+
+	/*
+	 * #PF can be fast only if the shadow page table is present, that
+	 * means we just need change the access bits (e.g: R/W, U/S...)
+	 * which can be done out of mmu-lock.
+	 */
+	if (!(error_code & PFERR_PRESENT_MASK))
+		return false;
+
+	if (unlikely(vcpu->vcpu_id > max_vcpu_spte()))

arch/x86/kvm/mmu.c exit: · line 2830

Quoted 3 lines

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

+		return false;
+
+	rmap = gfn_to_rmap(vcpu->kvm, gfn, PT_PAGE_TABLE_LEVEL);

What prevents sp->gfns from being freed while this is going on? Did I miss the patch that makes mmu pages freed by rcu? Also need barriers in kvm_mmu_get_page() to make sure sp->gfns is made visible before the page is hashed in.

Quoted 7 lines · Xiao Guangrong

Using PTE_LIST_WRITE_PROTECT bit in rmap store the write-protect status to avoid unnecessary shadow page walking

Also if no shadow page is indirect, the page is write-free

arch/x86/kvm/mmu.c static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn, · line 2297

Quoted 7 lines

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

@@ -2262,6 +2291,9 @@ static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 	}
 	if (need_unsync)
 		kvm_unsync_pages(vcpu, gfn);
+
+	*rmap &= ~PTE_LIST_WRITE_PROTECT;
+
 

So what are the rules for PTE_LIST_WRITE_PROTECT? Is is a cache for the mmu_need_write_protect?

I'd like to understand it, I guess it can be set while write protection is unneeded, and cleared on the next check?

Maybe split into two functions, one the normal mmu_need_write_protect (but renamed) and a new mmu_need_write_protect(), with locked and unlocked variants, calling the old one.

Quoted 17 lines · Xiao Guangrong

It depends on PTE_LIST_WRITE_PROTECT bit in rmap which let us quickly know whether the page is writable out of mmu-lock

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

---
 arch/x86/kvm/mmu.c         |   17 +++++++++++++----
 arch/x86/kvm/paging_tmpl.h |    2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

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

index 3887a07..c029185 100644
@@ -1148,6 +1148,12 @@ static int rmap_write_protect(struct kvm *kvm, u64 gfn)
 
 	*rmapp |= PTE_LIST_WRITE_PROTECT;
 

arch/x86/kvm/mmu.c static int rmap_write_protect(struct kvm *kvm, u64 gfn) · line 1156

Quoted 6 lines

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

+	/*
+	 * Setting PTE_LIST_WRITE_PROTECT bit before doing page
+	 * write-protect.
+	 */
+	smp_mb();
+

wmb only needed.

Would it be better to store this bit in all the sptes instead? We're touching them in any case. More work to clear them, but un-write-protecting a page is beneficial anyway as it can save a fault.

Quoted 26 lines · Xiao Guangrong

If the the present bit of page fault error code is set, it indicates the shadow page is populated on all levels, it means what we do is only modify the access bit which can be done out of mmu-lock

The tricks in this patch is avoiding the race between fast page fault path and write-protect path, write-protect path is a read-check-modify path:
read spte, check W bit, then clear W bit. What we do is populating a identification in spte, if write-protect meets it, it modify the spte even if the spte is readonly. See the comment in the code to get more information

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

+
+static bool page_fault_can_be_fast(struct kvm_vcpu *vcpu, gfn_t gfn,
+				   u32 error_code)
+{
+	unsigned long *rmap;
+	bool write = error_code & PFERR_WRITE_MASK;
+
+	/*
+	 * #PF can be fast only if the shadow page table is present, that
+	 * means we just need change the access bits (e.g: R/W, U/S...)
+	 * which can be done out of mmu-lock.
+	 */
+	if (!(error_code & PFERR_PRESENT_MASK))
+		return false;

arch/x86/kvm/mmu.c exit: · line 2830

Quoted 5 lines

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

+
+	if (unlikely(vcpu->vcpu_id > max_vcpu_spte()))
+		return false;
+
+	rmap = gfn_to_rmap(vcpu->kvm, gfn, PT_PAGE_TABLE_LEVEL);

What prevents sp->gfns from being freed while this is going on? Did I miss the patch that makes mmu pages freed by rcu?

Also need barriers in
kvm_mmu_get_page() to make sure sp->gfns is made visible before the page is hashed in.

+ smp_rmb()

Quoted 12 lines

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

+
+	/* Quickly check the page can be writable. */
+	if (write && (ACCESS_ONCE(*rmap) & PTE_LIST_WRITE_PROTECT))
+		return false;
+
+	return true;
+}
+
+typedef bool (*fast_pf_fetch_spte)(struct kvm_vcpu *vcpu, u64 *sptep,
+				   u64 *new_spte, gfn_t gfn, u32 expect_access,
+				   u64 spte);
+

arch/x86/kvm/mmu.c exit: · line 2848

Quoted 6 lines

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

+static bool
+fast_pf_fetch_direct_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 *new_spte,
+			  gfn_t gfn, u32 expect_access, u64 spte)
+{
+	struct kvm_mmu_page *sp = page_header(__pa(sptep));
+

Why is this stable? Another cpu could have removed it.

arch/x86/kvm/mmu.c exit: · line 2852

Quoted 5 lines

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

+	WARN_ON(!sp->role.direct);
+
+	if (kvm_mmu_page_get_gfn(sp, sptep - sp->spt) != gfn)
+		return false;

Simpler to check if the page is sync; if it is, gfn has not changed.

(but the check for a sync page is itself unstable).

arch/x86/kvm/mmu.c exit: · line 2858

Quoted 7 lines

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

+
+	set_spte(vcpu, new_spte, sp->role.access,
+		 expect_access & ACC_USER_MASK, expect_access & ACC_WRITE_MASK,
+		 sp->role.level, gfn, spte_to_pfn(spte), false, false,
+		 spte & SPTE_HOST_WRITEABLE, true);
+

What if the conditions have changed meanwhile? Should we set the spte atomically via set_spte? For example an mmu notifier clearing the spte in parallel.

What if another code path has read *spte, and did not re-read it because it holds the mmu lock and thinks only the guest can access it?

arch/x86/kvm/mmu.c exit: · line 2870

Quoted 13 lines

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

+	return true;
+}
+
+static bool
+fast_page_fault_fix_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 spte,
+			 gfn_t gfn, u32 expect_access,
+			 fast_pf_fetch_spte fn)
+{
+	u64 new_spte = 0ull;
+	int vcpu_id = vcpu->vcpu_id;
+
+	spte = mark_vcpu_id_spte(sptep, spte, vcpu_id);

What's the point of reading spte? It can change any minute, so the value is stale and we can't use it. We have to read and lock it atomically, no?

Your patches read the spte, then set the identity. In between some other path can change the spte.

Quoted 25 lines · Xiao Guangrong

Quoted 13 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

Quoted 10 lines

Quoted 7 lines

* Implementation
We can freely walk the page between walk_shadow_page_lockless_begin and walk_shadow_page_lockless_end, it can ensure all the shadow page is valid.

In the most case, cmpxchg is fair enough to change the access bit of spte, but the write-protect path on softmmu/nested mmu is a especial case: it is a read-check-modify path: read spte, check W bit, then clear W bit.

We also set gpte.D and gpte.A, no? How do you handle that?

Quoted 8 lines

Quoted 2 lines

We still need walk gust page table before fast page fault to check whether the access is valid.

Ok. But that's outside the mmu lock.

We can also skip that: if !sp->unsync, copy access rights and gpte.A/D into spare bits in the spte, and use that to check.

Quoted 8 lines

Great!

gpte.A need not be copied into spte since EFEC.P = 1 means the shadow page table is present, gpte.A must be set in this case.

And, we do not need to cache gpte access rights into spte, instead of it, we can atomicly read gpte to get these information (anyway, we need read gpte to get the gfn.)

Why do we need the gfn? If !sp->unsync then it is unchanged (it's also in sp->gfns).

Oh, needed for mark_page_dirty().

Caching gpte access in the spte will save decoding the access buts and sp->role.access.

Quoted 50 lines

I will do it in the next version.

Quoted 45 lines

Quoted 28 lines

Quoted 10 lines

Quoted 8 lines

In order
to avoid marking spte writable after/during page write-protect, we do the trick like below:

fast page fault path:
lock RCU
set identification in the spte

What if you can't (already taken)? Spin? Slow path?

In this patch, it allows to concurrently access on the same spte:
it freely set its identification on the spte, because i did not want to introduce other atomic operations.

You remind me that there may be a risk: if many vcpu fault on the same spte, it will retry the spte forever. Hmm, how about fix it like this:

if ( spte.identification = 0) {
set spte.identification = vcpu.id
goto cmpxchg-path
}

if (spte.identification == vcpu.id)
goto cmpxchg-path

Quoted 6 lines

return to guest and retry the address again;

cmpxchg-path:
do checks and cmpxchg

It can ensure the spte can be updated.

What's the difference between this and

if test_and_set_bit(spte.lock)
return_to_guest
else
do checks and cmpxchg

?

Quoted 1 line

test_and_set_bit is a atomic operation that is i want to avoid.

Right. Can you check what the effect is (with say test_and_set_bit(spte, 0) in the same path).

I'm not convinced it's significant.

Quoted 66 lines

Quoted 24 lines

Quoted 24 lines

Quoted 22 lines

Quoted 4 lines

The identification should be unique to avoid the below race:

Quoted 10 lines

VCPU 0 VCPU 1 VCPU 2
lock RCU
spte + identification
check conditions
do write-protect, clear
identification
lock RCU
set identification
cmpxchg + w - identification
OOPS!!!

Is it not sufficient to use just two bits?

pf_lock - taken by page fault path
wp_lock - taken by write protect path

pf cmpxchg checks both bits.

Quoted 6 lines

Quoted 2 lines

If we just use two byte as identification, it has to use atomic operations to maintain these bits? or i misunderstood?

Ah, you're using byte writes to set the identification? remember I didn't read the patches yet.

Yes.

Quoted 3 lines

Please check the optimization manual, I remember there are warnings there about using different sized accesses for the same location, particularly when locked ops are involved.

"using different sized accesses for the same location" is OK i guess, anyway, we can have the data struct of this style:

union {
	struct	{
		char byte1,
		char byte2,
		char byte3,
		char byte4
      	} foo;

	int bar;
}

Access .bar and .foo.byte1 is allowed.

And for the LOCK operation, i find these descriptions in the manual:

"Locked operations are atomic with respect to all other memory operations and all externally visible events"

And there is a warning:
Software should access semaphores (shared memory used for signalling between multiple processors) using identical addresses and operand lengths. For example, if one processor accesses a semaphore using a word access, other processors should not access the semaphore using a byte access.

Quoted 1 line

It is the warning you remember?

No, it should be safe, but performance is degraded if you mix loads and stores of different sizes to the same location.

See for example "2.1.5.2 L1 DCache", Store Forwarding section in the optimization guide.

Quoted 11 lines

In our case, we do not treat it as "semaphores": the "byte writes" does not need be atomic.

Quoted 4 lines

Maybe bitops are cheaper.
If the cost is similar, they're at least more similar to the rest of the kernel.

Quoted 2 lines

The "bitops" you mentioned is the atomic-style bitops? It is cheaper that byte writes?

They're not cheaper in general but may be compared to mixing byte writes and word loads.

Quoted 13 lines

Quoted 13 lines

Quoted 11 lines

What's the difference between this and

if test_and_set_bit(spte.lock)
return_to_guest
else
do checks and cmpxchg

?

Quoted 7 lines

Quoted 1 line

test_and_set_bit is a atomic operation that is i want to avoid.

Right. Can you check what the effect is (with say test_and_set_bit(spte, 0) in the same path).

I'm not convinced it's significant.

Okay, if you prefer to test_and_set_bit, we may introduce two bits:
fast_pf and write_protect, do things like this:

on fast page fault path:

old_spte = *spte

if (test_and_set_bit(spte.fast_pf))
return_to_guest

old_spte |= fast_pf

if (old_spte.wp)
clear-fast-pf bit and return_to_guest

if (!rmap.PTE_LIST_WRITE_PROTECT)
cmpxchg(spte, old_spte, old_spte +w - fast_pf)
else
clear-fast-pf bit

on page write-protect path:

	lock mmu-lock
	set rmap.PTE_LIST_WRITE_PROTECT
        smp_mb()
        if (spte.w || spte.fast_pf)
                spte -w -fast_pf + write_protect

        unlock mmu-lock

I think it works but can not make page fault fast for unsync sp ( e.g: two sptes point to the gfn, a page fault on one of the spte make gfn's sp become unsync.), it is not too bad since we can cache access bits in spte and properly make all sptes become writable on mmu_need_write_protect path in a new patchset.

Quoted 18 lines · Avi Kivity

Quoted 17 lines · Xiao Guangrong

It depends on PTE_LIST_WRITE_PROTECT bit in rmap which let us quickly know whether the page is writable out of mmu-lock

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

---
 arch/x86/kvm/mmu.c         |   17 +++++++++++++----
 arch/x86/kvm/paging_tmpl.h |    2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

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

index 3887a07..c029185 100644
@@ -1148,6 +1148,12 @@ static int rmap_write_protect(struct kvm *kvm, u64 gfn)
 
 	*rmapp |= PTE_LIST_WRITE_PROTECT;
 

Quoted 9 lines

arch/x86/kvm/mmu.c static int rmap_write_protect(struct kvm *kvm, u64 gfn) · line 1156

Quoted 6 lines

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

+	/*
+	 * Setting PTE_LIST_WRITE_PROTECT bit before doing page
+	 * write-protect.
+	 */
+	smp_mb();
+

wmb only needed.

We should ensure setting this bit before reading spte, it cooperates with fast page fault path to avoid this case:

On fast page fault path: On rmap_write_protect path:
read spte: old_spte = *spte
(reading spte is reordered to the front of
setting PTE_LIST_WRITE_PROTECT bit)
set spte.identification
smp_mb
if (!rmap.PTE_LIST_WRITE_PROTECT)
set rmap.PTE_LIST_WRITE_PROTECT
cmpxchg(sptep, spte, spte | WRITABLE)
see old_spte.identification is not set, so it does not write-protect this page
OOPS!!!

Quoted 4 lines

Would it be better to store this bit in all the sptes instead? We're touching them in any case. More work to clear them, but un-write-protecting a page is beneficial anyway as it can save a fault.

There are two reasons:
- if we set this bit in rmap, we can do the quickly check to see the page is writble before doing shadow page walking.

- since a full barrier is needed, we should use smp_mb for every spte like this:

while ((spte = rmap_next(rmapp, spte))) {
read spte
smp_mb
write-protect spte
}

smp_mb is called in the loop, i think it is not good, yes?

If you just want to save the fault, we can let all spte to be writeable in mmu_need_write_protect, but we should cache gpte access bits into spte firstly. It should be another patchset i think. :)

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

Ah, so it's protecting two paths at the same time: rmap.write_protect -> fast page fault, and lock(sptep) -> write protect.

The whole thing needs to be documented very carefully in locking.txt, otherwise mmu.c will be write-protected to everyone except you.

Foolish me, i should be crazy. Sorry for my mistake. :(

Unfortunately, it can not work, we can not get a stable gfn from gpte or sp->gfns[]. For example:

beginning:
Gpte = Gfn1
gfn_to_pfn(Gfn1) = Pfn
Spte = Pfn
Gfn1 is write-free
Gfn2 is write-protected

VCPU 0 VCPU 1 VCPU 2

fault on gpte
fast page fault path:
set Spte.fast_pf
get Gfn1 from Gpte/sp->gfns[]
if (Gfn1 is writable)
Pfn is swapped out:
Spte = 0
Gpte is modified to Gfn2, and Pfn is realloced and remapped to Gfn2, so:
Spte = Pfn

fast page fault path:
set Spte.fast_pf

cmpxchg Spte+w
OOPS!!!
<we see Spte is not changed and
happily make it writable, so gfn2 can be writable>

It seems only a unique identification can prevent this. :(

Quoted 3 lines · On Mon, 09 Apr 2012 15:28:47 +0300 Avi Kivity <[email protected]> wrote:

Ah, so it's protecting two paths at the same time: rmap.write_protect -> fast page fault, and lock(sptep) -> write protect.

Quoted 2 lines

The whole thing needs to be documented very carefully in locking.txt, otherwise mmu.c will be write-protected to everyone except you.

Before changing mmu_lock usage, better describe it in locking.txt.

I guess from mmu.c git history, it is already restricted to a few people who can describe that.

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 5 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

This patchset is really worrying to me.

It introduces a lot of concurrency into data structures that were not designed for it. Even if it is correct, it will be very hard to convince ourselves that it is correct, and if it isn't, to debug those subtle bugs. It will also be much harder to maintain the mmu code than it is now.

There are a lot of things to check. Just as an example, we need to be sure that if we use rcu_dereference() twice in the same code path, that any inconsistencies due to a write in between are benign. Doing that is a huge task.

But I appreciate the performance improvement and would like to see a simpler version make it in. This needs to reduce the amount of data touched in the fast path so it is easier to validate, and perhaps reduce the number of cases that the fast path works on.

I would like to see the fast path as simple as

  rcu_read_lock();

  (lockless shadow walk)
  spte = ACCESS_ONCE(*sptep);

if (!(spte & PT_MAY_ALLOW_WRITES))

        goto slow;

  gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->sptes)
  mark_page_dirty(kvm, gfn);

  new_spte = spte & ~(PT64_MAY_ALLOW_WRITES | PT_WRITABLE_MASK);
  if (cmpxchg(sptep, spte, new_spte) != spte)
       goto slow;

  rcu_read_unlock();
  return;

slow:
rcu_read_unlock();
slow_path();

It now becomes the responsibility of the slow path to maintain *sptep & PT_MAY_ALLOW_WRITES, but that path has a simpler concurrency model. It can be as simple as a clear_bit() before we update sp->gfns[] or if we add host write protection.

Quoted 36 lines · Xiao Guangrong

Foolish me, i should be crazy. Sorry for my mistake. :(

Unfortunately, it can not work, we can not get a stable gfn from gpte or sp->gfns[]. For example:

beginning:
Gpte = Gfn1
gfn_to_pfn(Gfn1) = Pfn
Spte = Pfn
Gfn1 is write-free
Gfn2 is write-protected

VCPU 0 VCPU 1 VCPU 2

fault on gpte
fast page fault path:
set Spte.fast_pf
get Gfn1 from Gpte/sp->gfns[]
if (Gfn1 is writable)
Pfn is swapped out:
Spte = 0
Gpte is modified to Gfn2, and Pfn is realloced and remapped to Gfn2, so:
Spte = Pfn

fast page fault path:
set Spte.fast_pf

cmpxchg Spte+w
OOPS!!!
<we see Spte is not changed and
happily make it writable, so gfn2 can be writable>

Quoted 2 lines

It seems only a unique identification can prevent this. :(

Ouch.

What about restricting this to role.direct=1? Then gfn is stable?

Okay, let's simplify it as possible:

- let it only fix the page fault with PFEC.P == 1 && PFEC.W = 0, that means

Okay, let's simplify it as possible:

- let it only fix the page fault with PFEC.P == 1 && PFEC.W = 0, that means unlock set_spte path can be dropped

- let it just fixes the page fault caused by dirty-log that means we always skip the spte which write-protected by shadow page protection.

Then, things should be fair simper:

In set_spte path, if the spte can be writable, we set ALLOW_WRITE bit In rmap_write_protect:

  if (spte.PT_WRITABLE_MASK) {
	WARN_ON(!(spte & ALLOW_WRITE));
	spte &= ~PT_WRITABLE_MASK;
	spte |= WRITE_PROTECT;

}

in fast page fault:

if (spte & PT_WRITABLE_MASK)

	return_go_guest;

if ((spte & ALLOW_WRITE) && !(spte & WRITE_PROTECT))
cmpxchg spte + PT_WRITABLE_MASK

The information all we needed comes from spte it is independence from other path, and no barriers.

Hmm, how about this one?

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 5 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

This patchset is really worrying to me.

It introduces a lot of concurrency into data structures that were not designed for it. Even if it is correct, it will be very hard to convince ourselves that it is correct, and if it isn't, to debug those subtle bugs. It will also be much harder to maintain the mmu code than it is now.

There are a lot of things to check. Just as an example, we need to be sure that if we use rcu_dereference() twice in the same code path, that any inconsistencies due to a write in between are benign. Doing that is a huge task.

But I appreciate the performance improvement and would like to see a simpler version make it in. This needs to reduce the amount of data touched in the fast path so it is easier to validate, and perhaps reduce the number of cases that the fast path works on.

I would like to see the fast path as simple as

  rcu_read_lock();

  (lockless shadow walk)
  spte = ACCESS_ONCE(*sptep);

if (!(spte & PT_MAY_ALLOW_WRITES))

        goto slow;

  gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->sptes)
  mark_page_dirty(kvm, gfn);

  new_spte = spte & ~(PT64_MAY_ALLOW_WRITES | PT_WRITABLE_MASK);
  if (cmpxchg(sptep, spte, new_spte) != spte)
       goto slow;

  rcu_read_unlock();
  return;

slow:
rcu_read_unlock();
slow_path();

It now becomes the responsibility of the slow path to maintain *sptep & PT_MAY_ALLOW_WRITES, but that path has a simpler concurrency model. It can be as simple as a clear_bit() before we update sp->gfns[] or if we add host write protection.

Sorry, it's too complicated for me. Marcelo, what's your take?

Quoted 61 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 5 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

This patchset is really worrying to me.

It introduces a lot of concurrency into data structures that were not designed for it. Even if it is correct, it will be very hard to convince ourselves that it is correct, and if it isn't, to debug those subtle bugs. It will also be much harder to maintain the mmu code than it is now.

There are a lot of things to check. Just as an example, we need to be sure that if we use rcu_dereference() twice in the same code path, that any inconsistencies due to a write in between are benign. Doing that is a huge task.

But I appreciate the performance improvement and would like to see a simpler version make it in. This needs to reduce the amount of data touched in the fast path so it is easier to validate, and perhaps reduce the number of cases that the fast path works on.

I would like to see the fast path as simple as

  rcu_read_lock();

  (lockless shadow walk)
  spte = ACCESS_ONCE(*sptep);

if (!(spte & PT_MAY_ALLOW_WRITES))

        goto slow;

  gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->sptes)
  mark_page_dirty(kvm, gfn);

  new_spte = spte & ~(PT64_MAY_ALLOW_WRITES | PT_WRITABLE_MASK);
  if (cmpxchg(sptep, spte, new_spte) != spte)
       goto slow;

  rcu_read_unlock();
  return;

slow:
rcu_read_unlock();
slow_path();

Quoted 6 lines

It now becomes the responsibility of the slow path to maintain *sptep & PT_MAY_ALLOW_WRITES, but that path has a simpler concurrency model. It can be as simple as a clear_bit() before we update sp->gfns[] or if we add host write protection.

Sorry, it's too complicated for me. Marcelo, what's your take?

The improvement is small and limited to special cases (migration should be rare and framebuffer memory accounts for a small percentage of total memory).

For one, how can this be safe against mmu notifier methods?

KSM |VCPU0 | VCPU1

		 	      | fault		| fault
			      | cow-page	|
			      |	set spte RW	|
			      |			|
write protect host pte	      |			|
grab mmu_lock		      |			|
remove writeable bit in spte  |			|
increase mmu_notifier_seq     |			|  spte = read-only spte
release mmu_lock	      |			|  cmpxchg succeeds, RO->RW!

MMU notifiers rely on the fault path sequence being

read host pte
read mmu_notifier_seq
spin_lock(mmu_lock)
if (mmu_notifier_seq changed)
goodbye, host pte value is stale
spin_unlock(mmu_lock)

By the example above, you cannot rely on the spte value alone, mmu_notifier_seq must be taken into account.

Quoted 66 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 5 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

This patchset is really worrying to me.

It introduces a lot of concurrency into data structures that were not designed for it. Even if it is correct, it will be very hard to convince ourselves that it is correct, and if it isn't, to debug those subtle bugs. It will also be much harder to maintain the mmu code than it is now.

There are a lot of things to check. Just as an example, we need to be sure that if we use rcu_dereference() twice in the same code path, that any inconsistencies due to a write in between are benign. Doing that is a huge task.

But I appreciate the performance improvement and would like to see a simpler version make it in. This needs to reduce the amount of data touched in the fast path so it is easier to validate, and perhaps reduce the number of cases that the fast path works on.

I would like to see the fast path as simple as

  rcu_read_lock();

  (lockless shadow walk)
  spte = ACCESS_ONCE(*sptep);

if (!(spte & PT_MAY_ALLOW_WRITES))

        goto slow;

  gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->sptes)
  mark_page_dirty(kvm, gfn);

  new_spte = spte & ~(PT64_MAY_ALLOW_WRITES | PT_WRITABLE_MASK);
  if (cmpxchg(sptep, spte, new_spte) != spte)
       goto slow;

  rcu_read_unlock();
  return;

slow:
rcu_read_unlock();
slow_path();

It now becomes the responsibility of the slow path to maintain *sptep & PT_MAY_ALLOW_WRITES, but that path has a simpler concurrency model. It can be as simple as a clear_bit() before we update sp->gfns[] or if we add host write protection.

Quoted 1 line

Sorry, it's too complicated for me. Marcelo, what's your take?

The improvement is small and limited to special cases (migration should be rare and framebuffer memory accounts for a small percentage of total memory).

Quoted 95 lines · Marcelo Tosatti

Quoted 61 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 5 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

This patchset is really worrying to me.

It introduces a lot of concurrency into data structures that were not designed for it. Even if it is correct, it will be very hard to convince ourselves that it is correct, and if it isn't, to debug those subtle bugs. It will also be much harder to maintain the mmu code than it is now.

There are a lot of things to check. Just as an example, we need to be sure that if we use rcu_dereference() twice in the same code path, that any inconsistencies due to a write in between are benign. Doing that is a huge task.

But I appreciate the performance improvement and would like to see a simpler version make it in. This needs to reduce the amount of data touched in the fast path so it is easier to validate, and perhaps reduce the number of cases that the fast path works on.

I would like to see the fast path as simple as

  rcu_read_lock();

  (lockless shadow walk)
  spte = ACCESS_ONCE(*sptep);

if (!(spte & PT_MAY_ALLOW_WRITES))

        goto slow;

  gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->sptes)
  mark_page_dirty(kvm, gfn);

  new_spte = spte & ~(PT64_MAY_ALLOW_WRITES | PT_WRITABLE_MASK);
  if (cmpxchg(sptep, spte, new_spte) != spte)
       goto slow;

  rcu_read_unlock();
  return;

slow:
rcu_read_unlock();
slow_path();

Quoted 6 lines

It now becomes the responsibility of the slow path to maintain *sptep & PT_MAY_ALLOW_WRITES, but that path has a simpler concurrency model. It can be as simple as a clear_bit() before we update sp->gfns[] or if we add host write protection.

Sorry, it's too complicated for me. Marcelo, what's your take?

The improvement is small and limited to special cases (migration should be rare and framebuffer memory accounts for a small percentage of total memory).

For one, how can this be safe against mmu notifier methods?

KSM |VCPU0 | VCPU1

		 	      | fault		| fault
			      | cow-page	|
			      |	set spte RW	|
			      |			|
write protect host pte	      |			|
grab mmu_lock		      |			|
remove writeable bit in spte  |			|
increase mmu_notifier_seq     |			|  spte = read-only spte
release mmu_lock	      |			|  cmpxchg succeeds, RO->RW!

MMU notifiers rely on the fault path sequence being

read host pte
read mmu_notifier_seq
spin_lock(mmu_lock)
if (mmu_notifier_seq changed)
goodbye, host pte value is stale
spin_unlock(mmu_lock)

Quoted 2 lines

By the example above, you cannot rely on the spte value alone, mmu_notifier_seq must be taken into account.

No.

When KSM change the host page to read-only, the HOST_WRITABLE bit of spte should be removed, that means, the spte should be changed that can be watched by cmpxchg.

Note: we mark spte to be writeable only if spte.HOST_WRITABLE is set.

Quoted 67 lines · Marcelo Tosatti

Quoted 66 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 5 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

This patchset is really worrying to me.

It introduces a lot of concurrency into data structures that were not designed for it. Even if it is correct, it will be very hard to convince ourselves that it is correct, and if it isn't, to debug those subtle bugs. It will also be much harder to maintain the mmu code than it is now.

There are a lot of things to check. Just as an example, we need to be sure that if we use rcu_dereference() twice in the same code path, that any inconsistencies due to a write in between are benign. Doing that is a huge task.

But I appreciate the performance improvement and would like to see a simpler version make it in. This needs to reduce the amount of data touched in the fast path so it is easier to validate, and perhaps reduce the number of cases that the fast path works on.

I would like to see the fast path as simple as

  rcu_read_lock();

  (lockless shadow walk)
  spte = ACCESS_ONCE(*sptep);

if (!(spte & PT_MAY_ALLOW_WRITES))

        goto slow;

  gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->sptes)
  mark_page_dirty(kvm, gfn);

  new_spte = spte & ~(PT64_MAY_ALLOW_WRITES | PT_WRITABLE_MASK);
  if (cmpxchg(sptep, spte, new_spte) != spte)
       goto slow;

  rcu_read_unlock();
  return;

slow:
rcu_read_unlock();
slow_path();

It now becomes the responsibility of the slow path to maintain *sptep & PT_MAY_ALLOW_WRITES, but that path has a simpler concurrency model. It can be as simple as a clear_bit() before we update sp->gfns[] or if we add host write protection.

Quoted 5 lines

Quoted 1 line

Sorry, it's too complicated for me. Marcelo, what's your take?

The improvement is small and limited to special cases (migration should be rare and framebuffer memory accounts for a small percentage of total memory).

Actually, although the framebuffer is small but it is modified really frequently, and another unlucky things is that dirty-log is also very frequently and need hold mmu-lock to do write-protect.

Yes, if Xwindow is not enabled, the benefit is limited. :)

Quoted 76 lines · Xiao Guangrong

Quoted 72 lines · Marcelo Tosatti

Quoted 61 lines · Avi Kivity

Quoted 12 lines · Xiao Guangrong

* Idea
The present bit of page fault error code (EFEC.P) indicates whether the page table is populated on all levels, if this bit is set, we can know the page fault is caused by the page-protection bits (e.g. W/R bit) or the reserved bits.

In KVM, in most cases, all this kind of page fault (EFEC.P = 1) can be simply fixed: the page fault caused by reserved bit
(EFFC.P = 1 && EFEC.RSV = 1) has already been filtered out in fast mmio path. What we need do to fix the rest page fault (EFEC.P = 1 && RSV != 1) is just increasing the corresponding access on the spte.

Quoted 5 lines

This pachset introduces a fast path to fix this kind of page fault: it is out of mmu-lock and need not walk host page table to get the mapping from gfn to pfn.

This patchset is really worrying to me.

It introduces a lot of concurrency into data structures that were not designed for it. Even if it is correct, it will be very hard to convince ourselves that it is correct, and if it isn't, to debug those subtle bugs. It will also be much harder to maintain the mmu code than it is now.

There are a lot of things to check. Just as an example, we need to be sure that if we use rcu_dereference() twice in the same code path, that any inconsistencies due to a write in between are benign. Doing that is a huge task.

But I appreciate the performance improvement and would like to see a simpler version make it in. This needs to reduce the amount of data touched in the fast path so it is easier to validate, and perhaps reduce the number of cases that the fast path works on.

I would like to see the fast path as simple as

  rcu_read_lock();

  (lockless shadow walk)
  spte = ACCESS_ONCE(*sptep);

if (!(spte & PT_MAY_ALLOW_WRITES))

        goto slow;

  gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->sptes)
  mark_page_dirty(kvm, gfn);

  new_spte = spte & ~(PT64_MAY_ALLOW_WRITES | PT_WRITABLE_MASK);
  if (cmpxchg(sptep, spte, new_spte) != spte)
       goto slow;

  rcu_read_unlock();
  return;

slow:
rcu_read_unlock();
slow_path();

Quoted 6 lines

It now becomes the responsibility of the slow path to maintain *sptep & PT_MAY_ALLOW_WRITES, but that path has a simpler concurrency model. It can be as simple as a clear_bit() before we update sp->gfns[] or if we add host write protection.

Sorry, it's too complicated for me. Marcelo, what's your take?

The improvement is small and limited to special cases (migration should be rare and framebuffer memory accounts for a small percentage of total memory).

Quoted 5 lines

Actually, although the framebuffer is small but it is modified really frequently, and another unlucky things is that dirty-log is also very frequently and need hold mmu-lock to do write-protect.

Yes, if Xwindow is not enabled, the benefit is limited. :)

Ignoring that fact, the safety of lockless set_spte and friends is not clear.

Perhaps the mmu_lock hold times by get_dirty are a large component here? If that can be alleviated, not only RO->RW faults benefit.

Perhaps the mmu_lock hold times by get_dirty are a large component here?

If that can be alleviated, not only RO->RW faults benefit.

Yes, if Xwindow is not enabled, the benefit is limited. :)

Perhaps the mmu_lock hold times by get_dirty are a large component here? If that can be alleviated, not only RO->RW faults benefit.

Quoted 4 lines · Marcelo Tosatti

Perhaps the mmu_lock hold times by get_dirty are a large component here? If that can be alleviated, not only RO->RW faults benefit.

Currently the longest holder in normal use is probably reading the dirty log and write protecting the shadow page tables.

We could fix that by switching to O(1) write protection (write-protecting PML4Es instead of PTEs). It would be interesting to combine O(1) write protection with lockless write-enabling.

Quoted 6 lines · On Tue, 10 Apr 2012 13:39:14 +0300 Avi Kivity <[email protected]> wrote:

Quoted 4 lines · Marcelo Tosatti

Perhaps the mmu_lock hold times by get_dirty are a large component here? If that can be alleviated, not only RO->RW faults benefit.

Quoted 7 lines

Currently the longest holder in normal use is probably reading the dirty log and write protecting the shadow page tables.

We could fix that by switching to O(1) write protection (write-protecting PML4Es instead of PTEs). It would be interesting to combine O(1) write protection with lockless write-enabling.

As Marcelo suggested during reviewing srcu-less dirty logging, we can mitigate the get_dirty's mmu_lock hold time problem cleanly, locally in get_dirty_log(), by using cond_resched_lock() -- although we need to introduce cond_rescheck_lock_cb() to conditionally flush TLB.

I have already started that work.

Actually I introduced rmap based get_dirty for that kind of fine-grained contention control.

I think we should do our best not to affect mmu so much just for the limited time of live migration.

Quoted 1 line · Marcelo Tosatti

Perhaps the mmu_lock hold times by get_dirty are a large component here?

That's my concern, because it affects the scaling of migration for wider guests.

Quoted 1 line

If that can be alleviated, not only RO->RW faults benefit.

Those are the most common types of faults on modern hardware, no?

Quoted 6 lines · Avi Kivity

Quoted 1 line · Marcelo Tosatti

Perhaps the mmu_lock hold times by get_dirty are a large component here?

That's my concern, because it affects the scaling of migration for wider guests.

Quoted 3 lines

Quoted 1 line

If that can be alleviated, not only RO->RW faults benefit.

Those are the most common types of faults on modern hardware, no?

Depends on your workload, of course. If there is memory pressure, 0->PRESENT might be very frequent. My point is that reduction of mmu_lock contention is a good thing overall.

No, i do not really agree with that.

We really can get great benefit from O(1) especially if lockless write-protect is introduced for O(1), live migration is very useful for cloud computing architecture to balance the overload on all nodes.

Quoted 5 lines · On Tue, 10 Apr 2012 19:58:44 +0800 Xiao Guangrong…

No, i do not really agree with that.

We really can get great benefit from O(1) especially if lockless write-protect is introduced for O(1), live migration is very useful for cloud computing architecture to balance the overload on all nodes.

Recently, you said to me that you were not familiar with live migration. Actually you did not know the basics of pre-copy live migration.

I know live migration better than you because NTT has Kemari and it uses live migration infrastructure. My work is originated from the data I got during profiling Kemari.

SRCU-less dirty logging was also motivated by the pressures from scheduler developers. Everything was really needed.

Have you ever used live migration for real service?

I cannot say whether O(1) is OK with me without any real background.

Well, my point is that live migration is so very useful that it is worth to be improved, the description of your also proves this point.

What is your really want to say but i missed?

Okay, let us to compare the performance number after O(1) implemented.

From my experience, I want to say that live migration is very difficult

Quoted 4 lines · On Wed, 11 Apr 2012 20:38:57 +0800 Xiao Guangrong…

Well, my point is that live migration is so very useful that it is worth to be improved, the description of your also proves this point.

What is your really want to say but i missed?

How to improve and what we should pay for that.

Note that I am not objecting to O(1) itself.

Do you remember that when we discussed O(1) issue last year, with Avi, the agreement was that we should take more time and look carefully with more measurements to confirm if it's really worthwhile.

The point is whether we should do O(1) now, including near future.

My opinion is that we should do what we can do now and wait for feedback from real users.

Before making the current code stable, I do not want to see it replaced so dramatically. Otherwise when can we use live migration with enough confidence? There may be another subtle bugs we should fix now.

In addition, XBRZLE and post-copy is now being developed in QEMU.

What do you think about this Avi, Marcelo?

I am testing the current live migration to see when and for what it can be used. I really want to see it become stable and usable for real services.

Quoted 1 line

Okay, let us to compare the performance number after O(1) implemented.

From my experience, I want to say that live migration is very difficult to say about performance. That is the problem I am now struggling with.

I developed dirty-log-perf unit-test for that but that was not enough.

Needless to say, checking the correctness is harder.

So I really do not want to see drastic change now without any real need or feedback from real users -- this is my point.

Currently the main performance bottleneck for migration is qemu, which is single threaded and generally inefficient. However I am sure that once the qemu bottlenecks will be removed we'll encounter kvm problems, particularly with wide (many vcpus) and large (lots of memory) guests. So it's a good idea to improve in this area. I agree we'll need to measure each change, perhaps with a test program until qemu catches up.

Quoted 3 lines

I am testing the current live migration to see when and for what it can be used. I really want to see it become stable and usable for real services.

Well, it's used in production now.

Quoted 2 lines

So I really do not want to see drastic change now without any real need or feedback from real users -- this is my point.

It's a good point, we should avoid change for its own sake.

Hi Avi,

Thanks very much for your review!

Sorry for the delay reply since i was on vacation.

Quoted 19 lines · Avi Kivity

Quoted 7 lines · Xiao Guangrong

Using PTE_LIST_WRITE_PROTECT bit in rmap store the write-protect status to avoid unnecessary shadow page walking

Also if no shadow page is indirect, the page is write-free

arch/x86/kvm/mmu.c static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn, · line 2297

Quoted 7 lines

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

@@ -2262,6 +2291,9 @@ static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 	}
 	if (need_unsync)
 		kvm_unsync_pages(vcpu, gfn);
+
+	*rmap &= ~PTE_LIST_WRITE_PROTECT;
+
 

So what are the rules for PTE_LIST_WRITE_PROTECT? Is is a cache for the mmu_need_write_protect?

Quoted 3 lines

I'd like to understand it, I guess it can be set while write protection is unneeded, and cleared on the next check?