KVM: MMU: avoid pte_list_desc running out in kvm_mmu_pte_write

PATCH Superseded Xiao Guangrong 3 participants 30 messages Aug 30, 2011 – Sep 15, 2011

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Xiao Guangrong

author

2011-08-30 02:32 UTC #0 lore raw

PATCH v3 1/11 KVM: MMU: avoid pte_list_desc running 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 5d7fbf0..b01afee 100644
@@ -592,6 +592,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)
 {

@@ -969,6 +974,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;

@@ -3585,6 +3598,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;

@@ -3655,7 +3674,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;

@@ -3716,10 +3735,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-30 02:32 UTC #1 lore raw

PATCH v3 2/11 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/kvm/emulate.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

arch/x86/kvm/emulate.c +20 ~~-15~~

index 0453c07..e24c269 100644
@@ -82,6 +82,7 @@
 #define RMExt       (4<<15)     /* Opcode extension in ModRM r/m if mod == 3 */
 #define Sse         (1<<18)     /* SSE Vector instruction */
 /* Misc flags */
+#define PageTable   (1 << 19)   /* instruction used to write page table */
 #define Prot        (1<<21) /* instruction generates #UD if not in prot-mode */
 #define VendorSpecific (1<<22) /* Vendor specific instruction */
 #define NoAccess    (1<<23) /* Don't access memory (lea/invlpg/verr etc) */

@@ -3018,10 +3019,10 @@ static struct opcode group7_rm7[] = {
 
 static struct opcode group1[] = {
 	I(Lock, em_add),
-	I(Lock, em_or),
+	I(Lock | PageTable, em_or),
 	I(Lock, em_adc),
 	I(Lock, em_sbb),
-	I(Lock, em_and),
+	I(Lock | PageTable, em_and),
 	I(Lock, em_sub),
 	I(Lock, em_xor),
 	I(0, em_cmp),

@@ -3076,18 +3077,21 @@ static struct group_dual group7 = { {
 
 static struct opcode group8[] = {
 	N, N, N, N,
-	D(DstMem | SrcImmByte | ModRM), D(DstMem | SrcImmByte | ModRM | Lock),
-	D(DstMem | SrcImmByte | ModRM | Lock), D(DstMem | SrcImmByte | ModRM | Lock),
+	D(DstMem | SrcImmByte | ModRM),
+	D(DstMem | SrcImmByte | ModRM | Lock | PageTable),
+	D(DstMem | SrcImmByte | ModRM | Lock),
+	D(DstMem | SrcImmByte | ModRM | Lock | PageTable),
 };
 
 static struct group_dual group9 = { {
-	N, D(DstMem64 | ModRM | Lock), N, N, N, N, N, N,
+	N, D(DstMem64 | ModRM | Lock | PageTable), N, N, N, N, N, N,
 }, {
 	N, N, N, N, N, N, N, N,
 } };
 
 static struct opcode group11[] = {
-	I(DstMem | SrcImm | ModRM | Mov, em_mov), X7(D(Undefined)),
+	I(DstMem | SrcImm | ModRM | Mov | PageTable, em_mov),
+	X7(D(Undefined)),
 };
 
 static struct gprefix pfx_0f_6f_0f_7f = {

@@ -3099,7 +3103,7 @@ static struct opcode opcode_table[256] = {
 	I6ALU(Lock, em_add),
 	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
 	/* 0x08 - 0x0F */
-	I6ALU(Lock, em_or),
+	I6ALU(Lock | PageTable, em_or),
 	D(ImplicitOps | Stack | No64), N,
 	/* 0x10 - 0x17 */
 	I6ALU(Lock, em_adc),

@@ -3108,7 +3112,7 @@ static struct opcode opcode_table[256] = {
 	I6ALU(Lock, em_sbb),
 	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
 	/* 0x20 - 0x27 */
-	I6ALU(Lock, em_and), N, N,
+	I6ALU(Lock | PageTable, em_and), N, N,
 	/* 0x28 - 0x2F */
 	I6ALU(Lock, em_sub), N, I(ByteOp | DstAcc | No64, em_das),
 	/* 0x30 - 0x37 */

@@ -3141,11 +3145,11 @@ static struct opcode opcode_table[256] = {
 	G(ByteOp | DstMem | SrcImm | ModRM | No64 | Group, group1),
 	G(DstMem | SrcImmByte | ModRM | Group, group1),
 	I2bv(DstMem | SrcReg | ModRM, em_test),
-	I2bv(DstMem | SrcReg | ModRM | Lock, em_xchg),
+	I2bv(DstMem | SrcReg | ModRM | Lock | PageTable, em_xchg),
 	/* 0x88 - 0x8F */
-	I2bv(DstMem | SrcReg | ModRM | Mov, em_mov),
+	I2bv(DstMem | SrcReg | ModRM | Mov | PageTable, em_mov),
 	I2bv(DstReg | SrcMem | ModRM | Mov, em_mov),
-	I(DstMem | SrcNone | ModRM | Mov, em_mov_rm_sreg),
+	I(DstMem | SrcNone | ModRM | Mov | PageTable, em_mov_rm_sreg),
 	D(ModRM | SrcMem | NoAccess | DstReg),
 	I(ImplicitOps | SrcMem16 | ModRM, em_mov_sreg_rm),
 	G(0, group1A),

@@ -3158,7 +3162,7 @@ static struct opcode opcode_table[256] = {
 	II(ImplicitOps | Stack, em_popf, popf), N, N,
 	/* 0xA0 - 0xA7 */
 	I2bv(DstAcc | SrcMem | Mov | MemAbs, em_mov),
-	I2bv(DstMem | SrcAcc | Mov | MemAbs, em_mov),
+	I2bv(DstMem | SrcAcc | Mov | MemAbs | PageTable, em_mov),
 	I2bv(SrcSI | DstDI | Mov | String, em_mov),
 	I2bv(SrcSI | DstDI | String, em_cmp),
 	/* 0xA8 - 0xAF */

@@ -3255,18 +3259,19 @@ static struct opcode twobyte_table[256] = {
 	D(DstMem | SrcReg | Src2CL | ModRM), N, N,
 	/* 0xA8 - 0xAF */
 	D(ImplicitOps | Stack), D(ImplicitOps | Stack),
-	DI(ImplicitOps, rsm), D(DstMem | SrcReg | ModRM | BitOp | Lock),
+	DI(ImplicitOps, rsm),
+	D(DstMem | SrcReg | ModRM | BitOp | Lock | PageTable),
 	D(DstMem | SrcReg | Src2ImmByte | ModRM),
 	D(DstMem | SrcReg | Src2CL | ModRM),
 	D(ModRM), I(DstReg | SrcMem | ModRM, em_imul),
 	/* 0xB0 - 0xB7 */
-	D2bv(DstMem | SrcReg | ModRM | Lock),
+	D2bv(DstMem | SrcReg | ModRM | Lock | PageTable),
 	D(DstReg | SrcMemFAddr | ModRM), D(DstMem | SrcReg | ModRM | BitOp | Lock),
 	D(DstReg | SrcMemFAddr | ModRM), D(DstReg | SrcMemFAddr | ModRM),
 	D(ByteOp | DstReg | SrcMem | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
 	/* 0xB8 - 0xBF */
 	N, N,
-	G(BitOp, group8), D(DstMem | SrcReg | ModRM | BitOp | Lock),
+	G(BitOp, group8), D(DstMem | SrcReg | ModRM | BitOp | Lock | PageTable),
 	D(DstReg | SrcMem | ModRM), D(DstReg | SrcMem | ModRM),
 	D(ByteOp | DstReg | SrcMem | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
 	/* 0xC0 - 0xCF */

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-30 02:32 UTC #2 lore raw

PATCH v3 3/11 KVM: x86: retry non-page-table writing instruction

In reply to Xiao Guangrong

If the emulation is caused by #PF and it is non-page_table writing instruction, it means the VM-EXIT is caused by shadow page protected, we can zap the shadow page and retry this instruction directly

The idea is from Avi

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

---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/include/asm/kvm_host.h    |    5 ++++
 arch/x86/kvm/emulate.c             |    5 ++++
 arch/x86/kvm/mmu.c                 |   22 +++++++++++++----
 arch/x86/kvm/x86.c                 |   47 ++++++++++++++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 5 deletions(-)

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

index 6040d11..fa87b63 100644
@@ -362,6 +362,7 @@ enum x86_intercept {
 #endif
 
 int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len);
+bool page_table_writing_insn(struct x86_emulate_ctxt *ctxt);
 #define EMULATION_FAILED -1
 #define EMULATION_OK 0
 #define EMULATION_RESTART 1

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

index 6ab4241..27a25df 100644
@@ -443,6 +443,9 @@ struct kvm_vcpu_arch {
 
 	cpumask_var_t wbinvd_dirty_mask;
 
+	unsigned long last_retry_eip;
+	unsigned long last_retry_addr;
+
 	struct {
 		bool halted;
 		gfn_t gfns[roundup_pow_of_two(ASYNC_PF_PER_VCPU)];

@@ -689,6 +692,7 @@ enum emulation_result {
 #define EMULTYPE_NO_DECODE	    (1 << 0)
 #define EMULTYPE_TRAP_UD	    (1 << 1)
 #define EMULTYPE_SKIP		    (1 << 2)
+#define EMULTYPE_RETRY		    (1 << 3)
 int x86_emulate_instruction(struct kvm_vcpu *vcpu, unsigned long cr2,
 			    int emulation_type, void *insn, int insn_len);

@@ -753,6 +757,7 @@ 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);
+int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn);
 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/emulate.c +5 -0

index e24c269..c62424e 100644
@@ -3691,6 +3691,11 @@ done:
 	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
 }
 
+bool page_table_writing_insn(struct x86_emulate_ctxt *ctxt)
+{
+	return ctxt->d & PageTable;
+}
+
 static bool string_insn_completed(struct x86_emulate_ctxt *ctxt)
 {
 	/* The second termination condition only applies for REPE

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

index b01afee..26aae11 100644
@@ -1997,7 +1997,7 @@ void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int goal_nr_mmu_pages)
 	kvm->arch.n_max_mmu_pages = goal_nr_mmu_pages;
 }
 
-static int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
+int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
 {
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node;

@@ -2007,6 +2007,7 @@ static int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
 	pgprintk("%s: looking for gfn %llx\n", __func__, gfn);
 	r = 0;
 
+	spin_lock(&kvm->mmu_lock);
 	for_each_gfn_indirect_valid_sp(kvm, sp, gfn, node) {
 		pgprintk("%s: gfn %llx role %x\n", __func__, gfn,
 			 sp->role.word);

@@ -2014,8 +2015,10 @@ static int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
 		kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
 	}
 	kvm_mmu_commit_zap_page(kvm, &invalid_list);
+	spin_unlock(&kvm->mmu_lock);
 	return r;
 }
+EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page);
 
 static void mmu_unshadow(struct kvm *kvm, gfn_t gfn)
 {

@@ -3697,9 +3700,7 @@ int kvm_mmu_unprotect_page_virt(struct kvm_vcpu *vcpu, gva_t gva)
 
 	gpa = kvm_mmu_gva_to_gpa_read(vcpu, gva, NULL);
 
-	spin_lock(&vcpu->kvm->mmu_lock);
 	r = kvm_mmu_unprotect_page(vcpu->kvm, gpa >> PAGE_SHIFT);
-	spin_unlock(&vcpu->kvm->mmu_lock);
 	return r;
 }
 EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page_virt);

@@ -3720,10 +3721,18 @@ void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu)
 	kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list);
 }
 
+static bool is_mmio_page_fault(struct kvm_vcpu *vcpu, gva_t addr)
+{
+	if (vcpu->arch.mmu.direct_map || mmu_is_nested(vcpu))
+		return vcpu_match_mmio_gpa(vcpu, addr);
+
+	return vcpu_match_mmio_gva(vcpu, addr);
+}
+
 int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
 		       void *insn, int insn_len)
 {
-	int r;
+	int r, emulation_type = EMULTYPE_RETRY;
 	enum emulation_result er;
 
 	r = vcpu->arch.mmu.page_fault(vcpu, cr2, error_code, false);

@@ -3735,7 +3744,10 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
 		goto out;
 	}
 
-	er = x86_emulate_instruction(vcpu, cr2, 0, insn, insn_len);
+	if (is_mmio_page_fault(vcpu, cr2))
+		emulation_type = 0;
+
+	er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len);
 
 	switch (er) {
 	case EMULATE_DONE:

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

59 lines not on this page · raw message

Signature · 1 line

1.7.5.4

Avi Kivity

2011-09-13 10:47 UTC #3 lore raw

Re: [PATCH v3 03/11] KVM: x86: retry non-page-table writing instruction

Quoted 9 lines · Xiao Guangrong

If the emulation is caused by #PF and it is non-page_table writing instruction, it means the VM-EXIT is caused by shadow page protected, we can zap the shadow page and retry this instruction directly
The idea is from Avi
int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len);
+bool page_table_writing_insn(struct x86_emulate_ctxt *ctxt);

Please use the usual x86_ prefix used in the emulator interface.

Quoted 37 lines

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

@@ -3720,10 +3721,18 @@ void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu)
  	kvm_mmu_commit_zap_page(vcpu->kvm,&invalid_list);
  }
 
+static bool is_mmio_page_fault(struct kvm_vcpu *vcpu, gva_t addr)
+{
+	if (vcpu->arch.mmu.direct_map || mmu_is_nested(vcpu))
+		return vcpu_match_mmio_gpa(vcpu, addr);
+
+	return vcpu_match_mmio_gva(vcpu, addr);
+}
+
  int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
  		       void *insn, int insn_len)
  {
-	int r;
+	int r, emulation_type = EMULTYPE_RETRY;
  	enum emulation_result er;
 
  	r = vcpu->arch.mmu.page_fault(vcpu, cr2, error_code, false);

@@ -3735,7 +3744,10 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
  		goto out;
  	}
 
-	er = x86_emulate_instruction(vcpu, cr2, 0, insn, insn_len);
+	if (is_mmio_page_fault(vcpu, cr2))
+		emulation_type = 0;
+
+	er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len);
 
  	switch (er) {
  	case EMULATE_DONE:

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

index 6b37f18..1afe59e 100644

arch/x86/kvm/x86.c static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva) · line 4821

Quoted 8 lines

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

@@ -4814,6 +4814,50 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva)
  	return false;
  }
 
+static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
+			      unsigned long cr2,  int emulation_type)
+{
+	if (!vcpu->arch.mmu.direct_map&&  !mmu_is_nested(vcpu))
+		gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);

If mmu_is_nested() cr2 is an ngpa, we have to translate it to a gpa, no?

btw, I don't see mmu.direct_map initialized for nested npt?

Quoted 19 lines

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

+
+	kvm_mmu_unprotect_page(vcpu->kvm, gpa>>  PAGE_SHIFT);
+
+	return true;
+}
+
  int x86_emulate_instruction(struct kvm_vcpu *vcpu,
  			    unsigned long cr2,
  			    int emulation_type,

@@ -4855,6 +4899,9 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
  		return EMULATE_DONE;
  	}
 
+	if (retry_instruction(ctxt, cr2, emulation_type))
+		return EMULATE_DONE;
+
  	/* this is needed for vmware backdoor interface to work since it
  	   changes registers values  during IO operation */
  	if (vcpu->arch.emulate_regs_need_sync_from_vcpu) {

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-09-13 18:21 UTC #4 lore raw

Re: [PATCH v3 03/11] KVM: x86: retry non-page-table writing instruction

Quoted 5 lines · Avi Kivity

Quoted 4 lines · Xiao Guangrong
If the emulation is caused by #PF and it is non-page_table writing instruction, it means the VM-EXIT is caused by shadow page protected, we can zap the shadow page and retry this instruction directly

Quoted 8 lines

Quoted 5 lines
The idea is from Avi
int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len);
+bool page_table_writing_insn(struct x86_emulate_ctxt *ctxt);
Please use the usual x86_ prefix used in the emulator interface.

OK, will fix.

Quoted 40 lines

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

@@ -3720,10 +3721,18 @@ void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu)
      kvm_mmu_commit_zap_page(vcpu->kvm,&invalid_list);
  }
 
+static bool is_mmio_page_fault(struct kvm_vcpu *vcpu, gva_t addr)
+{
+    if (vcpu->arch.mmu.direct_map || mmu_is_nested(vcpu))
+        return vcpu_match_mmio_gpa(vcpu, addr);
+
+    return vcpu_match_mmio_gva(vcpu, addr);
+}
+
  int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
                 void *insn, int insn_len)
  {
-    int r;
+    int r, emulation_type = EMULTYPE_RETRY;
      enum emulation_result er;
 
      r = vcpu->arch.mmu.page_fault(vcpu, cr2, error_code, false);

@@ -3735,7 +3744,10 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
          goto out;
      }
 
-    er = x86_emulate_instruction(vcpu, cr2, 0, insn, insn_len);
+    if (is_mmio_page_fault(vcpu, cr2))
+        emulation_type = 0;
+
+    er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len);
 
      switch (er) {
      case EMULATE_DONE:

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

index 6b37f18..1afe59e 100644
@@ -4814,6 +4814,50 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva)
      return false;
  }

Quoted 8 lines

arch/x86/kvm/x86.c static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva) · line 4854
Quoted 5 lines
arch/x86/kvm/x86.c +5 -0
+static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
+                  unsigned long cr2,  int emulation_type)
+{
+    if (!vcpu->arch.mmu.direct_map&&  !mmu_is_nested(vcpu))
+        gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);
If mmu_is_nested() cr2 is an ngpa, we have to translate it to a gpa, no?

Yeah, will fix it.

And this bug also exists in the current code: it always uses L2 gpa to emulate write operation.
I guess the reason that it is not triggered is: the gpa of L2's shadow page can not be touched by L2, it means no page table is write-protected by L2.

Quoted 2 lines

btw, I don't see mmu.direct_map initialized for nested npt?

nested_svm_vmrun() -> nested_svm_init_mmu_context():
static int nested_svm_init_mmu_context(struct kvm_vcpu *vcpu)

{
	int r;

	r = kvm_init_shadow_mmu(vcpu, &vcpu->arch.mmu);

	vcpu->arch.mmu.set_cr3           = nested_svm_set_tdp_cr3;
	vcpu->arch.mmu.get_cr3           = nested_svm_get_tdp_cr3;
	vcpu->arch.mmu.get_pdptr         = nested_svm_get_tdp_pdptr;
	vcpu->arch.mmu.inject_page_fault = nested_svm_inject_npf_exit;
	vcpu->arch.mmu.shadow_root_level = get_npt_level();
	vcpu->arch.walk_mmu              = &vcpu->arch.nested_mmu;

	return r;
}

It is initialized in kvm_init_shadow_mmu　:-)

Avi Kivity

2011-09-14 09:53 UTC #5 lore raw

Re: [PATCH v3 03/11] KVM: x86: retry non-page-table writing instruction

Quoted 7 lines · Xiao Guangrong

Quoted 7 lines
Quoted 6 lines
+static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
+ unsigned long cr2, int emulation_type)
+{
+ if (!vcpu->arch.mmu.direct_map&& !mmu_is_nested(vcpu))
+ gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);

Quoted 7 lines

Quoted 2 lines
If mmu_is_nested() cr2 is an ngpa, we have to translate it to a gpa, no?
Yeah, will fix it.
And this bug also exists in the current code: it always uses L2 gpa to emulate write operation.

Can you please send this fix separately, so it can be backported if needed?

Quoted 2 lines

I guess the reason that it is not triggered is: the gpa of L2's shadow page can not be touched by L2, it means no page table is write-protected by L2.

Yes. All real guest hypervisors will do that. But it is technically possible for a hypervisor to allow its guest access to the real page tables.

Quoted 20 lines

Quoted 2 lines

btw, I don't see mmu.direct_map initialized for nested npt?

nested_svm_vmrun() -> nested_svm_init_mmu_context():
static int nested_svm_init_mmu_context(struct kvm_vcpu *vcpu)

{
	int r;

	r = kvm_init_shadow_mmu(vcpu,&vcpu->arch.mmu);

	vcpu->arch.mmu.set_cr3           = nested_svm_set_tdp_cr3;
	vcpu->arch.mmu.get_cr3           = nested_svm_get_tdp_cr3;
	vcpu->arch.mmu.get_pdptr         = nested_svm_get_tdp_pdptr;
	vcpu->arch.mmu.inject_page_fault = nested_svm_inject_npf_exit;
	vcpu->arch.mmu.shadow_root_level = get_npt_level();
	vcpu->arch.walk_mmu              =&vcpu->arch.nested_mmu;

	return r;
}

Quoted 1 line

It is initialized in kvm_init_shadow_mmu　:-)

Yes, need new eyeglasses.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-09-14 10:17 UTC #6 lore raw

Re: [PATCH v3 03/11] KVM: x86: retry non-page-table writing instruction

Quoted 11 lines · Avi Kivity

Quoted 10 lines · Xiao Guangrong
Quoted 9 lines
Quoted 6 lines
+static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
+ unsigned long cr2, int emulation_type)
+{
+ if (!vcpu->arch.mmu.direct_map&& !mmu_is_nested(vcpu))
+ gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);
If mmu_is_nested() cr2 is an ngpa, we have to translate it to a gpa, no?

Quoted 7 lines

Quoted 4 lines
Yeah, will fix it.
And this bug also exists in the current code: it always uses L2 gpa to emulate write operation.
Can you please send this fix separately, so it can be backported if needed?

Sure, i will do it as soon as possible. :-)

Xiao Guangrong

author

2011-09-15 04:53 UTC #7 lore raw

Re: [PATCH v3 03/11] KVM: x86: retry non-page-table writing instruction

Quoted 14 lines · Xiao Guangrong

Quoted 13 lines · Avi Kivity
Quoted 12 lines · Xiao Guangrong
Quoted 9 lines
Quoted 6 lines
+static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
+ unsigned long cr2, int emulation_type)
+{
+ if (!vcpu->arch.mmu.direct_map&& !mmu_is_nested(vcpu))
+ gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);
If mmu_is_nested() cr2 is an ngpa, we have to translate it to a gpa, no?
Yeah, will fix it.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
And this bug also exists in the current code: it always uses L2 gpa to emulate write operation.
Can you please send this fix separately, so it can be backported if needed?
Sure, i will do it as soon as possible. :-)

