[PATCH 0/5] kvm: avoid delaying async_pf ready delivery

[PATCH 0/5] kvm: avoid delaying async_pf ready delivery

[Roman Kagan]

Make the guest immediately aware of async_pf's being resolved, to
prevent it from holding off higher priority tasks in favor of lower
priority ones, including nested guests.

Roman Kagan (5):
  kvm/x86: fix inversed check for async_pf MSR
  kvm: add helper for testing ready async_pf's
  kvm: kick vcpu when async_pf is resolved
  kvm/vmx: kick L2 guest to L1 by ready async_pf
  kvm/svm: kick L2 guest to L1 by ready async_pf

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>

 include/linux/kvm_host.h |  7 +++++++
 arch/x86/kvm/svm.c       | 10 ++++++++++
 arch/x86/kvm/vmx.c       |  9 +++++----
 arch/x86/kvm/x86.c       |  4 ++--
 virt/kvm/async_pf.c      | 11 +++++++----
 5 files changed, 31 insertions(+), 10 deletions(-)

-- 
2.9.3

[PATCH 1/5] kvm/x86: fix inversed check for async_pf MSR

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bf11fe4..14a46e9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8402,7 +8402,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu)
 {
 	if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED))
-		return true;
+		return false;
 	else
 		return !kvm_event_needs_reinjection(vcpu) &&
 			kvm_x86_ops->interrupt_allowed(vcpu);
-- 
2.9.3

[PATCH 2/5] kvm: add helper for testing ready async_pf's

[Roman Kagan]

Add helper inline function to test if there are async_pf's ready on a
given vCPU.  There are already 3 callsites for it and I'm about to add
more.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 include/linux/kvm_host.h | 7 +++++++
 arch/x86/kvm/x86.c       | 2 +-
 virt/kvm/async_pf.c      | 4 ++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 01c0b9c..e10516f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1207,4 +1207,11 @@ static inline bool vcpu_valid_wakeup(struct kvm_vcpu *vcpu)
 }
 #endif /* CONFIG_HAVE_KVM_INVALID_WAKEUPS */
 
