KVM: x86: improve reexecute_instruction

Then, no mmu specified code exists in the common function and drop two parameters in set_spte

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

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

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

index 01d7c2a..2a3c890 100644
@@ -2342,8 +2342,7 @@ static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 }
 
 static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
-		    unsigned pte_access, int user_fault,
-		    int write_fault, int level,
+		    unsigned pte_access, int level,
 		    gfn_t gfn, pfn_t pfn, bool speculative,
 		    bool can_unsync, bool host_writable)
 {

@@ -2378,9 +2377,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 
 	spte |= (u64)pfn << PAGE_SHIFT;
 
-	if ((pte_access & ACC_WRITE_MASK)
-	    || (!vcpu->arch.mmu.direct_map && write_fault
-		&& !is_write_protection(vcpu) && !user_fault)) {
+	if (pte_access & ACC_WRITE_MASK) {
 
 		/*
 		 * There are two cases:

@@ -2399,19 +2396,6 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 
 		spte |= PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE;
 
-		if (!vcpu->arch.mmu.direct_map
-		    && !(pte_access & ACC_WRITE_MASK)) {
-			spte &= ~PT_USER_MASK;
-			/*
-			 * If we converted a user page to a kernel page,
-			 * so that the kernel can write to it when cr0.wp=0,
-			 * then we should prevent the kernel from executing it
-			 * if SMEP is enabled.
-			 */
-			if (kvm_read_cr4_bits(vcpu, X86_CR4_SMEP))
-				spte |= PT64_NX_MASK;
-		}
-
 		/*
 		 * Optimization: for pte sync, if spte was writable the hash
 		 * lookup is unnecessary (and expensive). Write protection

@@ -2442,18 +2426,15 @@ done:
 
 static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			 unsigned pt_access, unsigned pte_access,
-			 int user_fault, int write_fault,
-			 int *emulate, int level, gfn_t gfn,
-			 pfn_t pfn, bool speculative,
-			 bool host_writable)
+			 int write_fault, int *emulate, int level, gfn_t gfn,
+			 pfn_t pfn, bool speculative, bool host_writable)
 {
 	int was_rmapped = 0;
 	int rmap_count;
 
-	pgprintk("%s: spte %llx access %x write_fault %d"
-		 " user_fault %d gfn %llx\n",
+	pgprintk("%s: spte %llx access %x write_fault %d gfn %llx\n",
 		 __func__, *sptep, pt_access,
-		 write_fault, user_fault, gfn);
+		 write_fault, gfn);
 
 	if (is_rmap_spte(*sptep)) {
 		/*

@@ -2477,9 +2458,8 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			was_rmapped = 1;
 	}
 
-	if (set_spte(vcpu, sptep, pte_access, user_fault, write_fault,
-		      level, gfn, pfn, speculative, true,
-		      host_writable)) {
+	if (set_spte(vcpu, sptep, pte_access, level, gfn, pfn, speculative,
+	      true, host_writable)) {
 		if (write_fault)
 			*emulate = 1;
 		kvm_mmu_flush_tlb(vcpu);

@@ -2571,10 +2551,9 @@ static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
 		return -1;
 
 	for (i = 0; i < ret; i++, gfn++, start++)
-		mmu_set_spte(vcpu, start, ACC_ALL,
-			     access, 0, 0, NULL,
-			     sp->role.level, gfn,
-			     page_to_pfn(pages[i]), true, true);
+		mmu_set_spte(vcpu, start, ACC_ALL, access, 0, NULL,
+			     sp->role.level, gfn, page_to_pfn(pages[i]),
+			     true, true);
 
 	return 0;
 }

@@ -2636,8 +2615,8 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			unsigned pte_access = ACC_ALL;
 
 			mmu_set_spte(vcpu, iterator.sptep, ACC_ALL, pte_access,
-				     0, write, &emulate,
-				     level, gfn, pfn, prefault, map_writable);
+				     write, &emulate, level, gfn, pfn,
+				     prefault, map_writable);
 			direct_pte_prefetch(vcpu, iterator.sptep);
 			++vcpu->stat.pf_fixed;
 			break;

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

index 891eb6d..ec481e9 100644
@@ -330,7 +330,7 @@ FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	 * we call mmu_set_spte() with host_writable = true because
 	 * pte_prefetch_gfn_to_pfn always gets a writable pfn.
 	 */
-	mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
+	mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0,
 		     NULL, PT_PAGE_TABLE_LEVEL, gfn, pfn, true, true);
 
 	return true;

@@ -405,7 +405,7 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
  */
 static int FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 			 struct guest_walker *gw,
-			 int user_fault, int write_fault, int hlevel,
+			 int write_fault, int hlevel,
 			 pfn_t pfn, bool map_writable, bool prefault)
 {
 	struct kvm_mmu_page *sp = NULL;

@@ -478,7 +478,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 
 	clear_sp_write_flooding_count(it.sptep);
 	mmu_set_spte(vcpu, it.sptep, access, gw->pte_access,
-		     user_fault, write_fault, &emulate, it.level,
+		     write_fault, &emulate, it.level,
 		     gw->gfn, pfn, prefault, map_writable);
 	FNAME(pte_prefetch)(vcpu, gw, it.sptep);
 

@@ -544,6 +544,21 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 		return 0;
 	}
 
