KVM: MMU: avoid pte_list_desc run out in kvm_mmu_pte_write

PATCH Superseded Xiao Guangrong 3 participants 23 messages Aug 2, 2011 – Aug 3, 2011

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Xiao Guangrong

author

2011-08-02 11:04 UTC #0 lore raw

PATCH v2 1/12 KVM: MMU: avoid pte_list_desc run out in kvm_mmu_pte_write

kvm_mmu_pte_write is unsafe since we need to alloc pte_list_desc in the function when spte is prefetched, unfortunately, we can not know how many spte need to be prefetched on this path, that means we can use out of the free pte_list_desc object in the cache, and BUG_ON() is triggered, also some path does not fill the cache, such as INS instruction emulated that does not trigger page fault

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

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

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

index 9335e1b..28991b2 100644
@@ -593,6 +593,11 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
 	return 0;
 }
 
+static int mmu_memory_cache_free_objects(struct kvm_mmu_memory_cache *cache)
+{
+	return cache->nobjs;
+}
+
 static void mmu_free_memory_cache(struct kvm_mmu_memory_cache *mc,
 				  struct kmem_cache *cache)
 {

@@ -970,6 +975,14 @@ static unsigned long *gfn_to_rmap(struct kvm *kvm, gfn_t gfn, int level)
 	return &linfo->rmap_pde;
 }
 
+static bool rmap_can_add(struct kvm_vcpu *vcpu)
+{
+	struct kvm_mmu_memory_cache *cache;
+
+	cache = &vcpu->arch.mmu_pte_list_desc_cache;
+	return mmu_memory_cache_free_objects(cache);
+}
+
 static int rmap_add(struct kvm_vcpu *vcpu, u64 *spte, gfn_t gfn)
 {
 	struct kvm_mmu_page *sp;

@@ -3583,6 +3596,12 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		break;
 	}
 
+	/*
+	 * No need to care whether allocation memory is successful
+	 * or not since pte prefetch is skiped if it does not have
+	 * enough objects in the cache.
+	 */
+	mmu_topup_memory_caches(vcpu);
 	spin_lock(&vcpu->kvm->mmu_lock);
 	if (atomic_read(&vcpu->kvm->arch.invlpg_counter) != invlpg_counter)
 		gentry = 0;

@@ -3653,7 +3672,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 			mmu_page_zap_pte(vcpu->kvm, sp, spte);
 			if (gentry &&
 			      !((sp->role.word ^ vcpu->arch.mmu.base_role.word)
-			      & mask.word))
+			      & mask.word) && rmap_can_add(vcpu))
 				mmu_pte_write_new_pte(vcpu, sp, spte, &gentry);
 			if (!remote_flush && need_remote_flush(entry, *spte))
 				remote_flush = true;

@@ -3714,10 +3733,6 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
 		goto out;
 	}
 
-	r = mmu_topup_memory_caches(vcpu);
-	if (r)
-		goto out;
-
 	er = x86_emulate_instruction(vcpu, cr2, 0, insn, insn_len);
 
 	switch (er) {

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:05 UTC #1 lore raw

PATCH v2 2/12 KVM: x86: tag the instructions which are used to write page table

The idea is from Avi:

| tag instructions that are typically used to modify the page tables, and drop
| shadow if any other instruction is used
| The list would include, I'd guess, and, or, bts, btc, mov, xchg, cmpxchg, and
| cmpxchg8b

This patch is used to tag the instructions and in the later path, shadow page is dropped if it is written by other instructions

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

---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |    8 ++++++++
 arch/x86/kvm/x86.c                 |    1 +
 arch/x86/kvm/x86.h                 |    5 +++++
 4 files changed, 15 insertions(+), 0 deletions(-)

arch/x86/include/asm/kvm_emulate.h +1 -0

index 6040d11..049a6f5 100644
@@ -244,6 +244,7 @@ struct x86_emulate_ctxt {
 	bool guest_mode; /* guest running a nested guest */
 	bool perm_ok; /* do not check permissions if true */
 	bool only_vendor_specific_insn;
+	bool page_table_written_insn;
 
 	bool have_exception;
 	struct x86_exception exception;

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

index 0453c07..3c027ac 100644
@@ -2516,6 +2516,7 @@ static int em_add(struct x86_emulate_ctxt *ctxt)
 static int em_or(struct x86_emulate_ctxt *ctxt)
 {
 	emulate_2op_SrcV("or", ctxt->src, ctxt->dst, ctxt->eflags);
+	tag_page_table_written_insn(ctxt);
 	return X86EMUL_CONTINUE;
 }

@@ -2534,6 +2535,7 @@ static int em_sbb(struct x86_emulate_ctxt *ctxt)
 static int em_and(struct x86_emulate_ctxt *ctxt)
 {
 	emulate_2op_SrcV("and", ctxt->src, ctxt->dst, ctxt->eflags);
+	tag_page_table_written_insn(ctxt);
 	return X86EMUL_CONTINUE;
 }

@@ -2572,6 +2574,7 @@ static int em_xchg(struct x86_emulate_ctxt *ctxt)
 	/* Write back the memory destination with implicit LOCK prefix. */
 	ctxt->dst.val = ctxt->src.orig_val;
 	ctxt->lock_prefix = 1;
+	tag_page_table_written_insn(ctxt);
 	return X86EMUL_CONTINUE;
 }

@@ -2610,6 +2613,7 @@ static int em_rdtsc(struct x86_emulate_ctxt *ctxt)
 static int em_mov(struct x86_emulate_ctxt *ctxt)
 {
 	ctxt->dst.val = ctxt->src.val;
+	tag_page_table_written_insn(ctxt);
 	return X86EMUL_CONTINUE;
 }

@@ -4135,6 +4139,7 @@ twobyte_insn:
 		break;
 	case 0xab:
 	      bts:		/* bts */
+		tag_page_table_written_insn(ctxt);
 		emulate_2op_SrcV_nobyte("bts", ctxt->src, ctxt->dst, ctxt->eflags);
 		break;
 	case 0xac: /* shrd imm8, r, r/m */

@@ -4148,6 +4153,7 @@ twobyte_insn:
 		 * Save real source value, then compare EAX against
 		 * destination.
 		 */
+		tag_page_table_written_insn(ctxt);
 		ctxt->src.orig_val = ctxt->src.val;
 		ctxt->src.val = ctxt->regs[VCPU_REGS_RAX];
 		emulate_2op_SrcV("cmp", ctxt->src, ctxt->dst, ctxt->eflags);

@@ -4192,6 +4198,7 @@ twobyte_insn:
 		break;
 	case 0xbb:
 	      btc:		/* btc */
+		tag_page_table_written_insn(ctxt);
 		emulate_2op_SrcV_nobyte("btc", ctxt->src, ctxt->dst, ctxt->eflags);
 		break;
 	case 0xbc: {		/* bsf */

@@ -4235,6 +4242,7 @@ twobyte_insn:
 							(u64) ctxt->src.val;
 		break;
 	case 0xc7:		/* Grp9 (cmpxchg8b) */
+		tag_page_table_written_insn(ctxt);
 		rc = em_grp9(ctxt);
 		break;
 	default:

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

index ea8f9f0..cf6fb29 100644
@@ -4831,6 +4831,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 		ctxt->interruptibility = 0;
 		ctxt->have_exception = false;
 		ctxt->perm_ok = false;
+		ctxt->page_table_written_insn = false;
 
 		ctxt->only_vendor_specific_insn
 			= emulation_type & EMULTYPE_TRAP_UD;

arch/x86/kvm/x86.h +5 -0

index d36fe23..b6e868f 100644
@@ -111,6 +111,11 @@ static inline bool vcpu_match_mmio_gpa(struct kvm_vcpu *vcpu, gpa_t gpa)
 	return false;
 }
 
+static inline void tag_page_table_written_insn(struct x86_emulate_ctxt *ctxt)
+{
+	ctxt->page_table_written_insn = true;
+}
+
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
 int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);

