Fix accessed bit tracking

Since we need to make the check atomic, move it to the place that will set the new spte.

Signed-off-by Avi Kivity <[email protected]>

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

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

index 16cedc9..b5a2d3d 100644
@@ -620,19 +620,11 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 	struct kvm_rmap_desc *desc;
 	struct kvm_rmap_desc *prev_desc;
 	struct kvm_mmu_page *sp;
-	pfn_t pfn;
 	gfn_t gfn;
 	unsigned long *rmapp;
 	int i;
 
-	if (!is_rmap_spte(*spte))
-		return;
 	sp = page_header(__pa(spte));
-	pfn = spte_to_pfn(*spte);
-	if (*spte & shadow_accessed_mask)
-		kvm_set_pfn_accessed(pfn);
-	if (is_writable_pte(*spte))
-		kvm_set_pfn_dirty(pfn);
 	gfn = kvm_mmu_page_get_gfn(sp, spte - sp->spt);
 	rmapp = gfn_to_rmap(kvm, gfn, sp->role.level);
 	if (!*rmapp) {

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

+	pfn = spte_to_pfn(*sptep);
+	if (*sptep & shadow_accessed_mask)
+		kvm_set_pfn_accessed(pfn);
+	if (is_writable_pte(*sptep))
+		kvm_set_pfn_dirty(pfn);
 	rmap_remove(kvm, sptep);
 	__set_spte(sptep, new_spte);
 }

Quoted 34 lines · Avi Kivity

Since we need to make the check atomic, move it to the place that will set the new spte.

Signed-off-by Avi Kivity<[email protected]>

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

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

index 16cedc9..b5a2d3d 100644
@@ -620,19 +620,11 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
  	struct kvm_rmap_desc *desc;
  	struct kvm_rmap_desc *prev_desc;
  	struct kvm_mmu_page *sp;
-	pfn_t pfn;
  	gfn_t gfn;
  	unsigned long *rmapp;
  	int i;
 
-	if (!is_rmap_spte(*spte))
-		return;
  	sp = page_header(__pa(spte));