+	if (write_fault && !(walker.pte_access & ACC_WRITE_MASK) &&
+		  !is_write_protection(vcpu) && !user_fault) {
+		walker.pte_access |= ACC_WRITE_MASK;
+		walker.pte_access &= ~ACC_USER_MASK;

Quoted 9 lines

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

+	if (write_fault && !(walker.pte_access & ACC_WRITE_MASK) &&
+		  !is_write_protection(vcpu) && !user_fault) {
+		walker.pte_access |= ACC_WRITE_MASK;
+		walker.pte_access &= ~ACC_USER_MASK;
+
+		/*
+		 * If we converted a user page to a kernel page,
+		 * so that the kernel can write to it when cr0.wp=0,
+		 * then we should prevent the kernel from executing it

arch/x86/kvm/paging_tmpl.h static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code, · line 560

Quoted 5 lines

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

+		 * if SMEP is enabled.
+		 */
+		if (kvm_read_cr4_bits(vcpu, X86_CR4_SMEP))
+			walker.pte_access &= ~ACC_EXEC_MASK;
+	}

Don't think you should modify walker.pte_access here, since it can be used afterwards (eg for handle_abnormal_pfn).

BTW, your patch is fixing a bug:

host_writable is ignored for CR0.WP emulation:

        if (host_writable)
                spte |= SPTE_HOST_WRITEABLE;
        else
                pte_access &= ~ACC_WRITE_MASK;

        spte |= (u64)pfn << PAGE_SHIFT;

        if ((pte_access & ACC_WRITE_MASK)
            || (!vcpu->arch.mmu.direct_map && write_fault
                && !is_write_protection(vcpu) && !user_fault)) {

We have two issues in current code:
- if target gfn is used as its page table, guest will refault then kvm will use small page size to map it. We need two #PF to fix its shadow page table

- sometimes, say a exception is triggered during vm-exit caused by #PF (see handle_exception() in vmx.c), we remove all the shadow pages shadowed by the target gfn before go into page fault path, it will cause infinite loop:
delete shadow pages shadowed by the gfn -> try to use large page size to map the gfn -> retry the access ->...

To fix these, We can adjust page size early if the target gfn is used as page table

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

---
 arch/x86/kvm/mmu.c         |   13 ++++---------
 arch/x86/kvm/paging_tmpl.h |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 10 deletions(-)

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

index 2a3c890..54fc61e 100644
@@ -2380,15 +2380,10 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	if (pte_access & ACC_WRITE_MASK) {
 
 		/*
-		 * There are two cases:
-		 * - the one is other vcpu creates new sp in the window
-		 *   between mapping_level() and acquiring mmu-lock.
-		 * - the another case is the new sp is created by itself
-		 *   (page-fault path) when guest uses the target gfn as
-		 *   its page table.
-		 * Both of these cases can be fixed by allowing guest to
-		 * retry the access, it will refault, then we can establish
-		 * the mapping by using small page.
+		 * Other vcpu creates new sp in the window between
+		 * mapping_level() and acquiring mmu-lock. We can
+		 * allow guest to retry the access, the mapping can
+		 * be fixed if guest refault.
 		 */
 		if (level > PT_PAGE_TABLE_LEVEL &&
 		    has_wrprotected_page(vcpu->kvm, gfn, level))

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

index ec481e9..32d77ff 100644
@@ -491,6 +491,36 @@ out_gpte_changed:
 	return 0;
 }
 
+ /*
+ * To see whether the mapped gfn can write its page table in the current
+ * mapping.
+ *
+ * It is the helper function of FNAME(page_fault). When guest uses large page
+ * size to map the writable gfn which is used as current page table, we should
+ * force kvm to use small page size to map it because new shadow page will be
+ * created when kvm establishes shadow page table that stop kvm using large
+ * page size. Do it early can avoid unnecessary #PF and emulation.
+ *
+ * Note: the PDPT page table is not checked for PAE-32 bit guest. It is ok
+ * since the PDPT is always shadowed, that means, we can not use large page
+ * size to map the gfn which is used as PDPT.
+ */
+static bool
+FNAME(mapped_gfn_can_write_current_pagetable)(struct guest_walker *walker)

Quoted 73 lines · Xiao Guangrong

We have two issues in current code:
- if target gfn is used as its page table, guest will refault then kvm will use small page size to map it. We need two #PF to fix its shadow page table

- sometimes, say a exception is triggered during vm-exit caused by #PF (see handle_exception() in vmx.c), we remove all the shadow pages shadowed by the target gfn before go into page fault path, it will cause infinite loop:
delete shadow pages shadowed by the gfn -> try to use large page size to map the gfn -> retry the access ->...

To fix these, We can adjust page size early if the target gfn is used as page table

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

---
 arch/x86/kvm/mmu.c         |   13 ++++---------
 arch/x86/kvm/paging_tmpl.h |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 10 deletions(-)

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

index 2a3c890..54fc61e 100644
@@ -2380,15 +2380,10 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	if (pte_access & ACC_WRITE_MASK) {
 
 		/*
-		 * There are two cases:
-		 * - the one is other vcpu creates new sp in the window
-		 *   between mapping_level() and acquiring mmu-lock.
-		 * - the another case is the new sp is created by itself
-		 *   (page-fault path) when guest uses the target gfn as
-		 *   its page table.
-		 * Both of these cases can be fixed by allowing guest to
-		 * retry the access, it will refault, then we can establish
-		 * the mapping by using small page.
+		 * Other vcpu creates new sp in the window between
+		 * mapping_level() and acquiring mmu-lock. We can
+		 * allow guest to retry the access, the mapping can
+		 * be fixed if guest refault.
 		 */
 		if (level > PT_PAGE_TABLE_LEVEL &&
 		    has_wrprotected_page(vcpu->kvm, gfn, level))

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

index ec481e9..32d77ff 100644
@@ -491,6 +491,36 @@ out_gpte_changed:
 	return 0;
 }
 
+ /*
+ * To see whether the mapped gfn can write its page table in the current
+ * mapping.
+ *
+ * It is the helper function of FNAME(page_fault). When guest uses large page
+ * size to map the writable gfn which is used as current page table, we should
+ * force kvm to use small page size to map it because new shadow page will be
+ * created when kvm establishes shadow page table that stop kvm using large
+ * page size. Do it early can avoid unnecessary #PF and emulation.
+ *
+ * Note: the PDPT page table is not checked for PAE-32 bit guest. It is ok
+ * since the PDPT is always shadowed, that means, we can not use large page
+ * size to map the gfn which is used as PDPT.
+ */
+static bool
+FNAME(mapped_gfn_can_write_current_pagetable)(struct guest_walker *walker)
+{
+	int level;
+	gfn_t mask = ~(KVM_PAGES_PER_HPAGE(walker->level) - 1);
+
+	if (!(walker->pte_access & ACC_WRITE_MASK))

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

Quoted 5 lines

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

+		return false;
+
+	for (level = walker->level; level <= walker->max_level; level++)
+		if (!((walker->gfn ^ walker->table_gfn[level - 1]) & mask))
+			return true;

XOR won't work. Just check with sums and integer comparison, ie. walker->gfn + KVM_PAGES_PER_HPAGE(walker->level).

Moreover, its confusing to have it checked at this level. What about doing at reexecute_instruction?

Quoted 74 lines · Marcelo Tosatti

Quoted 73 lines · Xiao Guangrong

We have two issues in current code:
- if target gfn is used as its page table, guest will refault then kvm will use small page size to map it. We need two #PF to fix its shadow page table

- sometimes, say a exception is triggered during vm-exit caused by #PF (see handle_exception() in vmx.c), we remove all the shadow pages shadowed by the target gfn before go into page fault path, it will cause infinite loop:
delete shadow pages shadowed by the gfn -> try to use large page size to map the gfn -> retry the access ->...

To fix these, We can adjust page size early if the target gfn is used as page table

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

---
 arch/x86/kvm/mmu.c         |   13 ++++---------
 arch/x86/kvm/paging_tmpl.h |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 10 deletions(-)

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

index 2a3c890..54fc61e 100644
@@ -2380,15 +2380,10 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	if (pte_access & ACC_WRITE_MASK) {
 
 		/*
-		 * There are two cases:
-		 * - the one is other vcpu creates new sp in the window
-		 *   between mapping_level() and acquiring mmu-lock.
-		 * - the another case is the new sp is created by itself
-		 *   (page-fault path) when guest uses the target gfn as
-		 *   its page table.
-		 * Both of these cases can be fixed by allowing guest to
-		 * retry the access, it will refault, then we can establish
-		 * the mapping by using small page.
+		 * Other vcpu creates new sp in the window between
+		 * mapping_level() and acquiring mmu-lock. We can
+		 * allow guest to retry the access, the mapping can
+		 * be fixed if guest refault.
 		 */
 		if (level > PT_PAGE_TABLE_LEVEL &&
 		    has_wrprotected_page(vcpu->kvm, gfn, level))

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

index ec481e9..32d77ff 100644
@@ -491,6 +491,36 @@ out_gpte_changed:
 	return 0;
 }
 
+ /*
+ * To see whether the mapped gfn can write its page table in the current
+ * mapping.
+ *
+ * It is the helper function of FNAME(page_fault). When guest uses large page
+ * size to map the writable gfn which is used as current page table, we should
+ * force kvm to use small page size to map it because new shadow page will be
+ * created when kvm establishes shadow page table that stop kvm using large
+ * page size. Do it early can avoid unnecessary #PF and emulation.
+ *
+ * Note: the PDPT page table is not checked for PAE-32 bit guest. It is ok
+ * since the PDPT is always shadowed, that means, we can not use large page
+ * size to map the gfn which is used as PDPT.
+ */
+static bool
+FNAME(mapped_gfn_can_write_current_pagetable)(struct guest_walker *walker)
+{
+	int level;
+	gfn_t mask = ~(KVM_PAGES_PER_HPAGE(walker->level) - 1);
+
+	if (!(walker->pte_access & ACC_WRITE_MASK))

Quoted 8 lines

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

Quoted 5 lines

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

+		return false;
+
+	for (level = walker->level; level <= walker->max_level; level++)
+		if (!((walker->gfn ^ walker->table_gfn[level - 1]) & mask))
+			return true;

XOR won't work. Just check with sums and integer comparison, ie. walker->gfn + KVM_PAGES_PER_HPAGE(walker->level).

It can not work since walker->gfn is not large-page-size aligned. For example, guest uses large page size to map 0x123000000 to physical address 0-2M, if guest faults on 0x123001000, walker->gfn = 0x1000.

The code "if (!((walker->gfn ^ walker->table_gfn[level - 1]) & mask))" is the same as "if (walker->gfn & mask == walker->table_gfn[level - 1] & mask)" - if any page in the large page area used as page table, we should use 4K page size to fix it.

In above example, if table_gfn is in the area [0, 2M), kvm is forced to use 4k page size.

Quoted 2 lines

Moreover, its confusing to have it checked at this level. What about doing at reexecute_instruction?

Hmm, this patch is trying to fix a bug described in the changelog:

======
- sometimes, say a exception is triggered during vm-exit caused by #PF (see handle_exception() in vmx.c), we remove all the shadow pages shadowed by the target gfn before go into page fault path, it will cause infinite loop:
delete shadow pages shadowed by the gfn -> try to use large page size to map the gfn -> retry the access ->...
======

Which is caused by this code:

	if (is_page_fault(intr_info)) {
		/* EPT won't cause page fault directly */
		BUG_ON(enable_ept);
		cr2 = vmcs_readl(EXIT_QUALIFICATION);
		trace_kvm_page_fault(cr2, error_code);

		if (kvm_event_needs_reinjection(vcpu))
			kvm_mmu_unprotect_page_virt(vcpu, cr2);
		return kvm_mmu_page_fault(vcpu, cr2, error_code, NULL, 0);
	}

This bug is introduced in commit c219346325.

Another way to fix it is doing this change:

(quoted diff) +1 -1

@@ -2395,7 +2395,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
                 */
                if (level > PT_PAGE_TABLE_LEVEL &&
                    has_wrprotected_page(vcpu->kvm, gfn, level))
-                       goto done;
+                       return 1;
 
 The disadvantage of this way is, it causes unnecessary emulation. For example,

if 0-2M is mapped in guest and only page 0 used as page table, any write to [4K, 2M) will need be emulated.

Your idea?

The current reexecute_instruction can not well detect the failed instruction emulation. It allows guest to retry all the instructions except it accesses on error pfn

For example, some cases are nested-write-protect - if the page we want to write is used as PDE but it chains to itself. Under this case, we should stop the emulation and report the case to userspace

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

---
 arch/x86/include/asm/kvm_host.h |    2 +
 arch/x86/kvm/paging_tmpl.h      |    2 +
 arch/x86/kvm/x86.c              |   82 +++++++++++++++++++++++++++++----------
 3 files changed, 65 insertions(+), 21 deletions(-)

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

index dc87b65..8d01c02 100644
@@ -575,6 +575,8 @@ struct kvm_arch {
 	u64 hv_guest_os_id;
 	u64 hv_hypercall;
 
+	/* synchronizing reexecute_instruction and page fault path. */
+	u64 page_fault_count;
 	#ifdef CONFIG_KVM_MMU_AUDIT
 	int audit_point;
 	#endif

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

index 32d77ff..85b8e0e 100644
@@ -614,6 +614,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	if (mmu_notifier_retry(vcpu->kvm, mmu_seq))
 		goto out_unlock;
 
+	vcpu->kvm->arch.page_fault_count++;
+
 	kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
 	kvm_mmu_free_some_pages(vcpu);
 	if (!force_pt_level)

arch/x86/kvm/x86.c +54 -21

index 3796f8c..5677869 100644
@@ -4756,29 +4756,27 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 {
 	gpa_t gpa = cr2;
+	gfn_t gfn;
 	pfn_t pfn;
-	unsigned int indirect_shadow_pages;
-
-	spin_lock(&vcpu->kvm->mmu_lock);
-	indirect_shadow_pages = vcpu->kvm->arch.indirect_shadow_pages;
-	spin_unlock(&vcpu->kvm->mmu_lock);
-
-	if (!indirect_shadow_pages)
-		return false;
+	u64 page_fault_count;
+	int emulate;
 
 	if (!vcpu->arch.mmu.direct_map) {
-		gpa = kvm_mmu_gva_to_gpa_read(vcpu, cr2, NULL);
+		/*
+		 * Write permission should be allowed since only
+		 * write access need to be emulated.
+		 */
+		gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);
+
+		/*
+		 * If the mapping is invalid in guest, let cpu retry
+		 * it to generate fault.
+		 */
 		if (gpa == UNMAPPED_GVA)
-			return true; /* let cpu generate fault */
+			return true;
 	}
 
-	/*
-	 * if emulation was due to access to shadowed page table
-	 * and it failed try to unshadow page and re-enter the
-	 * guest to let CPU execute the instruction.
-	 */
-	if (kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(gpa)))
-		return true;
+	gfn = gpa_to_gfn(gpa);
 
 	/*
 	 * Do not retry the unhandleable instruction if it faults on the

@@ -4786,13 +4784,55 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 	 * retry instruction -> write #PF -> emulation fail -> retry
 	 * instruction -> ...
 	 */
-	pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
-	if (!is_error_noslot_pfn(pfn)) {
-		kvm_release_pfn_clean(pfn);
+	pfn = gfn_to_pfn(vcpu->kvm, gfn);
+
+	/*
+	 * If the instruction failed on the error pfn, it can not be fixed,
+	 * report the error to userspace.
+	 */
+	if (is_error_noslot_pfn(pfn))
+		return false;
+
+	kvm_release_pfn_clean(pfn);
+
+	/* The instructions are well-emulated on direct mmu. */
+	if (vcpu->arch.mmu.direct_map) {
+		unsigned int indirect_shadow_pages;
+
+		spin_lock(&vcpu->kvm->mmu_lock);
+		indirect_shadow_pages = vcpu->kvm->arch.indirect_shadow_pages;
+		spin_unlock(&vcpu->kvm->mmu_lock);
+
+		if (indirect_shadow_pages)
+			kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+
 		return true;
 	}
 
-	return false;
+again:
+	page_fault_count = ACCESS_ONCE(vcpu->kvm->arch.page_fault_count);
+
+	/*
+	 * The instruction emulation is caused by fault access on cr2.
+	 * After unprotect the target page, we call
+	 * vcpu->arch.mmu.page_fault to fix the mapping of cr2. If it
+	 * return 1, mmu can not fix the mapping, we should report the
+	 * error, otherwise it is good to return to guest and let it
+	 * re-execute the instruction again.
+	 *
+	 * page_fault_count is used to avoid the race on other vcpus,
+	 * since after we unprotect the target page, other cpu can enter
+	 * page fault path and let the page be write-protected again.
+	 */
+	kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+	emulate = vcpu->arch.mmu.page_fault(vcpu, cr2, PFERR_WRITE_MASK, false);

Quoted 140 lines · Xiao Guangrong

The current reexecute_instruction can not well detect the failed instruction emulation. It allows guest to retry all the instructions except it accesses on error pfn

For example, some cases are nested-write-protect - if the page we want to write is used as PDE but it chains to itself. Under this case, we should stop the emulation and report the case to userspace

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

---
 arch/x86/include/asm/kvm_host.h |    2 +
 arch/x86/kvm/paging_tmpl.h      |    2 +
 arch/x86/kvm/x86.c              |   82 +++++++++++++++++++++++++++++----------
 3 files changed, 65 insertions(+), 21 deletions(-)

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

index dc87b65..8d01c02 100644
@@ -575,6 +575,8 @@ struct kvm_arch {
 	u64 hv_guest_os_id;
 	u64 hv_hypercall;
 
+	/* synchronizing reexecute_instruction and page fault path. */
+	u64 page_fault_count;
 	#ifdef CONFIG_KVM_MMU_AUDIT
 	int audit_point;
 	#endif

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

index 32d77ff..85b8e0e 100644
@@ -614,6 +614,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	if (mmu_notifier_retry(vcpu->kvm, mmu_seq))
 		goto out_unlock;
 
+	vcpu->kvm->arch.page_fault_count++;
+
 	kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
 	kvm_mmu_free_some_pages(vcpu);
 	if (!force_pt_level)

arch/x86/kvm/x86.c +54 -21

index 3796f8c..5677869 100644
@@ -4756,29 +4756,27 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 {
 	gpa_t gpa = cr2;
+	gfn_t gfn;
 	pfn_t pfn;
-	unsigned int indirect_shadow_pages;
-
-	spin_lock(&vcpu->kvm->mmu_lock);
-	indirect_shadow_pages = vcpu->kvm->arch.indirect_shadow_pages;
-	spin_unlock(&vcpu->kvm->mmu_lock);
-
-	if (!indirect_shadow_pages)
-		return false;
+	u64 page_fault_count;
+	int emulate;
 
 	if (!vcpu->arch.mmu.direct_map) {
-		gpa = kvm_mmu_gva_to_gpa_read(vcpu, cr2, NULL);
+		/*
+		 * Write permission should be allowed since only
+		 * write access need to be emulated.
+		 */
+		gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);
+
+		/*
+		 * If the mapping is invalid in guest, let cpu retry
+		 * it to generate fault.
+		 */
 		if (gpa == UNMAPPED_GVA)
-			return true; /* let cpu generate fault */
+			return true;
 	}
 
-	/*
-	 * if emulation was due to access to shadowed page table
-	 * and it failed try to unshadow page and re-enter the
-	 * guest to let CPU execute the instruction.
-	 */
-	if (kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(gpa)))
-		return true;
+	gfn = gpa_to_gfn(gpa);
 
 	/*
 	 * Do not retry the unhandleable instruction if it faults on the

@@ -4786,13 +4784,55 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 	 * retry instruction -> write #PF -> emulation fail -> retry
 	 * instruction -> ...
 	 */
-	pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
-	if (!is_error_noslot_pfn(pfn)) {
-		kvm_release_pfn_clean(pfn);
+	pfn = gfn_to_pfn(vcpu->kvm, gfn);
+
+	/*
+	 * If the instruction failed on the error pfn, it can not be fixed,
+	 * report the error to userspace.
+	 */
+	if (is_error_noslot_pfn(pfn))
+		return false;
+
+	kvm_release_pfn_clean(pfn);
+
+	/* The instructions are well-emulated on direct mmu. */
+	if (vcpu->arch.mmu.direct_map) {
+		unsigned int indirect_shadow_pages;
+
+		spin_lock(&vcpu->kvm->mmu_lock);
+		indirect_shadow_pages = vcpu->kvm->arch.indirect_shadow_pages;
+		spin_unlock(&vcpu->kvm->mmu_lock);
+
+		if (indirect_shadow_pages)
+			kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+
 		return true;
 	}
 
-	return false;
+again:
+	page_fault_count = ACCESS_ONCE(vcpu->kvm->arch.page_fault_count);
+
+	/*
+	 * The instruction emulation is caused by fault access on cr2.
+	 * After unprotect the target page, we call
+	 * vcpu->arch.mmu.page_fault to fix the mapping of cr2. If it
+	 * return 1, mmu can not fix the mapping, we should report the
+	 * error, otherwise it is good to return to guest and let it
+	 * re-execute the instruction again.
+	 *
+	 * page_fault_count is used to avoid the race on other vcpus,
+	 * since after we unprotect the target page, other cpu can enter
+	 * page fault path and let the page be write-protected again.
+	 */
+	kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+	emulate = vcpu->arch.mmu.page_fault(vcpu, cr2, PFERR_WRITE_MASK, false);

arch/x86/kvm/x86.c static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2) · line 4838

Quoted 10 lines

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

+
+	/* The page fault path called above can increase the count. */
+	if (page_fault_count + 1 !=
+		  ACCESS_ONCE(vcpu->kvm->arch.page_fault_count))
+		goto again;
+
+	return !emulate;
 }
 
 static bool retry_instruction(struct x86_emulate_ctxt *ctxt,\

Same comment as before: the only case where it should not attempt to emulate is when there is a condition which makes it impossible to fix (the information is available to detect that condition).

The earlier suggestion

"How about recording the gfn number for shadow pages that have been
shadowed in the current pagefault run?"

Was about that.

Quoted 156 lines · Marcelo Tosatti

Quoted 140 lines · Xiao Guangrong

The current reexecute_instruction can not well detect the failed instruction emulation. It allows guest to retry all the instructions except it accesses on error pfn

For example, some cases are nested-write-protect - if the page we want to write is used as PDE but it chains to itself. Under this case, we should stop the emulation and report the case to userspace

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

---
 arch/x86/include/asm/kvm_host.h |    2 +
 arch/x86/kvm/paging_tmpl.h      |    2 +
 arch/x86/kvm/x86.c              |   82 +++++++++++++++++++++++++++++----------
 3 files changed, 65 insertions(+), 21 deletions(-)

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

index dc87b65..8d01c02 100644
@@ -575,6 +575,8 @@ struct kvm_arch {
 	u64 hv_guest_os_id;
 	u64 hv_hypercall;
 
+	/* synchronizing reexecute_instruction and page fault path. */
+	u64 page_fault_count;
 	#ifdef CONFIG_KVM_MMU_AUDIT
 	int audit_point;
 	#endif

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

index 32d77ff..85b8e0e 100644
@@ -614,6 +614,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	if (mmu_notifier_retry(vcpu->kvm, mmu_seq))
 		goto out_unlock;
 
+	vcpu->kvm->arch.page_fault_count++;
+
 	kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
 	kvm_mmu_free_some_pages(vcpu);
 	if (!force_pt_level)

arch/x86/kvm/x86.c +54 -21

index 3796f8c..5677869 100644
@@ -4756,29 +4756,27 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 {
 	gpa_t gpa = cr2;
+	gfn_t gfn;
 	pfn_t pfn;
-	unsigned int indirect_shadow_pages;
-
-	spin_lock(&vcpu->kvm->mmu_lock);
-	indirect_shadow_pages = vcpu->kvm->arch.indirect_shadow_pages;
-	spin_unlock(&vcpu->kvm->mmu_lock);
-
-	if (!indirect_shadow_pages)
-		return false;
+	u64 page_fault_count;
+	int emulate;
 
 	if (!vcpu->arch.mmu.direct_map) {
-		gpa = kvm_mmu_gva_to_gpa_read(vcpu, cr2, NULL);
+		/*
+		 * Write permission should be allowed since only
+		 * write access need to be emulated.
+		 */
+		gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);
+
+		/*
+		 * If the mapping is invalid in guest, let cpu retry
+		 * it to generate fault.
+		 */
 		if (gpa == UNMAPPED_GVA)
-			return true; /* let cpu generate fault */
+			return true;
 	}
 
-	/*
-	 * if emulation was due to access to shadowed page table
-	 * and it failed try to unshadow page and re-enter the
-	 * guest to let CPU execute the instruction.
-	 */
-	if (kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(gpa)))
-		return true;
+	gfn = gpa_to_gfn(gpa);
 
 	/*
 	 * Do not retry the unhandleable instruction if it faults on the

@@ -4786,13 +4784,55 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 	 * retry instruction -> write #PF -> emulation fail -> retry
 	 * instruction -> ...
 	 */
-	pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
-	if (!is_error_noslot_pfn(pfn)) {
-		kvm_release_pfn_clean(pfn);
+	pfn = gfn_to_pfn(vcpu->kvm, gfn);
+
+	/*
+	 * If the instruction failed on the error pfn, it can not be fixed,
+	 * report the error to userspace.
+	 */
+	if (is_error_noslot_pfn(pfn))
+		return false;
+
+	kvm_release_pfn_clean(pfn);
+
+	/* The instructions are well-emulated on direct mmu. */
+	if (vcpu->arch.mmu.direct_map) {
+		unsigned int indirect_shadow_pages;
+
+		spin_lock(&vcpu->kvm->mmu_lock);
+		indirect_shadow_pages = vcpu->kvm->arch.indirect_shadow_pages;
+		spin_unlock(&vcpu->kvm->mmu_lock);
+
+		if (indirect_shadow_pages)
+			kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+
 		return true;
 	}
 
-	return false;
+again:
+	page_fault_count = ACCESS_ONCE(vcpu->kvm->arch.page_fault_count);
+
+	/*
+	 * The instruction emulation is caused by fault access on cr2.
+	 * After unprotect the target page, we call
+	 * vcpu->arch.mmu.page_fault to fix the mapping of cr2. If it
+	 * return 1, mmu can not fix the mapping, we should report the
+	 * error, otherwise it is good to return to guest and let it
+	 * re-execute the instruction again.
+	 *
+	 * page_fault_count is used to avoid the race on other vcpus,
+	 * since after we unprotect the target page, other cpu can enter
+	 * page fault path and let the page be write-protected again.
+	 */
+	kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+	emulate = vcpu->arch.mmu.page_fault(vcpu, cr2, PFERR_WRITE_MASK, false);

arch/x86/kvm/x86.c static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2) · line 4838

Quoted 10 lines

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

+
+	/* The page fault path called above can increase the count. */
+	if (page_fault_count + 1 !=
+		  ACCESS_ONCE(vcpu->kvm->arch.page_fault_count))
+		goto again;
+
+	return !emulate;
 }
 
 static bool retry_instruction(struct x86_emulate_ctxt *ctxt,\

Same comment as before: the only case where it should not attempt to emulate is when there is a condition which makes it impossible to fix (the information is available to detect that condition).

Quoted 6 lines

The earlier suggestion

"How about recording the gfn number for shadow pages that have been
shadowed in the current pagefault run?"

Was about that.

I think we can have a try. Is this change good to you, Marcelo?

[eric@localhost kvm]$ git diff

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

index 01d7c2a..e3d0001 100644
@@ -4359,24 +4359,34 @@ unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm)
        return nr_mmu_pages;
 }
 
-int kvm_mmu_get_spte_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 sptes[4])
+void kvm_mmu_get_sp_hierarchy(struct kvm_vcpu *vcpu, u64 addr,
+                             struct kvm_mmu_sp_hierarchy *hierarchy)
 {
        struct kvm_shadow_walk_iterator iterator;
        u64 spte;
-       int nr_sptes = 0;
+
+       hierarchy->max_level = hierarchy->nr_levels = 0;
 
        walk_shadow_page_lockless_begin(vcpu);
        for_each_shadow_entry_lockless(vcpu, addr, iterator, spte) {
-               sptes[iterator.level-1] = spte;
-               nr_sptes++;
+               struct kvm_mmu_page *sp =  page_header(__pa(iterator.sptep));
+
+               if (hierarchy->indirect_only && sp->role.direct)
+                       break;
+
+               if (!hierarchy->max_level)
+                       hierarchy->max_level = iterator.level;
+
+               hierarchy->shadow_gfns[iterator.level-1] = sp->gfn;

Quoted 51 lines

Quoted 10 lines

Same comment as before: the only case where it should not attempt to emulate is when there is a condition which makes it impossible to fix (the information is available to detect that condition).

The earlier suggestion

"How about recording the gfn number for shadow pages that have been
shadowed in the current pagefault run?"

Was about that.

I think we can have a try. Is this change good to you, Marcelo?

[eric@localhost kvm]$ git diff

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

index 01d7c2a..e3d0001 100644
@@ -4359,24 +4359,34 @@ unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm)
        return nr_mmu_pages;
 }
 
-int kvm_mmu_get_spte_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 sptes[4])
+void kvm_mmu_get_sp_hierarchy(struct kvm_vcpu *vcpu, u64 addr,
+                             struct kvm_mmu_sp_hierarchy *hierarchy)
 {
        struct kvm_shadow_walk_iterator iterator;
        u64 spte;
-       int nr_sptes = 0;
+
+       hierarchy->max_level = hierarchy->nr_levels = 0;
 
        walk_shadow_page_lockless_begin(vcpu);
        for_each_shadow_entry_lockless(vcpu, addr, iterator, spte) {
-               sptes[iterator.level-1] = spte;
-               nr_sptes++;
+               struct kvm_mmu_page *sp =  page_header(__pa(iterator.sptep));
+
+               if (hierarchy->indirect_only && sp->role.direct)
+                       break;
+
+               if (!hierarchy->max_level)
+                       hierarchy->max_level = iterator.level;
+
+               hierarchy->shadow_gfns[iterator.level-1] = sp->gfn;
+               hierarchy->sptes[iterator.level-1] = spte;
+               hierarchy->nr_levels++;
+
                if (!is_shadow_present_pte(spte))
                        break;
        }

arch/x86/kvm/mmu.c unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm) · line 4388

Quoted 4 lines

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

        walk_shadow_page_lockless_end(vcpu);
-
-       return nr_sptes;
 }

