KVM: x86: improve reexecute_instruction

Little cleanup for reexecute_instruction, also use gpa_to_gfn in retry_instruction

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

---
 arch/x86/kvm/x86.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

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

index 1c9c834..ad39018 100644

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

 	/*
 	 * 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_virt(vcpu, gva))
+	if (kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(gpa)))
 		return true;
 
-	gpa = kvm_mmu_gva_to_gpa_system(vcpu, gva, NULL);
-
-	if (gpa == UNMAPPED_GVA)
-		return true; /* let cpu generate fault */
-
 	/*
 	 * Do not retry the unhandleable instruction if it faults on the
 	 * readonly host memory, otherwise it will goto a infinite loop:

@@ -4828,7 +4827,7 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
 	if (!vcpu->arch.mmu.direct_map)
 		gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2, NULL);
 
-	kvm_mmu_unprotect_page(vcpu->kvm, gpa >> PAGE_SHIFT);
+	kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(gpa));
 
 	return true;
 }

Signature · 1 line

1.7.7.6

Quoted 16 lines · Xiao Guangrong

Little cleanup for reexecute_instruction, also use gpa_to_gfn in retry_instruction

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

---
 arch/x86/kvm/x86.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

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

index 1c9c834..ad39018 100644
@@ -4761,19 +4761,18 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva)
 	if (tdp_enabled)
 		return false;
 

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

Quoted 4 lines

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

+	gpa = kvm_mmu_gva_to_gpa_read(vcpu, gva, NULL);
+	if (gpa == UNMAPPED_GVA)
+		return true; /* let cpu generate fault */
+

Why change from _system to _read here? Purely cleanup patch should have no logical changes.

BTW, there is not much logic in using reexecute_instruction() at for x86_decode_insn (checks in reexecute_instruction() assume write to the cr2, for instance).
Fault propagation for x86_decode_insn seems completly broken (which is perhaps why reexecute_instruction() there survived).

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 |    7 +++++
 arch/x86/kvm/paging_tmpl.h      |   24 +++++++++++++-----
 arch/x86/kvm/x86.c              |   50 ++++++++++++++++++++++++--------------
 3 files changed, 55 insertions(+), 26 deletions(-)

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

index c431b33..de229e6 100644
@@ -502,6 +502,13 @@ struct kvm_vcpu_arch {
 		u64 msr_val;
 		struct gfn_to_hva_cache data;
 	} pv_eoi;
+
+	/*
+	 * Indicate whether the gfn is used as page table in guest which
+	 * is set when fix page fault and used to detect unhandeable
+	 * instruction.
+	 */
+	bool target_gfn_is_pt;
 };
 
 struct kvm_lpage_info {

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

index 0453fa0..ca1be75 100644
@@ -507,20 +507,27 @@ out_gpte_changed:
  */
 static bool
 FNAME(is_self_change_mapping)(struct kvm_vcpu *vcpu,
-			      struct guest_walker *walker, int user_fault)
+			      struct guest_walker *walker, int user_fault,
+			      bool *target_gfn_is_pt)
 {
 	int level;
 	gfn_t mask = ~(KVM_PAGES_PER_HPAGE(walker->level) - 1);
+	bool self_changed = false;
+
+	*target_gfn_is_pt = false;
 
 	if (!(walker->pte_access & ACC_WRITE_MASK ||
 	      (!is_write_protection(vcpu) && !user_fault)))
 		return false;
 
-	for (level = walker->level; level <= walker->max_level; level++)
-		if (!((walker->gfn ^ walker->table_gfn[level - 1]) & mask))
-			return true;
+	for (level = walker->level; level <= walker->max_level; level++) {
+		gfn_t gfn = walker->gfn ^ walker->table_gfn[level - 1];
+
+		self_changed |= !(gfn & mask);
+		*target_gfn_is_pt |= !gfn;
+	}
 
-	return false;
+	return self_changed;
 }
 
 /*

@@ -548,7 +555,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	int level = PT_PAGE_TABLE_LEVEL;
 	int force_pt_level;
 	unsigned long mmu_seq;
-	bool map_writable;
+	bool map_writable, is_self_change_mapping;
 
 	pgprintk("%s: addr %lx err %x\n", __func__, addr, error_code);
 

@@ -576,9 +583,12 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 		return 0;
 	}
 
+	is_self_change_mapping = FNAME(is_self_change_mapping)(vcpu,
+		      &walker, user_fault, &vcpu->arch.target_gfn_is_pt);
+
 	if (walker.level >= PT_DIRECTORY_LEVEL)
 		force_pt_level = mapping_level_dirty_bitmap(vcpu, walker.gfn)
-		   || FNAME(is_self_change_mapping)(vcpu, &walker, user_fault);
+		   || is_self_change_mapping;
 	else
 		force_pt_level = 1;
 	if (!force_pt_level) {

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

index b0a3678..44c6992 100644
@@ -4756,15 +4756,8 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 {
 	gpa_t gpa = cr2;

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

 	if (!vcpu->arch.mmu.direct_map) {
 		/*

@@ -4781,13 +4774,7 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 			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

@@ -4795,13 +4782,38 @@ 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);
+
+	/*

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

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

Quoted 11 lines

index b0a3678..44c6992 100644

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

@@ -4756,15 +4756,8 @@ 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;
-

arch/x86/kvm/x86.c static int handle_emulation_failure(struct kvm_vcpu *vcpu) · line 4767

Quoted 6 lines

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

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

This renders the previous patch obsolete, pretty much (please fold).

Quoted 32 lines

arch/x86/kvm/x86.c +8 -10

 	if (!vcpu->arch.mmu.direct_map) {
 		/*

@@ -4781,13 +4774,7 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 			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

@@ -4795,13 +4782,38 @@ 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))

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

Quoted 6 lines

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

+		return false;
+
+	kvm_release_pfn_clean(pfn);
+
+	/* The instructions are well-emulated on direct mmu. */
+	if (vcpu->arch.mmu.direct_map) {

!direct_map?

Quoted 12 lines

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

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

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

Quoted 10 lines

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

 
-	return false;
+	kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+
+	/* If the target gfn is used as page table, the fault can
+	 * not be avoided by unprotecting shadow page and it will
+	 * be reported to userspace.
+	 */
+	return !vcpu->arch.target_gfn_is_pt;
 }

