[PATCH] KVM: x86: clean up conditions for asynchronous page fault handling

[PATCH] KVM: x86: clean up conditions for asynchronous page fault handling

[Paolo Bonzini]

Even when asynchronous page fault is disabled, KVM does not want to pause
the host if a guest triggers a page fault; instead it will put it into
an artificial HLT state that allows running other host processes while
allowing interrupt delivery into the guest.

However, the way this feature is triggered is a bit confusing.
First, it is not used for page faults while a nested guest is
running: but this is not an issue since the artificial halt
is completely invisible to the guest, either L1 or L2.  Second,
it is used even if kvm_halt_in_guest() returns true; in this case,
the guest probably should not pay the additional latency cost of the
artificial halt, and thus we should handle the page fault in a
completely synchronous way.

By introducing a new function kvm_can_deliver_async_pf, this patch
commonizes the code that chooses whether to deliver an async page fault
(kvm_arch_async_page_not_present) and the code that chooses whether a
page fault should be handled synchronously (kvm_can_do_async_pf).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu.c | 13 ------------
 arch/x86/kvm/x86.c | 59 ++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 3384c539d150..771349e72d2a 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4040,19 +4040,6 @@ static int kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn)
 	return kvm_setup_async_pf(vcpu, gva, kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch);
 }
 
-bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu)
-{
-	if (unlikely(!lapic_in_kernel(vcpu) ||
-		     kvm_event_needs_reinjection(vcpu) ||
-		     vcpu->arch.exception.pending))
-		return false;
-
-	if (!vcpu->arch.apf.delivery_as_pf_vmexit && is_guest_mode(vcpu))
-		return false;
-
-	return kvm_x86_ops->interrupt_allowed(vcpu);
-}
-
 static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 			 gva_t gva, kvm_pfn_t *pfn, bool write, bool *writable)
 {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6200d5a51f13..2fe53b931b72 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9775,6 +9775,36 @@ static int apf_get_user(struct kvm_vcpu *vcpu, u32 *val)
 				      sizeof(u32));
 }
 
+static bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu)
+{
+	if (!vcpu->arch.apf.delivery_as_pf_vmexit && is_guest_mode(vcpu))
+		return false;
+
+	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED) ||
+	    (vcpu->arch.apf.send_user_only &&
+	     kvm_x86_ops->get_cpl(vcpu) == 0))
+		return false;
+
+	return true;
+}
+
+bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu)
+{
+	if (unlikely(!lapic_in_kernel(vcpu) ||
+		     kvm_event_needs_reinjection(vcpu) ||
+		     vcpu->arch.exception.pending))
+		return false;
+
+	if (kvm_hlt_in_guest(vcpu->kvm) && !kvm_can_deliver_async_pf(vcpu))
+		return false;
+
+	/*
+	 * If interrupts are off we cannot even use an artificial
+	 * halt state.
+	 */
+	return kvm_x86_ops->interrupt_allowed(vcpu);
+}
+
 void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 				     struct kvm_async_pf *work)
 {
@@ -9783,19 +9813,26 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 	trace_kvm_async_pf_not_present(work->arch.token, work->gva);
 	kvm_add_async_pf_gfn(vcpu, work->arch.gfn);
 
-	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED) ||
-	    (vcpu->arch.apf.send_user_only &&
-	     kvm_x86_ops->get_cpl(vcpu) == 0))
+	if (!kvm_can_deliver_async_pf(vcpu) ||
+	    apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) {
+		/*
+		 * It is not possible to deliver a paravirtualized asynchronous
+		 * page fault, but putting the guest in an artificial halt state
+		 * can be beneficial nevertheless: if an interrupt arrives, we
+		 * can deliver it timely and perhaps the guest will schedule
+		 * another process.  When the instruction that triggered a page
+		 * fault is retried, hopefully the page will be ready in the host.
+		 */
 		kvm_make_request(KVM_REQ_APF_HALT, vcpu);
-	else if (!apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) {
-		fault.vector = PF_VECTOR;
-		fault.error_code_valid = true;
-		fault.error_code = 0;
-		fault.nested_page_fault = false;
-		fault.address = work->arch.token;
-		fault.async_page_fault = true;
-		kvm_inject_page_fault(vcpu, &fault);
 	}
+
+	fault.vector = PF_VECTOR;
+	fault.error_code_valid = true;
+	fault.error_code = 0;
+	fault.nested_page_fault = false;
+	fault.address = work->arch.token;
+	fault.async_page_fault = true;
+	kvm_inject_page_fault(vcpu, &fault);
 }
 
 void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