Record gfns while shadowing in the vcpu struct, in a struct, along with cr2. Then validate
That way its guaranteed its not some other vcpu.

Changelog:
There are some changes from Marcelo and Gleb's review, thank you all!
- access indirect_shadow_pages in the protection of mmu-lock
- fix the issue when unhandleable instruction access on large page
- add a new test case for large page

The current reexecute_instruction can not well detect the failed instruction emulation. It allows guest to retry all the instructions except it accesses on error pfn.

For example, these cases can not be detected:
- for tdp used
currently, it refused to retry all instructions. If nested npt is used, the emulation may be caused by shadow page, it can be fixed by unshadow the shadow page.

- for shadow mmu
some cases are nested-write-protect, for example, if the page we want to write is used as PDE but it chains to itself. Under this case, we should stop the emulation and report the case to userspace.

There are two test cases based on kvm-unit-test can trigger a infinite loop on current code (ept = 0), after this patchset, it can report the error to Qemu.

Subject: [PATCH] access test: test unhandleable instruction

Test the instruction which can not be handled by kvm

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

Quoted 24 lines · Xiao Guangrong

Changelog:
There are some changes from Marcelo and Gleb's review, thank you all!
- access indirect_shadow_pages in the protection of mmu-lock
- fix the issue when unhandleable instruction access on large page
- add a new test case for large page