I am so sorry, the current code is good, it has already translated L2 gpa to L1 gpa:

vcpu->arch.nested_mmu.translate_gpa = translate_nested_gpa;

Please ignore it.

Xiao Guangrong

author

2011-08-30 02:33 UTC #8 lore raw

PATCH v3 4/11 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 1afe59e..85e6b4e 100644
@@ -4327,32 +4327,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;

@@ -4361,36 +4353,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-30 02:34 UTC #9 lore raw

PATCH v3 5/11 KVM: MMU: do not mark accessed 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 27a25df..58ea3a7 100644
@@ -356,7 +356,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 26aae11..7ec2a6a 100644
@@ -2206,11 +2206,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;

@@ -2361,10 +2356,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)

@@ -3532,18 +3525,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)

@@ -3614,7 +3595,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

Avi Kivity

2011-09-13 10:53 UTC #10 lore raw

Re: [PATCH v3 05/11] KVM: MMU: do not mark accessed bit on pte write path

Quoted 2 lines · 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

What about speculative sptes that are then only accessed via emulation?

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-09-13 18:27 UTC #11 lore raw

Re: [PATCH v3 05/11] KVM: MMU: do not mark accessed bit on pte write path

Quoted 6 lines · Avi Kivity

Quoted 2 lines · 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
What about speculative sptes that are then only accessed via emulation?

