KVM: fix tracing kvm_try_async_get_page

PATCH In review Xiao Guangrong 3 participants 26 messages Nov 1, 2010 – Nov 3, 2010

Subsystems (4)

x86 KVM Tracing KVM x86

lore

Share & tools

lore

Xiao Guangrong

author

2010-11-01 08:54 UTC #0 lore raw

PATCH v2 1/7 KVM: fix tracing kvm_try_async_get_page

Tracing 'async' and *pfn is useless, since 'async' is always true, and '*pfn' is always "fault_pfn'

We can trace 'gva' and 'gfn' instead, it can help us to see the life-cycle of an async_pf

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

---
 arch/x86/kvm/mmu.c         |    2 +-
 include/trace/events/kvm.h |   12 +++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

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

index 39cc0c6..5275c50 100644
@@ -2627,7 +2627,7 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool no_apf, gfn_t gfn,
 	put_page(pfn_to_page(*pfn));
 
 	if (!no_apf && can_do_async_pf(vcpu)) {
-		trace_kvm_try_async_get_page(async, *pfn);
+		trace_kvm_try_async_get_page(gva, gfn);
 		if (kvm_find_async_pf_gfn(vcpu, gfn)) {
 			trace_kvm_async_pf_doublefault(gva, gfn);
 			kvm_make_request(KVM_REQ_APF_HALT, vcpu);

include/trace/events/kvm.h +7 -5

index 9c2cc6a..30063c6 100644
@@ -188,18 +188,20 @@ TRACE_EVENT(kvm_age_page,
 #ifdef CONFIG_KVM_ASYNC_PF
 TRACE_EVENT(
 	kvm_try_async_get_page,
-	TP_PROTO(bool async, u64 pfn),
-	TP_ARGS(async, pfn),
+	TP_PROTO(u64 gva, u64 gfn),
+	TP_ARGS(gva, gfn),
 
 	TP_STRUCT__entry(
-		__field(__u64, pfn)
+		__field(u64, gva)
+		__field(u64, gfn)
 		),
 
 	TP_fast_assign(
-		__entry->pfn = (!async) ? pfn : (u64)-1;
+		__entry->gva = gva;
+		__entry->gfn = gfn;
 		),
 
-	TP_printk("pfn %#llx", __entry->pfn)
+	TP_printk("gva = %#llx, gfn = %#llx", __entry->gva, __entry->gfn)
 );
 
 TRACE_EVENT(

Signature · 1 line

1.7.0.4

Xiao Guangrong

author

2010-11-01 08:55 UTC #1 lore raw

PATCH v2 2/7 KVM: cleanup aysnc_pf tracepoints

Use 'DECLARE_EVENT_CLASS' to cleanup async_pf tracepoints

Acked-by Gleb Natapov <[email protected]>

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

---
 include/trace/events/kvm.h |   76 ++++++++++++++++++++-----------------------
 1 files changed, 35 insertions(+), 41 deletions(-)

include/trace/events/kvm.h +35 ~~-41~~

index 30063c6..7bec396 100644
@@ -186,59 +186,71 @@ TRACE_EVENT(kvm_age_page,
 );
 
 #ifdef CONFIG_KVM_ASYNC_PF
-TRACE_EVENT(
-	kvm_try_async_get_page,
+DECLARE_EVENT_CLASS(kvm_async_get_page_class,
+
 	TP_PROTO(u64 gva, u64 gfn),
+
 	TP_ARGS(gva, gfn),
 
 	TP_STRUCT__entry(
-		__field(u64, gva)
+		__field(__u64, gva)
 		__field(u64, gfn)
-		),
+	),
 
 	TP_fast_assign(
 		__entry->gva = gva;
 		__entry->gfn = gfn;
-		),
+	),
 
 	TP_printk("gva = %#llx, gfn = %#llx", __entry->gva, __entry->gfn)
 );
 
-TRACE_EVENT(
-	kvm_async_pf_not_present,
+DEFINE_EVENT(kvm_async_get_page_class, kvm_try_async_get_page,
+
+	TP_PROTO(u64 gva, u64 gfn),
+
+	TP_ARGS(gva, gfn)
+);
+
+DEFINE_EVENT(kvm_async_get_page_class, kvm_async_pf_doublefault,
+
+	TP_PROTO(u64 gva, u64 gfn),
+
+	TP_ARGS(gva, gfn)
+);
+
+DECLARE_EVENT_CLASS(kvm_async_pf_nopresent_ready,
+
 	TP_PROTO(u64 token, u64 gva),
+
 	TP_ARGS(token, gva),
 
 	TP_STRUCT__entry(
 		__field(__u64, token)
 		__field(__u64, gva)
-		),
+	),
 
 	TP_fast_assign(
 		__entry->token = token;
 		__entry->gva = gva;
-		),
+	),
+
+	TP_printk("token %#llx gva %#llx", __entry->token, __entry->gva)
 
-	TP_printk("token %#llx gva %#llx not present", __entry->token,
-		  __entry->gva)
 );
 
-TRACE_EVENT(
-	kvm_async_pf_ready,
+DEFINE_EVENT(kvm_async_pf_nopresent_ready, kvm_async_pf_not_present,
+
 	TP_PROTO(u64 token, u64 gva),
-	TP_ARGS(token, gva),
 
-	TP_STRUCT__entry(
-		__field(__u64, token)
-		__field(__u64, gva)
-		),
+	TP_ARGS(token, gva)
+);
 
-	TP_fast_assign(
-		__entry->token = token;
-		__entry->gva = gva;
-		),
+DEFINE_EVENT(kvm_async_pf_nopresent_ready, kvm_async_pf_ready,
+
+	TP_PROTO(u64 token, u64 gva),
 
-	TP_printk("token %#llx gva %#llx ready", __entry->token, __entry->gva)
+	TP_ARGS(token, gva)
 );
 
 TRACE_EVENT(

@@ -262,24 +274,6 @@ TRACE_EVENT(
 		  __entry->address, __entry->pfn)
 );
 
-TRACE_EVENT(
-	kvm_async_pf_doublefault,
-	TP_PROTO(u64 gva, u64 gfn),
-	TP_ARGS(gva, gfn),
-
-	TP_STRUCT__entry(
-		__field(u64, gva)
-		__field(u64, gfn)
-		),
-
-	TP_fast_assign(
-		__entry->gva = gva;
-		__entry->gfn = gfn;
-		),
-
-	TP_printk("gva = %#llx, gfn = %#llx", __entry->gva, __entry->gfn)
-);
-
 #endif
 
 #endif /* _TRACE_KVM_MAIN_H */

Signature · 1 line

1.7.0.4

Xiao Guangrong

author

2010-11-01 08:56 UTC #2 lore raw

PATCH v2 3/7 KVM: fix searching async gfn in kvm_async_pf_gfn_slot

In reply to Xiao Guangrong

Don't search later slots if the slot is empty

Acked-by Gleb Natapov <[email protected]>

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

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

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

index 2cfdf2d..9b543f4 100644
@@ -6206,8 +6206,8 @@ static u32 kvm_async_pf_gfn_slot(struct kvm_vcpu *vcpu, gfn_t gfn)
 	u32 key = kvm_async_pf_hash_fn(gfn);
 
 	for (i = 0; i < roundup_pow_of_two(ASYNC_PF_PER_VCPU) &&
-		     (vcpu->arch.apf.gfns[key] != gfn ||
-		      vcpu->arch.apf.gfns[key] == ~0); i++)
+		     (vcpu->arch.apf.gfns[key] != gfn &&
+		      vcpu->arch.apf.gfns[key] != ~0); i++)
 		key = kvm_async_pf_next_probe(key);
 
 	return key;

Signature · 1 line

1.7.0.4

Xiao Guangrong

author

2010-11-01 08:57 UTC #3 lore raw

PATCH v2 4/7 KVM: avoid unnecessary wait for a async pf

In reply to Xiao Guangrong

In current code, it checks async pf completion out of the wait context, like this:

if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&

		    !vcpu->arch.apf.halted)
			r = vcpu_enter_guest(vcpu);
		else {
			......
			kvm_vcpu_block(vcpu)
			 ^- waiting until 'async_pf.done' is not empty
}

kvm_check_async_pf_completion(vcpu)
^- delete list from async_pf.done

So, if we check aysnc pf completion first, it can be blocked at kvm_vcpu_block

Fixed by mark the vcpu is unhalted in kvm_check_async_pf_completion() path

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

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

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

index 9b543f4..4da8485 100644
@@ -6280,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
 	}