Signature · 1 line

1.7.5.4

Avi Kivity

2011-08-02 12:20 UTC #2 lore raw

Re: [PATCH v2 02/12] KVM: x86: tag the instructions which are used to write page table

Quoted 8 lines · Xiao Guangrong

The idea is from Avi:
| tag instructions that are typically used to modify the page tables, and drop
| shadow if any other instruction is used
| The list would include, I'd guess, and, or, bts, btc, mov, xchg, cmpxchg, and
| cmpxchg8b
This patch is used to tag the instructions and in the later path, shadow page is dropped if it is written by other instructions

We already have a mechanism for this, the decode tables. Why not add a new flag, PageTable, and set it on all relevant instructions?

Note we don't need to actually emulate, just decode, since page_fault can tell us whether a write failed due to page tables or mmio.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-08-03 06:00 UTC #3 lore raw

Re: [PATCH v2 02/12] KVM: x86: tag the instructions which are used to write page table

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Xiao Guangrong

The idea is from Avi:

| tag instructions that are typically used to modify the page tables, and drop
| shadow if any other instruction is used
| The list would include, I'd guess, and, or, bts, btc, mov, xchg, cmpxchg, and
| cmpxchg8b

Quoted 5 lines

Quoted 2 lines
This patch is used to tag the instructions and in the later path, shadow page is dropped if it is written by other instructions
We already have a mechanism for this, the decode tables. Why not add a new flag, PageTable, and set it on all relevant instructions?

OK, will do it in the next version.

Quoted 2 lines

Note we don't need to actually emulate, just decode, since page_fault can tell us whether a write failed due to page tables or mmio.

This is a interesting feature. If it happens, i will just drop the shadow pages and retry these instructions directly.

Avi Kivity

2011-08-03 08:09 UTC #4 lore raw

Re: [PATCH v2 02/12] KVM: x86: tag the instructions which are used to write page table

Quoted 5 lines · Xiao Guangrong

Quoted 2 lines
Note we don't need to actually emulate, just decode, since page_fault can tell us whether a write failed due to page tables or mmio.
This is a interesting feature. If it happens, i will just drop the shadow pages and retry these instructions directly.

Note it's a little dangerous. If the guest uses a non-page-table modifying instruction on the PDE that points to the instruction, then we will unmap the instruction and go to an infinite loop.

Maybe it's better to emulate if we can't find a fix for that.

One way would be to emulate every 20 instructions; this breaks us out of the loop but reduces costly emulations to 5%.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-08-03 09:22 UTC #5 lore raw

Re: [PATCH v2 02/12] KVM: x86: tag the instructions which are used to write page table

Hi Avi,

Quoted 9 lines · Avi Kivity

Quoted 5 lines · Xiao Guangrong
Quoted 2 lines
Note we don't need to actually emulate, just decode, since page_fault can tell us whether a write failed due to page tables or mmio.
This is a interesting feature. If it happens, i will just drop the shadow pages and retry these instructions directly.
Note it's a little dangerous. If the guest uses a non-page-table modifying instruction on the PDE that points to the instruction, then we will unmap the instruction and go to an infinite loop.

Yes, it is.

Quoted 4 lines

Maybe it's better to emulate if we can't find a fix for that.
One way would be to emulate every 20 instructions; this breaks us out of the loop but reduces costly emulations to 5%.

After much thought about this, may be this optimization is not good since:
- it is little complex
- this optimization is only applied to the instruction emulation caused by #PF
- it does not improve too much:
if we emulate the instruction, we need to do:
- decode instruction
- emulate it
- zap shadow pages
And do this, it can return to the guest, the guest can run the next instruction

if we retry the instruction, we need to do:
- decode instruction
- zap shadow pages
then return to the guest and retry the instruction, however, we will get page fault again(since the mapping is still read-only), so we will get another VM-exit and need to do:

  # trigger page fault
  - handle the page fault and change the mapping to writable
  - retry the instruction
  until now, the guest can run the next instruction

So, i do not think the new way is better, your opinion？

Avi Kivity

2011-08-03 09:25 UTC #6 lore raw

Re: [PATCH v2 02/12] KVM: x86: tag the instructions which are used to write page table

Quoted 21 lines · Xiao Guangrong

Quoted 4 lines
Maybe it's better to emulate if we can't find a fix for that.
One way would be to emulate every 20 instructions; this breaks us out of the loop but reduces costly emulations to 5%.
After much thought about this, may be this optimization is not good since:
- it is little complex
- this optimization is only applied to the instruction emulation caused by #PF
- it does not improve too much:
if we emulate the instruction, we need to do:
- decode instruction
- emulate it
- zap shadow pages
And do this, it can return to the guest, the guest can run the next instruction
if we retry the instruction, we need to do:
- decode instruction
- zap shadow pages
then return to the guest and retry the instruction, however, we will get page fault again(since the mapping is still read-only), so we will get another VM-exit and need to do:

Quoted 6 lines

   # trigger page fault
   - handle the page fault and change the mapping to writable
   - retry the instruction
   until now, the guest can run the next instruction

So, i do not think the new way is better, your opinion？

We can change to writeable and zap in the same exit, no? Basically call page_fault() again after zapping.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-08-03 09:39 UTC #7 lore raw

Re: [PATCH v2 02/12] KVM: x86: tag the instructions which are used to write page table

Quoted 27 lines · Avi Kivity

Quoted 26 lines · Xiao Guangrong
Quoted 4 lines
Maybe it's better to emulate if we can't find a fix for that.
One way would be to emulate every 20 instructions; this breaks us out of the loop but reduces costly emulations to 5%.
After much thought about this, may be this optimization is not good since:
- it is little complex
- this optimization is only applied to the instruction emulation caused by #PF
- it does not improve too much:
if we emulate the instruction, we need to do:
- decode instruction
- emulate it
- zap shadow pages
And do this, it can return to the guest, the guest can run the next instruction
if we retry the instruction, we need to do:
- decode instruction
- zap shadow pages
then return to the guest and retry the instruction, however, we will get page fault again(since the mapping is still read-only), so we will get another VM-exit and need to do:
   # trigger page fault
   - handle the page fault and change the mapping to writable
   - retry the instruction
   until now, the guest can run the next instruction

Quoted 4 lines

Quoted 1 line
So, i do not think the new way is better, your opinion？
We can change to writeable and zap in the same exit, no? Basically call page_fault() again after zapping.

OK. :-)

Um, how about cache the last eip when we do this optimization, if we meet the same eip, we can break out the potential infinite loop?

Avi Kivity

2011-08-03 09:40 UTC #8 lore raw

Re: [PATCH v2 02/12] KVM: x86: tag the instructions which are used to write page table

Quoted 4 lines · Xiao Guangrong

OK. :-)
Um, how about cache the last eip when we do this optimization, if we meet the same eip, we can break out the potential infinite loop?

Yes, that can work.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-08-02 11:06 UTC #9 lore raw

PATCH v2 3/12 zap sp if it is written by unaware instructions

In reply to Xiao Guangrong

Drop the shadow page if it is written by the instructions which is not typically used to modify the page table

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