The gfn is read and written only via emulation? I think this case is very very rare?

Avi Kivity

2011-09-14 09:55 UTC #12 lore raw

Re: [PATCH v3 05/11] KVM: MMU: do not mark accessed bit on pte write path

Quoted 10 lines · Xiao Guangrong

Quoted 6 lines · Avi Kivity
Quoted 2 lines · 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
What about speculative sptes that are then only accessed via emulation?
The gfn is read and written only via emulation? I think this case is very very rare?

Probably...

Marcelo? Can you think of another case where spte.accessed is needed?

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

2011-09-15 13:11 UTC #13 lore raw

Re: [PATCH v3 05/11] KVM: MMU: do not mark accessed bit on pte write path

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Xiao Guangrong
Quoted 4 lines · Avi Kivity
Quoted 2 lines · 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

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
What about speculative sptes that are then only accessed via emulation?
The gfn is read and written only via emulation? I think this case is very very rare?
Probably...

The access information will be transferred via the host pte, via get_user_pages, to MM layer, in that case.

Quoted 1 line

Marcelo? Can you think of another case where spte.accessed is needed?

No, an spte updated via emulation will either be accessed directly, or if via emulation, access to the gfn it points transferred via host pte.

Xiao Guangrong

author

2011-08-30 02:34 UTC #14 lore raw

