linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf()
@ 2020-06-10 17:55 Vitaly Kuznetsov
  2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov
  2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson
  0 siblings, 2 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2020-06-10 17:55 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel

schedule_work() returns 'false' only when the work is already on the queue
and this can't happen as kvm_setup_async_pf() always allocates a new one.
Also, to avoid potential race, it makes sense to to schedule_work() at the
very end after we've added it to the queue.

While on it, do some minor cleanup. gfn_to_pfn_async() mentioned in a
comment does not currently exist and, moreover, we can check
kvm_is_error_hva() at the very beginning, before we try to allocate work so
'retry_sync' label can go away completely.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 virt/kvm/async_pf.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index f1e07fae84e9..ba080088da76 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -164,7 +164,9 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 	if (vcpu->async_pf.queued >= ASYNC_PF_PER_VCPU)
 		return 0;
 
-	/* setup delayed work */
+	/* Arch specific code should not do async PF in this case */
+	if (unlikely(kvm_is_error_hva(hva)))
+		return 0;
 
 	/*
 	 * do alloc nowait since if we are going to sleep anyway we
@@ -183,24 +185,15 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 	mmget(work->mm);
 	kvm_get_kvm(work->vcpu->kvm);
 
-	/* this can't really happen otherwise gfn_to_pfn_async
-	   would succeed */
-	if (unlikely(kvm_is_error_hva(work->addr)))
-		goto retry_sync;
-
 	INIT_WORK(&work->work, async_pf_execute);
-	if (!schedule_work(&work->work))
-		goto retry_sync;
 
 	list_add_tail(&work->queue, &vcpu->async_pf.queue);
 	vcpu->async_pf.queued++;
 	kvm_arch_async_page_not_present(vcpu, work);
+
+	schedule_work(&work->work);
+
 	return 1;
-retry_sync:
-	kvm_put_kvm(work->vcpu->kvm);
-	mmput(work->mm);
-	kmem_cache_free(async_pf_cache, work);
-	return 0;
 }
 
 int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected
  2020-06-10 17:55 [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Vitaly Kuznetsov
@ 2020-06-10 17:55 ` Vitaly Kuznetsov
  2020-06-10 19:32   ` Vivek Goyal
  2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson
  1 sibling, 1 reply; 11+ messages in thread
From: Vitaly Kuznetsov @ 2020-06-10 17:55 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel

'Page not present' event may or may not get injected depending on
guest's state. If the event wasn't injected, there is no need to
inject the corresponding 'page ready' event as the guest may get
confused. E.g. Linux thinks that the corresponding 'page not present'
event wasn't delivered *yet* and allocates a 'dummy entry' for it.
This entry is never freed.

Note, 'wakeup all' events have no corresponding 'page not present'
event and always get injected.

s390 seems to always be able to inject 'page not present', the
change is effectively a nop.

Suggested-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/s390/include/asm/kvm_host.h | 2 +-
 arch/s390/kvm/kvm-s390.c         | 4 +++-
 arch/x86/include/asm/kvm_host.h  | 2 +-
 arch/x86/kvm/x86.c               | 7 +++++--
 include/linux/kvm_host.h         | 1 +
 virt/kvm/async_pf.c              | 2 +-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 3d554887794e..cee3cb6455a2 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu);
 void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
 			       struct kvm_async_pf *work);
 
-void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
+bool 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,
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 06bde4bad205..33fea4488ef3 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3923,11 +3923,13 @@ static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
 	}
 }
 
-void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
+bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 				     struct kvm_async_pf *work)
 {
 	trace_kvm_s390_pfault_init(vcpu, work->arch.pfault_token);
 	__kvm_inject_pfault_token(vcpu, true, work->arch.pfault_token);
+
+	return true;
 }
 
 void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6e03c021956a..f54e7499fc6a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1660,7 +1660,7 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm);
 void kvm_make_scan_ioapic_request_mask(struct kvm *kvm,
 				       unsigned long *vcpu_bitmap);
 
-void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
+bool 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,
 				 struct kvm_async_pf *work);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 13d0b0fa1a7c..75e4c68e9586 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10513,7 +10513,7 @@ bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu)
 	return kvm_arch_interrupt_allowed(vcpu);
 }
 
