kvm: better MWAIT emulation for guests
diff mbox

Message ID 1489098555-23856-1-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin March 9, 2017, 10:29 p.m. UTC
Some guests call mwait without checking the cpu flags.  We currently
emulate that as a NOP but on VMX we can do better: let guest stop the
CPU until timer or IPI.  CPU will be busy but that isn't any worse than
a NOP emulation.

Note that mwait within guests is not the same as on real hardware
because you must halt if you want to go deep into sleep.  Thus it isn't
a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
in the hypervisor leaf instead.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Documentation/virtual/kvm/cpuid.txt  | 3 +++
 arch/x86/include/uapi/asm/kvm_para.h | 1 +
 arch/x86/kvm/cpuid.c                 | 3 +++
 arch/x86/kvm/vmx.c                   | 4 ----
 4 files changed, 7 insertions(+), 4 deletions(-)

Comments

Gabriel L. Somlo March 10, 2017, 12:51 a.m. UTC | #1
On Fri, Mar 10, 2017 at 12:29:31AM +0200, Michael S. Tsirkin wrote:
> Some guests call mwait without checking the cpu flags.  We currently
> emulate that as a NOP but on VMX we can do better: let guest stop the
> CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> a NOP emulation.

Are you getting an IPI if another VCPU writes to the MONITOR-ed memory
location? If not, you'd be waking up too late and fail to meet the
specified behavior of the MONITOR/MWAIT instruction pair.

> Note that mwait within guests is not the same as on real hardware
> because you must halt if you want to go deep into sleep.  Thus it isn't
> a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> in the hypervisor leaf instead.

Is it a good idea to advertise MWAIT capability to guests? The
misbehaving ones will call it willy-nilly, true, but aren't compliant
ones better off falling back to some alternative method (typically
using a HLT-based idle loop instead of a MONITOR/MWAIT based one) ?