+	vcpu->arch.apf.halted = false;
 }
 
 bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu)

Signature · 1 line

1.7.0.4

Xiao Guangrong

author

2010-11-01 08:58 UTC #5 lore raw

PATCH v2 5/7 KVM: handle more completed apfs if possible

In reply to Xiao Guangrong

If it's no need to inject async #PF to PV guest we can handle more completed apfs at one time, so we can retry guest #PF as early as possible

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

---
 arch/x86/include/asm/kvm_host.h |    3 ++-
 arch/x86/kvm/x86.c              |    8 ++++++--
 virt/kvm/async_pf.c             |   28 ++++++++++++++++------------
 3 files changed, 24 insertions(+), 15 deletions(-)

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

index 1be0058..c95b3ff 100644
@@ -818,7 +818,8 @@ bool kvm_is_linear_rip(struct kvm_vcpu *vcpu, unsigned long linear_rip);
 
 void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 				     struct kvm_async_pf *work);
-void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
+/* return true if we can handle more completed apfs, false otherwise */
+bool kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 				 struct kvm_async_pf *work);
 void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
 			       struct kvm_async_pf *work);

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

index 4da8485..189664a 100644
@@ -6265,7 +6265,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 	}
 }
 
-void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
+bool kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 				 struct kvm_async_pf *work)
 {
 	trace_kvm_async_pf_ready(work->arch.token, work->gva);

@@ -6274,13 +6274,17 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 	else
 		kvm_del_async_pf_gfn(vcpu, work->arch.gfn);
 
+	vcpu->arch.apf.halted = false;
+
 	if ((vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED) &&
 	    !apf_put_user(vcpu, KVM_PV_REASON_PAGE_READY)) {
 		vcpu->arch.fault.error_code = 0;
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		return false;
 	}
-	vcpu->arch.apf.halted = false;
+
+	return true;
 }
 
 bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu)