The current reexecute_instruction can not well detect the failed instruction emulation. It allows guest to retry all the instructions except it accesses on error pfn.

For example, these cases can not be detected:
- for tdp used
currently, it refused to retry all instructions. If nested npt is used, the emulation may be caused by shadow page, it can be fixed by unshadow the shadow page.

- for shadow mmu
some cases are nested-write-protect, for example, if the page we want to write is used as PDE but it chains to itself. Under this case, we should stop the emulation and report the case to userspace.

There are two test cases based on kvm-unit-test can trigger a infinite loop on current code (ept = 0), after this patchset, it can report the error to Qemu.

Quoted 5 lines

Subject: [PATCH] access test: test unhandleable instruction

Test the instruction which can not be handled by kvm

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

Please submit the test for inclusion. There should be some way to make it fail..

program a timer interrupt and #GP?

Quoted 25 lines · Marcelo Tosatti

Quoted 24 lines · Xiao Guangrong

Changelog:
There are some changes from Marcelo and Gleb's review, thank you all!
- access indirect_shadow_pages in the protection of mmu-lock
- fix the issue when unhandleable instruction access on large page
- add a new test case for large page

The current reexecute_instruction can not well detect the failed instruction emulation. It allows guest to retry all the instructions except it accesses on error pfn.