-- 
1.8.3.1

Re: [PATCH] KVM: x86: clean up conditions for asynchronous page fault handling

[Radim Krčmář]

2019-06-13 13:03+0200, Paolo Bonzini:
> Even when asynchronous page fault is disabled, KVM does not want to pause
> the host if a guest triggers a page fault; instead it will put it into
> an artificial HLT state that allows running other host processes while
> allowing interrupt delivery into the guest.
> 
> However, the way this feature is triggered is a bit confusing.
> First, it is not used for page faults while a nested guest is
> running: but this is not an issue since the artificial halt
> is completely invisible to the guest, either L1 or L2.  Second,
> it is used even if kvm_halt_in_guest() returns true; in this case,
> the guest probably should not pay the additional latency cost of the
> artificial halt, and thus we should handle the page fault in a
> completely synchronous way.

The same reasoning would apply to kvm_mwait_in_guest(), so I would
disable APF with it as well.

> By introducing a new function kvm_can_deliver_async_pf, this patch
> commonizes the code that chooses whether to deliver an async page fault
> (kvm_arch_async_page_not_present) and the code that chooses whether a
> page fault should be handled synchronously (kvm_can_do_async_pf).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -9775,6 +9775,36 @@ static int apf_get_user(struct kvm_vcpu *vcpu, u32 *val)
> +bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu)
> +{
> +	if (unlikely(!lapic_in_kernel(vcpu) ||
> +		     kvm_event_needs_reinjection(vcpu) ||
> +		     vcpu->arch.exception.pending))
> +		return false;
> +
> +	if (kvm_hlt_in_guest(vcpu->kvm) && !kvm_can_deliver_async_pf(vcpu))
> +		return false;
> +
> +	/*
> +	 * If interrupts are off we cannot even use an artificial
> +	 * halt state.

Can't we?  The artificial halt state would be canceled by the host page
fault handler.

> +	 */
> +	return kvm_x86_ops->interrupt_allowed(vcpu);
> @@ -9783,19 +9813,26 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>  	trace_kvm_async_pf_not_present(work->arch.token, work->gva);
>  	kvm_add_async_pf_gfn(vcpu, work->arch.gfn);
>  
> -	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED) ||
> -	    (vcpu->arch.apf.send_user_only &&
> -	     kvm_x86_ops->get_cpl(vcpu) == 0))
> +	if (!kvm_can_deliver_async_pf(vcpu) ||
> +	    apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) {
> +		/*
> +		 * It is not possible to deliver a paravirtualized asynchronous
> +		 * page fault, but putting the guest in an artificial halt state
> +		 * can be beneficial nevertheless: if an interrupt arrives, we
> +		 * can deliver it timely and perhaps the guest will schedule
> +		 * another process.  When the instruction that triggered a page
> +		 * fault is retried, hopefully the page will be ready in the host.
> +		 */
>  		kvm_make_request(KVM_REQ_APF_HALT, vcpu);

A return is missing here, to prevent the delivery of PV APF.
(I'd probably keep the if/else.)

Thanks.

> -	else if (!apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) {
> -		fault.vector = PF_VECTOR;
> -		fault.error_code_valid = true;
> -		fault.error_code = 0;
> -		fault.nested_page_fault = false;
> -		fault.address = work->arch.token;
> -		fault.async_page_fault = true;
> -		kvm_inject_page_fault(vcpu, &fault);

>  	}
> +
> +	fault.vector = PF_VECTOR;
> +	fault.error_code_valid = true;
> +	fault.error_code = 0;
> +	fault.nested_page_fault = false;
> +	fault.address = work->arch.token;
> +	fault.async_page_fault = true;
> +	kvm_inject_page_fault(vcpu, &fault);
>  }
>  
>  void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> -- 
> 1.8.3.1
> 

Re: [PATCH] KVM: x86: clean up conditions for asynchronous page fault handling

[Paolo Bonzini]

On 13/06/19 19:12, Radim Krčmář wrote:
> 2019-06-13 13:03+0200, Paolo Bonzini:
>> Even when asynchronous page fault is disabled, KVM does not want to pause
>> the host if a guest triggers a page fault; instead it will put it into
>> an artificial HLT state that allows running other host processes while
>> allowing interrupt delivery into the guest.
>>
>> However, the way this feature is triggered is a bit confusing.
>> First, it is not used for page faults while a nested guest is
>> running: but this is not an issue since the artificial halt
>> is completely invisible to the guest, either L1 or L2.  Second,
>> it is used even if kvm_halt_in_guest() returns true; in this case,
>> the guest probably should not pay the additional latency cost of the
>> artificial halt, and thus we should handle the page fault in a
>> completely synchronous way.
> 
> The same reasoning would apply to kvm_mwait_in_guest(), so I would
> disable APF with it as well.

True, on the other hand it's not a very sensible condition to have
kvm_mwait_in_guest but not kvm_halt_in_guest.

>> By introducing a new function kvm_can_deliver_async_pf, this patch
>> commonizes the code that chooses whether to deliver an async page fault
>> (kvm_arch_async_page_not_present) and the code that chooses whether a
>> page fault should be handled synchronously (kvm_can_do_async_pf).
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> @@ -9775,6 +9775,36 @@ static int apf_get_user(struct kvm_vcpu *vcpu, u32 *val)
>> +bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu)
>> +{
>> +	if (unlikely(!lapic_in_kernel(vcpu) ||
>> +		     kvm_event_needs_reinjection(vcpu) ||
>> +		     vcpu->arch.exception.pending))
>> +		return false;
>> +
>> +	if (kvm_hlt_in_guest(vcpu->kvm) && !kvm_can_deliver_async_pf(vcpu))
>> +		return false;
>> +
>> +	/*
>> +	 * If interrupts are off we cannot even use an artificial
>> +	 * halt state.
> 
> Can't we?  The artificial halt state would be canceled by the host page
> fault handler.

hlt allows interrupts to be injected, which of course is not possible in
an interrupt-off region.  But, the only difference is that halt_poll is
not obeyed for synchronous page fault handling; either way, the vCPU
thread stays in the kernel but it is scheduled out while the page fault
is being handled.  This is only for lapic_in_kernel so hlt does not
leave KVM_RUN.

>> +	 */
>> +	return kvm_x86_ops->interrupt_allowed(vcpu);
>> @@ -9783,19 +9813,26 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>>  	trace_kvm_async_pf_not_present(work->arch.token, work->gva);
>>  	kvm_add_async_pf_gfn(vcpu, work->arch.gfn);
>>  
>> -	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED) ||
>> -	    (vcpu->arch.apf.send_user_only &&
>> -	     kvm_x86_ops->get_cpl(vcpu) == 0))
>> +	if (!kvm_can_deliver_async_pf(vcpu) ||
>> +	    apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) {
>> +		/*
>> +		 * It is not possible to deliver a paravirtualized asynchronous
>> +		 * page fault, but putting the guest in an artificial halt state
>> +		 * can be beneficial nevertheless: if an interrupt arrives, we
>> +		 * can deliver it timely and perhaps the guest will schedule
>> +		 * another process.  When the instruction that triggered a page
>> +		 * fault is retried, hopefully the page will be ready in the host.
>> +		 */
>>  		kvm_make_request(KVM_REQ_APF_HALT, vcpu);
> 
> A return is missing here, to prevent the delivery of PV APF.
> (I'd probably keep the if/else.)

Fixed in v2 (keeping the if/else but swapping the two arms).

Paolo

> Thanks.
> 
>> -	else if (!apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) {
>> -		fault.vector = PF_VECTOR;
>> -		fault.error_code_valid = true;
>> -		fault.error_code = 0;
>> -		fault.nested_page_fault = false;
>> -		fault.address = work->arch.token;
>> -		fault.async_page_fault = true;
>> -		kvm_inject_page_fault(vcpu, &fault);
> 
>>  	}
>> +
>> +	fault.vector = PF_VECTOR;
>> +	fault.error_code_valid = true;
>> +	fault.error_code = 0;
>> +	fault.nested_page_fault = false;
>> +	fault.address = work->arch.token;
>> +	fault.async_page_fault = true;
>> +	kvm_inject_page_fault(vcpu, &fault);
>>  }
>>  
>>  void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
>> -- 
>> 1.8.3.1
>>