virt/kvm/async_pf.c +16 ~~-12~~

index 60df9e0..d57ec92 100644
@@ -123,25 +123,29 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
 void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
 {
 	struct kvm_async_pf *work;
+	bool ret;
 
 	if (list_empty_careful(&vcpu->async_pf.done) ||
 	    !kvm_arch_can_inject_async_page_present(vcpu))
 		return;
 
-	spin_lock(&vcpu->async_pf.lock);
-	work = list_first_entry(&vcpu->async_pf.done, typeof(*work), link);
-	list_del(&work->link);
-	spin_unlock(&vcpu->async_pf.lock);
+	do {
+		spin_lock(&vcpu->async_pf.lock);
+		work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
+							link);
+		list_del(&work->link);
+		spin_unlock(&vcpu->async_pf.lock);
 
-	if (work->page)
-		kvm_arch_async_page_ready(vcpu, work);
-	kvm_arch_async_page_present(vcpu, work);
+		if (work->page)
+			kvm_arch_async_page_ready(vcpu, work);
+		ret = kvm_arch_async_page_present(vcpu, work);
 
-	list_del(&work->queue);
-	vcpu->async_pf.queued--;
-	if (work->page)
-		put_page(work->page);
-	kmem_cache_free(async_pf_cache, work);
+		list_del(&work->queue);
+		vcpu->async_pf.queued--;
+		if (work->page)
+			put_page(work->page);
+		kmem_cache_free(async_pf_cache, work);
+	} while (ret && !list_empty_careful(&vcpu->async_pf.done));
 }
 
 int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,

Signature · 1 line

1.7.0.4

Gleb Natapov

2010-11-01 09:24 UTC #6 lore raw

Re: [PATCH v2 5/7] KVM: handle more completed apfs if possible

Quoted 92 lines · Xiao Guangrong

If it's no need to inject async #PF to PV guest we can handle more completed apfs at one time, so we can retry guest #PF as early as possible

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

---
 arch/x86/include/asm/kvm_host.h |    3 ++-
 arch/x86/kvm/x86.c              |    8 ++++++--
 virt/kvm/async_pf.c             |   28 ++++++++++++++++------------
 3 files changed, 24 insertions(+), 15 deletions(-)

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

index 1be0058..c95b3ff 100644
@@ -818,7 +818,8 @@ bool kvm_is_linear_rip(struct kvm_vcpu *vcpu, unsigned long linear_rip);
 
 void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 				     struct kvm_async_pf *work);
-void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
+/* return true if we can handle more completed apfs, false otherwise */
+bool kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 				 struct kvm_async_pf *work);
 void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
 			       struct kvm_async_pf *work);

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

index 4da8485..189664a 100644
@@ -6265,7 +6265,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 	}
 }
 
-void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
+bool kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 				 struct kvm_async_pf *work)
 {
 	trace_kvm_async_pf_ready(work->arch.token, work->gva);

@@ -6274,13 +6274,17 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 	else
 		kvm_del_async_pf_gfn(vcpu, work->arch.gfn);
 
+	vcpu->arch.apf.halted = false;
+
 	if ((vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED) &&
 	    !apf_put_user(vcpu, KVM_PV_REASON_PAGE_READY)) {
 		vcpu->arch.fault.error_code = 0;
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		return false;
 	}
-	vcpu->arch.apf.halted = false;
+
+	return true;
 }
 
 bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu)

virt/kvm/async_pf.c +10 ~~-10~~

index 60df9e0..d57ec92 100644
@@ -123,25 +123,29 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
 void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
 {
 	struct kvm_async_pf *work;
+	bool ret;
 
 	if (list_empty_careful(&vcpu->async_pf.done) ||
 	    !kvm_arch_can_inject_async_page_present(vcpu))
 		return;
 
-	spin_lock(&vcpu->async_pf.lock);
-	work = list_first_entry(&vcpu->async_pf.done, typeof(*work), link);
-	list_del(&work->link);
-	spin_unlock(&vcpu->async_pf.lock);
+	do {
+		spin_lock(&vcpu->async_pf.lock);
+		work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
+							link);
+		list_del(&work->link);
+		spin_unlock(&vcpu->async_pf.lock);
 
-	if (work->page)
-		kvm_arch_async_page_ready(vcpu, work);
-	kvm_arch_async_page_present(vcpu, work);
+		if (work->page)
+			kvm_arch_async_page_ready(vcpu, work);
+		ret = kvm_arch_async_page_present(vcpu, work);
 
-	list_del(&work->queue);
-	vcpu->async_pf.queued--;
-	if (work->page)

virt/kvm/async_pf.c void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu) · line 150

Quoted 10 lines

virt/kvm/async_pf.c +6 -2

-		put_page(work->page);
-	kmem_cache_free(async_pf_cache, work);
+		list_del(&work->queue);
+		vcpu->async_pf.queued--;
+		if (work->page)
+			put_page(work->page);
+		kmem_cache_free(async_pf_cache, work);
+	} while (ret && !list_empty_careful(&vcpu->async_pf.done));
 }