+#ifdef CONFIG_KVM_ASYNC_PF
+static inline bool kvm_async_pf_has_ready(struct kvm_vcpu *vcpu)
+{
+	return !list_empty_careful(&vcpu->async_pf.done);
+}
+#endif
+
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 14a46e9..638fc5f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8182,7 +8182,7 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 
 static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
 {
-	if (!list_empty_careful(&vcpu->async_pf.done))
+	if (kvm_async_pf_has_ready(vcpu))
 		return true;
 
 	if (kvm_apic_has_events(vcpu))
diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index efeceb0a..9cced14 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -159,7 +159,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
 {
 	struct kvm_async_pf *work;
 
-	while (!list_empty_careful(&vcpu->async_pf.done) &&
+	while (kvm_async_pf_has_ready(vcpu) &&
 	      kvm_arch_can_inject_async_page_present(vcpu)) {
 		spin_lock(&vcpu->async_pf.lock);
 		work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
@@ -227,7 +227,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
 {
 	struct kvm_async_pf *work;
 
-	if (!list_empty_careful(&vcpu->async_pf.done))
+	if (kvm_async_pf_has_ready(vcpu))
 		return 0;
 
 	work = kmem_cache_zalloc(async_pf_cache, GFP_ATOMIC);
-- 
2.9.3

[PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[Roman Kagan]

When async_pf is ready the guest needs to be made aware of it ASAP,
because it may be holding off a higher priority task pending the
async_pf resolution in favor of a lower priority one.

In case async_pf's are harvested in vcpu context (x86) we have to not
only wake the vcpu up but kick it into host.

While at this, also replace the open-coded vcpu wakeup by the existing
helper.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 virt/kvm/async_pf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index 9cced14..5f0a66c 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work)
 	 * This memory barrier pairs with prepare_to_wait's set_current_state()
 	 */
 	smp_mb();
-	if (swait_active(&vcpu->wq))
-		swake_up(&vcpu->wq);
+#ifdef CONFIG_KVM_ASYNC_PF_SYNC
+	kvm_vcpu_wake_up(vcpu);
+#else
+	kvm_vcpu_kick(vcpu);
+#endif
 
 	mmput(mm);
 	kvm_put_kvm(vcpu->kvm);
-- 
2.9.3

[PATCH 4/5] kvm/vmx: kick L2 guest to L1 by ready async_pf

[Roman Kagan]

When async pagefault is resolved vCPU may be executing L2 guest.

In order to allow L1 take better scheduling decisions in such cases,
make L2 exit to L1 on a fake external interupt, without actually
injecting it (unless L2 has other reasons to vmexit).

This patch does that for x86/Intel.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 arch/x86/kvm/vmx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5382b82..fc97489 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10420,7 +10420,8 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)
 		return 0;
 	}
 
-	if ((kvm_cpu_has_interrupt(vcpu) || external_intr) &&
+	if ((kvm_cpu_has_interrupt(vcpu) || external_intr ||
+	     kvm_async_pf_has_ready(vcpu)) &&
 	    nested_exit_on_intr(vcpu)) {
 		if (vmx->nested.nested_run_pending)
 			return -EBUSY;
@@ -10772,9 +10773,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
 	    && nested_exit_intr_ack_set(vcpu)) {
 		int irq = kvm_cpu_get_interrupt(vcpu);
-		WARN_ON(irq < 0);
-		vmcs12->vm_exit_intr_info = irq |
-			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
+		if (irq >= 0)
+			vmcs12->vm_exit_intr_info = irq |
+				INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
 	}
 
 	trace_kvm_nested_vmexit_inject(vmcs12->vm_exit_reason,
-- 
2.9.3

[PATCH 5/5] kvm/svm: kick L2 guest to L1 by ready async_pf

[Roman Kagan]

When async pagefault is resolved vCPU may be executing L2 guest.

In order to allow L1 take better scheduling decisions in such cases,
make L2 exit to L1 on a fake external interupt, without actually
injecting it (unless L2 has other reasons to vmexit).

This patch does that for x86/AMD.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
Note 1: I didn't have a chance to test this; I'll do when I get access to an
AMD machine and let you know if anything goes wrong

Note 2: I mostly modelled this patch after the one for vmx but is looks not
very appealing for svm.  I'd appreciate being pointed at a better location
where to stick the fake external interrupt vmexit.

 arch/x86/kvm/svm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8ca1eca..1f6ae15 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3723,6 +3723,15 @@ static int mwait_interception(struct vcpu_svm *svm)
 	return nop_interception(svm);
 }
 
+static int svm_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (kvm_async_pf_has_ready(vcpu))
+		nested_svm_intr(svm);
+	return 0;
+}
+
 enum avic_ipi_failure_cause {
 	AVIC_IPI_FAILURE_INVALID_INT_TYPE,
 	AVIC_IPI_FAILURE_TARGET_NOT_RUNNING,
@@ -5406,6 +5415,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 
 	.check_intercept = svm_check_intercept,
 	.handle_external_intr = svm_handle_external_intr,
+	.check_nested_events = svm_check_nested_events,
 
 	.sched_in = svm_sched_in,
 
-- 
2.9.3

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[Christian Borntraeger]

On 12/02/2016 09:47 AM, Roman Kagan wrote:
> When async_pf is ready the guest needs to be made aware of it ASAP,
> because it may be holding off a higher priority task pending the
> async_pf resolution in favor of a lower priority one.
> 
> In case async_pf's are harvested in vcpu context (x86) we have to not
> only wake the vcpu up but kick it into host.
> 
> While at this, also replace the open-coded vcpu wakeup by the existing
> helper.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  virt/kvm/async_pf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index 9cced14..5f0a66c 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work)
>  	 * This memory barrier pairs with prepare_to_wait's set_current_state()
>  	 */
>  	smp_mb();
> -	if (swait_active(&vcpu->wq))
> -		swake_up(&vcpu->wq);
> +#ifdef CONFIG_KVM_ASYNC_PF_SYNC
> +	kvm_vcpu_wake_up(vcpu);
> +#else
> +	kvm_vcpu_kick(vcpu);
> +#endif

This will break s390, both functions are disabled for s390.
On s390 do not want to kick the CPU for a completion. Instead we implement
the kvm_async_page_present_sync call above and handle completion via an 
"pfault done" interrupt via the normal interrupt delivery.

> 
>  	mmput(mm);
>  	kvm_put_kvm(vcpu->kvm);
> 

Re: [PATCH 5/5] kvm/svm: kick L2 guest to L1 by ready async_pf

[Paolo Bonzini]

----- Original Message -----
> From: "Roman Kagan" <rkagan@virtuozzo.com>
> To: "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org
> Cc: "Denis Lunev" <den@virtuozzo.com>, "Roman Kagan" <rkagan@virtuozzo.com>
> Sent: Friday, December 2, 2016 9:47:54 AM
> Subject: [PATCH 5/5] kvm/svm: kick L2 guest to L1 by ready async_pf
> 
> When async pagefault is resolved vCPU may be executing L2 guest.
> 
> In order to allow L1 take better scheduling decisions in such cases,
> make L2 exit to L1 on a fake external interupt, without actually
> injecting it (unless L2 has other reasons to vmexit).
> 
> This patch does that for x86/AMD.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> Note 1: I didn't have a chance to test this; I'll do when I get access to an
> AMD machine and let you know if anything goes wrong
> 
> Note 2: I mostly modelled this patch after the one for vmx but is looks not
> very appealing for svm.  I'd appreciate being pointed at a better location
> where to stick the fake external interrupt vmexit.

This should not be necessary, SVM has a different mechanism (which
requires L1 cooperation) to handle async page faults.

Paolo

> 
>  arch/x86/kvm/svm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 8ca1eca..1f6ae15 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3723,6 +3723,15 @@ static int mwait_interception(struct vcpu_svm *svm)
>  	return nop_interception(svm);
>  }
>  
> +static int svm_check_nested_events(struct kvm_vcpu *vcpu, bool
> external_intr)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	if (kvm_async_pf_has_ready(vcpu))
> +		nested_svm_intr(svm);
> +	return 0;
> +}
> +
>  enum avic_ipi_failure_cause {
>  	AVIC_IPI_FAILURE_INVALID_INT_TYPE,
>  	AVIC_IPI_FAILURE_TARGET_NOT_RUNNING,
> @@ -5406,6 +5415,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init =
> {
>  
>  	.check_intercept = svm_check_intercept,
>  	.handle_external_intr = svm_handle_external_intr,
> +	.check_nested_events = svm_check_nested_events,
>  
>  	.sched_in = svm_sched_in,
>  
> --
> 2.9.3
> 
> 

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[Paolo Bonzini]

----- Original Message -----
> From: "Christian Borntraeger" <borntraeger@de.ibm.com>
> To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>,
> kvm@vger.kernel.org
> Cc: "Denis Lunev" <den@virtuozzo.com>
> Sent: Friday, December 2, 2016 10:35:02 AM
> Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved
> 
> On 12/02/2016 09:47 AM, Roman Kagan wrote:
> > When async_pf is ready the guest needs to be made aware of it ASAP,
> > because it may be holding off a higher priority task pending the
> > async_pf resolution in favor of a lower priority one.
> > 
> > In case async_pf's are harvested in vcpu context (x86) we have to not
> > only wake the vcpu up but kick it into host.
> > 
> > While at this, also replace the open-coded vcpu wakeup by the existing
> > helper.
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > ---
> >  virt/kvm/async_pf.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> > index 9cced14..5f0a66c 100644
> > --- a/virt/kvm/async_pf.c
> > +++ b/virt/kvm/async_pf.c
> > @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work)
> >  	 * This memory barrier pairs with prepare_to_wait's set_current_state()
> >  	 */
> >  	smp_mb();
> > -	if (swait_active(&vcpu->wq))
> > -		swake_up(&vcpu->wq);
> > +#ifdef CONFIG_KVM_ASYNC_PF_SYNC
> > +	kvm_vcpu_wake_up(vcpu);
> > +#else
> > +	kvm_vcpu_kick(vcpu);
> > +#endif
> 
> This will break s390, both functions are disabled for s390.
> On s390 do not want to kick the CPU for a completion. Instead we implement
> the kvm_async_page_present_sync call above and handle completion via an
> "pfault done" interrupt via the normal interrupt delivery.

Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390?
It was unused until now, but the patch makes sense as a cleanup.

Paolo

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[Christian Borntraeger]

On 12/02/2016 10:40 AM, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
>> From: "Christian Borntraeger" <borntraeger@de.ibm.com>
>> To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>,
>> kvm@vger.kernel.org
>> Cc: "Denis Lunev" <den@virtuozzo.com>
>> Sent: Friday, December 2, 2016 10:35:02 AM
>> Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved
>>
>> On 12/02/2016 09:47 AM, Roman Kagan wrote:
>>> When async_pf is ready the guest needs to be made aware of it ASAP,
>>> because it may be holding off a higher priority task pending the
>>> async_pf resolution in favor of a lower priority one.
>>>
>>> In case async_pf's are harvested in vcpu context (x86) we have to not
>>> only wake the vcpu up but kick it into host.
>>>
>>> While at this, also replace the open-coded vcpu wakeup by the existing
>>> helper.
>>>
>>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
>>> ---
>>>  virt/kvm/async_pf.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
>>> index 9cced14..5f0a66c 100644
>>> --- a/virt/kvm/async_pf.c
>>> +++ b/virt/kvm/async_pf.c
>>> @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work)
>>>  	 * This memory barrier pairs with prepare_to_wait's set_current_state()
>>>  	 */
>>>  	smp_mb();
>>> -	if (swait_active(&vcpu->wq))
>>> -		swake_up(&vcpu->wq);
>>> +#ifdef CONFIG_KVM_ASYNC_PF_SYNC
>>> +	kvm_vcpu_wake_up(vcpu);
>>> +#else
>>> +	kvm_vcpu_kick(vcpu);
>>> +#endif
>>
>> This will break s390, both functions are disabled for s390.
>> On s390 do not want to kick the CPU for a completion. Instead we implement
>> the kvm_async_page_present_sync call above and handle completion via an
>> "pfault done" interrupt via the normal interrupt delivery.
> 
> Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390?
> It was unused until now, but the patch makes sense as a cleanup.


We could enable that. It was some kind of a trigger, that we get a build error
when someone enables that for s390 (as it might not be want you want)

On the other hand,  I dont think that we need a wakeup at all for the
SYNC case. (as the interrupt will do that anyway)
Maybe something like that

-	if (swait_active(&vcpu->wq))
-		swake_up(&vcpu->wq);
+#ifndef CONFIG_KVM_ASYNC_PF_SYNC
+	kvm_vcpu_kick(vcpu);
+#endif

would be good enough. (needs more thinking to be sure)

Christian

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[Roman Kagan]

On Fri, Dec 02, 2016 at 10:35:02AM +0100, Christian Borntraeger wrote:
> On 12/02/2016 09:47 AM, Roman Kagan wrote:
> > When async_pf is ready the guest needs to be made aware of it ASAP,
> > because it may be holding off a higher priority task pending the
> > async_pf resolution in favor of a lower priority one.
> > 
> > In case async_pf's are harvested in vcpu context (x86) we have to not
> > only wake the vcpu up but kick it into host.
> > 
> > While at this, also replace the open-coded vcpu wakeup by the existing
> > helper.
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > ---
> >  virt/kvm/async_pf.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> > index 9cced14..5f0a66c 100644
> > --- a/virt/kvm/async_pf.c
> > +++ b/virt/kvm/async_pf.c
> > @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work)
> >  	 * This memory barrier pairs with prepare_to_wait's set_current_state()
> >  	 */
> >  	smp_mb();
> > -	if (swait_active(&vcpu->wq))
> > -		swake_up(&vcpu->wq);
> > +#ifdef CONFIG_KVM_ASYNC_PF_SYNC
> > +	kvm_vcpu_wake_up(vcpu);
> > +#else
> > +	kvm_vcpu_kick(vcpu);
> > +#endif
> 
> This will break s390, both functions are disabled for s390.

