[PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Aili Yao]

From: Aili Yao <yaoaili@kingsoft.com>

When we isolate some pyhiscal cores, We may not use them for kvm guests,
We may use them for other purposes like DPDK, or we can make some kvm
guests isolated and some not, the global judgement pi_inject_timer is
not enough; We may make wrong decisions:

In such a scenario, the guests without isolated cores will not be
permitted to use vmx preemption timer, and tscdeadline fastpath also be
disabled, both will lead to performance penalty.

So check whether the vcpu->cpu is isolated, if not, don't post timer
interrupt.

And when qemu enable -cpu-pm feature for guests, all the available
disable_exit will be set, including mwait,halt,pause,cstate, when
this operation succeed, hlt_in_guest,pause_in_guest,cstate_in_guest
will all be definitly set true with one special case, mwait_in_guest,
this feature's enablement is depended on the HOST cpu feature support;

When cpu-pm is successfully enabled, and hlt_in_guest is true and
mwait_in_guest is false, the guest cant't use Monitor/Mwait instruction
for idle operation, instead, the guest may use halt for that purpose, as
we have enable the cpu-pm feature and hlt_in_guest is true, we will also
minimize the guest exit; For such a scenario, Monitor/Mwait instruction
support is totally disabled, the guest has no way to use Mwait to exit from
non-root mode;

For cpu-pm feature, hlt_in_guest and others except mwait_in_guest will
be a good hint for it. So replace it with hlt_in_guest.

Signed-off-by: Aili Yao <yaoaili@kingsoft.com>
---
 arch/x86/kvm/lapic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 759952dd1222..42aef1accd6b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -34,6 +34,7 @@
 #include <asm/delay.h>
 #include <linux/atomic.h>
 #include <linux/jump_label.h>
+#include <linux/sched/isolation.h>
 #include "kvm_cache_regs.h"
 #include "irq.h"
 #include "ioapic.h"
@@ -113,13 +114,14 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
 
 static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
 {
-	return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
+	return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
+		!housekeeping_cpu(vcpu->cpu, HK_FLAG_TIMER);
 }
 
 bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
 {
 	return kvm_x86_ops.set_hv_timer
-	       && !(kvm_mwait_in_guest(vcpu->kvm) ||
+	       && !(kvm_hlt_in_guest(vcpu->kvm) ||
 		    kvm_can_post_timer_interrupt(vcpu));
 }
 EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
-- 
2.25.1

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Sean Christopherson]

On Wed, Nov 24, 2021, Aili Yao wrote:
> When cpu-pm is successfully enabled, and hlt_in_guest is true and
> mwait_in_guest is false, the guest cant't use Monitor/Mwait instruction
> for idle operation, instead, the guest may use halt for that purpose, as
> we have enable the cpu-pm feature and hlt_in_guest is true, we will also
> minimize the guest exit; For such a scenario, Monitor/Mwait instruction
> support is totally disabled, the guest has no way to use Mwait to exit from
> non-root mode;
> 
> For cpu-pm feature, hlt_in_guest and others except mwait_in_guest will
> be a good hint for it. So replace it with hlt_in_guest.

This should be a separate patch from the housekeeping_cpu() check, if we add
the housekeeping check.