---
 arch/x86/include/asm/kvm_host.h |    4 ++--
 arch/x86/kvm/mmu.c              |    6 +++---
 arch/x86/kvm/paging_tmpl.h      |    3 ++-
 arch/x86/kvm/x86.c              |   11 +++++++----
 4 files changed, 14 insertions(+), 10 deletions(-)

arch/x86/include/asm/kvm_host.h +2 -2

index 307e3cf..a386755 100644
@@ -664,7 +664,7 @@ void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
 int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3);
 
 int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
-			  const void *val, int bytes);
+			  const void *val, int bytes, bool page_table_written);
 int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 		  gpa_t addr, unsigned long *ret);
 u8 kvm_get_guest_memory_type(struct kvm_vcpu *vcpu, gfn_t gfn);

@@ -750,7 +750,7 @@ int fx_init(struct kvm_vcpu *vcpu);
 void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu);
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes,
-		       bool guest_initiated);
+		       bool guest_initiated, bool page_table_written);
 int kvm_mmu_unprotect_page_virt(struct kvm_vcpu *vcpu, gva_t gva);
 void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu);
 int kvm_mmu_load(struct kvm_vcpu *vcpu);

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

index 28991b2..82d5d5a 100644
@@ -3541,7 +3541,7 @@ static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, gfn_t gfn)
 
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes,
-		       bool guest_initiated)
+		       bool guest_initiated, bool page_table_written)
 {
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	union kvm_mmu_page_role mask = { .word = 0 };

@@ -3627,7 +3627,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		pte_size = sp->role.cr4_pae ? 8 : 4;
 		misaligned = (offset ^ (offset + bytes - 1)) & ~(pte_size - 1);
 		misaligned |= bytes < 4;
-		if (misaligned || flooded) {
+		if (misaligned || flooded || !page_table_written) {
 			/*
 			 * Misaligned accesses are too much trouble to fix
 			 * up; also, they usually indicate a page is not used

@@ -4008,7 +4008,7 @@ static int kvm_pv_mmu_write(struct kvm_vcpu *vcpu,
 	if (r)
 		return r;
 
-	if (!emulator_write_phys(vcpu, addr, &value, bytes))
+	if (!emulator_write_phys(vcpu, addr, &value, bytes, true))
 		return -EFAULT;
 
 	return 1;

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

index 507e2b8..5edbba6 100644
@@ -710,7 +710,8 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 
 	if (mmu_topup_memory_caches(vcpu))
 		return;
-	kvm_mmu_pte_write(vcpu, pte_gpa, NULL, sizeof(pt_element_t), 0);
+	kvm_mmu_pte_write(vcpu, pte_gpa, NULL, sizeof(pt_element_t),
+			  false, true);
 }
 
 static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr, u32 access,

arch/x86/kvm/x86.c +7 -4

index cf6fb29..fbb3a44 100644
@@ -4058,14 +4058,14 @@ static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
 }
 
 int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
-			const void *val, int bytes)
+			const void *val, int bytes, bool page_table_written)
 {
 	int ret;
 
 	ret = kvm_write_guest(vcpu->kvm, gpa, val, bytes);
 	if (ret < 0)
 		return 0;
-	kvm_mmu_pte_write(vcpu, gpa, val, bytes, 1);
+	kvm_mmu_pte_write(vcpu, gpa, val, bytes, true, page_table_written);
 	return 1;
 }

@@ -4103,7 +4103,10 @@ static int read_emulate(struct kvm_vcpu *vcpu, gpa_t gpa,
 static int write_emulate(struct kvm_vcpu *vcpu, gpa_t gpa,
 			 void *val, int bytes)
 {
-	return emulator_write_phys(vcpu, gpa, val, bytes);
+	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+
+	return emulator_write_phys(vcpu, gpa, val, bytes,
+				   ctxt->page_table_written_insn);
 }
 
 static int write_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, int bytes, void *val)

@@ -4302,7 +4305,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 	if (!exchanged)
 		return X86EMUL_CMPXCHG_FAILED;
 
-	kvm_mmu_pte_write(vcpu, gpa, new, bytes, 1);
+	kvm_mmu_pte_write(vcpu, gpa, new, bytes, true, true);
 
 	return X86EMUL_CONTINUE;

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:06 UTC #10 lore raw

PATCH v2 4/12 KVM: x86: cleanup port-in/port-out emulated

In reply to Xiao Guangrong

Remove the same code between emulator_pio_in_emulated and emulator_pio_out_emulated

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

---
 arch/x86/kvm/x86.c |   59 ++++++++++++++++++++++-----------------------------
 1 files changed, 26 insertions(+), 33 deletions(-)

arch/x86/kvm/x86.c +26 ~~-33~~

index fbb3a44..5400a65 100644
@@ -4330,32 +4330,24 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 	return r;
 }
 
-
-static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
-				    int size, unsigned short port, void *val,
-				    unsigned int count)
+static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
+			       unsigned short port, void *val,
+			       unsigned int count, bool in)
 {
-	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
-
-	if (vcpu->arch.pio.count)
-		goto data_avail;
-
-	trace_kvm_pio(0, port, size, count);
+	trace_kvm_pio(!in, port, size, count);
 
 	vcpu->arch.pio.port = port;
-	vcpu->arch.pio.in = 1;
+	vcpu->arch.pio.in = in;
 	vcpu->arch.pio.count  = count;
 	vcpu->arch.pio.size = size;
 
 	if (!kernel_pio(vcpu, vcpu->arch.pio_data)) {
-	data_avail:
-		memcpy(val, vcpu->arch.pio_data, size * count);
 		vcpu->arch.pio.count = 0;
 		return 1;
 	}
 
 	vcpu->run->exit_reason = KVM_EXIT_IO;
-	vcpu->run->io.direction = KVM_EXIT_IO_IN;
+	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
 	vcpu->run->io.size = size;
 	vcpu->run->io.data_offset = KVM_PIO_PAGE_OFFSET * PAGE_SIZE;
 	vcpu->run->io.count = count;

@@ -4364,36 +4356,37 @@ static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
 	return 0;
 }
 
-static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
-				     int size, unsigned short port,
-				     const void *val, unsigned int count)
+static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
+				    int size, unsigned short port, void *val,
+				    unsigned int count)
 {
 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	int ret;
 
-	trace_kvm_pio(1, port, size, count);
-
-	vcpu->arch.pio.port = port;
-	vcpu->arch.pio.in = 0;
-	vcpu->arch.pio.count = count;
-	vcpu->arch.pio.size = size;
-
-	memcpy(vcpu->arch.pio_data, val, size * count);
+	if (vcpu->arch.pio.count)
+		goto data_avail;
 
-	if (!kernel_pio(vcpu, vcpu->arch.pio_data)) {
+	ret = emulator_pio_in_out(vcpu, size, port, val, count, true);
+	if (ret) {
+data_avail:
+		memcpy(val, vcpu->arch.pio_data, size * count);
 		vcpu->arch.pio.count = 0;
 		return 1;
 	}
 
-	vcpu->run->exit_reason = KVM_EXIT_IO;
-	vcpu->run->io.direction = KVM_EXIT_IO_OUT;
-	vcpu->run->io.size = size;
-	vcpu->run->io.data_offset = KVM_PIO_PAGE_OFFSET * PAGE_SIZE;
-	vcpu->run->io.count = count;
-	vcpu->run->io.port = port;
-
 	return 0;
 }
 
+static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
+				     int size, unsigned short port,
+				     const void *val, unsigned int count)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+
+	memcpy(vcpu->arch.pio_data, val, size * count);
+	return emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
+}
+
 static unsigned long get_segment_base(struct kvm_vcpu *vcpu, int seg)
 {
 	return kvm_x86_ops->get_segment_base(vcpu, seg);

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:07 UTC #11 lore raw

PATCH v2 5/12 KVM: x86: fast emulate repeat string write instructions

In reply to Xiao Guangrong

We usually use repeat string instructions to clear the page, for example, we call memset to clear a page table, stosb is used in this function, and repeated for 1024 times, that means we should occupy mmu lock for 1024 times and walking shadow page cache for 1024 times, it is terrible

In fact, if it is the repeat string instructions emulated and it is not a IO/MMIO access, we can zap all the corresponding shadow pages and return to the guest, then the mapping can became writable and we can directly write the page

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

---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/x86.c                 |   37 +++++++++++++++++++++++++++++------
 2 files changed, 31 insertions(+), 7 deletions(-)

arch/x86/include/asm/kvm_emulate.h +1 -0

index 049a6f5..66abd82 100644
@@ -245,6 +245,7 @@ struct x86_emulate_ctxt {
 	bool perm_ok; /* do not check permissions if true */
 	bool only_vendor_specific_insn;
 	bool page_table_written_insn;
+	bool pio_mmio_emulate;
 
 	bool have_exception;
 	struct x86_exception exception;

arch/x86/kvm/x86.c +30 -7

index 5400a65..64f920d 100644
@@ -4073,7 +4073,7 @@ struct read_write_emulator_ops {
 	int (*read_write_prepare)(struct kvm_vcpu *vcpu, void *val,
 				  int bytes);
 	int (*read_write_emulate)(struct kvm_vcpu *vcpu, gpa_t gpa,
-				  void *val, int bytes);
+				  void *val, int bytes, int *retvalue);
 	int (*read_write_mmio)(struct kvm_vcpu *vcpu, gpa_t gpa,
 			       int bytes, void *val);
 	int (*read_write_exit_mmio)(struct kvm_vcpu *vcpu, gpa_t gpa,

@@ -4095,18 +4095,37 @@ static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes)
 }
 
 static int read_emulate(struct kvm_vcpu *vcpu, gpa_t gpa,
-			void *val, int bytes)
+			void *val, int bytes, int *retvalue)
 {
+	*retvalue = X86EMUL_CONTINUE;
 	return !kvm_read_guest(vcpu->kvm, gpa, val, bytes);
 }
 
 static int write_emulate(struct kvm_vcpu *vcpu, gpa_t gpa,
-			 void *val, int bytes)
+			 void *val, int bytes, int *retvalue)
 {
 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+	bool page_table_written = ctxt->page_table_written_insn;
 
-	return emulator_write_phys(vcpu, gpa, val, bytes,
-				   ctxt->page_table_written_insn);
+	*retvalue = X86EMUL_CONTINUE;
+
+	/*
+	 * Zap the shadow page and retry the instruction if it is
+	 * the repetition instruction and no need to be emulated
+	 * for every repetition(neither io access nor mmio access).
+	 * The later access can make the page writable and #PF
+	 * can be avoided.
+	 *
+	 * We can not do this if the guest is in real mode since all
+	 * exceptions are intercepted in real mode, if the emulation
+	 * is caused by exception, we will retry this emulation for every.
+	 */
+	if (ctxt->rep_prefix && !ctxt->pio_mmio_emulate && is_protmode(vcpu)) {
+		page_table_written = false;
+		*retvalue = X86EMUL_RETRY_INSTR;
+	}
+
+	return emulator_write_phys(vcpu, gpa, val, bytes, page_table_written);
 }
 
 static int write_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, int bytes, void *val)