-void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
+bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 				     struct kvm_async_pf *work)
 {
 	struct x86_exception fault;
@@ -10530,6 +10530,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 		fault.address = work->arch.token;
 		fault.async_page_fault = true;
 		kvm_inject_page_fault(vcpu, &fault);
+		return true;
 	} else {
 		/*
 		 * It is not possible to deliver a paravirtualized asynchronous
@@ -10540,6 +10541,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 		 * fault is retried, hopefully the page will be ready in the host.
 		 */
 		kvm_make_request(KVM_REQ_APF_HALT, vcpu);
+		return false;
 	}
 }
 
@@ -10557,7 +10559,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 		kvm_del_async_pf_gfn(vcpu, work->arch.gfn);
 	trace_kvm_async_pf_ready(work->arch.token, work->cr2_or_gpa);
 
-	if (kvm_pv_async_pf_enabled(vcpu) &&
+	if ((work->wakeup_all || work->notpresent_injected) &&
+	    kvm_pv_async_pf_enabled(vcpu) &&
 	    !apf_put_user_ready(vcpu, work->arch.token)) {
 		vcpu->arch.apf.pageready_pending = true;
 		kvm_apic_set_irq(vcpu, &irq, NULL);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 802b9e2306f0..2456dc5338f8 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -206,6 +206,7 @@ struct kvm_async_pf {
 	unsigned long addr;
 	struct kvm_arch_async_pf arch;
 	bool   wakeup_all;
+	bool notpresent_injected;
 };
 
 void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index ba080088da76..a36828fbf40a 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -189,7 +189,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 
 	list_add_tail(&work->queue, &vcpu->async_pf.queue);
 	vcpu->async_pf.queued++;
-	kvm_arch_async_page_not_present(vcpu, work);
+	work->notpresent_injected = kvm_arch_async_page_not_present(vcpu, work);
 
 	schedule_work(&work->work);
 
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf()
  2020-06-10 17:55 [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Vitaly Kuznetsov
  2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov
@ 2020-06-10 18:14 ` Sean Christopherson
  2020-06-11  0:07   ` Paolo Bonzini
  2020-06-11  8:31   ` Vitaly Kuznetsov
  1 sibling, 2 replies; 11+ messages in thread
From: Sean Christopherson @ 2020-06-10 18:14 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel

On Wed, Jun 10, 2020 at 07:55:31PM +0200, Vitaly Kuznetsov wrote:
> schedule_work() returns 'false' only when the work is already on the queue
> and this can't happen as kvm_setup_async_pf() always allocates a new one.
> Also, to avoid potential race, it makes sense to to schedule_work() at the
> very end after we've added it to the queue.
> 
> While on it, do some minor cleanup. gfn_to_pfn_async() mentioned in a
> comment does not currently exist and, moreover, we can check
> kvm_is_error_hva() at the very beginning, before we try to allocate work so
> 'retry_sync' label can go away completely.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  virt/kvm/async_pf.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index f1e07fae84e9..ba080088da76 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -164,7 +164,9 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  	if (vcpu->async_pf.queued >= ASYNC_PF_PER_VCPU)
>  		return 0;
>  
> -	/* setup delayed work */
> +	/* Arch specific code should not do async PF in this case */
> +	if (unlikely(kvm_is_error_hva(hva)))

This feels like it should be changed to a WARN_ON_ONCE in a follow-up.
With the WARN, the comment could probably be dropped.

I'd also be in favor of changing the return type to a boolean.  I think
you alluded to it earlier, the current semantics are quite confusing as they
invert the normal "return 0 on success".

For this patch:

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected
  2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov
@ 2020-06-10 19:32   ` Vivek Goyal
  2020-06-10 19:47     ` Sean Christopherson
  2020-06-11  8:14     ` Vitaly Kuznetsov
  0 siblings, 2 replies; 11+ messages in thread
From: Vivek Goyal @ 2020-06-10 19:32 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: kvm, Paolo Bonzini, Sean Christopherson, Wanpeng Li, Jim Mattson,
	linux-kernel

On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote:
> 'Page not present' event may or may not get injected depending on
> guest's state. If the event wasn't injected, there is no need to
> inject the corresponding 'page ready' event as the guest may get
> confused. E.g. Linux thinks that the corresponding 'page not present'
> event wasn't delivered *yet* and allocates a 'dummy entry' for it.
> This entry is never freed.
> 
> Note, 'wakeup all' events have no corresponding 'page not present'
> event and always get injected.
> 
> s390 seems to always be able to inject 'page not present', the
> change is effectively a nop.
> 
> Suggested-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 2 +-
>  arch/s390/kvm/kvm-s390.c         | 4 +++-
>  arch/x86/include/asm/kvm_host.h  | 2 +-
>  arch/x86/kvm/x86.c               | 7 +++++--
>  include/linux/kvm_host.h         | 1 +
>  virt/kvm/async_pf.c              | 2 +-
>  6 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 3d554887794e..cee3cb6455a2 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu);
>  void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
>  			       struct kvm_async_pf *work);
>  
> -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>  				     struct kvm_async_pf *work);