Thanks,
--Gabriel

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  Documentation/virtual/kvm/cpuid.txt  | 3 +++
>  arch/x86/include/uapi/asm/kvm_para.h | 1 +
>  arch/x86/kvm/cpuid.c                 | 3 +++
>  arch/x86/kvm/vmx.c                   | 4 ----
>  4 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/cpuid.txt b/Documentation/virtual/kvm/cpuid.txt
> index 3c65feb..5caa234 100644
> --- a/Documentation/virtual/kvm/cpuid.txt
> +++ b/Documentation/virtual/kvm/cpuid.txt
> @@ -54,6 +54,9 @@ KVM_FEATURE_PV_UNHALT              ||     7 || guest checks this feature bit
>                                     ||       || before enabling paravirtualized
>                                     ||       || spinlock support.
>  ------------------------------------------------------------------------------
> +KVM_FEATURE_MWAIT                  ||     8 || guest can use monitor/mwait
> +                                   ||       || to halt the VCPU.
> +------------------------------------------------------------------------------
>  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||    24 || host will warn if no guest-side
>                                     ||       || per-cpu warps are expected in
>                                     ||       || kvmclock.
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> index cff0bb6..9cc77a7 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -24,6 +24,7 @@
>  #define KVM_FEATURE_STEAL_TIME		5
>  #define KVM_FEATURE_PV_EOI		6
>  #define KVM_FEATURE_PV_UNHALT		7
> +#define KVM_FEATURE_MWAIT		8
>  
>  /* The last 8 bits are used to indicate how to interpret the flags field
>   * in pvclock structure. If no bits are set, all flags are ignored.
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index efde6cc..fe3d292 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		if (sched_info_on())
>  			entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
>  
> +		if (this_cpu_has(X86_FEATURE_MWAIT))
> +			entry->eax = (1 << KVM_FEATURE_MWAIT);
> +
>  		entry->ebx = 0;
>  		entry->ecx = 0;
>  		entry->edx = 0;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4bfe349..b167aba 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3547,13 +3547,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>  	      CPU_BASED_USE_IO_BITMAPS |
>  	      CPU_BASED_MOV_DR_EXITING |
>  	      CPU_BASED_USE_TSC_OFFSETING |
> -	      CPU_BASED_MWAIT_EXITING |
> -	      CPU_BASED_MONITOR_EXITING |
>  	      CPU_BASED_INVLPG_EXITING |
>  	      CPU_BASED_RDPMC_EXITING;
>  
> -	printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
> -
>  	opt = CPU_BASED_TPR_SHADOW |
>  	      CPU_BASED_USE_MSR_BITMAPS |
>  	      CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> -- 
> MST
Michael S. Tsirkin March 10, 2017, 1:12 a.m. UTC | #2
On Thu, Mar 09, 2017 at 07:51:27PM -0500, Gabriel L. Somlo wrote:
> On Fri, Mar 10, 2017 at 12:29:31AM +0200, Michael S. Tsirkin wrote:
> > Some guests call mwait without checking the cpu flags.  We currently
> > emulate that as a NOP but on VMX we can do better: let guest stop the
> > CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> > a NOP emulation.
> 
> Are you getting an IPI if another VCPU writes to the MONITOR-ed memory
> location?

In my testing yes.

> If not, you'd be waking up too late and fail to meet the
> specified behavior of the MONITOR/MWAIT instruction pair.
> 
> > Note that mwait within guests is not the same as on real hardware
> > because you must halt if you want to go deep into sleep.  Thus it isn't
> > a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> > in the hypervisor leaf instead.
> 
> Is it a good idea to advertise MWAIT capability to guests?

I think it isn't so this patch does not do it.

> The
> misbehaving ones will call it willy-nilly, true, but aren't compliant
> ones better off falling back to some alternative method (typically
> using a HLT-based idle loop instead of a MONITOR/MWAIT based one) ?
> 
> Thanks,
> --Gabriel
> 
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  Documentation/virtual/kvm/cpuid.txt  | 3 +++
> >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> >  arch/x86/kvm/cpuid.c                 | 3 +++
> >  arch/x86/kvm/vmx.c                   | 4 ----
> >  4 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/cpuid.txt b/Documentation/virtual/kvm/cpuid.txt
> > index 3c65feb..5caa234 100644
> > --- a/Documentation/virtual/kvm/cpuid.txt
> > +++ b/Documentation/virtual/kvm/cpuid.txt
> > @@ -54,6 +54,9 @@ KVM_FEATURE_PV_UNHALT              ||     7 || guest checks this feature bit
> >                                     ||       || before enabling paravirtualized
> >                                     ||       || spinlock support.
> >  ------------------------------------------------------------------------------
> > +KVM_FEATURE_MWAIT                  ||     8 || guest can use monitor/mwait
> > +                                   ||       || to halt the VCPU.
> > +------------------------------------------------------------------------------
> >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||    24 || host will warn if no guest-side
> >                                     ||       || per-cpu warps are expected in
> >                                     ||       || kvmclock.
> > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > index cff0bb6..9cc77a7 100644
> > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > @@ -24,6 +24,7 @@
> >  #define KVM_FEATURE_STEAL_TIME		5
> >  #define KVM_FEATURE_PV_EOI		6
> >  #define KVM_FEATURE_PV_UNHALT		7
> > +#define KVM_FEATURE_MWAIT		8
> >  
> >  /* The last 8 bits are used to indicate how to interpret the flags field
> >   * in pvclock structure. If no bits are set, all flags are ignored.
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index efde6cc..fe3d292 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >  		if (sched_info_on())
> >  			entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
> >  
> > +		if (this_cpu_has(X86_FEATURE_MWAIT))
> > +			entry->eax = (1 << KVM_FEATURE_MWAIT);
> > +
> >  		entry->ebx = 0;
> >  		entry->ecx = 0;
> >  		entry->edx = 0;
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 4bfe349..b167aba 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -3547,13 +3547,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
> >  	      CPU_BASED_USE_IO_BITMAPS |
> >  	      CPU_BASED_MOV_DR_EXITING |
> >  	      CPU_BASED_USE_TSC_OFFSETING |
> > -	      CPU_BASED_MWAIT_EXITING |
> > -	      CPU_BASED_MONITOR_EXITING |
> >  	      CPU_BASED_INVLPG_EXITING |
> >  	      CPU_BASED_RDPMC_EXITING;
> >  
> > -	printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
> > -
> >  	opt = CPU_BASED_TPR_SHADOW |
> >  	      CPU_BASED_USE_MSR_BITMAPS |
> >  	      CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> > -- 
> > MST
Jim Mattson March 10, 2017, 11:46 p.m. UTC | #3
On Thu, Mar 9, 2017 at 2:29 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Some guests call mwait without checking the cpu flags.  We currently

"Some guests"? What guests other than Mac OS X are so ill-behaved?

> emulate that as a NOP but on VMX we can do better: let guest stop the
> CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> a NOP emulation.
>
> Note that mwait within guests is not the same as on real hardware
> because you must halt if you want to go deep into sleep.  Thus it isn't
> a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> in the hypervisor leaf instead.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  Documentation/virtual/kvm/cpuid.txt  | 3 +++
>  arch/x86/include/uapi/asm/kvm_para.h | 1 +
>  arch/x86/kvm/cpuid.c                 | 3 +++
>  arch/x86/kvm/vmx.c                   | 4 ----
>  4 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/cpuid.txt b/Documentation/virtual/kvm/cpuid.txt
> index 3c65feb..5caa234 100644
> --- a/Documentation/virtual/kvm/cpuid.txt
> +++ b/Documentation/virtual/kvm/cpuid.txt
> @@ -54,6 +54,9 @@ KVM_FEATURE_PV_UNHALT              ||     7 || guest checks this feature bit
>                                     ||       || before enabling paravirtualized
>                                     ||       || spinlock support.
>  ------------------------------------------------------------------------------
> +KVM_FEATURE_MWAIT                  ||     8 || guest can use monitor/mwait
> +                                   ||       || to halt the VCPU.
> +------------------------------------------------------------------------------
>  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||    24 || host will warn if no guest-side
>                                     ||       || per-cpu warps are expected in
>                                     ||       || kvmclock.
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> index cff0bb6..9cc77a7 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -24,6 +24,7 @@
>  #define KVM_FEATURE_STEAL_TIME         5
>  #define KVM_FEATURE_PV_EOI             6
>  #define KVM_FEATURE_PV_UNHALT          7
> +#define KVM_FEATURE_MWAIT              8
>
>  /* The last 8 bits are used to indicate how to interpret the flags field
>   * in pvclock structure. If no bits are set, all flags are ignored.
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index efde6cc..fe3d292 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>                 if (sched_info_on())
>                         entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
>
> +               if (this_cpu_has(X86_FEATURE_MWAIT))
> +                       entry->eax = (1 << KVM_FEATURE_MWAIT);
> +
>                 entry->ebx = 0;
>                 entry->ecx = 0;
>                 entry->edx = 0;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4bfe349..b167aba 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3547,13 +3547,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>               CPU_BASED_USE_IO_BITMAPS |
>               CPU_BASED_MOV_DR_EXITING |
>               CPU_BASED_USE_TSC_OFFSETING |
> -             CPU_BASED_MWAIT_EXITING |
> -             CPU_BASED_MONITOR_EXITING |
>               CPU_BASED_INVLPG_EXITING |
>               CPU_BASED_RDPMC_EXITING;
>
> -       printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
> -
>         opt = CPU_BASED_TPR_SHADOW |
>               CPU_BASED_USE_MSR_BITMAPS |
>               CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> --
> MST
Michael S. Tsirkin March 12, 2017, 12:01 a.m. UTC | #4
On Fri, Mar 10, 2017 at 03:46:45PM -0800, Jim Mattson wrote:
> On Thu, Mar 9, 2017 at 2:29 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Some guests call mwait without checking the cpu flags.  We currently
> 
> "Some guests"? What guests other than Mac OS X are so ill-behaved?

I heard about Mac OSX only but even that is hearsay for me
so I didn't want to say that explicitly.

> > emulate that as a NOP but on VMX we can do better: let guest stop the
> > CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> > a NOP emulation.
> >
> > Note that mwait within guests is not the same as on real hardware
> > because you must halt if you want to go deep into sleep.  Thus it isn't
> > a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> > in the hypervisor leaf instead.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  Documentation/virtual/kvm/cpuid.txt  | 3 +++
> >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> >  arch/x86/kvm/cpuid.c                 | 3 +++
> >  arch/x86/kvm/vmx.c                   | 4 ----
> >  4 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/virtual/kvm/cpuid.txt b/Documentation/virtual/kvm/cpuid.txt
> > index 3c65feb..5caa234 100644
> > --- a/Documentation/virtual/kvm/cpuid.txt
> > +++ b/Documentation/virtual/kvm/cpuid.txt
> > @@ -54,6 +54,9 @@ KVM_FEATURE_PV_UNHALT              ||     7 || guest checks this feature bit
> >                                     ||       || before enabling paravirtualized
> >                                     ||       || spinlock support.
> >  ------------------------------------------------------------------------------
> > +KVM_FEATURE_MWAIT                  ||     8 || guest can use monitor/mwait
> > +                                   ||       || to halt the VCPU.
> > +------------------------------------------------------------------------------
> >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||    24 || host will warn if no guest-side
> >                                     ||       || per-cpu warps are expected in
> >                                     ||       || kvmclock.
> > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > index cff0bb6..9cc77a7 100644
> > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > @@ -24,6 +24,7 @@
> >  #define KVM_FEATURE_STEAL_TIME         5
> >  #define KVM_FEATURE_PV_EOI             6
> >  #define KVM_FEATURE_PV_UNHALT          7
> > +#define KVM_FEATURE_MWAIT              8
> >
> >  /* The last 8 bits are used to indicate how to interpret the flags field
> >   * in pvclock structure. If no bits are set, all flags are ignored.
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index efde6cc..fe3d292 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >                 if (sched_info_on())
> >                         entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
> >
> > +               if (this_cpu_has(X86_FEATURE_MWAIT))
> > +                       entry->eax = (1 << KVM_FEATURE_MWAIT);
> > +
> >                 entry->ebx = 0;
> >                 entry->ecx = 0;
> >                 entry->edx = 0;
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 4bfe349..b167aba 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -3547,13 +3547,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
> >               CPU_BASED_USE_IO_BITMAPS |
> >               CPU_BASED_MOV_DR_EXITING |
> >               CPU_BASED_USE_TSC_OFFSETING |
> > -             CPU_BASED_MWAIT_EXITING |
> > -             CPU_BASED_MONITOR_EXITING |
> >               CPU_BASED_INVLPG_EXITING |
> >               CPU_BASED_RDPMC_EXITING;
> >
> > -       printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
> > -
> >         opt = CPU_BASED_TPR_SHADOW |
> >               CPU_BASED_USE_MSR_BITMAPS |
> >               CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> > --
> > MST
Gabriel L. Somlo March 12, 2017, 9:18 p.m. UTC | #5
On Sun, Mar 12, 2017 at 02:01:32AM +0200, Michael S. Tsirkin wrote:
> On Fri, Mar 10, 2017 at 03:46:45PM -0800, Jim Mattson wrote:
> > On Thu, Mar 9, 2017 at 2:29 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > Some guests call mwait without checking the cpu flags.  We currently
> > 
> > "Some guests"? What guests other than Mac OS X are so ill-behaved?
> 
> I heard about Mac OSX only but even that is hearsay for me
> so I didn't want to say that explicitly.

As the likely origin of said hearsay, I can confirm that Mac OS 5, 6,
and 7 (Leopard through Lion) had that problem: unless explicitly
provided with kernel command line argument "idlehalt=0" they'd
implicitly assume MONITOR and MWAIT availability, without checking CPUID.

As of MountainLion (10.8.x), that is no longer the case, and OS X
gracefully falls back to an idle loop which doesn't use MONITOR/MWAIT.

Regards,
--Gabriel
 
> > > emulate that as a NOP but on VMX we can do better: let guest stop the
> > > CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> > > a NOP emulation.
> > >
> > > Note that mwait within guests is not the same as on real hardware
> > > because you must halt if you want to go deep into sleep.  Thus it isn't
> > > a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> > > in the hypervisor leaf instead.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  Documentation/virtual/kvm/cpuid.txt  | 3 +++
> > >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> > >  arch/x86/kvm/cpuid.c                 | 3 +++
> > >  arch/x86/kvm/vmx.c                   | 4 ----
> > >  4 files changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/virtual/kvm/cpuid.txt b/Documentation/virtual/kvm/cpuid.txt
> > > index 3c65feb..5caa234 100644
> > > --- a/Documentation/virtual/kvm/cpuid.txt
> > > +++ b/Documentation/virtual/kvm/cpuid.txt
> > > @@ -54,6 +54,9 @@ KVM_FEATURE_PV_UNHALT              ||     7 || guest checks this feature bit
> > >                                     ||       || before enabling paravirtualized
> > >                                     ||       || spinlock support.
> > >  ------------------------------------------------------------------------------
> > > +KVM_FEATURE_MWAIT                  ||     8 || guest can use monitor/mwait
> > > +                                   ||       || to halt the VCPU.
> > > +------------------------------------------------------------------------------
> > >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||    24 || host will warn if no guest-side
> > >                                     ||       || per-cpu warps are expected in
> > >                                     ||       || kvmclock.
> > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > index cff0bb6..9cc77a7 100644
> > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > @@ -24,6 +24,7 @@
> > >  #define KVM_FEATURE_STEAL_TIME         5
> > >  #define KVM_FEATURE_PV_EOI             6
> > >  #define KVM_FEATURE_PV_UNHALT          7
> > > +#define KVM_FEATURE_MWAIT              8
> > >
> > >  /* The last 8 bits are used to indicate how to interpret the flags field
> > >   * in pvclock structure. If no bits are set, all flags are ignored.
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index efde6cc..fe3d292 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> > >                 if (sched_info_on())
> > >                         entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
> > >
> > > +               if (this_cpu_has(X86_FEATURE_MWAIT))
> > > +                       entry->eax = (1 << KVM_FEATURE_MWAIT);
> > > +
> > >                 entry->ebx = 0;
> > >                 entry->ecx = 0;
> > >                 entry->edx = 0;
> > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > index 4bfe349..b167aba 100644
> > > --- a/arch/x86/kvm/vmx.c
> > > +++ b/arch/x86/kvm/vmx.c
> > > @@ -3547,13 +3547,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
> > >               CPU_BASED_USE_IO_BITMAPS |
> > >               CPU_BASED_MOV_DR_EXITING |
> > >               CPU_BASED_USE_TSC_OFFSETING |
> > > -             CPU_BASED_MWAIT_EXITING |
> > > -             CPU_BASED_MONITOR_EXITING |
> > >               CPU_BASED_INVLPG_EXITING |
> > >               CPU_BASED_RDPMC_EXITING;
> > >
> > > -       printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
> > > -
> > >         opt = CPU_BASED_TPR_SHADOW |
> > >               CPU_BASED_USE_MSR_BITMAPS |
> > >               CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> > > --
> > > MST
Wanpeng Li March 13, 2017, 7:44 a.m. UTC | #6
Cc Peterz,
2017-03-10 9:12 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>:
> On Thu, Mar 09, 2017 at 07:51:27PM -0500, Gabriel L. Somlo wrote:
>> On Fri, Mar 10, 2017 at 12:29:31AM +0200, Michael S. Tsirkin wrote:
>> > Some guests call mwait without checking the cpu flags.  We currently
>> > emulate that as a NOP but on VMX we can do better: let guest stop the
>> > CPU until timer or IPI.  CPU will be busy but that isn't any worse than
>> > a NOP emulation.
>>
>> Are you getting an IPI if another VCPU writes to the MONITOR-ed memory
>> location?
>
> In my testing yes.

Why there is still an IPI if monitor/mwait is used in guest?

>
>> If not, you'd be waking up too late and fail to meet the
>> specified behavior of the MONITOR/MWAIT instruction pair.
>>
>> > Note that mwait within guests is not the same as on real hardware
>> > because you must halt if you want to go deep into sleep.  Thus it isn't
>> > a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
>> > in the hypervisor leaf instead.
>>
>> Is it a good idea to advertise MWAIT capability to guests?
>
> I think it isn't so this patch does not do it.
>
>> The
>> misbehaving ones will call it willy-nilly, true, but aren't compliant
>> ones better off falling back to some alternative method (typically
>> using a HLT-based idle loop instead of a MONITOR/MWAIT based one) ?
>>
>> Thanks,
>> --Gabriel
>>

[...]

>> > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>> >             if (sched_info_on())
>> >                     entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
>> >
>> > +           if (this_cpu_has(X86_FEATURE_MWAIT))
>> > +                   entry->eax = (1 << KVM_FEATURE_MWAIT);

s/"="/"|=", otherwise you almost kill other features.

Regards,
Wanpeng Li
Radim Krčmář March 13, 2017, 3:46 p.m. UTC | #7
2017-03-10 00:29+0200, Michael S. Tsirkin:
> Some guests call mwait without checking the cpu flags.  We currently
> emulate that as a NOP but on VMX we can do better: let guest stop the
> CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> a NOP emulation.
> 
> Note that mwait within guests is not the same as on real hardware
> because you must halt if you want to go deep into sleep.

SDM (25.3 CHANGES TO INSTRUCTION BEHAVIOR IN VMX NON-ROOT OPERATION)
says that "MWAIT operates normally".  What is the reason why MWAIT
inside VMX cannot reach the same states as MWAIT outside VMX?

>                                                           Thus it isn't
> a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> in the hypervisor leaf instead.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
  [...]
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> +		if (this_cpu_has(X86_FEATURE_MWAIT))
> +			entry->eax = (1 << KVM_FEATURE_MWAIT);

I'd rather not add it as a paravirt feature:

 - MWAIT requires the software to provide a target state, but we're not
   doing anything to expose those states.
   The feature would need very constrained setup, which is hard to
   support

 - we've had requests to support MWAIT emulation for Linux and fully
   emulating MWAIT would be best.
   MWAIT is not going to enabled by default, of course; it would be
   targeted at LPAR-like uses of KVM.

What about keeping just the last hunk to improve OS X, for now?

Thanks.

> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -3547,13 +3547,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>  	      CPU_BASED_USE_IO_BITMAPS |
>  	      CPU_BASED_MOV_DR_EXITING |
>  	      CPU_BASED_USE_TSC_OFFSETING |
> -	      CPU_BASED_MWAIT_EXITING |
> -	      CPU_BASED_MONITOR_EXITING |
>  	      CPU_BASED_INVLPG_EXITING |
>  	      CPU_BASED_RDPMC_EXITING;
>  
> -	printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
> -
>  	opt = CPU_BASED_TPR_SHADOW |
>  	      CPU_BASED_USE_MSR_BITMAPS |
>  	      CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> -- 
> MST
Michael S. Tsirkin March 13, 2017, 4:08 p.m. UTC | #8
On Mon, Mar 13, 2017 at 04:46:20PM +0100, Radim Krčmář wrote:
> 2017-03-10 00:29+0200, Michael S. Tsirkin:
> > Some guests call mwait without checking the cpu flags.  We currently
> > emulate that as a NOP but on VMX we can do better: let guest stop the
> > CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> > a NOP emulation.
> > 
> > Note that mwait within guests is not the same as on real hardware
> > because you must halt if you want to go deep into sleep.
> 
> SDM (25.3 CHANGES TO INSTRUCTION BEHAVIOR IN VMX NON-ROOT OPERATION)
> says that "MWAIT operates normally".  What is the reason why MWAIT
> inside VMX cannot reach the same states as MWAIT outside VMX?

If you are going into a deep sleep state with huge latency you are
better off exiting and paying an extra microsecond latency
since a chance some other task will want to schedule seems higher.

> >                                                           Thus it isn't
> > a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> > in the hypervisor leaf instead.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
>   [...]
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> > +		if (this_cpu_has(X86_FEATURE_MWAIT))
> > +			entry->eax = (1 << KVM_FEATURE_MWAIT);
> 
> I'd rather not add it as a paravirt feature:
> 
>  - MWAIT requires the software to provide a target state, but we're not
>    doing anything to expose those states.

Current linux guests just discover these states based on
CPU model, so we do expose enough info.

>    The feature would need very constrained setup, which is hard to
>    support

Why would it? It works without any tweaking on several boxes
I own.

>  - we've had requests to support MWAIT emulation for Linux and fully
>    emulating MWAIT would be best.
>    MWAIT is not going to enabled by default, of course; it would be
>    targeted at LPAR-like uses of KVM.

Yes I think this limited emulation is safe to enable by default.
Pretending mwait is equivalent to halt maybe isn't.

> What about keeping just the last hunk to improve OS X, for now?
> 
> Thanks.

IMHO if we have a new functionality we are better of creating
some way for guests to discover it is there.
Do we really have to argue about a single bit in HV leaf?
What harm does it do?

> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > @@ -3547,13 +3547,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
> >  	      CPU_BASED_USE_IO_BITMAPS |
> >  	      CPU_BASED_MOV_DR_EXITING |
> >  	      CPU_BASED_USE_TSC_OFFSETING |
> > -	      CPU_BASED_MWAIT_EXITING |
> > -	      CPU_BASED_MONITOR_EXITING |
> >  	      CPU_BASED_INVLPG_EXITING |
> >  	      CPU_BASED_RDPMC_EXITING;
> >  
> > -	printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
> > -
> >  	opt = CPU_BASED_TPR_SHADOW |
> >  	      CPU_BASED_USE_MSR_BITMAPS |
> >  	      CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> > -- 
> > MST
Radim Krčmář March 13, 2017, 7:39 p.m. UTC | #9
2017-03-13 18:08+0200, Michael S. Tsirkin:
> On Mon, Mar 13, 2017 at 04:46:20PM +0100, Radim Krčmář wrote:
>> 2017-03-10 00:29+0200, Michael S. Tsirkin:
>> > Some guests call mwait without checking the cpu flags.  We currently
>> > emulate that as a NOP but on VMX we can do better: let guest stop the
>> > CPU until timer or IPI.  CPU will be busy but that isn't any worse than
>> > a NOP emulation.
>> > 
>> > Note that mwait within guests is not the same as on real hardware
>> > because you must halt if you want to go deep into sleep.
>> 
>> SDM (25.3 CHANGES TO INSTRUCTION BEHAVIOR IN VMX NON-ROOT OPERATION)
>> says that "MWAIT operates normally".  What is the reason why MWAIT
>> inside VMX cannot reach the same states as MWAIT outside VMX?
> 
> If you are going into a deep sleep state with huge latency you are
> better off exiting and paying an extra microsecond latency
> since a chance some other task will want to schedule seems higher.

Oh, so MWAIT behavior is same and can reach deep sleep, just use-cases
differ ... If the guest VCPU is running on isolated CPU, then you might
want to reach a deep state to save power when there is no better use.

>> >                                                           Thus it isn't
>> > a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
>> > in the hypervisor leaf instead.
>> > 
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>>   [...]
>> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>> > +		if (this_cpu_has(X86_FEATURE_MWAIT))
>> > +			entry->eax = (1 << KVM_FEATURE_MWAIT);
>> 
>> I'd rather not add it as a paravirt feature:
>> 
>>  - MWAIT requires the software to provide a target state, but we're not
>>    doing anything to expose those states.
> 
> Current linux guests just discover these states based on
> CPU model, so we do expose enough info.

Linux still filters the hardcoded hints through CPUID[5].edx, which is 0
in our case.

>>    The feature would need very constrained setup, which is hard to
>>    support
> 
> Why would it? It works without any tweaking on several boxes
> I own.

MWAIT hints do not always mean the same, so they could lead to different
power/performance tradeoffs than the applications expects.  We should at
least specify that the paravirt feature allows only hint 0.

You probably don't run weird combinations of host/guest CPUs.

>>  - we've had requests to support MWAIT emulation for Linux and fully
>>    emulating MWAIT would be best.
>>    MWAIT is not going to enabled by default, of course; it would be
>>    targeted at LPAR-like uses of KVM.
> 
> Yes I think this limited emulation is safe to enable by default.
> Pretending mwait is equivalent to halt maybe isn't.

Right, we must keep the VCPU thread running when emulating mwait as it
is different from a hlt.

>> What about keeping just the last hunk to improve OS X, for now?
>> 
>> Thanks.
> 
> IMHO if we have a new functionality we are better of creating
> some way for guests to discover it is there.
> 
> Do we really have to argue about a single bit in HV leaf?
> What harm does it do?

It adds code to both guest and hosts and needs documentation ...
The bit is acceptable.  I just see no point in having it when there
already is a detection mechanism for mwait.

In any case, this patch should also remove VM exits under SVM and add
KVM_CAP_MWAIT for userspace.  Userspace can then set the MWAIT feature
if it wishes the guest to use it in a more standard way.

I can do a cleanup due to unused VM exits on top of it.

Thanks.
Michael S. Tsirkin March 13, 2017, 8:03 p.m. UTC | #10
On Mon, Mar 13, 2017 at 08:39:11PM +0100, Radim Krčmář wrote:
> 2017-03-13 18:08+0200, Michael S. Tsirkin:
> > On Mon, Mar 13, 2017 at 04:46:20PM +0100, Radim Krčmář wrote:
> >> 2017-03-10 00:29+0200, Michael S. Tsirkin:
> >> > Some guests call mwait without checking the cpu flags.  We currently
> >> > emulate that as a NOP but on VMX we can do better: let guest stop the
> >> > CPU until timer or IPI.  CPU will be busy but that isn't any worse than
> >> > a NOP emulation.
> >> > 
> >> > Note that mwait within guests is not the same as on real hardware
> >> > because you must halt if you want to go deep into sleep.
> >> 
> >> SDM (25.3 CHANGES TO INSTRUCTION BEHAVIOR IN VMX NON-ROOT OPERATION)
> >> says that "MWAIT operates normally".  What is the reason why MWAIT
> >> inside VMX cannot reach the same states as MWAIT outside VMX?
> > 
> > If you are going into a deep sleep state with huge latency you are
> > better off exiting and paying an extra microsecond latency
> > since a chance some other task will want to schedule seems higher.
> 
> Oh, so MWAIT behavior is same and can reach deep sleep, just use-cases
> differ ... If the guest VCPU is running on isolated CPU, then you might
> want to reach a deep state to save power when there is no better use.
> 
> >> >                                                           Thus it isn't
> >> > a good idea to use the regular MWAIT flag in CPUID for that.  Add a flag
> >> > in the hypervisor leaf instead.
> >> > 
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > ---
> >>   [...]
> >> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> >> > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >> > +		if (this_cpu_has(X86_FEATURE_MWAIT))
> >> > +			entry->eax = (1 << KVM_FEATURE_MWAIT);
> >> 
> >> I'd rather not add it as a paravirt feature:
> >> 
> >>  - MWAIT requires the software to provide a target state, but we're not
> >>    doing anything to expose those states.
> > 
> > Current linux guests just discover these states based on
> > CPU model, so we do expose enough info.
> 
> Linux still filters the hardcoded hints through CPUID[5].edx, which is 0
> in our case.
> 
> >>    The feature would need very constrained setup, which is hard to
> >>    support
> > 
> > Why would it? It works without any tweaking on several boxes
> > I own.
> 
> MWAIT hints do not always mean the same, so they could lead to different
> power/performance tradeoffs than the applications expects.  We should at
> least specify that the paravirt feature allows only hint 0.
> 
> You probably don't run weird combinations of host/guest CPUs.
> 
> >>  - we've had requests to support MWAIT emulation for Linux and fully
> >>    emulating MWAIT would be best.
> >>    MWAIT is not going to enabled by default, of course; it would be
> >>    targeted at LPAR-like uses of KVM.
> > 
> > Yes I think this limited emulation is safe to enable by default.
> > Pretending mwait is equivalent to halt maybe isn't.
> 
> Right, we must keep the VCPU thread running when emulating mwait as it
> is different from a hlt.
> 
> >> What about keeping just the last hunk to improve OS X, for now?
> >> 
> >> Thanks.
> > 
> > IMHO if we have a new functionality we are better of creating
> > some way for guests to discover it is there.
> > 
> > Do we really have to argue about a single bit in HV leaf?
> > What harm does it do?
> 
> It adds code to both guest and hosts and needs documentation ...
> The bit is acceptable.  I just see no point in having it when there
> already is a detection mechanism for mwait.

We don't want to use that standard detection mechanism IMHO at least
not in all cases.

> In any case, this patch should also remove VM exits under SVM

AMD does not have MWAIT AFAIK. In any case, I don't see
why can't SVM be a separate patch.

> and add
> KVM_CAP_MWAIT for userspace.

Sure, why not. Will do.

> Userspace can then set the MWAIT feature
> if it wishes the guest to use it in a more standard way.
> 
> I can do a cleanup due to unused VM exits on top of it.
> 
> Thanks.

Patch
diff mbox

diff --git a/Documentation/virtual/kvm/cpuid.txt b/Documentation/virtual/kvm/cpuid.txt
index 3c65feb..5caa234 100644
--- a/Documentation/virtual/kvm/cpuid.txt
+++ b/Documentation/virtual/kvm/cpuid.txt
@@ -54,6 +54,9 @@  KVM_FEATURE_PV_UNHALT              ||     7 || guest checks this feature bit
                                    ||       || before enabling paravirtualized
                                    ||       || spinlock support.
 ------------------------------------------------------------------------------
+KVM_FEATURE_MWAIT                  ||     8 || guest can use monitor/mwait
+                                   ||       || to halt the VCPU.
+------------------------------------------------------------------------------
 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||    24 || host will warn if no guest-side
                                    ||       || per-cpu warps are expected in
                                    ||       || kvmclock.
diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
index cff0bb6..9cc77a7 100644
--- a/arch/x86/include/uapi/asm/kvm_para.h
+++ b/arch/x86/include/uapi/asm/kvm_para.h
@@ -24,6 +24,7 @@ 
 #define KVM_FEATURE_STEAL_TIME		5
 #define KVM_FEATURE_PV_EOI		6
 #define KVM_FEATURE_PV_UNHALT		7
+#define KVM_FEATURE_MWAIT		8
 
 /* The last 8 bits are used to indicate how to interpret the flags field
  * in pvclock structure. If no bits are set, all flags are ignored.
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index efde6cc..fe3d292 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -594,6 +594,9 @@  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		if (sched_info_on())
 			entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
 
+		if (this_cpu_has(X86_FEATURE_MWAIT))
+			entry->eax = (1 << KVM_FEATURE_MWAIT);
+
 		entry->ebx = 0;
 		entry->ecx = 0;
 		entry->edx = 0;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4bfe349..b167aba 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3547,13 +3547,9 @@  static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 	      CPU_BASED_USE_IO_BITMAPS |
 	      CPU_BASED_MOV_DR_EXITING |
 	      CPU_BASED_USE_TSC_OFFSETING |
-	      CPU_BASED_MWAIT_EXITING |
-	      CPU_BASED_MONITOR_EXITING |
 	      CPU_BASED_INVLPG_EXITING |
 	      CPU_BASED_RDPMC_EXITING;
 
-	printk(KERN_ERR "cleared CPU_BASED_MWAIT_EXITING + CPU_BASED_MONITOR_EXITING\n");
-
 	opt = CPU_BASED_TPR_SHADOW |
 	      CPU_BASED_USE_MSR_BITMAPS |
 	      CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;