@@ -4167,10 +4186,12 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
 	if (ret)
 		goto mmio;
 
-	if (ops->read_write_emulate(vcpu, gpa, val, bytes))
-		return X86EMUL_CONTINUE;
+	if (ops->read_write_emulate(vcpu, gpa, val, bytes, &ret))
+		return ret;
 
 mmio:
+	vcpu->arch.emulate_ctxt.pio_mmio_emulate = true;
+
 	/*
 	 * Is this MMIO handled locally?
 	 */

@@ -4336,6 +4357,7 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 {
 	trace_kvm_pio(!in, port, size, count);
 
+	vcpu->arch.emulate_ctxt.pio_mmio_emulate = true;
 	vcpu->arch.pio.port = port;
 	vcpu->arch.pio.in = in;
 	vcpu->arch.pio.count  = count;

@@ -4828,6 +4850,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 		ctxt->have_exception = false;
 		ctxt->perm_ok = false;
 		ctxt->page_table_written_insn = false;
+		ctxt->pio_mmio_emulate = false;
 
 		ctxt->only_vendor_specific_insn
 			= emulation_type & EMULTYPE_TRAP_UD;

Signature · 1 line

1.7.5.4

Avi Kivity

2011-08-03 08:10 UTC #12 lore raw

Re: [PATCH v2 05/12] KVM: x86: fast emulate repeat string write instructions

Quoted 9 lines · Xiao Guangrong

We usually use repeat string instructions to clear the page, for example, we call memset to clear a page table, stosb is used in this function, and repeated for 1024 times, that means we should occupy mmu lock for 1024 times and walking shadow page cache for 1024 times, it is terrible
In fact, if it is the repeat string instructions emulated and it is not a IO/MMIO access, we can zap all the corresponding shadow pages and return to the guest, then the mapping can became writable and we can directly write the page

This isn't needed if we zap before emulating, right?

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-08-03 09:29 UTC #13 lore raw

Re: [PATCH v2 05/12] KVM: x86: fast emulate repeat string write instructions

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Xiao Guangrong
We usually use repeat string instructions to clear the page, for example, we call memset to clear a page table, stosb is used in this function, and repeated for 1024 times, that means we should occupy mmu lock for 1024 times and walking shadow page cache for 1024 times, it is terrible

Quoted 7 lines

Quoted 4 lines
In fact, if it is the repeat string instructions emulated and it is not a IO/MMIO access, we can zap all the corresponding shadow pages and return to the guest, then the mapping can became writable and we can directly write the page
This isn't needed if we zap before emulating, right?

Yes, it is, i will improve it.

Xiao Guangrong

author

2011-08-02 11:07 UTC #14 lore raw

PATCH v2 6/12 KVM: MMU: do not mark access bit on pte write path

In reply to Xiao Guangrong

In current code, the accessed bit is always set when page fault occurred, do not need to set it on pte write path

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

---
 arch/x86/include/asm/kvm_host.h |    1 -
 arch/x86/kvm/mmu.c              |   22 +---------------------
 2 files changed, 1 insertions(+), 22 deletions(-)

arch/x86/include/asm/kvm_host.h +0 -1

index a386755..b256660 100644
@@ -355,7 +355,6 @@ struct kvm_vcpu_arch {
 	gfn_t last_pt_write_gfn;
 	int   last_pt_write_count;
 	u64  *last_pte_updated;
-	gfn_t last_pte_gfn;
 
 	struct fpu guest_fpu;
 	u64 xcr0;

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

index 82d5d5a..f76be59 100644
@@ -2204,11 +2204,6 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	if (set_mmio_spte(sptep, gfn, pfn, pte_access))
 		return 0;
 
-	/*
-	 * We don't set the accessed bit, since we sometimes want to see
-	 * whether the guest actually used the pte (in order to detect
-	 * demand paging).
-	 */
 	spte = PT_PRESENT_MASK;
 	if (!speculative)
 		spte |= shadow_accessed_mask;

@@ -2359,10 +2354,8 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 		}
 	}
 	kvm_release_pfn_clean(pfn);