-	pfn = spte_to_pfn(*spte);
-	if (*spte&  shadow_accessed_mask)
-		kvm_set_pfn_accessed(pfn);
-	if (is_writable_pte(*spte))
-		kvm_set_pfn_dirty(pfn);
  	gfn = kvm_mmu_page_get_gfn(sp, spte - sp->spt);
  	rmapp = gfn_to_rmap(kvm, gfn, sp->role.level);
  	if (!*rmapp) {

@@ -668,6 +660,15 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 

arch/x86/kvm/mmu.c static void rmap_remove(struct kvm *kvm, u64 *spte)

Quoted 7 lines

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

  static void drop_spte(struct kvm *kvm, u64 *sptep, u64 new_spte)
  {
+	pfn_t pfn;
+
+	if (!is_rmap_spte(*sptep))
+		return;
 

Missing __set_spte(sptep, new_spte); ?

Avi Kivity wrote:

arch/x86/kvm/mmu.c static void __set_spte(u64 *sptep, u64 spte) · line 304

Quoted 13 lines

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

+static u64 __xchg_spte(u64 *sptep, u64 new_spte)
+{
+#ifdef CONFIG_X86_64
+	return xchg(sptep, new_spte);
+#else
+	u64 old_spte;
+
+	do {
+		old_spte = *sptep;
+	} while (cmpxchg64(sptep, old_spte, new_spte) != old_spte);
+
+	return old;

it's a typo: 'old' -> 'old_spte' :-)

The kvm mmu synchronizes shadow ptes using the mmu lock, however the cpu will happily ignore the lock when setting the accessed bit. This can cause the accessed bit to be lost. Luckily this only results in incorrect page selection for swap.

Quoted 6 lines · Avi Kivity

The kvm mmu synchronizes shadow ptes using the mmu lock, however the cpu will happily ignore the lock when setting the accessed bit. This can cause the accessed bit to be lost. Luckily this only results in incorrect page selection for swap.

Atomic operation is heavy and slow, it hurts performance. Incorrect page selection for swap also hurts performance.

I think there are very rare competitions happened and cause the accessed bit to be lost. Since there is no incorrect result when the accessed bit is lost. Is this problem over concern?

Quoted 10 lines · Lai Jiangshan

Quoted 8 lines · Avi Kivity

The kvm mmu synchronizes shadow ptes using the mmu lock, however the cpu will happily ignore the lock when setting the accessed bit. This can cause
the accessed bit to be lost. Luckily this only results in incorrect page
selection for swap.

Quoted 3 lines

Atomic operation is heavy and slow, it hurts performance. Incorrect page selection for swap also hurts performance.

We can avoid the exchange in most cases, for example if the new spte has the accessed bit set (already in the patch set) or if the page is already marked as accessed, or if we see the old spte has the accessed bit set (so no race can occur). I'll update the patches to avoid atomics when possible.

I don't think atomics are that expensive, though, ~20 cycles on modern processors?

Quoted 7 lines

We can avoid the exchange in most cases, for example if the new spte has the accessed bit set (already in the patch set) or if the page is already marked as accessed, or if we see the old spte has the accessed bit set (so no race can occur). I'll update the patches to avoid atomics when possible.

Umm, the reason that we need atomics here is to avoid vcpu to update spte when we read A bit form it, so, perhaps we can use below way to avoid atomics completely:

- set reserved bit in spte
- get A bit form spte
- set new spte

the worst case is cause vcpu #PF here, but it doesn't matter since the old mapping is already invalid, also need a remote tlb flush later.

Quoted 4 lines

I don't think atomics are that expensive, though, ~20 cycles on modern processors?

Yes, but atomics are "LOCK" instructions, it can stop multiple cpus runing in parallel.

Quoted 15 lines · Xiao Guangrong

Quoted 7 lines

We can avoid the exchange in most cases, for example if the new spte has the accessed bit set (already in the patch set) or if the page is already marked as accessed, or if we see the old spte has the accessed bit set (so no race can occur). I'll update the patches to avoid atomics when possible.

Umm, the reason that we need atomics here is to avoid vcpu to update spte when we read A bit
form it, so, perhaps we can use below way to avoid atomics completely:

- set reserved bit in spte
- get A bit form spte
- set new spte

Quoted 4 lines

the worst case is cause vcpu #PF here, but it doesn't matter since the old mapping is already invalid,
also need a remote tlb flush later.

To set the reserved bit in the spte, you need an atomic operation (well, unless you use a sub-word-acccess to set a reserved bit in the high 32 bits).

Quoted 8 lines

Quoted 5 lines

I don't think atomics are that expensive, though, ~20 cycles on modern processors?

Yes, but atomics are "LOCK" instructions, it can stop multiple cpus runing in parallel.

Only if those cpus are accessing the same word you're accessing.

Quoted 23 lines · Avi Kivity

Quoted 21 lines · Xiao Guangrong

Quoted 8 lines

We can avoid the exchange in most cases, for example if the new spte has the accessed bit set (already in the patch set) or if the page is already marked as accessed, or if we see the old spte has the accessed bit set (so no race can occur). I'll update the patches to avoid atomics when possible.

Umm, the reason that we need atomics here is to avoid vcpu to update spte when we read A bit
form it, so, perhaps we can use below way to avoid atomics completely:

- set reserved bit in spte
- get A bit form spte
- set new spte

the worst case is cause vcpu #PF here, but it doesn't matter since the old mapping is already invalid,
also need a remote tlb flush later.

Quoted 4 lines

To set the reserved bit in the spte, you need an atomic operation (well, unless you use a sub-word-acccess to set a reserved bit in the high 32 bits).

I think we no need atomic here, for example, we can do it like this:

*spte |= RSVD_BIT
[ maybe need a write barrier here? ]

After this sentence completed, we can ensure that the spte can not updated A bit by vcpu, so we can get A bit safely.

Quoted 7 lines

Quoted 4 lines

Yes, but atomics are "LOCK" instructions, it can stop multiple cpus runing in parallel.

Only if those cpus are accessing the same word you're accessing.

Oh, you are right, the LOCK only locked the memory defined by the destination operand, but i also recall that page table access can pass LOCK instruction, below description is form intel' spec Vol. 3 7-5:

Locked operations are atomic with respect to all other memory operations and all externally visible events. Only instruction fetch and page table accesses can pass locked instructions. Locked instructions can be used to synchronize data written by one processor and read by another processor.

Quoted 27 lines · Xiao Guangrong

Quoted 24 lines · Avi Kivity

Quoted 22 lines · Xiao Guangrong

Quoted 9 lines

We can avoid the exchange in most cases, for example if the new spte has
the accessed bit set (already in the patch set) or if the page is already marked as accessed, or if we see the old spte has the accessed bit set (so no race can occur). I'll update the patches to avoid atomics when possible.

Umm, the reason that we need atomics here is to avoid vcpu to update spte when we read A bit
form it, so, perhaps we can use below way to avoid atomics completely:

- set reserved bit in spte
- get A bit form spte
- set new spte

the worst case is cause vcpu #PF here, but it doesn't matter since the old mapping is already invalid,
also need a remote tlb flush later.

Quoted 9 lines

Quoted 4 lines

To set the reserved bit in the spte, you need an atomic operation (well, unless you use a sub-word-acccess to set a reserved bit in the high 32 bits).

I think we no need atomic here, for example, we can do it like this:

*spte |= RSVD_BIT
[ maybe need a write barrier here? ]

That can drop an A bit. If *spte starts out with A cleared, we can have

cpu0 cpu1

fetch *spte (A=0)
set A bit
write *spte (A=0, RSVD=1)

Quoted 5 lines

After this sentence completed, we can ensure that the spte can not updated A bit
by vcpu, so we can get A bit safely.

You also need a remote tlb flush...

Quoted 14 lines

Quoted 8 lines

Quoted 5 lines

Yes, but atomics are "LOCK" instructions, it can stop multiple cpus runing in parallel.

Only if those cpus are accessing the same word you're accessing.

Oh, you are right, the LOCK only locked the memory defined by the destination operand,
but i also recall that page table access can pass LOCK instruction, below description
is form intel' spec Vol. 3 7-5:

Quoted 8 lines

Locked operations are atomic with respect to all other memory operations and all externally
visible events. Only instruction fetch and page table accesses can pass locked instructions.
Locked instructions can be used to synchronize data written by one processor and read by another
processor.

But actually setting the A bit will use LOCK itself. So in the following sequence

write pte (A=0)
test_and_clear_bit(A, pte)
access memory through pte

the test_and_clear_bit can return A=1 due to speculation and the parapgraph above, but setting the A bit by the processor will happen with a bus lock, so it won't lose information.