The idea was

How about recording the gfn number for shadow pages that have been shadowed in the current pagefault run? (which is cheap, compared to shadowing these pages).

If failed instruction emulation is write to one of these gfns, then fail.

Quoted 11 lines · Marcelo Tosatti

Quoted 11 lines

index b0a3678..44c6992 100644

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

@@ -4756,15 +4756,8 @@ 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;
-

Quoted 8 lines

arch/x86/kvm/x86.c static int handle_emulation_failure(struct kvm_vcpu *vcpu) · line 4767

Quoted 6 lines

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

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

This renders the previous patch obsolete, pretty much (please fold).

Will try.

Quoted 32 lines

Quoted 32 lines

arch/x86/kvm/x86.c +8 -10

 	if (!vcpu->arch.mmu.direct_map) {
 		/*

@@ -4781,13 +4774,7 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 			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

@@ -4795,13 +4782,38 @@ 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))

Quoted 8 lines

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

Quoted 6 lines

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

+		return false;
+
+	kvm_release_pfn_clean(pfn);
+
+	/* The instructions are well-emulated on direct mmu. */
+	if (vcpu->arch.mmu.direct_map) {

!direct_map?

No. This logic is, if it is direct mmu, we just unprotect the page shadowed by nested mmu, then let guest retry the instruction, no need to detect unhandlable instruction.

Quoted 25 lines

Quoted 12 lines

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

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

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

Quoted 10 lines

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

 
-	return false;
+	kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+
+	/* If the target gfn is used as page table, the fault can
+	 * not be avoided by unprotecting shadow page and it will
+	 * be reported to userspace.
+	 */
+	return !vcpu->arch.target_gfn_is_pt;
 }

The idea was

Quoted 6 lines

How about recording the gfn number for shadow pages that have been shadowed in the current pagefault run? (which is cheap, compared to shadowing these pages).

If failed instruction emulation is write to one of these gfns, then fail.

If i understood correctly, i do not think it is simpler than the way in this patch.

There is the change to apply the idea:

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

index c431b33..2163de8 100644
@@ -502,6 +502,8 @@ struct kvm_vcpu_arch {
 		u64 msr_val;
 		struct gfn_to_hva_cache data;
 	} pv_eoi;
+
+	gfn_t pt_gfns[4];
 };
 
 struct kvm_lpage_info {

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

index 0453fa0..ac4210f 100644
@@ -523,6 +523,18 @@ FNAME(is_self_change_mapping)(struct kvm_vcpu *vcpu,
 	return false;
 }
 
+static void FNAME(cache_pt_gfns)(struct kvm_vcpu *vcpu, struct guest_walker *walker)
+{
+	int level;
+
+	/* Reset all gfns to -1, then we can detect the levels which is not used in guest. */
+	for (level = 0; level < 4; level++)
+		vcpu->arch.pt_gfns[level] = (gfn_t)(-1);
+
+	for (level = walker->level; level <= walker->max_level; level++)
+		vcpu->arch.pt_gfns[level - 1] = walker->table_gfn[level - 1];
+}
+
 /*
  * Page fault handler.  There are several causes for a page fault:
  *   - there is no shadow pte for the guest pte

@@ -576,6 +588,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 		return 0;
 	}
 
+	 FNAME(cache_pt_gfns)(vcpu, &walker);
+
 	if (walker.level >= PT_DIRECTORY_LEVEL)
 		force_pt_level = mapping_level_dirty_bitmap(vcpu, walker.gfn)
 		   || FNAME(is_self_change_mapping)(vcpu, &walker, user_fault);

arch/x86/kvm/x86.c +45 -19

index b0a3678..b86ee24 100644
@@ -4753,18 +4753,25 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 	return r;
 }
 
+static bool is_gfn_used_as_pt(struct kvm_vcpu *vcpu, gfn_t gfn)
+{
+	int level;
+
+	for (level = 0; level < 4; level++) {
+		if (vcpu->arch.pt_gfns[level] == (gfn_t)-1)
+			continue;
+		if (gfn == vcpu->arch.pt_gfns[level])
+			return true;
+	}
+
+	return false;
+}
+
 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;
 
 	if (!vcpu->arch.mmu.direct_map) {
 		/*

@@ -4781,13 +4788,7 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, unsigned long cr2)
 			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

@@ -4795,13 +4796,38 @@ 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;
+	kvm_mmu_unprotect_page(vcpu->kvm, gfn);
+
+	/* If the target gfn is used as page table, the fault can
+	 * not be avoided by unprotecting shadow page and it will
+	 * be reported to userspace.
+	 */
+	return !is_gfn_used_as_pt(vcpu, gfn);
 }
 
 static bool retry_instruction(struct x86_emulate_ctxt *ctxt,

You can see we need to record more things in the vcpu struct (bool vs. gfn_t [4]) and my patch can fold is_gfn_used_as_pt into a existed function FNAME(is_self_change_mapping).

Hmm?