PATCH v3 6/11 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 7ec2a6a..ed3e778 100644
@@ -1808,7 +1808,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;

@@ -1816,17 +1816,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

Avi Kivity

2011-09-13 11:00 UTC #15 lore raw

Re: [PATCH v3 06/11] KVM: MMU: cleanup FNAME(invlpg)

Quoted 5 lines · 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)
+static gpa_t FNAME(get_first_pte_gpa)(struct kvm_mmu_page *sp)

spte

arch/x86/kvm/paging_tmpl.h out_unlock: · line 667

Quoted 8 lines

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

+{
+	int offset = 0;
+
+	if (PTTYPE == 32)
+		offset = sp->role.quadrant<<  PT64_LEVEL_BITS;
+
+	return gfn_to_gpa(sp->gfn) + offset * sizeof(pt_element_t);
+}

Only works for L1 tables, yes? Need a comment so it isn't reused for L2.

Quoted 13 lines

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

@@ -663,7 +673,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
  	gpa_t pte_gpa = -1;
  	int level;
  	u64 *sptep;
-	int need_flush = 0;
 
  	vcpu_clear_mmio_info(vcpu, gva);

@@ -675,36 +684,20 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 
  		sp = page_header(__pa(sptep));
  		if (is_last_spte(*sptep, level)) {
-			int offset, shift;

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

Quoted 10 lines

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

-
  			if (!sp->unsync)
  				break;
 
-			shift = PAGE_SHIFT -
-				  (PT_LEVEL_BITS - PT64_LEVEL_BITS) * level;
-			offset = sp->role.quadrant<<  shift;
-
-			pte_gpa = (sp->gfn<<  PAGE_SHIFT) + offset;
+			pte_gpa = FNAME(get_first_pte_gpa)(sp);

Here is can be used for L2 - I think we can use 2MB host pages to back 4MB guest mappings.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-09-13 18:29 UTC #16 lore raw

Re: [PATCH v3 06/11] KVM: MMU: cleanup FNAME(invlpg)

Quoted 9 lines · Avi Kivity

Quoted 5 lines · 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)
+static gpa_t FNAME(get_first_pte_gpa)(struct kvm_mmu_page *sp)
spte