> Signed-off-by: Aili Yao <yaoaili@kingsoft.com>
> ---
>  arch/x86/kvm/lapic.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 759952dd1222..42aef1accd6b 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -34,6 +34,7 @@
>  #include <asm/delay.h>
>  #include <linux/atomic.h>
>  #include <linux/jump_label.h>
> +#include <linux/sched/isolation.h>
>  #include "kvm_cache_regs.h"
>  #include "irq.h"
>  #include "ioapic.h"
> @@ -113,13 +114,14 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
>  
>  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
>  {
> -	return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> +	return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> +		!housekeeping_cpu(vcpu->cpu, HK_FLAG_TIMER);

Why not check kvm_{hlt,mwait}_in_guest()?  IIUC, non-housekeeping CPUs don't _have_
to be associated 1:1 with a vCPU, in which case posting the timer is unlikely
to be a performance win even though the target isn't a housekeeping CPU.

And wouldn't exposing HLT/MWAIT to a vCPU that's on a housekeeping CPU be a bogus
configuration?

>  }
>  
>  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
>  {
>  	return kvm_x86_ops.set_hv_timer
> -	       && !(kvm_mwait_in_guest(vcpu->kvm) ||
> +	       && !(kvm_hlt_in_guest(vcpu->kvm) ||

This is incorrect, the HLT vs. MWAIT isn't purely a posting interrupts thing.  The
VMX preemption timer counts down in C0, C1, and C2, but not deeper sleep states.
HLT is always C1, thus it's safe to use the VMX preemption timer even if the guest
can execute HLT without exiting.

The timer isn't compatible with MWAIT because it stops counting in C3 (or lower),
i.e. the guest can cause the timer to stop counting.

>  		    kvm_can_post_timer_interrupt(vcpu));
>  }
>  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> -- 

Splicing in Wanpeng's version to try and merge the two threads:

On Tue, Nov 23, 2021 at 10:00 PM Wanpeng Li <kernellwp@gmail.com> wrote:
> ---
>  arch/x86/kvm/lapic.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 759952dd1222..8257566d44c7 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -113,14 +113,13 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
>
>  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
>  {
> -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> +       return pi_inject_timer && kvm_mwait_in_guest(vcpu->kvm) && kvm_vcpu_apicv_active(vcpu);

As Aili's changelog pointed out, MWAIT may not be advertised to the guest. 

So I think we want this?  With a non-functional, opinionated refactoring of
kvm_can_use_hv_timer() because I'm terrible at reading !(a || b).

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 40270d7bc597..c77cb386d03d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -113,14 +113,25 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)

 static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
 {
-       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
+       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
+              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
 }

 bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
 {
-       return kvm_x86_ops.set_hv_timer
-              && !(kvm_mwait_in_guest(vcpu->kvm) ||
-                   kvm_can_post_timer_interrupt(vcpu));
+       /*
+        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
+        * guest can execute MWAIT without exiting as the timer will stop
+        * counting if the core enters C3 or lower.  HLT in the guest is ok as
+        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
+        *
+        * Don't use the hypervisor timer if KVM can post a timer interrupt to
+        * the guest since posted the timer avoids taking an extra a VM-Exit
+        * when the timer expires.
+        */
+       return kvm_x86_ops.set_hv_timer &&
+              !kvm_mwait_in_guest(vcpu->kvm) &&
+              !kvm_can_post_timer_interrupt(vcpu));
 }
 EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Aili Yao]

On Tue, 7 Dec 2021 23:23:03 +0000
Sean Christopherson <seanjc@google.com> wrote:

> On Wed, Nov 24, 2021, Aili Yao wrote:
> > When cpu-pm is successfully enabled, and hlt_in_guest is true and
> > mwait_in_guest is false, the guest cant't use Monitor/Mwait instruction
> > for idle operation, instead, the guest may use halt for that purpose, as
> > we have enable the cpu-pm feature and hlt_in_guest is true, we will also
> > minimize the guest exit; For such a scenario, Monitor/Mwait instruction
> > support is totally disabled, the guest has no way to use Mwait to exit from
> > non-root mode;
> > 
> > For cpu-pm feature, hlt_in_guest and others except mwait_in_guest will
> > be a good hint for it. So replace it with hlt_in_guest.  
> 
> This should be a separate patch from the housekeeping_cpu() check, if we add
> the housekeeping check.
> 
> > Signed-off-by: Aili Yao <yaoaili@kingsoft.com>
> > ---
> >  arch/x86/kvm/lapic.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 759952dd1222..42aef1accd6b 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -34,6 +34,7 @@
> >  #include <asm/delay.h>
> >  #include <linux/atomic.h>
> >  #include <linux/jump_label.h>
> > +#include <linux/sched/isolation.h>
> >  #include "kvm_cache_regs.h"
> >  #include "irq.h"
> >  #include "ioapic.h"
> > @@ -113,13 +114,14 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> >  
> >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> >  {
> > -	return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > +	return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> > +		!housekeeping_cpu(vcpu->cpu, HK_FLAG_TIMER);  
> 
> Why not check kvm_{hlt,mwait}_in_guest()?  IIUC, non-housekeeping CPUs don't _have_
> to be associated 1:1 with a vCPU, in which case posting the timer is unlikely
> to be a performance win even though the target isn't a housekeeping CPU.

Yes, non-housekeeping CPUs can be assigned to multi vCPUs, I don't think it's a common configuration;
But this can happen.

> And wouldn't exposing HLT/MWAIT to a vCPU that's on a housekeeping CPU be a bogus
> configuration?

Agree, it's a bogus configuration and not suppose to like this, but this can happen;

It seems we can't cover all the abnormal cases in a single line. So I think just checking for
most right configurations is needed.

> >  }
> >  
> >  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
> >  {
> >  	return kvm_x86_ops.set_hv_timer
> > -	       && !(kvm_mwait_in_guest(vcpu->kvm) ||
> > +	       && !(kvm_hlt_in_guest(vcpu->kvm) ||  
> 
> This is incorrect, the HLT vs. MWAIT isn't purely a posting interrupts thing.  The
> VMX preemption timer counts down in C0, C1, and C2, but not deeper sleep states.
> HLT is always C1, thus it's safe to use the VMX preemption timer even if the guest
> can execute HLT without exiting.
> The timer isn't compatible with MWAIT because it stops counting in C3 (or lower),
> i.e. the guest can cause the timer to stop counting.

Thanks for your pointer, now i know this.

> 
> >  		    kvm_can_post_timer_interrupt(vcpu));
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> > --   
> 
> Splicing in Wanpeng's version to try and merge the two threads:
> 
> On Tue, Nov 23, 2021 at 10:00 PM Wanpeng Li <kernellwp@gmail.com> wrote:
> > ---
> >  arch/x86/kvm/lapic.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 759952dd1222..8257566d44c7 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -113,14 +113,13 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> >
> >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> >  {
> > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > +       return pi_inject_timer && kvm_mwait_in_guest(vcpu->kvm) && kvm_vcpu_apicv_active(vcpu);  
> 
> As Aili's changelog pointed out, MWAIT may not be advertised to the guest. 
> 
> So I think we want this?  With a non-functional, opinionated refactoring of
> kvm_can_use_hv_timer() because I'm terrible at reading !(a || b).
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 40270d7bc597..c77cb386d03d 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -113,14 +113,25 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> 
>  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
>  {
> -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> +       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> +              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
>  }

I think only kvm_hlt_in_guest() check is enough here, as for current code, if kvm_mwait_in_guest() is true,
kvm_hlt_in_guest must be ture, if kvm_mwait_in_guest() is false, kvm_hlt_in_guest() could also
be true.

>  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
>  {
> -       return kvm_x86_ops.set_hv_timer
> -              && !(kvm_mwait_in_guest(vcpu->kvm) ||
> -                   kvm_can_post_timer_interrupt(vcpu));
> +       /*
> +        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
> +        * guest can execute MWAIT without exiting as the timer will stop
> +        * counting if the core enters C3 or lower.  HLT in the guest is ok as
> +        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
> +        *
> +        * Don't use the hypervisor timer if KVM can post a timer interrupt to
> +        * the guest since posted the timer avoids taking an extra a VM-Exit
> +        * when the timer expires.
> +        */
> +       return kvm_x86_ops.set_hv_timer &&
> +              !kvm_mwait_in_guest(vcpu->kvm) &&
> +              !kvm_can_post_timer_interrupt(vcpu));
>  }
>  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> 

I think this modification covers most used configurations and it's right.
Thanks!

--Aili Yao
 

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Aili Yao]

On Tue, 7 Dec 2021 23:23:03 +0000
Sean Christopherson <seanjc@google.com> wrote:

> 
>  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
>  {
> -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> +       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> +              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
>  }
> 
>  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
>  {
> -       return kvm_x86_ops.set_hv_timer
> -              && !(kvm_mwait_in_guest(vcpu->kvm) ||
> -                   kvm_can_post_timer_interrupt(vcpu));
> +       /*
> +        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
> +        * guest can execute MWAIT without exiting as the timer will stop
> +        * counting if the core enters C3 or lower.  HLT in the guest is ok as
> +        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
> +        *
> +        * Don't use the hypervisor timer if KVM can post a timer interrupt to
> +        * the guest since posted the timer avoids taking an extra a VM-Exit
> +        * when the timer expires.
> +        */
> +       return kvm_x86_ops.set_hv_timer &&
> +              !kvm_mwait_in_guest(vcpu->kvm) &&
> +              !kvm_can_post_timer_interrupt(vcpu));
>  }
>  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> 