No need to change kvm_arch_async_page_present() to return anything. You can do while loop like this:

while (!list_empty_careful(&vcpu->async_pf.done) &&

       kvm_arch_can_inject_async_page_present(vcpu)) {
}

If kvm_arch_async_page_present() call injects exception kvm_arch_can_inject_async_page_present() will return false on next iteration.

--
Gleb.

Xiao Guangrong

author

2010-11-01 09:30 UTC #7 lore raw

Re: [PATCH v2 5/7] KVM: handle more completed apfs if possible

Quoted 13 lines · Gleb Natapov

virt/kvm/async_pf.c void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu) · line 149
Quoted 10 lines
virt/kvm/async_pf.c +6 -2
-		put_page(work->page);
-	kmem_cache_free(async_pf_cache, work);
+		list_del(&work->queue);
+		vcpu->async_pf.queued--;
+		if (work->page)
+			put_page(work->page);
+		kmem_cache_free(async_pf_cache, work);
+	} while (ret && !list_empty_careful(&vcpu->async_pf.done));
 }
No need to change kvm_arch_async_page_present() to return anything. You can do while loop like this:

Quoted 7 lines

while (!list_empty_careful(&vcpu->async_pf.done) &&
       kvm_arch_can_inject_async_page_present(vcpu)) {
}
If kvm_arch_async_page_present() call injects exception kvm_arch_can_inject_async_page_present() will return false on next iteration.

Yeah, it's a better way, i'll fix it, thanks Gleb!

Xiao Guangrong

author

2010-11-02 09:31 UTC #8 lore raw

PATCH v3 5/7 KVM: handle more completed apfs if possible

In reply to Gleb Natapov

If it's no need to inject async #PF to PV guest we can handle more completed apfs at one time, so we can retry guest #PF as early as possible

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

---
 virt/kvm/async_pf.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

virt/kvm/async_pf.c +16 ~~-16~~

index 60df9e0..100c66e 100644
@@ -124,24 +124,24 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
 {
 	struct kvm_async_pf *work;
 
-	if (list_empty_careful(&vcpu->async_pf.done) ||
-	    !kvm_arch_can_inject_async_page_present(vcpu))
-		return;
-
-	spin_lock(&vcpu->async_pf.lock);
-	work = list_first_entry(&vcpu->async_pf.done, typeof(*work), link);
-	list_del(&work->link);
-	spin_unlock(&vcpu->async_pf.lock);
+	while (!list_empty_careful(&vcpu->async_pf.done) &&
+	      kvm_arch_can_inject_async_page_present(vcpu)) {
+		spin_lock(&vcpu->async_pf.lock);
+		work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
+					      link);
+		list_del(&work->link);
+		spin_unlock(&vcpu->async_pf.lock);
 
-	if (work->page)
-		kvm_arch_async_page_ready(vcpu, work);
-	kvm_arch_async_page_present(vcpu, work);
+		if (work->page)
+			kvm_arch_async_page_ready(vcpu, work);
+		kvm_arch_async_page_present(vcpu, work);
 
-	list_del(&work->queue);
-	vcpu->async_pf.queued--;
-	if (work->page)
-		put_page(work->page);
-	kmem_cache_free(async_pf_cache, work);
+		list_del(&work->queue);
+		vcpu->async_pf.queued--;
+		if (work->page)
+			put_page(work->page);
+		kmem_cache_free(async_pf_cache, work);
+	}
 }
 
 int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,

Signature · 1 line

1.7.0.4

Gleb Natapov

Acked-by on v3 5/7

2010-11-02 12:38 UTC #9 lore raw

Re: [PATCH v3 5/7] KVM: handle more completed apfs if possible

Quoted 5 lines · Xiao Guangrong

If it's no need to inject async #PF to PV guest we can handle more completed apfs at one time, so we can retry guest #PF as early as possible
Signed-off-by Xiao Guangrong <[email protected]>

Acked-by Gleb Natapov <[email protected]>

--
Gleb.

Xiao Guangrong

author

2010-11-01 08:59 UTC #10 lore raw

RFC PATCH v2 6/7 KVM: fix the race while wakeup all pv guest

In reply to Xiao Guangrong

In kvm_async_pf_wakeup_all(), we add a dummy apf to vcpu->async_pf.done without holding vcpu->async_pf.lock, it will break if we are handling apfs at this time.

Also use 'list_empty_careful()' instead of 'list_empty()'

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

---
 virt/kvm/async_pf.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

virt/kvm/async_pf.c +4 -1

index d57ec92..6ef3373 100644
@@ -200,7 +200,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
 {
 	struct kvm_async_pf *work;
 
-	if (!list_empty(&vcpu->async_pf.done))
+	if (!list_empty_careful(&vcpu->async_pf.done))
 		return 0;
 
 	work = kmem_cache_zalloc(async_pf_cache, GFP_ATOMIC);

@@ -211,7 +211,10 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
 	get_page(bad_page);
 	INIT_LIST_HEAD(&work->queue); /* for list_del to work */
 
+	spin_lock(&vcpu->async_pf.lock);
 	list_add_tail(&work->link, &vcpu->async_pf.done);
+	spin_unlock(&vcpu->async_pf.lock);
+
 	vcpu->async_pf.queued++;
 	return 0;
 }