Indeed.  Sorry I didn't notice that #ifndef CONFIG_S390 ...

> On s390 do not want to kick the CPU for a completion. Instead we implement
> the kvm_async_page_present_sync call above and handle completion via an 
> "pfault done" interrupt via the normal interrupt delivery.

I wonder if waking up is necessary at all then?

Thanks,
Roman.

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[Roman Kagan]

On Fri, Dec 02, 2016 at 11:00:00AM +0100, Christian Borntraeger wrote:
> On 12/02/2016 10:40 AM, Paolo Bonzini wrote:
> > ----- Original Message -----
> >> From: "Christian Borntraeger" <borntraeger@de.ibm.com>
> >> To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>,
> >> kvm@vger.kernel.org
> >> Cc: "Denis Lunev" <den@virtuozzo.com>
> >> Sent: Friday, December 2, 2016 10:35:02 AM
> >> Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved
> >>
> >> On 12/02/2016 09:47 AM, Roman Kagan wrote:
> >>> When async_pf is ready the guest needs to be made aware of it ASAP,
> >>> because it may be holding off a higher priority task pending the
> >>> async_pf resolution in favor of a lower priority one.
> >>>
> >>> In case async_pf's are harvested in vcpu context (x86) we have to not
> >>> only wake the vcpu up but kick it into host.
> >>>
> >>> While at this, also replace the open-coded vcpu wakeup by the existing
> >>> helper.
> >>>
> >>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> >>> ---
> >>>  virt/kvm/async_pf.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> >>> index 9cced14..5f0a66c 100644
> >>> --- a/virt/kvm/async_pf.c
> >>> +++ b/virt/kvm/async_pf.c
> >>> @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work)
> >>>  	 * This memory barrier pairs with prepare_to_wait's set_current_state()
> >>>  	 */
> >>>  	smp_mb();
> >>> -	if (swait_active(&vcpu->wq))
> >>> -		swake_up(&vcpu->wq);
> >>> +#ifdef CONFIG_KVM_ASYNC_PF_SYNC
> >>> +	kvm_vcpu_wake_up(vcpu);
> >>> +#else
> >>> +	kvm_vcpu_kick(vcpu);
> >>> +#endif
> >>
> >> This will break s390, both functions are disabled for s390.
> >> On s390 do not want to kick the CPU for a completion. Instead we implement
> >> the kvm_async_page_present_sync call above and handle completion via an
> >> "pfault done" interrupt via the normal interrupt delivery.
> > 
> > Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390?
> > It was unused until now, but the patch makes sense as a cleanup.
> 
> 
> We could enable that. It was some kind of a trigger, that we get a build error
> when someone enables that for s390 (as it might not be want you want)
> 
> On the other hand,  I dont think that we need a wakeup at all for the
> SYNC case. (as the interrupt will do that anyway)
> Maybe something like that
> 
> -	if (swait_active(&vcpu->wq))
> -		swake_up(&vcpu->wq);
> +#ifndef CONFIG_KVM_ASYNC_PF_SYNC
> +	kvm_vcpu_kick(vcpu);
> +#endif
> 
> would be good enough. (needs more thinking to be sure)