For example, these cases can not be detected:
- for tdp used
currently, it refused to retry all instructions. If nested npt is used, the emulation may be caused by shadow page, it can be fixed by unshadow the shadow page.

- for shadow mmu
some cases are nested-write-protect, for example, if the page we want to write is used as PDE but it chains to itself. Under this case, we should stop the emulation and report the case to userspace.

There are two test cases based on kvm-unit-test can trigger a infinite loop on current code (ept = 0), after this patchset, it can report the error to Qemu.

Quoted 8 lines

Quoted 5 lines

Subject: [PATCH] access test: test unhandleable instruction

Test the instruction which can not be handled by kvm

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

Please submit the test for inclusion. There should be some way to make it fail..

Yes.

But it is not easy. If the test cases run normally, kvm will report a error to Qemu then Qemu will exit the vcpu thread after dumping the vcpu state.

We need to do something to let guest can be aware that the error report is triggered. I guess we can add a option in Qemu, say '-notify-guest' and allow Qemu to inject #GP to guest with a special ERROR_CODE if error is reported.

Quoted 1 line

program a timer interrupt and #GP?

Could you please explain the detail?

Quoted 44 lines · Xiao Guangrong

Quoted 25 lines · Marcelo Tosatti

Quoted 24 lines · Xiao Guangrong