Signature · 1 line

1.7.0.4

Xiao Guangrong

author

2010-11-01 09:00 UTC #12 lore raw

RFC PATCH v2 7/7 KVM: KVM: don't break vcpu 'halt' state due to apfs

In reply to Xiao Guangrong

Don't make a KVM_REQ_UNHALT request after async pf is completed since it can break guest's 'HLT' instruction.

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

---
 arch/x86/kvm/x86.c       |   13 ++++++++++---
 include/linux/kvm_host.h |    6 ++++++
 virt/kvm/kvm_main.c      |    9 ++++++++-
 3 files changed, 24 insertions(+), 4 deletions(-)

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

index 189664a..c57fb38 100644
@@ -6105,13 +6105,20 @@ void kvm_arch_flush_shadow(struct kvm *kvm)
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 {
-	return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
+	if ((vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
 		!vcpu->arch.apf.halted)
-		|| !list_empty_careful(&vcpu->async_pf.done)
 		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
 		|| vcpu->arch.nmi_pending ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
-		 kvm_cpu_has_interrupt(vcpu));
+		 kvm_cpu_has_interrupt(vcpu)))
+		return 1;
+
+	if (!list_empty_careful(&vcpu->async_pf.done)) {
+		vcpu->arch.apf.halted = false;
+		return 2;
+	}
+
+	return 0;
 }
 
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu)

include/linux/kvm_host.h +6 -0

index 462b982..8def043 100644
@@ -436,6 +436,12 @@ void kvm_arch_hardware_disable(void *garbage);
 int kvm_arch_hardware_setup(void);
 void kvm_arch_hardware_unsetup(void);
 void kvm_arch_check_processor_compat(void *rtn);
+
+/*
+ * return value: > 0 if the vcpu is runnable, 0 if not.
+ * Especially, if the return value == 1, we should make a
+ * 'KVM_REQ_UNHALT' request.
+ */
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
 
 void kvm_free_physmem(struct kvm *kvm);

virt/kvm/kvm_main.c +8 -1

index dbe1d6a..9ccf105 100644
@@ -1375,14 +1375,21 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
 void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 {
 	DEFINE_WAIT(wait);
+	int ret;
 
 	for (;;) {
 		prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
 
-		if (kvm_arch_vcpu_runnable(vcpu)) {
+		ret = kvm_arch_vcpu_runnable(vcpu);
+
+		if (ret == 1) {
 			kvm_make_request(KVM_REQ_UNHALT, vcpu);
 			break;
 		}
+
+		if (ret > 1)
+			break;
+
 		if (kvm_cpu_has_pending_timer(vcpu))
 			break;
 		if (signal_pending(current))

Signature · 1 line

1.7.0.4

Gleb Natapov

2010-11-01 12:55 UTC #13 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 23 lines · Xiao Guangrong

Don't make a KVM_REQ_UNHALT request after async pf is completed since it can break guest's 'HLT' instruction.

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

---
 arch/x86/kvm/x86.c       |   13 ++++++++++---
 include/linux/kvm_host.h |    6 ++++++
 virt/kvm/kvm_main.c      |    9 ++++++++-
 3 files changed, 24 insertions(+), 4 deletions(-)

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

index 189664a..c57fb38 100644
@@ -6105,13 +6105,20 @@ void kvm_arch_flush_shadow(struct kvm *kvm)
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 {
-	return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
+	if ((vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
 		!vcpu->arch.apf.halted)
-		|| !list_empty_careful(&vcpu->async_pf.done)
 		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED

arch/x86/kvm/x86.c void kvm_arch_flush_shadow(struct kvm *kvm) · line 6119

Quoted 10 lines

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

 		|| vcpu->arch.nmi_pending ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
-		 kvm_cpu_has_interrupt(vcpu));
+		 kvm_cpu_has_interrupt(vcpu)))
+		return 1;
+
+	if (!list_empty_careful(&vcpu->async_pf.done)) {
+		vcpu->arch.apf.halted = false;
+		return 2;
+	}

kvm_arch_vcpu_runnable() shouldn't change vcpu state. I don't like the way it propagates internal x86 state to kvm_vcpu_block() too. May be what you are looking for is the patch below? (not tested).

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

index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;

@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }

--
Gleb.

Xiao Guangrong

author

2010-11-02 02:25 UTC #14 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

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

Quoted 13 lines · Gleb Natapov

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

index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;

if nmi/interrupt and apfs completed event occur at the same time, we will miss to exit halt sate. Maybe we can check the pending event here, see below patch please.

arch/x86/kvm/x86.c void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, · line 6285

Quoted 10 lines

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

 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;

@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }

Have a stupid question, why we make the vcpu runnable here? Sorry i don't know kvm pv guest to much. :-(

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

index 2044302..27a712b 100644
@@ -5267,6 +5267,12 @@ out:
 	return r;
 }
 