As my knowlege of s390 approaches zero so that only your thinking can be
applied here ;) , how about me redoing this patch leaving the open-coded
wakeup as is, and then you submitting another patch on top dropping that
wakeup if you find that appropriate?

Thanks,
Roman.

Re: [PATCH 5/5] kvm/svm: kick L2 guest to L1 by ready async_pf

[Roman Kagan]

On Fri, Dec 02, 2016 at 04:35:24AM -0500, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Roman Kagan" <rkagan@virtuozzo.com>
> > To: "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org
> > Cc: "Denis Lunev" <den@virtuozzo.com>, "Roman Kagan" <rkagan@virtuozzo.com>
> > Sent: Friday, December 2, 2016 9:47:54 AM
> > Subject: [PATCH 5/5] kvm/svm: kick L2 guest to L1 by ready async_pf
> > 
> > When async pagefault is resolved vCPU may be executing L2 guest.
> > 
> > In order to allow L1 take better scheduling decisions in such cases,
> > make L2 exit to L1 on a fake external interupt, without actually
> > injecting it (unless L2 has other reasons to vmexit).
> > 
> > This patch does that for x86/AMD.
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > ---
> > Note 1: I didn't have a chance to test this; I'll do when I get access to an
> > AMD machine and let you know if anything goes wrong
> > 
> > Note 2: I mostly modelled this patch after the one for vmx but is looks not
> > very appealing for svm.  I'd appreciate being pointed at a better location
> > where to stick the fake external interrupt vmexit.
> 
> This should not be necessary, SVM has a different mechanism (which
> requires L1 cooperation) to handle async page faults.