-	if (speculative) {
+	if (speculative)
 		vcpu->arch.last_pte_updated = sptep;
-		vcpu->arch.last_pte_gfn = gfn;
-	}
 }
 
 static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)

@@ -3527,18 +3520,6 @@ static bool last_updated_pte_accessed(struct kvm_vcpu *vcpu)
 	return !!(spte && (*spte & shadow_accessed_mask));
 }
 
-static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, gfn_t gfn)
-{
-	u64 *spte = vcpu->arch.last_pte_updated;
-
-	if (spte
-	    && vcpu->arch.last_pte_gfn == gfn
-	    && shadow_accessed_mask
-	    && !(*spte & shadow_accessed_mask)
-	    && is_shadow_present_pte(*spte))
-		set_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
-}
-
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes,
 		       bool guest_initiated, bool page_table_written)

@@ -3609,7 +3590,6 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	++vcpu->kvm->stat.mmu_pte_write;
 	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_PTE_WRITE);
 	if (guest_initiated) {
-		kvm_mmu_access_page(vcpu, gfn);
 		if (gfn == vcpu->arch.last_pt_write_gfn
 		    && !last_updated_pte_accessed(vcpu)) {
 			++vcpu->arch.last_pt_write_count;

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:08 UTC #15 lore raw

PATCH v2 7/12 KVM: MMU: cleanup FNAME(invlpg)

In reply to Xiao Guangrong

Directly Use mmu_page_zap_pte to zap spte in FNAME(invlpg), also remove the same code between FNAME(invlpg) and FNAME(sync_page)

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

---
 arch/x86/kvm/mmu.c         |   16 ++++++++++------
 arch/x86/kvm/paging_tmpl.h |   42 +++++++++++++++---------------------------
 2 files changed, 25 insertions(+), 33 deletions(-)

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

index f76be59..8e97ffa 100644
@@ -1809,7 +1809,7 @@ static void validate_direct_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	}
 }
 