+static bool kvm_vcpu_leave_halt_state(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.nmi_pending ||
+		(kvm_arch_interrupt_allowed(vcpu) &&
+		 kvm_cpu_has_interrupt(vcpu));
+}
 
 static int __vcpu_run(struct kvm_vcpu *vcpu)
 {

@@ -5299,8 +5305,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (kvm_vcpu_leave_halt_state(vcpu))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;

@@ -6113,9 +6120,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 		!vcpu->arch.apf.halted)
 		|| !list_empty_careful(&vcpu->async_pf.done)
 		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
-		|| vcpu->arch.nmi_pending ||
-		(kvm_arch_interrupt_allowed(vcpu) &&
-		 kvm_cpu_has_interrupt(vcpu));
+		|| kvm_vcpu_leave_halt_state(vcpu);
 }
 
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu)

Gleb Natapov

2010-11-02 06:56 UTC #15 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 19 lines · Xiao Guangrong

arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5300
Quoted 13 lines · Gleb Natapov
arch/x86/kvm/x86.c +3 -2
index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
if nmi/interrupt and apfs completed event occur at the same time, we will miss to exit halt sate. Maybe we can check the pending event here, see below patch please.

No, we will not. If nmi/interrupt and apfs completed event occur at the same time kvm_vcpu_block() will exit with KVM_REQ_UNHALT set, but cpu will not be unhalted because of list_empty_careful(&vcpu->async_pf.done) check. Vcpu then will process pending apf completion and enter kvm_vcpu_block() again which will immediately exit because kvm_arch_vcpu_runnable() will return true since there is pending nmi/interrupt. This time vcpu will be unhalted.

Quoted 5 lines

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

 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;

@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;

Quoted 8 lines

arch/x86/kvm/x86.c void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, · line 6285
Quoted 5 lines
arch/x86/kvm/x86.c +1 -0
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }
Have a stupid question, why we make the vcpu runnable here? Sorry i don't know kvm pv guest to much. :-(

Because kvm_arch_vcpu_runnable() does not check for pending exceptions. Since now we do not unhalt vcpu when apf completion happens we need to unhalt it here. But, as I said, the patch is untested.

--
Gleb.

Xiao Guangrong

author

2010-11-02 07:27 UTC #16 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 20 lines · Gleb Natapov

Quoted 19 lines · Xiao Guangrong
arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5300
Quoted 13 lines · Gleb Natapov
arch/x86/kvm/x86.c +3 -2
index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
if nmi/interrupt and apfs completed event occur at the same time, we will miss to exit halt sate. Maybe we can check the pending event here, see below patch please.

Quoted 7 lines

No, we will not. If nmi/interrupt and apfs completed event occur at the same time kvm_vcpu_block() will exit with KVM_REQ_UNHALT set, but cpu will not be unhalted because of list_empty_careful(&vcpu->async_pf.done) check. Vcpu then will process pending apf completion and enter kvm_vcpu_block() again which will immediately exit because kvm_arch_vcpu_runnable() will return true since there is pending nmi/interrupt. This time vcpu will be unhalted.

Thanks for your explanation, but if it has nmi/interrupt pending, kvm_arch_can_inject_async_page_present() always return false in PV guest case, it can not process pending apf completion, so, the vcpu is remain halt state forever?

Also, the pv guest can only handle an apf completion at one time, it can not ensure vcpu->async_pf.done is empty after kvm_check_async_pf_completion()

Quoted 14 lines

Quoted 14 lines
arch/x86/kvm/x86.c void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, · line 6285
Quoted 11 lines
arch/x86/kvm/x86.c +1 -0
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }
Have a stupid question, why we make the vcpu runnable here? Sorry i don't know kvm pv guest to much. :-(

Quoted 4 lines

Because kvm_arch_vcpu_runnable() does not check for pending exceptions. Since now we do not unhalt vcpu when apf completion happens we need to unhalt it here. But, as I said, the patch is untested.

As i know, exception can not let guest exit halt state, only NMI/interruption can do it, yes? :-)

Gleb Natapov

2010-11-02 07:45 UTC #17 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 29 lines · Xiao Guangrong

Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Xiao Guangrong
arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5300
Quoted 13 lines · Gleb Natapov
arch/x86/kvm/x86.c +3 -2
index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
if nmi/interrupt and apfs completed event occur at the same time, we will miss to exit halt sate. Maybe we can check the pending event here, see below patch please.
Quoted 7 lines
No, we will not. If nmi/interrupt and apfs completed event occur at the same time kvm_vcpu_block() will exit with KVM_REQ_UNHALT set, but cpu will not be unhalted because of list_empty_careful(&vcpu->async_pf.done) check. Vcpu then will process pending apf completion and enter kvm_vcpu_block() again which will immediately exit because kvm_arch_vcpu_runnable() will return true since there is pending nmi/interrupt. This time vcpu will be unhalted.

Quoted 5 lines

Thanks for your explanation, but if it has nmi/interrupt pending, kvm_arch_can_inject_async_page_present() always return false in PV guest case, it can not process pending apf completion, so, the vcpu is remain halt state forever?