Hmm, I didn't realize that...  I'm afraid we have a problem then: my
patch that triggered all this, "kvm/x86: skip async_pf when in guest
mode", makes async_pf's not delivered if the vcpu is executing L2 on
all x86, so that mechanism will stop working.

I'm also curious what L1 cooperation is assumed here.  E.g. would L1
running linux (which would set up async_pf) and a non-KVM hypervisor
cooperate as expected?  Looks like I need another dive into svm.c...

Roman.

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

Hi Roman,

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.9-rc7 next-20161202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Roman-Kagan/kvm-avoid-delaying-async_pf-ready-delivery/20161203-081205
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: s390-default_defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All errors (new ones prefixed by >>):

>> ERROR: "kvm_vcpu_wake_up" [arch/s390/kvm/kvm.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 16418 bytes --]

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[Christian Borntraeger]

On 12/02/2016 12:34 PM, Roman Kagan wrote:
> On Fri, Dec 02, 2016 at 11:00:00AM +0100, Christian Borntraeger wrote:
>> On 12/02/2016 10:40 AM, Paolo Bonzini wrote:
>>> ----- Original Message -----
>>>> From: "Christian Borntraeger" <borntraeger@de.ibm.com>
>>>> To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>,
>>>> kvm@vger.kernel.org
>>>> Cc: "Denis Lunev" <den@virtuozzo.com>
>>>> Sent: Friday, December 2, 2016 10:35:02 AM
>>>> Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved
>>>>
>>>> On 12/02/2016 09:47 AM, Roman Kagan wrote:
>>>>> When async_pf is ready the guest needs to be made aware of it ASAP,
>>>>> because it may be holding off a higher priority task pending the
>>>>> async_pf resolution in favor of a lower priority one.
>>>>>
>>>>> In case async_pf's are harvested in vcpu context (x86) we have to not
>>>>> only wake the vcpu up but kick it into host.
>>>>>
>>>>> While at this, also replace the open-coded vcpu wakeup by the existing
>>>>> helper.
>>>>>
>>>>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
>>>>> ---
>>>>>  virt/kvm/async_pf.c | 7 +++++--
>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
>>>>> index 9cced14..5f0a66c 100644
>>>>> --- a/virt/kvm/async_pf.c
>>>>> +++ b/virt/kvm/async_pf.c
>>>>> @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work)
>>>>>  	 * This memory barrier pairs with prepare_to_wait's set_current_state()
>>>>>  	 */
>>>>>  	smp_mb();
>>>>> -	if (swait_active(&vcpu->wq))
>>>>> -		swake_up(&vcpu->wq);
>>>>> +#ifdef CONFIG_KVM_ASYNC_PF_SYNC
>>>>> +	kvm_vcpu_wake_up(vcpu);
>>>>> +#else
>>>>> +	kvm_vcpu_kick(vcpu);
>>>>> +#endif
>>>>
>>>> This will break s390, both functions are disabled for s390.
>>>> On s390 do not want to kick the CPU for a completion. Instead we implement
>>>> the kvm_async_page_present_sync call above and handle completion via an
>>>> "pfault done" interrupt via the normal interrupt delivery.
>>>
>>> Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390?
>>> It was unused until now, but the patch makes sense as a cleanup.
>>
>>
>> We could enable that. It was some kind of a trigger, that we get a build error
>> when someone enables that for s390 (as it might not be want you want)
>>
>> On the other hand,  I dont think that we need a wakeup at all for the
>> SYNC case. (as the interrupt will do that anyway)
>> Maybe something like that
>>
>> -	if (swait_active(&vcpu->wq))
>> -		swake_up(&vcpu->wq);
>> +#ifndef CONFIG_KVM_ASYNC_PF_SYNC
>> +	kvm_vcpu_kick(vcpu);
>> +#endif
>>
>> would be good enough. (needs more thinking to be sure)
> 
> As my knowlege of s390 approaches zero so that only your thinking can be
> applied here ;) , how about me redoing this patch leaving the open-coded
> wakeup as is, and then you submitting another patch on top dropping that
> wakeup if you find that appropriate?