Hi Vitaly,

A minor nit. Using return code to figure out if exception was injected
or not is little odd. How about we pass a pointer instead as parameter
and kvm_arch_async_page_not_present() sets it to true if page not
present exception was injected. This probably will be easier to
read.

If for some reason you don't like above, atleats it warrants a comment
explaining what do 0 and 1 mean.

Otherwise both the patches look good to me. I tested and I can confirm
that now page ready events are not being delivered to guest if page
not present was not injected.

Thanks
Vivek

>  
>  void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 06bde4bad205..33fea4488ef3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3923,11 +3923,13 @@ static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
>  	}
>  }
>  
> -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>  				     struct kvm_async_pf *work)
>  {
>  	trace_kvm_s390_pfault_init(vcpu, work->arch.pfault_token);
>  	__kvm_inject_pfault_token(vcpu, true, work->arch.pfault_token);
> +
> +	return true;
>  }
>  
>  void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6e03c021956a..f54e7499fc6a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1660,7 +1660,7 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm);
>  void kvm_make_scan_ioapic_request_mask(struct kvm *kvm,
>  				       unsigned long *vcpu_bitmap);
>  
> -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> +bool 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,
>  				 struct kvm_async_pf *work);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 13d0b0fa1a7c..75e4c68e9586 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10513,7 +10513,7 @@ bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu)
>  	return kvm_arch_interrupt_allowed(vcpu);
>  }
>  
> -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>  				     struct kvm_async_pf *work)
>  {
>  	struct x86_exception fault;
> @@ -10530,6 +10530,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>  		fault.address = work->arch.token;
>  		fault.async_page_fault = true;
>  		kvm_inject_page_fault(vcpu, &fault);
> +		return true;
>  	} else {
>  		/*
>  		 * It is not possible to deliver a paravirtualized asynchronous
> @@ -10540,6 +10541,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>  		 * fault is retried, hopefully the page will be ready in the host.
>  		 */
>  		kvm_make_request(KVM_REQ_APF_HALT, vcpu);
> +		return false;
>  	}
>  }
>  
> @@ -10557,7 +10559,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
>  		kvm_del_async_pf_gfn(vcpu, work->arch.gfn);
>  	trace_kvm_async_pf_ready(work->arch.token, work->cr2_or_gpa);
>  
> -	if (kvm_pv_async_pf_enabled(vcpu) &&
> +	if ((work->wakeup_all || work->notpresent_injected) &&
> +	    kvm_pv_async_pf_enabled(vcpu) &&
>  	    !apf_put_user_ready(vcpu, work->arch.token)) {
>  		vcpu->arch.apf.pageready_pending = true;
>  		kvm_apic_set_irq(vcpu, &irq, NULL);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 802b9e2306f0..2456dc5338f8 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -206,6 +206,7 @@ struct kvm_async_pf {
>  	unsigned long addr;
>  	struct kvm_arch_async_pf arch;
>  	bool   wakeup_all;
> +	bool notpresent_injected;
>  };
>  
>  void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index ba080088da76..a36828fbf40a 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -189,7 +189,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  
>  	list_add_tail(&work->queue, &vcpu->async_pf.queue);
>  	vcpu->async_pf.queued++;
> -	kvm_arch_async_page_not_present(vcpu, work);
> +	work->notpresent_injected = kvm_arch_async_page_not_present(vcpu, work);
>  
>  	schedule_work(&work->work);
>  
> -- 
> 2.25.4
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected
  2020-06-10 19:32   ` Vivek Goyal
@ 2020-06-10 19:47     ` Sean Christopherson
  2020-06-10 19:57       ` Vivek Goyal
  2020-06-11  8:14     ` Vitaly Kuznetsov
  1 sibling, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2020-06-10 19:47 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Vitaly Kuznetsov, kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson,
	linux-kernel

On Wed, Jun 10, 2020 at 03:32:11PM -0400, Vivek Goyal wrote:
> On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote:
> > 'Page not present' event may or may not get injected depending on
> > guest's state. If the event wasn't injected, there is no need to
> > inject the corresponding 'page ready' event as the guest may get
> > confused. E.g. Linux thinks that the corresponding 'page not present'
> > event wasn't delivered *yet* and allocates a 'dummy entry' for it.
> > This entry is never freed.
> > 
> > Note, 'wakeup all' events have no corresponding 'page not present'
> > event and always get injected.
> > 
> > s390 seems to always be able to inject 'page not present', the
> > change is effectively a nop.
> > 
> > Suggested-by: Vivek Goyal <vgoyal@redhat.com>
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > ---
> >  arch/s390/include/asm/kvm_host.h | 2 +-
> >  arch/s390/kvm/kvm-s390.c         | 4 +++-
> >  arch/x86/include/asm/kvm_host.h  | 2 +-
> >  arch/x86/kvm/x86.c               | 7 +++++--
> >  include/linux/kvm_host.h         | 1 +
> >  virt/kvm/async_pf.c              | 2 +-
> >  6 files changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index 3d554887794e..cee3cb6455a2 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu);
> >  void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
> >  			       struct kvm_async_pf *work);
> >  
> > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> >  				     struct kvm_async_pf *work);
> 
> Hi Vitaly,
> 
> A minor nit. Using return code to figure out if exception was injected
> or not is little odd. How about we pass a pointer instead as parameter
> and kvm_arch_async_page_not_present() sets it to true if page not
> present exception was injected. This probably will be easier to
> read.
> 
> If for some reason you don't like above, atleats it warrants a comment
> explaining what do 0 and 1 mean.
> 
> Otherwise both the patches look good to me. I tested and I can confirm
> that now page ready events are not being delivered to guest if page
> not present was not injected.