Sorry, I am little confused here now:
if kvm_can_post_timer_interrupt(vcpu) return true(cpu-pm enabled), then the kvm_can_use_hv_timer will always be false;
if kvm_can_post_timer_interrupt(vcpu) return false(cpu-pm disable),then kvm_mwait_in_guest(vcpu->kvm) can't be true ether;
It seems we don't need kvm_mwait_in_guest(vcpu->kvm) here?

Sorry, I am just a little confused and not too sure about this, if anything wrong, just ignore it.

Thanks!

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Sean Christopherson]

On Wed, Dec 08, 2021, Aili Yao wrote:
> On Tue, 7 Dec 2021 23:23:03 +0000
> Sean Christopherson <seanjc@google.com> wrote:
> 
> > 
> >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> >  {
> > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > +       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> > +              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
> >  }
> > 
> >  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
> >  {
> > -       return kvm_x86_ops.set_hv_timer
> > -              && !(kvm_mwait_in_guest(vcpu->kvm) ||
> > -                   kvm_can_post_timer_interrupt(vcpu));
> > +       /*
> > +        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
> > +        * guest can execute MWAIT without exiting as the timer will stop
> > +        * counting if the core enters C3 or lower.  HLT in the guest is ok as
> > +        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
> > +        *
> > +        * Don't use the hypervisor timer if KVM can post a timer interrupt to
> > +        * the guest since posted the timer avoids taking an extra a VM-Exit
> > +        * when the timer expires.
> > +        */
> > +       return kvm_x86_ops.set_hv_timer &&
> > +              !kvm_mwait_in_guest(vcpu->kvm) &&
> > +              !kvm_can_post_timer_interrupt(vcpu));
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> > 
> 
> Sorry, I am little confused here now:
> if kvm_can_post_timer_interrupt(vcpu) return true(cpu-pm enabled), then the kvm_can_use_hv_timer will always be false;
> if kvm_can_post_timer_interrupt(vcpu) return false(cpu-pm disable),then kvm_mwait_in_guest(vcpu->kvm) can't be true ether;
> It seems we don't need kvm_mwait_in_guest(vcpu->kvm) here?

We do, it's to prevent the guest from enter C3+ and stopping the VMX preemption
timer, e.g. if either kvm_vcpu_apicv_active() or pi_inject_timer evaluates false.

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Aili Yao]

On Thu, 9 Dec 2021 18:20:57 +0000
Sean Christopherson <seanjc@google.com> wrote:

> On Wed, Dec 08, 2021, Aili Yao wrote:
> > On Tue, 7 Dec 2021 23:23:03 +0000
> > Sean Christopherson <seanjc@google.com> wrote:
> >   
> > > 
> > >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> > >  {
> > > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > > +       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> > > +              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
> > >  }
> > > 
> > >  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
> > >  {
> > > -       return kvm_x86_ops.set_hv_timer
> > > -              && !(kvm_mwait_in_guest(vcpu->kvm) ||
> > > -                   kvm_can_post_timer_interrupt(vcpu));
> > > +       /*
> > > +        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
> > > +        * guest can execute MWAIT without exiting as the timer will stop
> > > +        * counting if the core enters C3 or lower.  HLT in the guest is ok as
> > > +        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
> > > +        *
> > > +        * Don't use the hypervisor timer if KVM can post a timer interrupt to
> > > +        * the guest since posted the timer avoids taking an extra a VM-Exit
> > > +        * when the timer expires.
> > > +        */
> > > +       return kvm_x86_ops.set_hv_timer &&
> > > +              !kvm_mwait_in_guest(vcpu->kvm) &&
> > > +              !kvm_can_post_timer_interrupt(vcpu));
> > >  }
> > >  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> > >   
> > 
> > Sorry, I am little confused here now:
> > if kvm_can_post_timer_interrupt(vcpu) return true(cpu-pm enabled), then the kvm_can_use_hv_timer will always be false;
> > if kvm_can_post_timer_interrupt(vcpu) return false(cpu-pm disable),then kvm_mwait_in_guest(vcpu->kvm) can't be true ether;
> > It seems we don't need kvm_mwait_in_guest(vcpu->kvm) here?  
> 
> We do, it's to prevent the guest from enter C3+ and stopping the VMX preemption
> timer, e.g. if either kvm_vcpu_apicv_active() or pi_inject_timer evaluates false.