Yes, Will fix.

Quoted 11 lines

arch/x86/kvm/paging_tmpl.h out_unlock: · line 667
Quoted 8 lines
arch/x86/kvm/paging_tmpl.h +8 -0
+{
+    int offset = 0;
+
+    if (PTTYPE == 32)
+        offset = sp->role.quadrant<<  PT64_LEVEL_BITS;
+
+    return gfn_to_gpa(sp->gfn) + offset * sizeof(pt_element_t);
+}
Only works for L1 tables, yes? Need a comment so it isn't reused for L2.

OK.

Quoted 17 lines

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

@@ -663,7 +673,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
      gpa_t pte_gpa = -1;
      int level;
      u64 *sptep;
-    int need_flush = 0;
 
      vcpu_clear_mmio_info(vcpu, gva);

@@ -675,36 +684,20 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 
          sp = page_header(__pa(sptep));
          if (is_last_spte(*sptep, level)) {
-            int offset, shift;
-
              if (!sp->unsync)
                  break;

Quoted 10 lines

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 690
Quoted 6 lines
arch/x86/kvm/paging_tmpl.h +1 -5
-            shift = PAGE_SHIFT -
-                  (PT_LEVEL_BITS - PT64_LEVEL_BITS) * level;
-            offset = sp->role.quadrant<<  shift;
-
-            pte_gpa = (sp->gfn<<  PAGE_SHIFT) + offset;
+            pte_gpa = FNAME(get_first_pte_gpa)(sp);
Here is can be used for L2 - I think we can use 2MB host pages to back 4MB guest mappings.

Only unsync shadow page is fetched here, and its level is always 1.

Avi Kivity

2011-09-14 09:57 UTC #17 lore raw

Re: [PATCH v3 06/11] KVM: MMU: cleanup FNAME(invlpg)

Quoted 17 lines · Xiao Guangrong

Quoted 17 lines
Quoted 15 lines
@@ -675,36 +684,20 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
           sp = page_header(__pa(sptep));
           if (is_last_spte(*sptep, level)) {
 -            int offset, shift;
 -
               if (!sp->unsync)
                   break;
- shift = PAGE_SHIFT -
- (PT_LEVEL_BITS - PT64_LEVEL_BITS) * level;
- offset = sp->role.quadrant<< shift;
-
- pte_gpa = (sp->gfn<< PAGE_SHIFT) + offset;
+ pte_gpa = FNAME(get_first_pte_gpa)(sp);

Quoted 5 lines

Quoted 2 lines
Here is can be used for L2 - I think we can use 2MB host pages to back 4MB guest mappings.
Only unsync shadow page is fetched here, and its level is always 1.

Right.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-08-30 02:34 UTC #18 lore raw

PATCH v3 7/11 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 |    4 +---
 arch/x86/kvm/mmu.c              |   38 +++++++++++++++-----------------------
 arch/x86/kvm/paging_tmpl.h      |   30 ++++++++++++++++++------------
 arch/x86/kvm/x86.c              |    4 ++--
 4 files changed, 36 insertions(+), 40 deletions(-)

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

index 58ea3a7..927ba73 100644
@@ -460,7 +460,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.

@@ -754,8 +753,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);
+		       const u8 *new, int bytes);
 int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn);
 int kvm_mmu_unprotect_page_virt(struct kvm_vcpu *vcpu, gva_t gva);
 void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu);

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