Why does kvm_arch_async_page_not_present() need to "return" anything?  It
has access to @work, e.g. simply replace "return true" with
"work->notpresent_injected = true".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected
  2020-06-10 19:47     ` Sean Christopherson
@ 2020-06-10 19:57       ` Vivek Goyal
  2020-06-10 23:53         ` Paolo Bonzini
  0 siblings, 1 reply; 11+ messages in thread
From: Vivek Goyal @ 2020-06-10 19:57 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson,
	linux-kernel

On Wed, Jun 10, 2020 at 12:47:38PM -0700, Sean Christopherson wrote:
> On Wed, Jun 10, 2020 at 03:32:11PM -0400, Vivek Goyal wrote:
> > On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote:
> > > 'Page not present' event may or may not get injected depending on
> > > guest's state. If the event wasn't injected, there is no need to
> > > inject the corresponding 'page ready' event as the guest may get
> > > confused. E.g. Linux thinks that the corresponding 'page not present'
> > > event wasn't delivered *yet* and allocates a 'dummy entry' for it.
> > > This entry is never freed.
> > > 
> > > Note, 'wakeup all' events have no corresponding 'page not present'
> > > event and always get injected.
> > > 
> > > s390 seems to always be able to inject 'page not present', the
> > > change is effectively a nop.
> > > 
> > > Suggested-by: Vivek Goyal <vgoyal@redhat.com>
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > ---
> > >  arch/s390/include/asm/kvm_host.h | 2 +-
> > >  arch/s390/kvm/kvm-s390.c         | 4 +++-
> > >  arch/x86/include/asm/kvm_host.h  | 2 +-
> > >  arch/x86/kvm/x86.c               | 7 +++++--
> > >  include/linux/kvm_host.h         | 1 +
> > >  virt/kvm/async_pf.c              | 2 +-
> > >  6 files changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > > index 3d554887794e..cee3cb6455a2 100644
> > > --- a/arch/s390/include/asm/kvm_host.h
> > > +++ b/arch/s390/include/asm/kvm_host.h
> > > @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu);
> > >  void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
> > >  			       struct kvm_async_pf *work);
> > >  
> > > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> > > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> > >  				     struct kvm_async_pf *work);
> > 
> > Hi Vitaly,
> > 
> > A minor nit. Using return code to figure out if exception was injected
> > or not is little odd. How about we pass a pointer instead as parameter
> > and kvm_arch_async_page_not_present() sets it to true if page not
> > present exception was injected. This probably will be easier to
> > read.
> > 
> > If for some reason you don't like above, atleats it warrants a comment
> > explaining what do 0 and 1 mean.
> > 
> > Otherwise both the patches look good to me. I tested and I can confirm
> > that now page ready events are not being delivered to guest if page
> > not present was not injected.
> 
> Why does kvm_arch_async_page_not_present() need to "return" anything?  It
> has access to @work, e.g. simply replace "return true" with
> "work->notpresent_injected = true".

We could do it and I thought about it. But modifying work->notpresent_injected
inside kvm_arch_async_page_not_present() again feels very unintuitive.

I personally find it better that initialization of
work->notpresent_injected is very explicit at the site where this 
structure has been allocated and being initialized. (Instead of a
a callee function silently initializing a filed of this structure).

Thanks
Vivek


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected
  2020-06-10 19:57       ` Vivek Goyal