Great thanks for your explanation!
Now i am clear!

--Aili Yao

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Aili Yao]

On Tue, 7 Dec 2021 23:23:03 +0000
Sean Christopherson <seanjc@google.com> wrote:
> On Tue, Nov 23, 2021 at 10:00 PM Wanpeng Li <kernellwp@gmail.com> wrote:
> > ---
> >  arch/x86/kvm/lapic.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 759952dd1222..8257566d44c7 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -113,14 +113,13 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> >
> >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> >  {
> > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > +       return pi_inject_timer && kvm_mwait_in_guest(vcpu->kvm) && kvm_vcpu_apicv_active(vcpu);  
> 
> As Aili's changelog pointed out, MWAIT may not be advertised to the guest. 
> 
> So I think we want this?  With a non-functional, opinionated refactoring of
> kvm_can_use_hv_timer() because I'm terrible at reading !(a || b).
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 40270d7bc597..c77cb386d03d 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -113,14 +113,25 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> 
>  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
>  {
> -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> +       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> +              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
>  }
> 
>  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
>  {
> -       return kvm_x86_ops.set_hv_timer
> -              && !(kvm_mwait_in_guest(vcpu->kvm) ||
> -                   kvm_can_post_timer_interrupt(vcpu));
> +       /*
> +        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
> +        * guest can execute MWAIT without exiting as the timer will stop
> +        * counting if the core enters C3 or lower.  HLT in the guest is ok as
> +        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
> +        *
> +        * Don't use the hypervisor timer if KVM can post a timer interrupt to
> +        * the guest since posted the timer avoids taking an extra a VM-Exit
> +        * when the timer expires.
> +        */
> +       return kvm_x86_ops.set_hv_timer &&
> +              !kvm_mwait_in_guest(vcpu->kvm) &&
> +              !kvm_can_post_timer_interrupt(vcpu));
>  }
>  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> 

It seems Sean and Wanpeng are busy with some other more important issues;
So Please let me try to merge Sean, Wanpeng's ideas and suggestions together,also including my opinions
into one possible approach and get it reviewed, Only if others are OK with this;

I will post a new patch for this later today or tomorrow.

Thanks!

--Aili Yao

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Sean Christopherson]