index ed3e778..f6de2fc 100644
@@ -3530,8 +3530,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)
+		       const u8 *new, int bytes)
 {
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	union kvm_mmu_page_role mask = { .word = 0 };

@@ -3540,7 +3539,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;
 
 	/*

@@ -3555,19 +3554,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;

@@ -3593,22 +3589,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 +18 ~~-12~~

index 7862c05..3395ab2 100644
@@ -670,20 +670,27 @@ 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);
 
-	spin_lock(&vcpu->kvm->mmu_lock);
+	/*
+	 * No need to check return value here, rmap_can_add() can
+	 * help us to skip pte prefetch later.
+	 */
+	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;

@@ -692,22 +699,21 @@ 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 (!rmap_can_add(vcpu))
+				break;
+
+			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), 0);
 }
 
 static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr, u32 access,

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

index 85e6b4e..7870493 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, 1);
+	kvm_mmu_pte_write(vcpu, gpa, val, bytes);
 	return 1;
 }

@@ -4302,7 +4302,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);
 
 	return X86EMUL_CONTINUE;

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-30 02:34 UTC #19 lore raw

PATCH v3 8/11 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 f6de2fc..9ac0dc8 100644
@@ -3589,7 +3589,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-30 02:35 UTC #20 lore raw

PATCH v3 9/11 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 |  187 +++++++++++++++++++++++++++++++---------------------
 1 files changed, 112 insertions(+), 75 deletions(-)