Changelog:
There are some changes from Marcelo and Gleb's review, thank you all!
- access indirect_shadow_pages in the protection of mmu-lock
- fix the issue when unhandleable instruction access on large page
- add a new test case for large page

The current reexecute_instruction can not well detect the failed instruction emulation. It allows guest to retry all the instructions except it accesses on error pfn.

For example, these cases can not be detected:
- for tdp used
currently, it refused to retry all instructions. If nested npt is used, the emulation may be caused by shadow page, it can be fixed by unshadow the shadow page.

- for shadow mmu
some cases are nested-write-protect, for example, if the page we want to write is used as PDE but it chains to itself. Under this case, we should stop the emulation and report the case to userspace.

There are two test cases based on kvm-unit-test can trigger a infinite loop on current code (ept = 0), after this patchset, it can report the error to Qemu.

Quoted 8 lines

Quoted 5 lines

Subject: [PATCH] access test: test unhandleable instruction

Test the instruction which can not be handled by kvm

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

Please submit the test for inclusion. There should be some way to make it fail..

Yes.

But it is not easy. If the test cases run normally, kvm will report a error to Qemu then Qemu will exit the vcpu thread after dumping the vcpu state.

We need to do something to let guest can be aware that the error report is triggered. I guess we can add a option in Qemu, say '-notify-guest' and allow Qemu to inject #GP to guest with a special ERROR_CODE if error is reported.

Quoted 3 lines

Quoted 1 line

program a timer interrupt and #GP?

Could you please explain the detail?

Before the instruction which writes continuously to the pagetable, arm say lapic timer. #GP on the interrupt handler and test with failure.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

program a timer interrupt and #GP?

Could you please explain the detail?

Before the instruction which writes continuously to the pagetable, arm say lapic timer. #GP on the interrupt handler and test with failure.

Sorry, I am confused about this. After Qemu exits due to KVM_EXIT_INTERNAL_ERROR, the vm is stopped then interrupt can not be injected to guest. Or i missed something?

Quoted 4 lines · Xiao Guangrong

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

program a timer interrupt and #GP?

Could you please explain the detail?

Quoted 5 lines

Quoted 2 lines

Before the instruction which writes continuously to the pagetable, arm say lapic timer. #GP on the interrupt handler and test with failure.

Sorry, I am confused about this. After Qemu exits due to KVM_EXIT_INTERNAL_ERROR, the vm is stopped then interrupt can not be injected to guest. Or i missed something?

Yes, but without fixed kernel kvm-unit test executable loops continuously. Perhaps its more appropriate to fix generically, nevermind.