-static void mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp,
+static bool mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp,
 			     u64 *spte)
 {
 	u64 pte;

@@ -1817,17 +1817,21 @@ static void mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp,
 
 	pte = *spte;
 	if (is_shadow_present_pte(pte)) {
-		if (is_last_spte(pte, sp->role.level))
+		if (is_last_spte(pte, sp->role.level)) {
 			drop_spte(kvm, spte);
-		else {
+			if (is_large_pte(pte))
+				--kvm->stat.lpages;
+		} else {
 			child = page_header(pte & PT64_BASE_ADDR_MASK);
 			drop_parent_pte(child, spte);
 		}
-	} else if (is_mmio_spte(pte))
+		return true;
+	}
+
+	if (is_mmio_spte(pte))
 		mmu_spte_clear_no_track(spte);
 
-	if (is_large_pte(pte))
-		--kvm->stat.lpages;
+	return false;
 }
 
 static void kvm_mmu_page_unlink_children(struct kvm *kvm,

arch/x86/kvm/paging_tmpl.h +15 ~~-27~~

83 lines not on this page · raw message

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:08 UTC #16 lore raw

PATCH v2 8/12 KVM: MMU: fast prefetch spte on invlpg path

In reply to Xiao Guangrong

Fast prefetch spte for the unsync shadow page on invlpg path

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

---
 arch/x86/include/asm/kvm_host.h |    3 +--
 arch/x86/kvm/mmu.c              |   37 +++++++++++++++----------------------
 arch/x86/kvm/paging_tmpl.h      |   23 ++++++++++-------------
 arch/x86/kvm/x86.c              |    4 ++--
 4 files changed, 28 insertions(+), 39 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -2

index b256660..2c08458 100644
@@ -456,7 +456,6 @@ struct kvm_arch {
 	unsigned int n_requested_mmu_pages;
 	unsigned int n_max_mmu_pages;
 	unsigned int indirect_shadow_pages;
-	atomic_t invlpg_counter;
 	struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
 	/*
 	 * Hash table of struct kvm_mmu_page.

@@ -749,7 +748,7 @@ int fx_init(struct kvm_vcpu *vcpu);
 void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu);
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes,
-		       bool guest_initiated, bool page_table_written);
+		       bool page_table_written);
 int kvm_mmu_unprotect_page_virt(struct kvm_vcpu *vcpu, gva_t gva);
 void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu);
 int kvm_mmu_load(struct kvm_vcpu *vcpu);

arch/x86/kvm/mmu.c +15 ~~-22~~

index 8e97ffa..5517115 100644
@@ -3526,7 +3526,7 @@ static bool last_updated_pte_accessed(struct kvm_vcpu *vcpu)
 
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes,
-		       bool guest_initiated, bool page_table_written)
+		       bool page_table_written)
 {
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	union kvm_mmu_page_role mask = { .word = 0 };

@@ -3535,7 +3535,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	LIST_HEAD(invalid_list);
 	u64 entry, gentry, *spte;
 	unsigned pte_size, page_offset, misaligned, quadrant, offset;
-	int level, npte, invlpg_counter, r, flooded = 0;
+	int level, npte, r, flooded = 0;
 	bool remote_flush, local_flush, zap_page;
 
 	/*

@@ -3550,19 +3550,16 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 
 	pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);
 
-	invlpg_counter = atomic_read(&vcpu->kvm->arch.invlpg_counter);
-
 	/*
 	 * Assume that the pte write on a page table of the same type
 	 * as the current vcpu paging mode since we update the sptes only
 	 * when they have the same mode.
 	 */
-	if ((is_pae(vcpu) && bytes == 4) || !new) {
+	if (is_pae(vcpu) && bytes == 4) {
 		/* Handle a 32-bit guest writing two halves of a 64-bit gpte */
-		if (is_pae(vcpu)) {
-			gpa &= ~(gpa_t)7;
-			bytes = 8;
-		}
+		gpa &= ~(gpa_t)7;
+		bytes = 8;
+
 		r = kvm_read_guest(vcpu->kvm, gpa, &gentry, min(bytes, 8));
 		if (r)
 			gentry = 0;

@@ -3588,22 +3585,18 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	 */
 	mmu_topup_memory_caches(vcpu);
 	spin_lock(&vcpu->kvm->mmu_lock);
-	if (atomic_read(&vcpu->kvm->arch.invlpg_counter) != invlpg_counter)
-		gentry = 0;
 	kvm_mmu_free_some_pages(vcpu);
 	++vcpu->kvm->stat.mmu_pte_write;
 	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_PTE_WRITE);
-	if (guest_initiated) {
-		if (gfn == vcpu->arch.last_pt_write_gfn
-		    && !last_updated_pte_accessed(vcpu)) {
-			++vcpu->arch.last_pt_write_count;
-			if (vcpu->arch.last_pt_write_count >= 3)
-				flooded = 1;
-		} else {
-			vcpu->arch.last_pt_write_gfn = gfn;
-			vcpu->arch.last_pt_write_count = 1;
-			vcpu->arch.last_pte_updated = NULL;
-		}
+	if (gfn == vcpu->arch.last_pt_write_gfn
+	    && !last_updated_pte_accessed(vcpu)) {
+		++vcpu->arch.last_pt_write_count;
+		if (vcpu->arch.last_pt_write_count >= 3)
+			flooded = 1;
+	} else {
+		vcpu->arch.last_pt_write_gfn = gfn;
+		vcpu->arch.last_pt_write_count = 1;
+		vcpu->arch.last_pte_updated = NULL;
 	}
 
 	mask.cr0_wp = mask.cr4_pae = mask.nxe = 1;

arch/x86/kvm/paging_tmpl.h +10 ~~-13~~

index 0f915b5..3466229 100644
@@ -666,20 +666,22 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 {
 	struct kvm_shadow_walk_iterator iterator;
 	struct kvm_mmu_page *sp;
-	gpa_t pte_gpa = -1;
 	int level;
 	u64 *sptep;
 
 	vcpu_clear_mmio_info(vcpu, gva);
+	mmu_topup_memory_caches(vcpu);
 
 	spin_lock(&vcpu->kvm->mmu_lock);
-
 	for_each_shadow_entry(vcpu, gva, iterator) {
 		level = iterator.level;
 		sptep = iterator.sptep;
 
 		sp = page_header(__pa(sptep));
 		if (is_last_spte(*sptep, level)) {
+			pt_element_t gpte;
+			gpa_t pte_gpa;
+
 			if (!sp->unsync)
 				break;

@@ -688,23 +690,18 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 
 			if (mmu_page_zap_pte(vcpu->kvm, sp, sptep))
 				kvm_flush_remote_tlbs(vcpu->kvm);
+
+			if (kvm_read_guest_atomic(vcpu->kvm, pte_gpa, &gpte,
+						  sizeof(pt_element_t)))
+				break;
+
+			FNAME(update_pte)(vcpu, sp, sptep, &gpte);
 		}
 
 		if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
 			break;
 	}
-
-	atomic_inc(&vcpu->kvm->arch.invlpg_counter);
-
 	spin_unlock(&vcpu->kvm->mmu_lock);
-
-	if (pte_gpa == -1)
-		return;
-
-	if (mmu_topup_memory_caches(vcpu))
-		return;
-	kvm_mmu_pte_write(vcpu, pte_gpa, NULL, sizeof(pt_element_t),
-			  false, true);
 }
 
 static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr, u32 access,

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

index 64f920d..fd83f3c 100644
@@ -4065,7 +4065,7 @@ int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
 	ret = kvm_write_guest(vcpu->kvm, gpa, val, bytes);
 	if (ret < 0)
 		return 0;
-	kvm_mmu_pte_write(vcpu, gpa, val, bytes, true, page_table_written);
+	kvm_mmu_pte_write(vcpu, gpa, val, bytes, page_table_written);
 	return 1;
 }

@@ -4326,7 +4326,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 	if (!exchanged)
 		return X86EMUL_CMPXCHG_FAILED;
 
-	kvm_mmu_pte_write(vcpu, gpa, new, bytes, true, true);
+	kvm_mmu_pte_write(vcpu, gpa, new, bytes, true);
 
 	return X86EMUL_CONTINUE;

Signature · 1 line

1.7.5.4

Marcelo Tosatti

2011-08-02 19:07 UTC #17 lore raw

Re: [PATCH v2 08/12] KVM: MMU: fast prefetch spte on invlpg path

Quoted 114 lines · Xiao Guangrong

Fast prefetch spte for the unsync shadow page on invlpg path

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

---
 arch/x86/include/asm/kvm_host.h |    3 +--
 arch/x86/kvm/mmu.c              |   37 +++++++++++++++----------------------
 arch/x86/kvm/paging_tmpl.h      |   23 ++++++++++-------------
 arch/x86/kvm/x86.c              |    4 ++--
 4 files changed, 28 insertions(+), 39 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -2

index b256660..2c08458 100644
@@ -456,7 +456,6 @@ struct kvm_arch {
 	unsigned int n_requested_mmu_pages;
 	unsigned int n_max_mmu_pages;
 	unsigned int indirect_shadow_pages;
-	atomic_t invlpg_counter;
 	struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
 	/*
 	 * Hash table of struct kvm_mmu_page.

@@ -749,7 +748,7 @@ int fx_init(struct kvm_vcpu *vcpu);
 void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu);
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes,
-		       bool guest_initiated, bool page_table_written);
+		       bool page_table_written);
 int kvm_mmu_unprotect_page_virt(struct kvm_vcpu *vcpu, gva_t gva);
 void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu);
 int kvm_mmu_load(struct kvm_vcpu *vcpu);

arch/x86/kvm/mmu.c +15 ~~-22~~

index 8e97ffa..5517115 100644
@@ -3526,7 +3526,7 @@ static bool last_updated_pte_accessed(struct kvm_vcpu *vcpu)
 
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes,
-		       bool guest_initiated, bool page_table_written)
+		       bool page_table_written)
 {
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	union kvm_mmu_page_role mask = { .word = 0 };

@@ -3535,7 +3535,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	LIST_HEAD(invalid_list);
 	u64 entry, gentry, *spte;
 	unsigned pte_size, page_offset, misaligned, quadrant, offset;
-	int level, npte, invlpg_counter, r, flooded = 0;
+	int level, npte, r, flooded = 0;
 	bool remote_flush, local_flush, zap_page;
 
 	/*

@@ -3550,19 +3550,16 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 
 	pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);
 
-	invlpg_counter = atomic_read(&vcpu->kvm->arch.invlpg_counter);
-
 	/*
 	 * Assume that the pte write on a page table of the same type
 	 * as the current vcpu paging mode since we update the sptes only
 	 * when they have the same mode.
 	 */
-	if ((is_pae(vcpu) && bytes == 4) || !new) {
+	if (is_pae(vcpu) && bytes == 4) {
 		/* Handle a 32-bit guest writing two halves of a 64-bit gpte */
-		if (is_pae(vcpu)) {
-			gpa &= ~(gpa_t)7;
-			bytes = 8;
-		}
+		gpa &= ~(gpa_t)7;
+		bytes = 8;
+
 		r = kvm_read_guest(vcpu->kvm, gpa, &gentry, min(bytes, 8));
 		if (r)
 			gentry = 0;

@@ -3588,22 +3585,18 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	 */
 	mmu_topup_memory_caches(vcpu);
 	spin_lock(&vcpu->kvm->mmu_lock);
-	if (atomic_read(&vcpu->kvm->arch.invlpg_counter) != invlpg_counter)
-		gentry = 0;
 	kvm_mmu_free_some_pages(vcpu);
 	++vcpu->kvm->stat.mmu_pte_write;
 	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_PTE_WRITE);
-	if (guest_initiated) {
-		if (gfn == vcpu->arch.last_pt_write_gfn
-		    && !last_updated_pte_accessed(vcpu)) {
-			++vcpu->arch.last_pt_write_count;
-			if (vcpu->arch.last_pt_write_count >= 3)
-				flooded = 1;
-		} else {
-			vcpu->arch.last_pt_write_gfn = gfn;
-			vcpu->arch.last_pt_write_count = 1;
-			vcpu->arch.last_pte_updated = NULL;
-		}
+	if (gfn == vcpu->arch.last_pt_write_gfn
+	    && !last_updated_pte_accessed(vcpu)) {
+		++vcpu->arch.last_pt_write_count;
+		if (vcpu->arch.last_pt_write_count >= 3)
+			flooded = 1;
+	} else {
+		vcpu->arch.last_pt_write_gfn = gfn;
+		vcpu->arch.last_pt_write_count = 1;
+		vcpu->arch.last_pte_updated = NULL;
 	}
 
 	mask.cr0_wp = mask.cr4_pae = mask.nxe = 1;

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

index 0f915b5..3466229 100644

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 673

Quoted 9 lines

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

@@ -666,20 +666,22 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 {
 	struct kvm_shadow_walk_iterator iterator;
 	struct kvm_mmu_page *sp;
-	gpa_t pte_gpa = -1;
 	int level;
 	u64 *sptep;
 
 	vcpu_clear_mmio_info(vcpu, gva);
+	mmu_topup_memory_caches(vcpu);

Should handle failure.

Xiao Guangrong

author

2011-08-03 06:01 UTC #18 lore raw

Re: [PATCH v2 08/12] KVM: MMU: fast prefetch spte on invlpg path

Quoted 13 lines · Marcelo Tosatti

Quoted 13 lines

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

 	mask.cr0_wp = mask.cr4_pae = mask.nxe = 1;

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

index 0f915b5..3466229 100644
@@ -666,20 +666,22 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 {
 	struct kvm_shadow_walk_iterator iterator;
 	struct kvm_mmu_page *sp;
-	gpa_t pte_gpa = -1;
 	int level;
 	u64 *sptep;

Quoted 5 lines

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 673
Quoted 2 lines
arch/x86/kvm/paging_tmpl.h +1 -0
 	vcpu_clear_mmio_info(vcpu, gva);
+	mmu_topup_memory_caches(vcpu);
Should handle failure.

Yes, thanks Marcelo! I will fix it.

Xiao Guangrong

author

2011-08-02 11:09 UTC #19 lore raw

PATCH v2 9/12 KVM: MMU: remove unnecessary kvm_mmu_free_some_pages

In reply to Xiao Guangrong

In kvm_mmu_pte_write, we do not need to alloc shadow page, so calling kvm_mmu_free_some_pages is really unnecessary

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

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

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

index 5517115..9d0acdf 100644
@@ -3585,7 +3585,6 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	 */
 	mmu_topup_memory_caches(vcpu);
 	spin_lock(&vcpu->kvm->mmu_lock);
-	kvm_mmu_free_some_pages(vcpu);
 	++vcpu->kvm->stat.mmu_pte_write;
 	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_PTE_WRITE);
 	if (gfn == vcpu->arch.last_pt_write_gfn

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:09 UTC #20 lore raw

PATCH v2 10/12 KVM: MMU: split kvm_mmu_pte_write function

In reply to Xiao Guangrong

kvm_mmu_pte_write is too long, we split it for better readable

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

---
 arch/x86/kvm/mmu.c |  189 +++++++++++++++++++++++++++++++---------------------
 1 files changed, 113 insertions(+), 76 deletions(-)

arch/x86/kvm/mmu.c +113 ~~-76~~

index 9d0acdf..c20c9ff 100644
@@ -3524,49 +3524,29 @@ static bool last_updated_pte_accessed(struct kvm_vcpu *vcpu)
 	return !!(spte && (*spte & shadow_accessed_mask));
 }
 
-void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
-		       const u8 *new, int bytes,
-		       bool page_table_written)
+static u64 mmu_pte_write_fetch_gpte(struct kvm_vcpu *vcpu, gpa_t *gpa,
+				    const u8 *new, int *bytes)
 {
-	gfn_t gfn = gpa >> PAGE_SHIFT;
-	union kvm_mmu_page_role mask = { .word = 0 };
-	struct kvm_mmu_page *sp;
-	struct hlist_node *node;
-	LIST_HEAD(invalid_list);
-	u64 entry, gentry, *spte;
-	unsigned pte_size, page_offset, misaligned, quadrant, offset;
-	int level, npte, r, flooded = 0;
-	bool remote_flush, local_flush, zap_page;
-
-	/*
-	 * If we don't have indirect shadow pages, it means no page is
-	 * write-protected, so we can exit simply.
-	 */
-	if (!ACCESS_ONCE(vcpu->kvm->arch.indirect_shadow_pages))
-		return;
-
-	zap_page = remote_flush = local_flush = false;
-	offset = offset_in_page(gpa);
-
-	pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);
+	u64 gentry;
+	int r;
 
 	/*
 	 * Assume that the pte write on a page table of the same type
 	 * as the current vcpu paging mode since we update the sptes only
 	 * when they have the same mode.
 	 */
-	if (is_pae(vcpu) && bytes == 4) {
+	if (is_pae(vcpu) && *bytes == 4) {
 		/* Handle a 32-bit guest writing two halves of a 64-bit gpte */
-		gpa &= ~(gpa_t)7;
-		bytes = 8;
+		*gpa &= ~(gpa_t)7;
+		*bytes = 8;
 
-		r = kvm_read_guest(vcpu->kvm, gpa, &gentry, min(bytes, 8));
+		r = kvm_read_guest(vcpu->kvm, *gpa, &gentry, min(*bytes, 8));
 		if (r)
 			gentry = 0;
 		new = (const u8 *)&gentry;
 	}
 
-	switch (bytes) {
+	switch (*bytes) {
 	case 4:
 		gentry = *(const u32 *)new;
 		break;

@@ -3578,71 +3558,128 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		break;
 	}
 
-	/*
-	 * No need to care whether allocation memory is successful
-	 * or not since pte prefetch is skiped if it does not have
-	 * enough objects in the cache.
-	 */
-	mmu_topup_memory_caches(vcpu);
-	spin_lock(&vcpu->kvm->mmu_lock);
-	++vcpu->kvm->stat.mmu_pte_write;
-	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_PTE_WRITE);
+	return gentry;
+}
+
+/*
+ * If we're seeing too many writes to a page, it may no longer be a page table,
+ * or we may be forking, in which case it is better to unmap the page.
+ */
+static bool detect_write_flooding(struct kvm_vcpu *vcpu, gfn_t gfn)
+{
+	bool flooded = false;
+
 	if (gfn == vcpu->arch.last_pt_write_gfn
 	    && !last_updated_pte_accessed(vcpu)) {
 		++vcpu->arch.last_pt_write_count;
 		if (vcpu->arch.last_pt_write_count >= 3)
-			flooded = 1;
+			flooded = true;
 	} else {
 		vcpu->arch.last_pt_write_gfn = gfn;
 		vcpu->arch.last_pt_write_count = 1;
 		vcpu->arch.last_pte_updated = NULL;
 	}
 
+	return flooded;
+}
+
+
+/*
+ * Misaligned accesses are too much trouble to fix up; also, they usually
+ * indicate a page is not used as a page table.
+ */
+static bool detect_write_misaligned(struct kvm_mmu_page *sp, gpa_t gpa,
+				    int bytes)
+{
+	unsigned offset, pte_size, misaligned;
+
+	pgprintk("misaligned: gpa %llx bytes %d role %x\n",
+		 gpa, bytes, sp->role.word);
+
+	offset = offset_in_page(gpa);
+	pte_size = sp->role.cr4_pae ? 8 : 4;
+	misaligned = (offset ^ (offset + bytes - 1)) & ~(pte_size - 1);
+	misaligned |= bytes < 4;
+
+	return misaligned;
+}
+
+static u64 *get_written_sptes(struct kvm_mmu_page *sp, gpa_t gpa, int *nspte)
+{
+	unsigned page_offset, quadrant;
+	u64 *spte;
+	int level;
+
+	page_offset = offset_in_page(gpa);
+	level = sp->role.level;
+	*nspte = 1;
+	if (!sp->role.cr4_pae) {
+		page_offset <<= 1;	/* 32->64 */
+		/*
+		 * A 32-bit pde maps 4MB while the shadow pdes map
+		 * only 2MB.  So we need to double the offset again
+		 * and zap two pdes instead of one.
+		 */
+		if (level == PT32_ROOT_LEVEL) {
+			page_offset &= ~7; /* kill rounding error */
+			page_offset <<= 1;
+			*nspte = 2;
+		}
+		quadrant = page_offset >> PAGE_SHIFT;
+		page_offset &= ~PAGE_MASK;
+		if (quadrant != sp->role.quadrant)
+			return NULL;
+	}
+
+	spte = &sp->spt[page_offset / sizeof(*spte)];
+	return spte;
+}
+
+void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
+		       const u8 *new, int bytes,
+		       bool page_table_written)
+{
+	gfn_t gfn = gpa >> PAGE_SHIFT;
+	union kvm_mmu_page_role mask = { .word = 0 };
+	struct kvm_mmu_page *sp;
+	struct hlist_node *node;
+	LIST_HEAD(invalid_list);
+	u64 entry, gentry, *spte;
+	int npte;
+	bool remote_flush, local_flush, zap_page, flooded, misaligned;
+
+	/*
+	 * If we don't have indirect shadow pages, it means no page is
+	 * write-protected, so we can exit simply.
+	 */
+	if (!ACCESS_ONCE(vcpu->kvm->arch.indirect_shadow_pages))
+		return;
+
+	zap_page = remote_flush = local_flush = false;
+
+	pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);
+
+	gentry = mmu_pte_write_fetch_gpte(vcpu, &gpa, new, &bytes);
+	mmu_topup_memory_caches(vcpu);
+	spin_lock(&vcpu->kvm->mmu_lock);
+	++vcpu->kvm->stat.mmu_pte_write;
+	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_PTE_WRITE);
+
+	flooded = detect_write_flooding(vcpu, gfn);
 	mask.cr0_wp = mask.cr4_pae = mask.nxe = 1;
 	for_each_gfn_indirect_valid_sp(vcpu->kvm, sp, gfn, node) {
-		pte_size = sp->role.cr4_pae ? 8 : 4;
-		misaligned = (offset ^ (offset + bytes - 1)) & ~(pte_size - 1);
-		misaligned |= bytes < 4;
+		misaligned = detect_write_misaligned(sp, gpa, bytes);
 		if (misaligned || flooded || !page_table_written) {
-			/*
-			 * Misaligned accesses are too much trouble to fix
-			 * up; also, they usually indicate a page is not used
-			 * as a page table.
-			 *
-			 * If we're seeing too many writes to a page,
-			 * it may no longer be a page table, or we may be
-			 * forking, in which case it is better to unmap the
-			 * page.
-			 */
-			pgprintk("misaligned: gpa %llx bytes %d role %x\n",
-				 gpa, bytes, sp->role.word);
 			zap_page |= !!kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
 						     &invalid_list);
 			++vcpu->kvm->stat.mmu_flooded;
 			continue;
 		}
-		page_offset = offset;
-		level = sp->role.level;
-		npte = 1;
-		if (!sp->role.cr4_pae) {
-			page_offset <<= 1;	/* 32->64 */
-			/*
-			 * A 32-bit pde maps 4MB while the shadow pdes map
-			 * only 2MB.  So we need to double the offset again
-			 * and zap two pdes instead of one.
-			 */
-			if (level == PT32_ROOT_LEVEL) {
-				page_offset &= ~7; /* kill rounding error */
-				page_offset <<= 1;
-				npte = 2;
-			}
-			quadrant = page_offset >> PAGE_SHIFT;
-			page_offset &= ~PAGE_MASK;
-			if (quadrant != sp->role.quadrant)
-				continue;
-		}
-		local_flush = true;
-		spte = &sp->spt[page_offset / sizeof(*spte)];
+
+		spte = get_written_sptes(sp, gpa, &npte);
+		if (!spte)
+			continue;
+
 		while (npte--) {
 			entry = *spte;
 			mmu_page_zap_pte(vcpu->kvm, sp, spte);

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:10 UTC #21 lore raw

PATCH v2 11/12 KVM: MMU: fix detecting misaligned accessed

In reply to Xiao Guangrong

Sometimes, we only modify the last one byte of a pte to update status bit, for example, clear_bit is used to clear r/w bit in linux kernel and 'andb' instruction is used in this function, in this case, kvm_mmu_pte_write will treat it as misaligned access, and the shadow page table is zapped

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

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

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

index c20c9ff..67eead6 100644
@@ -3598,6 +3598,14 @@ static bool detect_write_misaligned(struct kvm_mmu_page *sp, gpa_t gpa,
 
 	offset = offset_in_page(gpa);
 	pte_size = sp->role.cr4_pae ? 8 : 4;
+
+	/*
+	 * Sometimes, the OS only writes the last one bytes to update status
+	 * bits, for example, in linux, andb instruction is used in clear_bit().
+	 */
+	if (!(offset & (pte_size - 1)) && bytes == 1)
+		return false;
+
 	misaligned = (offset ^ (offset + bytes - 1)) & ~(pte_size - 1);
 	misaligned |= bytes < 4;

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-02 11:11 UTC #22 lore raw

PATCH v2 12/12 KVM: MMU: improve write flooding detected

In reply to Xiao Guangrong

Detecting write-flooding does not work well, when we handle page written, if the last speculative spte is not accessed, we treat the page is write-flooding, however, we can speculative spte on many path, such as pte prefetch, page synced, that means the last speculative spte may be not point to the written page and the written page can be accessed via other sptes, so depends on the Accessed bit of the last speculative spte is not enough

Instead of detected page accessed, we can detect whether the spte is accessed or not, if the spte is not accessed but it is written frequently, we treat is not a page table or it not used for a long time

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

---
 arch/x86/include/asm/kvm_host.h |    6 +---
 arch/x86/kvm/mmu.c              |   51 ++++++++++-----------------------------
 arch/x86/kvm/paging_tmpl.h      |    9 +-----
 3 files changed, 17 insertions(+), 49 deletions(-)

arch/x86/include/asm/kvm_host.h +2 -4

index 2c08458..50fe100 100644
@@ -239,6 +239,8 @@ struct kvm_mmu_page {
 	int clear_spte_count;
 #endif
 
+	int write_flooding_count;
+
 	struct rcu_head rcu;
 };

@@ -352,10 +354,6 @@ struct kvm_vcpu_arch {
 	struct kvm_mmu_memory_cache mmu_page_cache;
 	struct kvm_mmu_memory_cache mmu_page_header_cache;
 
-	gfn_t last_pt_write_gfn;
-	int   last_pt_write_count;
-	u64  *last_pte_updated;
-
 	struct fpu guest_fpu;
 	u64 xcr0;

arch/x86/kvm/mmu.c +13 ~~-38~~

109 lines not on this page · raw message

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

index 3466229..82063b2 100644
@@ -595,11 +595,9 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	 */
 	if (!r) {
 		pgprintk("%s: guest page fault\n", __func__);
-		if (!prefault) {
+		if (!prefault)
 			inject_page_fault(vcpu, &walker.fault);
-			/* reset fork detector */
-			vcpu->arch.last_pt_write_count = 0;
-		}
+
 		return 0;
 	}

@@ -637,9 +635,6 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	pgprintk("%s: shadow pte %p %llx emulate %d\n", __func__,
 		 sptep, *sptep, emulate);
 
-	if (!emulate)
-		vcpu->arch.last_pt_write_count = 0; /* reset fork detector */
-
 	++vcpu->stat.pf_fixed;
 	trace_kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
 	spin_unlock(&vcpu->kvm->mmu_lock);

Signature · 1 line

1.7.5.4