arch/x86/kvm/mmu.c +112 ~~-75~~

index 9ac0dc8..cfe24fe 100644
@@ -3529,48 +3529,28 @@ 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)
+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;
-
-		r = kvm_read_guest(vcpu->kvm, gpa, &gentry, min(bytes, 8));
+		*gpa &= ~(gpa_t)7;
+		*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;

@@ -3582,71 +3562,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)
+{
+	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) {
-			/*
-			 * 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;
-		}
+
+		spte = get_written_sptes(sp, gpa, &npte);
+		if (!spte)
+			continue;
+
 		local_flush = true;
-		spte = &sp->spt[page_offset / sizeof(*spte)];
 		while (npte--) {
 			entry = *spte;
 			mmu_page_zap_pte(vcpu->kvm, sp, spte);

Signature · 1 line

1.7.5.4

Xiao Guangrong

author

2011-08-30 02:35 UTC #21 lore raw

PATCH v3 10/11 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 cfe24fe..adaa160 100644
@@ -3601,6 +3601,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-30 02:36 UTC #22 lore raw

PATCH v3 11/11 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 after it is written, 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              |   57 +++++++++++++--------------------------
 arch/x86/kvm/paging_tmpl.h      |   12 +++-----
 3 files changed, 26 insertions(+), 49 deletions(-)

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

index 927ba73..9d17238 100644
@@ -239,6 +239,8 @@ struct kvm_mmu_page {
 	int clear_spte_count;
 #endif
 
+	int write_flooding_count;
+
 	struct rcu_head rcu;
 };

@@ -353,10 +355,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 +19 ~~-38~~

115 lines not on this page · raw message

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

index 3395ab2..379a795 100644
@@ -497,6 +497,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 	     shadow_walk_next(&it)) {
 		gfn_t table_gfn;
 
+		clear_sp_write_flooding_count(it.sptep);
 		drop_large_spte(vcpu, it.sptep);
 
 		sp = NULL;

@@ -522,6 +523,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 	     shadow_walk_next(&it)) {
 		gfn_t direct_gfn;
 
+		clear_sp_write_flooding_count(it.sptep);
 		validate_direct_spte(vcpu, it.sptep, direct_access);
 
 		drop_large_spte(vcpu, it.sptep);

@@ -536,6 +538,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		link_shadow_page(it.sptep, sp);
 	}
 
+	clear_sp_write_flooding_count(it.sptep);
 	mmu_set_spte(vcpu, it.sptep, access, gw->pte_access,
 		     user_fault, write_fault, emulate, it.level,
 		     gw->gfn, pfn, prefault, map_writable);

@@ -599,11 +602,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;
 	}

@@ -641,9 +642,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

Avi Kivity

2011-09-13 11:07 UTC #23 lore raw

Re: [PATCH v3 11/11] KVM: MMU: improve write flooding detected

Quoted 7 lines · 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

Quoted 5 lines

Instead of detected page accessed, we can detect whether the spte is accessed after it is written, 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

The spte may not be accessed, but other sptes in the same page can be accessed. An example is the fixmap area for kmap_atomic(), there will be a lot of pte writes but other sptes will be accessed without going through soft-mmu at all.

I think you have to read the parent_ptes->spte.accessed bits to be sure.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-09-13 19:16 UTC #24 lore raw

Re: [PATCH v3 11/11] KVM: MMU: improve write flooding detected

Quoted 8 lines · Avi Kivity

Quoted 7 lines · 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

Quoted 7 lines

Quoted 5 lines
Instead of detected page accessed, we can detect whether the spte is accessed after it is written, 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
The spte may not be accessed, but other sptes in the same page can be accessed. An example is the fixmap area for kmap_atomic(), there will be a lot of pte writes but other sptes will be accessed without going through soft-mmu at all.

I think this kind of shadow pae is mostly the last page table(level=1), maybe we can skip the write-flooding for the last shadow page, because the last shadow page can become unsync and it can not let page table write-protected.

Quoted 2 lines

I think you have to read the parent_ptes->spte.accessed bits to be sure.

I guess the overload of this way is little high:
- it needs to walk parent ptes for every shadow pages
- we need to clear the parent_ptes->spte.accessed bit when the page is written, and the tlb flush is needed.
no?

Avi Kivity

2011-09-14 09:59 UTC #25 lore raw

Re: [PATCH v3 11/11] KVM: MMU: improve write flooding detected

Quoted 6 lines · Xiao Guangrong

Quoted 2 lines
The spte may not be accessed, but other sptes in the same page can be accessed. An example is the fixmap area for kmap_atomic(), there will be a lot of pte writes but other sptes will be accessed without going through soft-mmu at all.
I think this kind of shadow pae is mostly the last page table(level=1), maybe we can skip the write-flooding for the last shadow page, because the last shadow page can become unsync and it can not let page table write-protected.

Yes.

Quoted 10 lines

Quoted 3 lines
I think you have to read the parent_ptes->spte.accessed bits to be sure.
I guess the overload of this way is little high:
- it needs to walk parent ptes for every shadow pages
- we need to clear the parent_ptes->spte.accessed bit when the page is written, and the tlb flush is needed.
no?

Right.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2011-09-13 09:51 UTC #26 lore raw

In reply to Xiao Guangrong

Quoted 8 lines · Xiao Guangrong

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

arch/x86/kvm/mmu.c int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code, · line 3739

Quoted 9 lines

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

@@ -3716,10 +3735,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);

Suppose we are out of memory, can't this get us in an endless loop?

return -ENOMEM breaks as out (and kills the guest, likely).

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-09-13 10:21 UTC #27 lore raw

Quoted 21 lines · Avi Kivity

Quoted 8 lines · Xiao Guangrong
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
arch/x86/kvm/mmu.c int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code, · line 3739
Quoted 9 lines
arch/x86/kvm/mmu.c +0 -4
@@ -3716,10 +3735,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);
 
Suppose we are out of memory, can't this get us in an endless loop?

Quoted 2 lines

return -ENOMEM breaks as out (and kills the guest, likely).

If memory is not enough, we just clear sptes on pte_write path(not prefetch spte), the later page fault path can return -1 to let guest crash. Hmm?

Avi Kivity

2011-09-13 10:50 UTC #28 lore raw

Quoted 20 lines · Xiao Guangrong

Quoted 19 lines · Avi Kivity
Quoted 17 lines · Xiao Guangrong
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
@@ -3716,10 +3735,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);

Quoted 8 lines

Quoted 4 lines
Suppose we are out of memory, can't this get us in an endless loop?
return -ENOMEM breaks as out (and kills the guest, likely).
If memory is not enough, we just clear sptes on pte_write path(not prefetch spte), the later page fault path can return -1 to let guest crash. Hmm?

Yes.

btw, is rmap_can_add() sufficent? We allocate more than just rmaps in mmu_topup_memory_caches(). I guess it is, but this is getting tricky.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2011-09-13 19:29 UTC #29 lore raw

Quoted 26 lines · Avi Kivity

Quoted 25 lines · Xiao Guangrong
Quoted 23 lines · Avi Kivity
Quoted 8 lines · Xiao Guangrong
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
@@ -3716,10 +3735,6 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
Quoted 9 lines
           goto out;
       }
- r = mmu_topup_memory_caches(vcpu);
- if (r)
- goto out;
-
       er = x86_emulate_instruction(vcpu, cr2, 0, insn, insn_len);
Suppose we are out of memory, can't this get us in an endless loop?
return -ENOMEM breaks as out (and kills the guest, likely).

Quoted 8 lines

Quoted 3 lines
If memory is not enough, we just clear sptes on pte_write path(not prefetch spte), the later page fault path can return -1 to let guest crash. Hmm?
Yes.
btw, is rmap_can_add() sufficent? We allocate more than just rmaps in mmu_topup_memory_caches(). I guess it is, but this is getting tricky.

rmap_can_add() is used to avoid prefetching sptes more than the number of rmaps in the cache, because we do not know the exact number of sptes to be fetched. :-)