@ 2020-06-10 23:53         ` Paolo Bonzini
  0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2020-06-10 23:53 UTC (permalink / raw)
  To: Vivek Goyal, Sean Christopherson
  Cc: Vitaly Kuznetsov, kvm, Wanpeng Li, Jim Mattson, linux-kernel

On 10/06/20 21:57, Vivek Goyal wrote:
> I personally find it better that initialization of
> work->notpresent_injected is very explicit at the site where this 
> structure has been allocated and being initialized. (Instead of a
> a callee function silently initializing a filed of this structure).

I agree.

Paolo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf()
  2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson
@ 2020-06-11  0:07   ` Paolo Bonzini
  2020-06-11  8:31   ` Vitaly Kuznetsov
  1 sibling, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2020-06-11  0:07 UTC (permalink / raw)
  To: Sean Christopherson, Vitaly Kuznetsov
  Cc: kvm, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel

On 10/06/20 20:14, Sean Christopherson wrote:
>> -	/* setup delayed work */
>> +	/* Arch specific code should not do async PF in this case */
>> +	if (unlikely(kvm_is_error_hva(hva)))
> This feels like it should be changed to a WARN_ON_ONCE in a follow-up.
> With the WARN, the comment could probably be dropped.

I think a race is possible in principle where the memslots are changed
(for example) between s390's page fault handler and the gfn_to_hva call
in kvm_arch_setup_async_pf.

Queued both, thanks!

Paolo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected
  2020-06-10 19:32   ` Vivek Goyal
  2020-06-10 19:47     ` Sean Christopherson
@ 2020-06-11  8:14     ` Vitaly Kuznetsov
  1 sibling, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2020-06-11  8:14 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: kvm, Paolo Bonzini, Sean Christopherson, Wanpeng Li, Jim Mattson,
	linux-kernel

Vivek Goyal <vgoyal@redhat.com> writes:

> On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote:
>> 'Page not present' event may or may not get injected depending on
>> guest's state. If the event wasn't injected, there is no need to
>> inject the corresponding 'page ready' event as the guest may get
>> confused. E.g. Linux thinks that the corresponding 'page not present'
>> event wasn't delivered *yet* and allocates a 'dummy entry' for it.
>> This entry is never freed.
>> 
>> Note, 'wakeup all' events have no corresponding 'page not present'
>> event and always get injected.
>> 
>> s390 seems to always be able to inject 'page not present', the
>> change is effectively a nop.
>> 
>> Suggested-by: Vivek Goyal <vgoyal@redhat.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h | 2 +-
>>  arch/s390/kvm/kvm-s390.c         | 4 +++-
>>  arch/x86/include/asm/kvm_host.h  | 2 +-
>>  arch/x86/kvm/x86.c               | 7 +++++--
>>  include/linux/kvm_host.h         | 1 +
>>  virt/kvm/async_pf.c              | 2 +-
>>  6 files changed, 12 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 3d554887794e..cee3cb6455a2 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu);
>>  void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
>>  			       struct kvm_async_pf *work);
>>  
>> -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>> +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>>  				     struct kvm_async_pf *work);
>
> Hi Vitaly,
>
> A minor nit. Using return code to figure out if exception was injected
> or not is little odd. How about we pass a pointer instead as parameter
> and kvm_arch_async_page_not_present() sets it to true if page not
> present exception was injected. This probably will be easier to
> read.
>
> If for some reason you don't like above, atleats it warrants a comment
> explaining what do 0 and 1 mean.
>