Yes, please do you patch with the swake_up and I will come up with a followup.

Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]

Hi Roman,

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.9-rc8 next-20161205]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Roman-Kagan/kvm-avoid-delaying-async_pf-ready-delivery/20161203-081205
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: s390-alldefconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All errors (new ones prefixed by >>):

   arch/s390/built-in.o: In function `async_pf_execute':
>> arch/s390/kvm/../../../virt/kvm/async_pf.o:(.text+0x26cee): undefined reference to `kvm_vcpu_wake_up'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6718 bytes --]

Re: [PATCH 5/5] kvm/svm: kick L2 guest to L1 by ready async_pf

[Roman Kagan]

On Fri, Dec 02, 2016 at 03:33:03PM +0300, Roman Kagan wrote:
> On Fri, Dec 02, 2016 at 04:35:24AM -0500, Paolo Bonzini wrote:
> > 
> > 
> > ----- Original Message -----
> > > From: "Roman Kagan" <rkagan@virtuozzo.com>
> > > To: "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org
> > > Cc: "Denis Lunev" <den@virtuozzo.com>, "Roman Kagan" <rkagan@virtuozzo.com>
> > > Sent: Friday, December 2, 2016 9:47:54 AM
> > > Subject: [PATCH 5/5] kvm/svm: kick L2 guest to L1 by ready async_pf
> > > 
> > > When async pagefault is resolved vCPU may be executing L2 guest.
> > > 
> > > In order to allow L1 take better scheduling decisions in such cases,
> > > make L2 exit to L1 on a fake external interupt, without actually
> > > injecting it (unless L2 has other reasons to vmexit).
> > > 
> > > This patch does that for x86/AMD.
> > > 
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > > ---
> > > Note 1: I didn't have a chance to test this; I'll do when I get access to an
> > > AMD machine and let you know if anything goes wrong
> > > 
> > > Note 2: I mostly modelled this patch after the one for vmx but is looks not
> > > very appealing for svm.  I'd appreciate being pointed at a better location
> > > where to stick the fake external interrupt vmexit.
> > 
> > This should not be necessary, SVM has a different mechanism (which
> > requires L1 cooperation) to handle async page faults.
> 
> Hmm, I didn't realize that...  I'm afraid we have a problem then: my
> patch that triggered all this, "kvm/x86: skip async_pf when in guest
> mode", makes async_pf's not delivered if the vcpu is executing L2 on
> all x86, so that mechanism will stop working.
> 
> I'm also curious what L1 cooperation is assumed here.  E.g. would L1
> running linux (which would set up async_pf) and a non-KVM hypervisor
> cooperate as expected?  Looks like I need another dive into svm.c...

Turns out at least VirtualBox triggers an assertion when a guest vmexits
on a #PF when NPT is on, see
https://www.virtualbox.org/browser/vbox/trunk/src/VBox/VMM/VMMR0/HMSVMR0.cpp#L5353
(BTW this is also what KVM does on Intel).

So I tend to think that in general what this patch did -- kicking L2
into L1 by an "external interrupt" with no valid interrupt info -- was
less likely to confuse L1.

I've now grabbed a suitable AMD machine and it at least passed a smoke
test with KVM on KVM.

Unfortunately I failed to run VirtualBox in a L1 guest (I haven't
figured out why) but looking at the code it should be ok with external
interrupt vmexit, too.

So unless there are better ideas I'll respin the series taking into
account the comments accumulated so far.

Roman.