kvm_event_needs_reinjection() checks for nmi/interrupts that need reinjection (not injection). Those are nmi/interrupts that was injected but injection failed for some reason. For nmi, for instance, kvm_arch_vcpu_runnable() checks vcpu->arch.nmi_pending, but kvm_event_needs_reinjection() checks for vcpu->arch.nmi_injected. NMI moves from nmi_pending to nmi_injected when it is injected into vcpu for the first time. CPU cannot be halted in this state.

Quoted 3 lines

Also, the pv guest can only handle an apf completion at one time, it can not ensure vcpu->async_pf.done is empty after kvm_check_async_pf_completion()

In case of PV guest it will be woken up by apf completion by kvm_arch_async_page_present() below.

Quoted 14 lines

Quoted 14 lines
Quoted 14 lines
arch/x86/kvm/x86.c void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, · line 6285
Quoted 11 lines
arch/x86/kvm/x86.c +1 -0
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }
Have a stupid question, why we make the vcpu runnable here? Sorry i don't know kvm pv guest to much. :-(

Quoted 6 lines

Quoted 4 lines
Because kvm_arch_vcpu_runnable() does not check for pending exceptions. Since now we do not unhalt vcpu when apf completion happens we need to unhalt it here. But, as I said, the patch is untested.
As i know, exception can not let guest exit halt state, only NMI/interruption can do it, yes? :-)

On physical HW exception cannot happen while cpu is in halt state, but with PV we define what guest can and cannot expect, so when guest explicitly enables apf it should be able to handle it during halt.

--
Gleb.

Xiao Guangrong

author

2010-11-02 09:05 UTC #18 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 8 lines · Gleb Natapov

kvm_event_needs_reinjection() checks for nmi/interrupts that need reinjection (not injection). Those are nmi/interrupts that was injected but injection failed for some reason. For nmi, for instance, kvm_arch_vcpu_runnable() checks vcpu->arch.nmi_pending, but kvm_event_needs_reinjection() checks for vcpu->arch.nmi_injected. NMI moves from nmi_pending to nmi_injected when it is injected into vcpu for the first time. CPU cannot be halted in this state.

Yeah, nmi is handled like this way, but for interrupt:
If interruption controller is in userspace (!irqchip_in_kernel(v->kvm)), kvm_arch_vcpu_runnable() checks vcpu->arch.interrupt.pending and kvm_event_needs_reinjection() also checks vcpu->arch.interrupt.pending.

Consider this case:

- Guest vcpu executes 'HLT'
- wakeup the vcpu and inject interrupt and apfs is completed at this time
- then the vcpu can't handle apf completion and .done list keeps nonempty.

Can this case happen? Sorry if i missed it again.

Quoted 5 lines

Quoted 3 lines
Also, the pv guest can only handle an apf completion at one time, it can not ensure vcpu->async_pf.done is empty after kvm_check_async_pf_completion()
In case of PV guest it will be woken up by apf completion by kvm_arch_async_page_present() below.

......

Quoted 5 lines

Quoted 1 line
As i know, exception can not let guest exit halt state, only NMI/interruption can do it, yes? :-)
On physical HW exception cannot happen while cpu is in halt state, but with PV we define what guest can and cannot expect, so when guest explicitly enables apf it should be able to handle it during halt.

Ah, i see, thanks your very much.

Gleb Natapov

2010-11-02 09:14 UTC #19 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 16 lines · Xiao Guangrong

Quoted 8 lines · Gleb Natapov
kvm_event_needs_reinjection() checks for nmi/interrupts that need reinjection (not injection). Those are nmi/interrupts that was injected but injection failed for some reason. For nmi, for instance, kvm_arch_vcpu_runnable() checks vcpu->arch.nmi_pending, but kvm_event_needs_reinjection() checks for vcpu->arch.nmi_injected. NMI moves from nmi_pending to nmi_injected when it is injected into vcpu for the first time. CPU cannot be halted in this state.
Yeah, nmi is handled like this way, but for interrupt:
If interruption controller is in userspace (!irqchip_in_kernel(v->kvm)), kvm_arch_vcpu_runnable() checks vcpu->arch.interrupt.pending and kvm_event_needs_reinjection() also checks vcpu->arch.interrupt.pending.

Quoted 8 lines

Consider this case:
- Guest vcpu executes 'HLT'
- wakeup the vcpu and inject interrupt and apfs is completed at this time
- then the vcpu can't handle apf completion and .done list keeps nonempty.
Can this case happen? Sorry if i missed it again.

If irqchip is in userspace apf is disabled (see mmu.c:can_do_async_pf()). The reason for this is that when irqchip_in_kernel(v->kvm) cpu sleeps in userspace during halt, so all event that can cause it to be unhalted should be generated in userspace too. This is also the reason you can't have pit in kernel and irqchip in userpsace.

--
Gleb.

Xiao Guangrong

author

2010-11-02 09:25 UTC #20 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 6 lines · Gleb Natapov

If irqchip is in userspace apf is disabled (see mmu.c:can_do_async_pf()). The reason for this is that when irqchip_in_kernel(v->kvm) cpu sleeps in userspace during halt, so all event that can cause it to be unhalted should be generated in userspace too. This is also the reason you can't have pit in kernel and irqchip in userpsace.