I think it's the 'kvm_arch_async_page_not_present' name which is a bit
misleading now, if we rename it to something like
kvm_arch_inject_apf_not_present() then it becomes a bit more clear
what's going on. We may as well write the code as

    if (kvm_arch_inject_apf_not_present())
        work->notpresent_injected = true;

or change the return type to int so it'll be

    if (!kvm_arch_inject_apf_not_present())
        work->notpresent_injected = true;

> Otherwise both the patches look good to me. I tested and I can confirm
> that now page ready events are not being delivered to guest if page
> not present was not injected.

Thank you for testing!

-- 
Vitaly


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf()
  2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson
  2020-06-11  0:07   ` Paolo Bonzini
@ 2020-06-11  8:31   ` Vitaly Kuznetsov
  2020-06-11 15:35     ` Sean Christopherson
  1 sibling, 1 reply; 11+ messages in thread
From: Vitaly Kuznetsov @ 2020-06-11  8:31 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

>
> I'd also be in favor of changing the return type to a boolean.  I think
> you alluded to it earlier, the current semantics are quite confusing as they
> invert the normal "return 0 on success".

Yes, will do a follow-up.

KVM/x86 code has an intertwined mix of:
- normal 'int' functions ('0 on success')
- bool functions ('true'/'1' on success)
- 'int' exit handlers ('1'/'0' on success depending if exit to userspace
  was required)
- ...

I think we can try to standardize this to:
- 'int' when error is propagated outside of KVM (userspace, other kernel
  subsystem,...)
- 'bool' when the function is internal to KVM and the result is binary
 ('is_exit_required()', 'was_pf_injected()', 'will_have_another_beer()',
 ...)
- 'enum' for the rest.
And, if there's a good reason for making an exception, require a
comment. (leaving aside everything returning a pointer, of course as
these are self-explanatory -- unless it's 'void *' :-))

>
> For this patch:
>
> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
>

Thank you!

-- 
Vitaly


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf()
  2020-06-11  8:31   ` Vitaly Kuznetsov
@ 2020-06-11 15:35     ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2020-06-11 15:35 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel

On Thu, Jun 11, 2020 at 10:31:08AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> >
> > I'd also be in favor of changing the return type to a boolean.  I think
> > you alluded to it earlier, the current semantics are quite confusing as they
> > invert the normal "return 0 on success".
> 
> Yes, will do a follow-up.
> 
> KVM/x86 code has an intertwined mix of:
> - normal 'int' functions ('0 on success')
> - bool functions ('true'/'1' on success)
> - 'int' exit handlers ('1'/'0' on success depending if exit to userspace
>   was required)
> - ...
> 
> I think we can try to standardize this to:
> - 'int' when error is propagated outside of KVM (userspace, other kernel
>   subsystem,...)
> - 'bool' when the function is internal to KVM and the result is binary
>  ('is_exit_required()', 'was_pf_injected()', 'will_have_another_beer()',
>  ...)
> - 'enum' for the rest.
> And, if there's a good reason for making an exception, require a
> comment. (leaving aside everything returning a pointer, of course as
> these are self-explanatory -- unless it's 'void *' :-))

Agreed for 'bool' case, but 'int' versus 'enum' is less straightforward as
there are a huge number of functions that _may_ propagate an error outside
of KVM, including all of the exit handlers.  As Paolo point out[*], it'd
be quite easy to end up with a mixture of enum/#define and 0/1 code, which
would be an even bigger mess than what we have today.  There are
undoubtedly cases that could be converted to an enum, but they're probably
few and far between as it requires total encapsulation, e.g. the emulator.

[*] https://lkml.kernel.org/r/3d827e8b-a04e-0a93-4bb4-e0e9d59036da@redhat.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-06-11 15:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 17:55 [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Vitaly Kuznetsov
2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov
2020-06-10 19:32   ` Vivek Goyal
2020-06-10 19:47     ` Sean Christopherson
2020-06-10 19:57       ` Vivek Goyal
2020-06-10 23:53         ` Paolo Bonzini
2020-06-11  8:14     ` Vitaly Kuznetsov
2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson
2020-06-11  0:07   ` Paolo Bonzini
2020-06-11  8:31   ` Vitaly Kuznetsov
2020-06-11 15:35     ` Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).