On Thu, Dec 16, 2021, Aili Yao wrote:
> On Tue, 7 Dec 2021 23:23:03 +0000
> Sean Christopherson <seanjc@google.com> wrote:
> > On Tue, Nov 23, 2021 at 10:00 PM Wanpeng Li <kernellwp@gmail.com> wrote:
> > > ---
> > >  arch/x86/kvm/lapic.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 759952dd1222..8257566d44c7 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -113,14 +113,13 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> > >
> > >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> > >  {
> > > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > > +       return pi_inject_timer && kvm_mwait_in_guest(vcpu->kvm) && kvm_vcpu_apicv_active(vcpu);  
> > 
> > As Aili's changelog pointed out, MWAIT may not be advertised to the guest. 
> > 
> > So I think we want this?  With a non-functional, opinionated refactoring of
> > kvm_can_use_hv_timer() because I'm terrible at reading !(a || b).
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 40270d7bc597..c77cb386d03d 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -113,14 +113,25 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> > 
> >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> >  {
> > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > +       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> > +              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
> >  }
> > 
> >  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
> >  {
> > -       return kvm_x86_ops.set_hv_timer
> > -              && !(kvm_mwait_in_guest(vcpu->kvm) ||
> > -                   kvm_can_post_timer_interrupt(vcpu));
> > +       /*
> > +        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
> > +        * guest can execute MWAIT without exiting as the timer will stop
> > +        * counting if the core enters C3 or lower.  HLT in the guest is ok as
> > +        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
> > +        *
> > +        * Don't use the hypervisor timer if KVM can post a timer interrupt to
> > +        * the guest since posted the timer avoids taking an extra a VM-Exit
> > +        * when the timer expires.
> > +        */
> > +       return kvm_x86_ops.set_hv_timer &&
> > +              !kvm_mwait_in_guest(vcpu->kvm) &&
> > +              !kvm_can_post_timer_interrupt(vcpu));
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> > 
> 
> It seems Sean and Wanpeng are busy with some other more important issues;
> So Please let me try to merge Sean, Wanpeng's ideas and suggestions together,also including my opinions
> into one possible approach and get it reviewed, Only if others are OK with this;
> 
> I will post a new patch for this later today or tomorrow.

Sorry, I was waiting for someone to say "this works", but never actually said as
much.

Does the above change address your use case?  If not, what's missing?

Re: [PATCH v2] KVM: LAPIC: Per vCPU control over kvm_can_post_timer_interrupt

[Aili Yao]

On Thu, 16 Dec 2021 15:45:57 +0000
Sean Christopherson <seanjc@google.com> wrote:

> On Thu, Dec 16, 2021, Aili Yao wrote:
> > On Tue, 7 Dec 2021 23:23:03 +0000
> > Sean Christopherson <seanjc@google.com> wrote:  
> > > On Tue, Nov 23, 2021 at 10:00 PM Wanpeng Li <kernellwp@gmail.com> wrote:  
> > > > ---
> > > >  arch/x86/kvm/lapic.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > index 759952dd1222..8257566d44c7 100644
> > > > --- a/arch/x86/kvm/lapic.c
> > > > +++ b/arch/x86/kvm/lapic.c
> > > > @@ -113,14 +113,13 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> > > >
> > > >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> > > >  {
> > > > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > > > +       return pi_inject_timer && kvm_mwait_in_guest(vcpu->kvm) && kvm_vcpu_apicv_active(vcpu);    
> > > 
> > > As Aili's changelog pointed out, MWAIT may not be advertised to the guest. 
> > > 
> > > So I think we want this?  With a non-functional, opinionated refactoring of
> > > kvm_can_use_hv_timer() because I'm terrible at reading !(a || b).
> > > 
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 40270d7bc597..c77cb386d03d 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -113,14 +113,25 @@ static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> > > 
> > >  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> > >  {
> > > -       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> > > +       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
> > > +              (kvm_mwait_in_guest(vcpu) || kvm_hlt_in_guest(vcpu));
> > >  }
> > > 
> > >  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
> > >  {
> > > -       return kvm_x86_ops.set_hv_timer
> > > -              && !(kvm_mwait_in_guest(vcpu->kvm) ||
> > > -                   kvm_can_post_timer_interrupt(vcpu));
> > > +       /*
> > > +        * Don't use the hypervisor timer, a.k.a. VMX Preemption Timer, if the
> > > +        * guest can execute MWAIT without exiting as the timer will stop
> > > +        * counting if the core enters C3 or lower.  HLT in the guest is ok as
> > > +        * HLT is effectively C1 and the timer counts in C0, C1, and C2.
> > > +        *
> > > +        * Don't use the hypervisor timer if KVM can post a timer interrupt to
> > > +        * the guest since posted the timer avoids taking an extra a VM-Exit
> > > +        * when the timer expires.
> > > +        */
> > > +       return kvm_x86_ops.set_hv_timer &&
> > > +              !kvm_mwait_in_guest(vcpu->kvm) &&
> > > +              !kvm_can_post_timer_interrupt(vcpu));
> > >  }
> > >  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);
> > >   
> > 
> > It seems Sean and Wanpeng are busy with some other more important issues;
> > So Please let me try to merge Sean, Wanpeng's ideas and suggestions together,also including my opinions
> > into one possible approach and get it reviewed, Only if others are OK with this;
> > 
> > I will post a new patch for this later today or tomorrow.  
> 
> Sorry, I was waiting for someone to say "this works", but never actually said as
> much.
> 
> Does the above change address your use case?  If not, what's missing?

After a little modifications, This works in my test.

static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
{
-       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
+       return pi_inject_timer && kvm_vcpu_apicv_active(vcpu) &&
+              (kvm_mwait_in_guest(vcpu->kvm) || kvm_hlt_in_guest(vcpu->kvm));
 }

and also you can delete or keep kvm_mwait_in_guest() check;

Thanks!