Oh, thank you very much for answering so many questions, and your patch is looks good for me! ;-)

Gleb Natapov

2010-11-02 12:39 UTC #21 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 11 lines · Xiao Guangrong

Quoted 6 lines · Gleb Natapov
If irqchip is in userspace apf is disabled (see mmu.c:can_do_async_pf()). The reason for this is that when irqchip_in_kernel(v->kvm) cpu sleeps in userspace during halt, so all event that can cause it to be unhalted should be generated in userspace too. This is also the reason you can't have pit in kernel and irqchip in userpsace.
Oh, thank you very much for answering so many questions, and your patch is looks good for me! ;-)

It is still not tested though :)

--
Gleb.

Xiao Guangrong

author

2010-11-03 09:43 UTC #22 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

In reply to Gleb Natapov

Quoted 17 lines · Gleb Natapov

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

index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;

arch/x86/kvm/x86.c void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, · line 6286

Quoted 7 lines

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

@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }

Tested with full virtualization guests, it works well for me. Thanks!

Gleb Natapov

2010-11-03 09:45 UTC #23 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 20 lines · Xiao Guangrong

Quoted 18 lines · Gleb Natapov

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

index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;

@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;

Quoted 8 lines

arch/x86/kvm/x86.c void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, · line 6286
Quoted 6 lines
arch/x86/kvm/x86.c +1 -0
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }
 
Tested with full virtualization guests, it works well for me. Thanks!

Thank you. Will repost properly.

--
Gleb.

Marcelo Tosatti

2010-11-03 13:43 UTC #24 lore raw

Re: [RFC PATCH v2 7/7] KVM: KVM: don't break vcpu 'halt' state due to apfs

Quoted 29 lines · Gleb Natapov

Quoted 20 lines · Xiao Guangrong

Quoted 18 lines · Gleb Natapov

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

index 2cfdf2d..f7aed95 100644
@@ -5295,8 +5295,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			{
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
-					vcpu->arch.mp_state =
-						KVM_MP_STATE_RUNNABLE;
+					if (list_empty_careful(&vcpu->async_pf.done))
+						vcpu->arch.mp_state =
+							KVM_MP_STATE_RUNNABLE;
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;

@@ -6279,6 +6280,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		vcpu->arch.fault.error_code = 0;

Quoted 8 lines

arch/x86/kvm/x86.c void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, · line 6286
Quoted 6 lines
arch/x86/kvm/x86.c +1 -0
 		vcpu->arch.fault.address = work->arch.token;
 		kvm_inject_page_fault(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 }
 
Tested with full virtualization guests, it works well for me. Thanks!

Quoted 1 line

Thank you. Will repost properly.

There is no need to optimize this. Current code is simpler, less error-prone. Applied 1-6, thanks.

Gleb Natapov

Acked-by on v2 1/7

2010-11-01 13:09 UTC #25 lore raw

In reply to Xiao Guangrong

Quoted 7 lines · Xiao Guangrong

Tracing 'async' and *pfn is useless, since 'async' is always true, and '*pfn' is always "fault_pfn'
We can trace 'gva' and 'gfn' instead, it can help us to see the life-cycle of an async_pf
Signed-off-by Xiao Guangrong <[email protected]>

Acked-by Gleb Natapov <[email protected]>

Quoted 50 lines

---
 arch/x86/kvm/mmu.c         |    2 +-
 include/trace/events/kvm.h |   12 +++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

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

index 39cc0c6..5275c50 100644
@@ -2627,7 +2627,7 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool no_apf, gfn_t gfn,
 	put_page(pfn_to_page(*pfn));
 
 	if (!no_apf && can_do_async_pf(vcpu)) {
-		trace_kvm_try_async_get_page(async, *pfn);
+		trace_kvm_try_async_get_page(gva, gfn);
 		if (kvm_find_async_pf_gfn(vcpu, gfn)) {
 			trace_kvm_async_pf_doublefault(gva, gfn);
 			kvm_make_request(KVM_REQ_APF_HALT, vcpu);

include/trace/events/kvm.h +7 -5

index 9c2cc6a..30063c6 100644
@@ -188,18 +188,20 @@ TRACE_EVENT(kvm_age_page,
 #ifdef CONFIG_KVM_ASYNC_PF
 TRACE_EVENT(
 	kvm_try_async_get_page,
-	TP_PROTO(bool async, u64 pfn),
-	TP_ARGS(async, pfn),
+	TP_PROTO(u64 gva, u64 gfn),
+	TP_ARGS(gva, gfn),
 
 	TP_STRUCT__entry(
-		__field(__u64, pfn)
+		__field(u64, gva)
+		__field(u64, gfn)
 		),
 
 	TP_fast_assign(
-		__entry->pfn = (!async) ? pfn : (u64)-1;
+		__entry->gva = gva;
+		__entry->gfn = gfn;
 		),
 
-	TP_printk("pfn %#llx", __entry->pfn)
+	TP_printk("gva = %#llx, gfn = %#llx", __entry->gva, __entry->gfn)
 );
 
 TRACE_EVENT(

Signature · 1 line

1.7.0.4

--
Gleb.