[PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

Treat monitor and mwait instructions as nop, which is architecturally
correct (but inefficient) behavior. We do this to prevent misbehaving
guests (e.g. OS X <= 10.7) from crashing after they fail to check for
monitor/mwait availability via cpuid.

Since mwait-based idle loops relying on these nop-emulated instructions
would keep the host CPU pegged at 100%, do NOT advertise their presence
via cpuid, to prevent compliant guests from using them inadvertently.

Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
---

New in v2: remove invalid_op handler functions which were only used to
           handle exits caused by monitor and mwait

On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
> >If we really want to be paranoid and worry about guests
> >that use this strange way to trigger invalid opcode,
> >we can make it possible for userspace to enable/disable
> >this hack, and teach qemu to set it.
> >
> >That would make it even safer than it was.
> >
> >Not sure it's worth it, just a thought.
> 
> Since we don't trap on non-exposed other instructions (new SSE and
> whatdoiknow) I don't think it's really bad to just expose
> MONITOR/MWAIT as nops.

So AFAICT, linux prefers to use mwait for idling if cpuid tells it that
it's available. If we keep telling everyone that we do NOT have monitor
and mwait available, compliant guests will never end up using them, and
this hack would remain completely invisible to them, which is good
(better to use hlt-based idle loops when you're a vm guest, that would
actually allow the host to relax while you're halted :)

So the only time anyone would be able to tell we have this hack would be
when they're about to receive an invalid opcode for using monitor/mwait
in violation of what CPUID (would have) told them. That's what happens
to OS X prior to 10.8, which is when I'm hypothesizing the Apple devs
begain to seriously think about their OS running as a vm guest (on fusion
and parallels)...

Instead of killing the misbehaving guest with an invalid opcode, we'd
allow them to peg the host CPU with their monitor == mwait == nop idle
loop instead, which, at least on OS X, should be tolerable long enough
to run 'rm -rf System/Library/Extensions/AppleIntelCPUPowerManagement.kext'
and reboot the guest, after which things would settle down by reverting
the guest to a hlt-based idle loop.

The only reason I can think of to add functionality for enabling/disabling
this hack would be to protect against a malicious guest which would use
mwait *on purpose* to peg the host CPU. But a malicious guest could just
run "for(;;);" in ring 0 and accomplish the same goal, so we wouldn't
really gain anything in exchange for the added complexity...

Thanks,
  Gabriel

 arch/x86/kvm/cpuid.c |  2 ++
 arch/x86/kvm/svm.c   | 28 ++++++++++++++++++++--------
 arch/x86/kvm/vmx.c   | 20 ++++++++++++++++----
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index f47a104..d094fc6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -283,6 +283,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
 	/* cpuid 1.ecx */
 	const u32 kvm_supported_word4_x86_features =
+		/* NOTE: MONITOR (and MWAIT) are emulated as NOP,
+		 * but *not* advertised to guests via CPUID ! */
 		F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
 		0 /* DS-CPL, VMX, SMX, EST */ |
 		0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 7f4f9c2..0b7d58d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2770,12 +2770,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
 	return 1;
 }
 
-static int invalid_op_interception(struct vcpu_svm *svm)
-{
-	kvm_queue_exception(&svm->vcpu, UD_VECTOR);
-	return 1;
-}
-
 static int task_switch_interception(struct vcpu_svm *svm)
 {
 	u16 tss_selector;
@@ -3287,6 +3281,24 @@ static int pause_interception(struct vcpu_svm *svm)
 	return 1;
 }
 
+static int nop_interception(struct vcpu_svm *svm)
+{
+	skip_emulated_instruction(&(svm->vcpu));
+	return 1;
+}
+
+static int monitor_interception(struct vcpu_svm *svm)
+{
+	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
+	return nop_interception(svm);
+}
+
+static int mwait_interception(struct vcpu_svm *svm)
+{
+	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
+	return nop_interception(svm);
+}
+
 static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_READ_CR0]			= cr_interception,
 	[SVM_EXIT_READ_CR3]			= cr_interception,
@@ -3344,8 +3356,8 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_CLGI]				= clgi_interception,
 	[SVM_EXIT_SKINIT]			= skinit_interception,
 	[SVM_EXIT_WBINVD]                       = emulate_on_interception,
-	[SVM_EXIT_MONITOR]			= invalid_op_interception,
-	[SVM_EXIT_MWAIT]			= invalid_op_interception,
+	[SVM_EXIT_MONITOR]			= monitor_interception,
+	[SVM_EXIT_MWAIT]			= mwait_interception,
 	[SVM_EXIT_XSETBV]			= xsetbv_interception,
 	[SVM_EXIT_NPF]				= pf_interception,
 };
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 33e8c02..3ccbcb1 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5669,12 +5669,24 @@ static int handle_pause(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
-static int handle_invalid_op(struct kvm_vcpu *vcpu)
+static int handle_nop(struct kvm_vcpu *vcpu)
 {
-	kvm_queue_exception(vcpu, UD_VECTOR);
+	skip_emulated_instruction(vcpu);
 	return 1;
 }
 
+static int handle_mwait(struct kvm_vcpu *vcpu)
+{
+	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
+	return handle_nop(vcpu);
+}
+
+static int handle_monitor(struct kvm_vcpu *vcpu)
+{
+	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
+	return handle_nop(vcpu);
+}
+
 /*
  * To run an L2 guest, we need a vmcs02 based on the L1-specified vmcs12.
  * We could reuse a single VMCS for all the L2 guests, but we also want the
@@ -6571,8 +6583,8 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[EXIT_REASON_EPT_VIOLATION]	      = handle_ept_violation,
 	[EXIT_REASON_EPT_MISCONFIG]           = handle_ept_misconfig,
 	[EXIT_REASON_PAUSE_INSTRUCTION]       = handle_pause,
-	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_invalid_op,
-	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_invalid_op,
+	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_mwait,
+	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
 	[EXIT_REASON_INVEPT]                  = handle_invept,
 };
 
-- 
1.9.0

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
> Treat monitor and mwait instructions as nop, which is architecturally
> correct (but inefficient) behavior. We do this to prevent misbehaving
> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
> monitor/mwait availability via cpuid.
> 
> Since mwait-based idle loops relying on these nop-emulated instructions
> would keep the host CPU pegged at 100%, do NOT advertise their presence
> via cpuid, to prevent compliant guests from using them inadvertently.
> 
> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> ---
> 
> New in v2: remove invalid_op handler functions which were only used to
>            handle exits caused by monitor and mwait
> 
> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
> > On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
> > >If we really want to be paranoid and worry about guests
> > >that use this strange way to trigger invalid opcode,
> > >we can make it possible for userspace to enable/disable
> > >this hack, and teach qemu to set it.
> > >
> > >That would make it even safer than it was.
> > >
> > >Not sure it's worth it, just a thought.
> > 
> > Since we don't trap on non-exposed other instructions (new SSE and
> > whatdoiknow) I don't think it's really bad to just expose
> > MONITOR/MWAIT as nops.

Would it make sense to make this a module parameter,
(e.g., "int emulate_mwait") ?

Default would be 0 (no emulation). 1 would mean "emulate as nop", and
if anyone ever figures out how to do proper page-locking based
emulation we could use 2 to enable that, etc. ?

Not sure we'd want qemu to enable/disable it automatically, though...

What do you all think ?

Thanks,
--Gabriel

> 
> So AFAICT, linux prefers to use mwait for idling if cpuid tells it that
> it's available. If we keep telling everyone that we do NOT have monitor
> and mwait available, compliant guests will never end up using them, and
> this hack would remain completely invisible to them, which is good
> (better to use hlt-based idle loops when you're a vm guest, that would
> actually allow the host to relax while you're halted :)
> 
> So the only time anyone would be able to tell we have this hack would be
> when they're about to receive an invalid opcode for using monitor/mwait
> in violation of what CPUID (would have) told them. That's what happens
> to OS X prior to 10.8, which is when I'm hypothesizing the Apple devs
> begain to seriously think about their OS running as a vm guest (on fusion
> and parallels)...
> 
> Instead of killing the misbehaving guest with an invalid opcode, we'd
> allow them to peg the host CPU with their monitor == mwait == nop idle
> loop instead, which, at least on OS X, should be tolerable long enough
> to run 'rm -rf System/Library/Extensions/AppleIntelCPUPowerManagement.kext'
> and reboot the guest, after which things would settle down by reverting
> the guest to a hlt-based idle loop.
> 
> The only reason I can think of to add functionality for enabling/disabling
> this hack would be to protect against a malicious guest which would use
> mwait *on purpose* to peg the host CPU. But a malicious guest could just
> run "for(;;);" in ring 0 and accomplish the same goal, so we wouldn't
> really gain anything in exchange for the added complexity...
> 
> Thanks,
>   Gabriel
> 
>  arch/x86/kvm/cpuid.c |  2 ++
>  arch/x86/kvm/svm.c   | 28 ++++++++++++++++++++--------
>  arch/x86/kvm/vmx.c   | 20 ++++++++++++++++----
>  3 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index f47a104..d094fc6 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -283,6 +283,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
>  	/* cpuid 1.ecx */
>  	const u32 kvm_supported_word4_x86_features =
> +		/* NOTE: MONITOR (and MWAIT) are emulated as NOP,
> +		 * but *not* advertised to guests via CPUID ! */
>  		F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
>  		0 /* DS-CPL, VMX, SMX, EST */ |
>  		0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 7f4f9c2..0b7d58d 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2770,12 +2770,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> -static int invalid_op_interception(struct vcpu_svm *svm)
> -{
> -	kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> -	return 1;
> -}
> -
>  static int task_switch_interception(struct vcpu_svm *svm)
>  {
>  	u16 tss_selector;
> @@ -3287,6 +3281,24 @@ static int pause_interception(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> +static int nop_interception(struct vcpu_svm *svm)
> +{
> +	skip_emulated_instruction(&(svm->vcpu));
> +	return 1;
> +}
> +
> +static int monitor_interception(struct vcpu_svm *svm)
> +{
> +	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> +	return nop_interception(svm);
> +}
> +
> +static int mwait_interception(struct vcpu_svm *svm)
> +{
> +	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> +	return nop_interception(svm);
> +}
> +
>  static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>  	[SVM_EXIT_READ_CR0]			= cr_interception,
>  	[SVM_EXIT_READ_CR3]			= cr_interception,
> @@ -3344,8 +3356,8 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>  	[SVM_EXIT_CLGI]				= clgi_interception,
>  	[SVM_EXIT_SKINIT]			= skinit_interception,
>  	[SVM_EXIT_WBINVD]                       = emulate_on_interception,
> -	[SVM_EXIT_MONITOR]			= invalid_op_interception,
> -	[SVM_EXIT_MWAIT]			= invalid_op_interception,
> +	[SVM_EXIT_MONITOR]			= monitor_interception,
> +	[SVM_EXIT_MWAIT]			= mwait_interception,
>  	[SVM_EXIT_XSETBV]			= xsetbv_interception,
>  	[SVM_EXIT_NPF]				= pf_interception,
>  };
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 33e8c02..3ccbcb1 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5669,12 +5669,24 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> -static int handle_invalid_op(struct kvm_vcpu *vcpu)
> +static int handle_nop(struct kvm_vcpu *vcpu)
>  {
> -	kvm_queue_exception(vcpu, UD_VECTOR);
> +	skip_emulated_instruction(vcpu);
>  	return 1;
>  }
>  
> +static int handle_mwait(struct kvm_vcpu *vcpu)
> +{
> +	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> +	return handle_nop(vcpu);
> +}
> +
> +static int handle_monitor(struct kvm_vcpu *vcpu)
> +{
> +	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> +	return handle_nop(vcpu);
> +}
> +
>  /*
>   * To run an L2 guest, we need a vmcs02 based on the L1-specified vmcs12.
>   * We could reuse a single VMCS for all the L2 guests, but we also want the
> @@ -6571,8 +6583,8 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
>  	[EXIT_REASON_EPT_VIOLATION]	      = handle_ept_violation,
>  	[EXIT_REASON_EPT_MISCONFIG]           = handle_ept_misconfig,
>  	[EXIT_REASON_PAUSE_INSTRUCTION]       = handle_pause,
> -	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_invalid_op,
> -	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_invalid_op,
> +	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_mwait,
> +	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
>  	[EXIT_REASON_INVEPT]                  = handle_invept,
>  };
>  
> -- 
> 1.9.0
> 

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Alexander Graf]

> Am 02.06.2014 um 21:25 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> 
>> On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
>> Treat monitor and mwait instructions as nop, which is architecturally
>> correct (but inefficient) behavior. We do this to prevent misbehaving
>> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
>> monitor/mwait availability via cpuid.
>> 
>> Since mwait-based idle loops relying on these nop-emulated instructions
>> would keep the host CPU pegged at 100%, do NOT advertise their presence
>> via cpuid, to prevent compliant guests from using them inadvertently.
>> 
>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
>> ---
>> 
>> New in v2: remove invalid_op handler functions which were only used to
>>           handle exits caused by monitor and mwait
>> 
>>> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
>>>> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
>>>> If we really want to be paranoid and worry about guests
>>>> that use this strange way to trigger invalid opcode,
>>>> we can make it possible for userspace to enable/disable
>>>> this hack, and teach qemu to set it.
>>>> 
>>>> That would make it even safer than it was.
>>>> 
>>>> Not sure it's worth it, just a thought.
>>> 
>>> Since we don't trap on non-exposed other instructions (new SSE and
>>> whatdoiknow) I don't think it's really bad to just expose
>>> MONITOR/MWAIT as nops.
> 
> Would it make sense to make this a module parameter,
> (e.g., "int emulate_mwait") ?
> 
> Default would be 0 (no emulation). 1 would mean "emulate as nop", and
> if anyone ever figures out how to do proper page-locking based
> emulation we could use 2 to enable that, etc. ?
> 
> Not sure we'd want qemu to enable/disable it automatically, though...
> 
> What do you all think ?

I don't like module parameters - they're system global and there's a good chance you want to run non-osx in parallel ;).

I'd either link this to the cpuid bits or enable it forcefully through ENABLE_CAP per vcpu.

Alex

> 
> Thanks,
> --Gabriel
> 
>> 
>> So AFAICT, linux prefers to use mwait for idling if cpuid tells it that
>> it's available. If we keep telling everyone that we do NOT have monitor
>> and mwait available, compliant guests will never end up using them, and
>> this hack would remain completely invisible to them, which is good
>> (better to use hlt-based idle loops when you're a vm guest, that would
>> actually allow the host to relax while you're halted :)
>> 
>> So the only time anyone would be able to tell we have this hack would be
>> when they're about to receive an invalid opcode for using monitor/mwait
>> in violation of what CPUID (would have) told them. That's what happens
>> to OS X prior to 10.8, which is when I'm hypothesizing the Apple devs
>> begain to seriously think about their OS running as a vm guest (on fusion
>> and parallels)...
>> 
>> Instead of killing the misbehaving guest with an invalid opcode, we'd
>> allow them to peg the host CPU with their monitor == mwait == nop idle
>> loop instead, which, at least on OS X, should be tolerable long enough
>> to run 'rm -rf System/Library/Extensions/AppleIntelCPUPowerManagement.kext'
>> and reboot the guest, after which things would settle down by reverting
>> the guest to a hlt-based idle loop.
>> 
>> The only reason I can think of to add functionality for enabling/disabling
>> this hack would be to protect against a malicious guest which would use
>> mwait *on purpose* to peg the host CPU. But a malicious guest could just
>> run "for(;;);" in ring 0 and accomplish the same goal, so we wouldn't
>> really gain anything in exchange for the added complexity...
>> 
>> Thanks,
>>  Gabriel
>> 
>> arch/x86/kvm/cpuid.c |  2 ++
>> arch/x86/kvm/svm.c   | 28 ++++++++++++++++++++--------
>> arch/x86/kvm/vmx.c   | 20 ++++++++++++++++----
>> 3 files changed, 38 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index f47a104..d094fc6 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -283,6 +283,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>>        0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
>>    /* cpuid 1.ecx */
>>    const u32 kvm_supported_word4_x86_features =
>> +        /* NOTE: MONITOR (and MWAIT) are emulated as NOP,
>> +         * but *not* advertised to guests via CPUID ! */
>>        F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
>>        0 /* DS-CPL, VMX, SMX, EST */ |
>>        0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 7f4f9c2..0b7d58d 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2770,12 +2770,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
>>    return 1;
>> }
>> 
>> -static int invalid_op_interception(struct vcpu_svm *svm)
>> -{
>> -    kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>> -    return 1;
>> -}
>> -
>> static int task_switch_interception(struct vcpu_svm *svm)
>> {
>>    u16 tss_selector;
>> @@ -3287,6 +3281,24 @@ static int pause_interception(struct vcpu_svm *svm)
>>    return 1;
>> }
>> 
>> +static int nop_interception(struct vcpu_svm *svm)
>> +{
>> +    skip_emulated_instruction(&(svm->vcpu));
>> +    return 1;
>> +}
>> +
>> +static int monitor_interception(struct vcpu_svm *svm)
>> +{
>> +    printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
>> +    return nop_interception(svm);
>> +}
>> +
>> +static int mwait_interception(struct vcpu_svm *svm)
>> +{
>> +    printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
>> +    return nop_interception(svm);
>> +}
>> +
>> static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>>    [SVM_EXIT_READ_CR0]            = cr_interception,
>>    [SVM_EXIT_READ_CR3]            = cr_interception,
>> @@ -3344,8 +3356,8 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>>    [SVM_EXIT_CLGI]                = clgi_interception,
>>    [SVM_EXIT_SKINIT]            = skinit_interception,
>>    [SVM_EXIT_WBINVD]                       = emulate_on_interception,
>> -    [SVM_EXIT_MONITOR]            = invalid_op_interception,
>> -    [SVM_EXIT_MWAIT]            = invalid_op_interception,
>> +    [SVM_EXIT_MONITOR]            = monitor_interception,
>> +    [SVM_EXIT_MWAIT]            = mwait_interception,
>>    [SVM_EXIT_XSETBV]            = xsetbv_interception,
>>    [SVM_EXIT_NPF]                = pf_interception,
>> };
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 33e8c02..3ccbcb1 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -5669,12 +5669,24 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>>    return 1;
>> }
>> 
>> -static int handle_invalid_op(struct kvm_vcpu *vcpu)
>> +static int handle_nop(struct kvm_vcpu *vcpu)
>> {
>> -    kvm_queue_exception(vcpu, UD_VECTOR);
>> +    skip_emulated_instruction(vcpu);
>>    return 1;
>> }
>> 
>> +static int handle_mwait(struct kvm_vcpu *vcpu)
>> +{
>> +    printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
>> +    return handle_nop(vcpu);
>> +}
>> +
>> +static int handle_monitor(struct kvm_vcpu *vcpu)
>> +{
>> +    printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
>> +    return handle_nop(vcpu);
>> +}
>> +
>> /*
>>  * To run an L2 guest, we need a vmcs02 based on the L1-specified vmcs12.
>>  * We could reuse a single VMCS for all the L2 guests, but we also want the
>> @@ -6571,8 +6583,8 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
>>    [EXIT_REASON_EPT_VIOLATION]          = handle_ept_violation,
>>    [EXIT_REASON_EPT_MISCONFIG]           = handle_ept_misconfig,
>>    [EXIT_REASON_PAUSE_INSTRUCTION]       = handle_pause,
>> -    [EXIT_REASON_MWAIT_INSTRUCTION]          = handle_invalid_op,
>> -    [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_invalid_op,
>> +    [EXIT_REASON_MWAIT_INSTRUCTION]          = handle_mwait,
>> +    [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
>>    [EXIT_REASON_INVEPT]                  = handle_invept,
>> };
>> 
>> -- 
>> 1.9.0
>> 

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Mon, Jun 02, 2014 at 09:48:19PM +0200, Alexander Graf wrote:
> 
> 
> > Am 02.06.2014 um 21:25 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> > 
> >> On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
> >> Treat monitor and mwait instructions as nop, which is architecturally
> >> correct (but inefficient) behavior. We do this to prevent misbehaving
> >> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
> >> monitor/mwait availability via cpuid.
> >> 
> >> Since mwait-based idle loops relying on these nop-emulated instructions
> >> would keep the host CPU pegged at 100%, do NOT advertise their presence
> >> via cpuid, to prevent compliant guests from using them inadvertently.
> >> 
> >> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> >> ---
> >> 
> >> New in v2: remove invalid_op handler functions which were only used to
> >>           handle exits caused by monitor and mwait
> >> 
> >>> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
> >>>> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
> >>>> If we really want to be paranoid and worry about guests
> >>>> that use this strange way to trigger invalid opcode,
> >>>> we can make it possible for userspace to enable/disable
> >>>> this hack, and teach qemu to set it.
> >>>> 
> >>>> That would make it even safer than it was.
> >>>> 
> >>>> Not sure it's worth it, just a thought.
> >>> 
> >>> Since we don't trap on non-exposed other instructions (new SSE and
> >>> whatdoiknow) I don't think it's really bad to just expose
> >>> MONITOR/MWAIT as nops.
> > 
> > Would it make sense to make this a module parameter,
> > (e.g., "int emulate_mwait") ?
> > 
> > Default would be 0 (no emulation). 1 would mean "emulate as nop", and
> > if anyone ever figures out how to do proper page-locking based
> > emulation we could use 2 to enable that, etc. ?
> > 
> > Not sure we'd want qemu to enable/disable it automatically, though...
> > 
> > What do you all think ?
> 
> I don't like module parameters - they're system global and there's a good chance you want to run non-osx in parallel ;).
> 
> I'd either link this to the cpuid bits or enable it forcefully through ENABLE_CAP per vcpu.
> 
> Alex

Point is that.
Paolo here thinks it's safe to just make it a NOP unconditionally.
so module parameter would be there as a debugging tool:
as a means for users to test with old kvm behaviour if they see breakage.
Which we don't expect, so no need to waste cycles creating a pretty
interface for it.

> > 
> > Thanks,
> > --Gabriel
> > 
> >> 
> >> So AFAICT, linux prefers to use mwait for idling if cpuid tells it that
> >> it's available. If we keep telling everyone that we do NOT have monitor
> >> and mwait available, compliant guests will never end up using them, and
> >> this hack would remain completely invisible to them, which is good
> >> (better to use hlt-based idle loops when you're a vm guest, that would
> >> actually allow the host to relax while you're halted :)
> >> 
> >> So the only time anyone would be able to tell we have this hack would be
> >> when they're about to receive an invalid opcode for using monitor/mwait
> >> in violation of what CPUID (would have) told them. That's what happens
> >> to OS X prior to 10.8, which is when I'm hypothesizing the Apple devs
> >> begain to seriously think about their OS running as a vm guest (on fusion
> >> and parallels)...
> >> 
> >> Instead of killing the misbehaving guest with an invalid opcode, we'd
> >> allow them to peg the host CPU with their monitor == mwait == nop idle
> >> loop instead, which, at least on OS X, should be tolerable long enough
> >> to run 'rm -rf System/Library/Extensions/AppleIntelCPUPowerManagement.kext'
> >> and reboot the guest, after which things would settle down by reverting
> >> the guest to a hlt-based idle loop.
> >> 
> >> The only reason I can think of to add functionality for enabling/disabling
> >> this hack would be to protect against a malicious guest which would use
> >> mwait *on purpose* to peg the host CPU. But a malicious guest could just
> >> run "for(;;);" in ring 0 and accomplish the same goal, so we wouldn't
> >> really gain anything in exchange for the added complexity...
> >> 
> >> Thanks,
> >>  Gabriel
> >> 
> >> arch/x86/kvm/cpuid.c |  2 ++
> >> arch/x86/kvm/svm.c   | 28 ++++++++++++++++++++--------
> >> arch/x86/kvm/vmx.c   | 20 ++++++++++++++++----
> >> 3 files changed, 38 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> >> index f47a104..d094fc6 100644
> >> --- a/arch/x86/kvm/cpuid.c
> >> +++ b/arch/x86/kvm/cpuid.c
> >> @@ -283,6 +283,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >>        0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
> >>    /* cpuid 1.ecx */
> >>    const u32 kvm_supported_word4_x86_features =
> >> +        /* NOTE: MONITOR (and MWAIT) are emulated as NOP,
> >> +         * but *not* advertised to guests via CPUID ! */
> >>        F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
> >>        0 /* DS-CPL, VMX, SMX, EST */ |
> >>        0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index 7f4f9c2..0b7d58d 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -2770,12 +2770,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
> >>    return 1;
> >> }
> >> 
> >> -static int invalid_op_interception(struct vcpu_svm *svm)
> >> -{
> >> -    kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> >> -    return 1;
> >> -}
> >> -
> >> static int task_switch_interception(struct vcpu_svm *svm)
> >> {
> >>    u16 tss_selector;
> >> @@ -3287,6 +3281,24 @@ static int pause_interception(struct vcpu_svm *svm)
> >>    return 1;
> >> }
> >> 
> >> +static int nop_interception(struct vcpu_svm *svm)
> >> +{
> >> +    skip_emulated_instruction(&(svm->vcpu));
> >> +    return 1;
> >> +}
> >> +
> >> +static int monitor_interception(struct vcpu_svm *svm)
> >> +{
> >> +    printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> >> +    return nop_interception(svm);
> >> +}
> >> +
> >> +static int mwait_interception(struct vcpu_svm *svm)
> >> +{
> >> +    printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> >> +    return nop_interception(svm);
> >> +}
> >> +
> >> static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
> >>    [SVM_EXIT_READ_CR0]            = cr_interception,
> >>    [SVM_EXIT_READ_CR3]            = cr_interception,
> >> @@ -3344,8 +3356,8 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
> >>    [SVM_EXIT_CLGI]                = clgi_interception,
> >>    [SVM_EXIT_SKINIT]            = skinit_interception,
> >>    [SVM_EXIT_WBINVD]                       = emulate_on_interception,
> >> -    [SVM_EXIT_MONITOR]            = invalid_op_interception,
> >> -    [SVM_EXIT_MWAIT]            = invalid_op_interception,
> >> +    [SVM_EXIT_MONITOR]            = monitor_interception,
> >> +    [SVM_EXIT_MWAIT]            = mwait_interception,
> >>    [SVM_EXIT_XSETBV]            = xsetbv_interception,
> >>    [SVM_EXIT_NPF]                = pf_interception,
> >> };
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index 33e8c02..3ccbcb1 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -5669,12 +5669,24 @@ static int handle_pause(struct kvm_vcpu *vcpu)
> >>    return 1;
> >> }
> >> 
> >> -static int handle_invalid_op(struct kvm_vcpu *vcpu)
> >> +static int handle_nop(struct kvm_vcpu *vcpu)
> >> {
> >> -    kvm_queue_exception(vcpu, UD_VECTOR);
> >> +    skip_emulated_instruction(vcpu);
> >>    return 1;
> >> }
> >> 
> >> +static int handle_mwait(struct kvm_vcpu *vcpu)
> >> +{
> >> +    printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> >> +    return handle_nop(vcpu);
> >> +}
> >> +
> >> +static int handle_monitor(struct kvm_vcpu *vcpu)
> >> +{
> >> +    printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> >> +    return handle_nop(vcpu);
> >> +}
> >> +
> >> /*
> >>  * To run an L2 guest, we need a vmcs02 based on the L1-specified vmcs12.
> >>  * We could reuse a single VMCS for all the L2 guests, but we also want the
> >> @@ -6571,8 +6583,8 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
> >>    [EXIT_REASON_EPT_VIOLATION]          = handle_ept_violation,
> >>    [EXIT_REASON_EPT_MISCONFIG]           = handle_ept_misconfig,
> >>    [EXIT_REASON_PAUSE_INSTRUCTION]       = handle_pause,
> >> -    [EXIT_REASON_MWAIT_INSTRUCTION]          = handle_invalid_op,
> >> -    [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_invalid_op,
> >> +    [EXIT_REASON_MWAIT_INSTRUCTION]          = handle_mwait,
> >> +    [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
> >>    [EXIT_REASON_INVEPT]                  = handle_invept,
> >> };
> >> 
> >> -- 
> >> 1.9.0
> >> 

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Mon, Jun 02, 2014 at 03:25:30PM -0400, Gabriel L. Somlo wrote:
> On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
> > Treat monitor and mwait instructions as nop, which is architecturally
> > correct (but inefficient) behavior. We do this to prevent misbehaving
> > guests (e.g. OS X <= 10.7) from crashing after they fail to check for
> > monitor/mwait availability via cpuid.
> > 
> > Since mwait-based idle loops relying on these nop-emulated instructions
> > would keep the host CPU pegged at 100%, do NOT advertise their presence
> > via cpuid, to prevent compliant guests from using them inadvertently.
> > 
> > Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> > ---
> > 
> > New in v2: remove invalid_op handler functions which were only used to
> >            handle exits caused by monitor and mwait
> > 
> > On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
> > > On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
> > > >If we really want to be paranoid and worry about guests
> > > >that use this strange way to trigger invalid opcode,
> > > >we can make it possible for userspace to enable/disable
> > > >this hack, and teach qemu to set it.
> > > >
> > > >That would make it even safer than it was.
> > > >
> > > >Not sure it's worth it, just a thought.
> > > 
> > > Since we don't trap on non-exposed other instructions (new SSE and
> > > whatdoiknow) I don't think it's really bad to just expose
> > > MONITOR/MWAIT as nops.
> 
> Would it make sense to make this a module parameter,
> (e.g., "int emulate_mwait") ?
> 
> Default would be 0 (no emulation). 1 would mean "emulate as nop", and
> if anyone ever figures out how to do proper page-locking based
> emulation we could use 2 to enable that, etc. ?

If it's a module parameter, default should be one that's
safe for all guests, that would be 1 in the list above.

> Not sure we'd want qemu to enable/disable it automatically, though...
> 
> What do you all think ?
> 
> Thanks,
> --Gabriel
> 
> > 
> > So AFAICT, linux prefers to use mwait for idling if cpuid tells it that
> > it's available. If we keep telling everyone that we do NOT have monitor
> > and mwait available, compliant guests will never end up using them, and
> > this hack would remain completely invisible to them, which is good
> > (better to use hlt-based idle loops when you're a vm guest, that would
> > actually allow the host to relax while you're halted :)
> > 
> > So the only time anyone would be able to tell we have this hack would be
> > when they're about to receive an invalid opcode for using monitor/mwait
> > in violation of what CPUID (would have) told them. That's what happens
> > to OS X prior to 10.8, which is when I'm hypothesizing the Apple devs
> > begain to seriously think about their OS running as a vm guest (on fusion
> > and parallels)...
> > 
> > Instead of killing the misbehaving guest with an invalid opcode, we'd
> > allow them to peg the host CPU with their monitor == mwait == nop idle
> > loop instead, which, at least on OS X, should be tolerable long enough
> > to run 'rm -rf System/Library/Extensions/AppleIntelCPUPowerManagement.kext'
> > and reboot the guest, after which things would settle down by reverting
> > the guest to a hlt-based idle loop.
> > 
> > The only reason I can think of to add functionality for enabling/disabling
> > this hack would be to protect against a malicious guest which would use
> > mwait *on purpose* to peg the host CPU. But a malicious guest could just
> > run "for(;;);" in ring 0 and accomplish the same goal, so we wouldn't
> > really gain anything in exchange for the added complexity...
> > 
> > Thanks,
> >   Gabriel
> > 
> >  arch/x86/kvm/cpuid.c |  2 ++
> >  arch/x86/kvm/svm.c   | 28 ++++++++++++++++++++--------
> >  arch/x86/kvm/vmx.c   | 20 ++++++++++++++++----
> >  3 files changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index f47a104..d094fc6 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -283,6 +283,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >  		0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
> >  	/* cpuid 1.ecx */
> >  	const u32 kvm_supported_word4_x86_features =
> > +		/* NOTE: MONITOR (and MWAIT) are emulated as NOP,
> > +		 * but *not* advertised to guests via CPUID ! */
> >  		F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
> >  		0 /* DS-CPL, VMX, SMX, EST */ |
> >  		0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 7f4f9c2..0b7d58d 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -2770,12 +2770,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
> >  	return 1;
> >  }
> >  
> > -static int invalid_op_interception(struct vcpu_svm *svm)
> > -{
> > -	kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> > -	return 1;
> > -}
> > -
> >  static int task_switch_interception(struct vcpu_svm *svm)
> >  {
> >  	u16 tss_selector;
> > @@ -3287,6 +3281,24 @@ static int pause_interception(struct vcpu_svm *svm)
> >  	return 1;
> >  }
> >  
> > +static int nop_interception(struct vcpu_svm *svm)
> > +{
> > +	skip_emulated_instruction(&(svm->vcpu));
> > +	return 1;
> > +}
> > +
> > +static int monitor_interception(struct vcpu_svm *svm)
> > +{
> > +	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> > +	return nop_interception(svm);
> > +}
> > +
> > +static int mwait_interception(struct vcpu_svm *svm)
> > +{
> > +	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> > +	return nop_interception(svm);
> > +}
> > +
> >  static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
> >  	[SVM_EXIT_READ_CR0]			= cr_interception,
> >  	[SVM_EXIT_READ_CR3]			= cr_interception,
> > @@ -3344,8 +3356,8 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
> >  	[SVM_EXIT_CLGI]				= clgi_interception,
> >  	[SVM_EXIT_SKINIT]			= skinit_interception,
> >  	[SVM_EXIT_WBINVD]                       = emulate_on_interception,
> > -	[SVM_EXIT_MONITOR]			= invalid_op_interception,
> > -	[SVM_EXIT_MWAIT]			= invalid_op_interception,
> > +	[SVM_EXIT_MONITOR]			= monitor_interception,
> > +	[SVM_EXIT_MWAIT]			= mwait_interception,
> >  	[SVM_EXIT_XSETBV]			= xsetbv_interception,
> >  	[SVM_EXIT_NPF]				= pf_interception,
> >  };
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 33e8c02..3ccbcb1 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -5669,12 +5669,24 @@ static int handle_pause(struct kvm_vcpu *vcpu)
> >  	return 1;
> >  }
> >  
> > -static int handle_invalid_op(struct kvm_vcpu *vcpu)
> > +static int handle_nop(struct kvm_vcpu *vcpu)
> >  {
> > -	kvm_queue_exception(vcpu, UD_VECTOR);
> > +	skip_emulated_instruction(vcpu);
> >  	return 1;
> >  }
> >  
> > +static int handle_mwait(struct kvm_vcpu *vcpu)
> > +{
> > +	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> > +	return handle_nop(vcpu);
> > +}
> > +
> > +static int handle_monitor(struct kvm_vcpu *vcpu)
> > +{
> > +	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> > +	return handle_nop(vcpu);
> > +}
> > +
> >  /*
> >   * To run an L2 guest, we need a vmcs02 based on the L1-specified vmcs12.
> >   * We could reuse a single VMCS for all the L2 guests, but we also want the
> > @@ -6571,8 +6583,8 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
> >  	[EXIT_REASON_EPT_VIOLATION]	      = handle_ept_violation,
> >  	[EXIT_REASON_EPT_MISCONFIG]           = handle_ept_misconfig,
> >  	[EXIT_REASON_PAUSE_INSTRUCTION]       = handle_pause,
> > -	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_invalid_op,
> > -	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_invalid_op,
> > +	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_mwait,
> > +	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
> >  	[EXIT_REASON_INVEPT]                  = handle_invept,
> >  };
> >  
> > -- 
> > 1.9.0
> > 

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Alexander Graf]

> Am 02.06.2014 um 22:20 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> 
>> On Mon, Jun 02, 2014 at 09:48:19PM +0200, Alexander Graf wrote:
>> 
>> 
>>>> Am 02.06.2014 um 21:25 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
>>>> 
>>>> On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
>>>> Treat monitor and mwait instructions as nop, which is architecturally
>>>> correct (but inefficient) behavior. We do this to prevent misbehaving
>>>> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
>>>> monitor/mwait availability via cpuid.
>>>> 
>>>> Since mwait-based idle loops relying on these nop-emulated instructions
>>>> would keep the host CPU pegged at 100%, do NOT advertise their presence
>>>> via cpuid, to prevent compliant guests from using them inadvertently.
>>>> 
>>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
>>>> ---
>>>> 
>>>> New in v2: remove invalid_op handler functions which were only used to
>>>>          handle exits caused by monitor and mwait
>>>> 
>>>>>> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
>>>>>> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
>>>>>> If we really want to be paranoid and worry about guests
>>>>>> that use this strange way to trigger invalid opcode,
>>>>>> we can make it possible for userspace to enable/disable
>>>>>> this hack, and teach qemu to set it.
>>>>>> 
>>>>>> That would make it even safer than it was.
>>>>>> 
>>>>>> Not sure it's worth it, just a thought.
>>>>> 
>>>>> Since we don't trap on non-exposed other instructions (new SSE and
>>>>> whatdoiknow) I don't think it's really bad to just expose
>>>>> MONITOR/MWAIT as nops.
>>> 
>>> Would it make sense to make this a module parameter,
>>> (e.g., "int emulate_mwait") ?
>>> 
>>> Default would be 0 (no emulation). 1 would mean "emulate as nop", and
>>> if anyone ever figures out how to do proper page-locking based
>>> emulation we could use 2 to enable that, etc. ?
>>> 
>>> Not sure we'd want qemu to enable/disable it automatically, though...
>>> 
>>> What do you all think ?
>> 
>> I don't like module parameters - they're system global and there's a good chance you want to run non-osx in parallel ;).
>> 
>> I'd either link this to the cpuid bits or enable it forcefully through ENABLE_CAP per vcpu.
>> 
>> Alex
> 
> Point is that.
> Paolo here thinks it's safe to just make it a NOP unconditionally.
> so module parameter would be there as a debugging tool:
> as a means for users to test with old kvm behaviour if they see breakage.
> Which we don't expect, so no need to waste cycles creating a pretty
> interface for it.

Both interfaces already exist, so where's the problem? I'm fine with making it always nop too though.

Gabriel was asking how to make it switchable - and the only thing I'd nak is a module parameter because it's not useful.


Alex

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Mon, Jun 02, 2014 at 10:35:56PM +0200, Alexander Graf wrote:
> 
> 
> > Am 02.06.2014 um 22:20 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> > 
> >> On Mon, Jun 02, 2014 at 09:48:19PM +0200, Alexander Graf wrote:
> >> 
> >> 
> >>>> Am 02.06.2014 um 21:25 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> >>>> 
> >>>> On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
> >>>> Treat monitor and mwait instructions as nop, which is architecturally
> >>>> correct (but inefficient) behavior. We do this to prevent misbehaving
> >>>> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
> >>>> monitor/mwait availability via cpuid.
> >>>> 
> >>>> Since mwait-based idle loops relying on these nop-emulated instructions
> >>>> would keep the host CPU pegged at 100%, do NOT advertise their presence
> >>>> via cpuid, to prevent compliant guests from using them inadvertently.
> >>>> 
> >>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> >>>> ---
> >>>> 
> >>>> New in v2: remove invalid_op handler functions which were only used to
> >>>>          handle exits caused by monitor and mwait
> >>>> 
> >>>>>> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
> >>>>>> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
> >>>>>> If we really want to be paranoid and worry about guests
> >>>>>> that use this strange way to trigger invalid opcode,
> >>>>>> we can make it possible for userspace to enable/disable
> >>>>>> this hack, and teach qemu to set it.
> >>>>>> 
> >>>>>> That would make it even safer than it was.
> >>>>>> 
> >>>>>> Not sure it's worth it, just a thought.
> >>>>> 
> >>>>> Since we don't trap on non-exposed other instructions (new SSE and
> >>>>> whatdoiknow) I don't think it's really bad to just expose
> >>>>> MONITOR/MWAIT as nops.
> >>> 
> >>> Would it make sense to make this a module parameter,
> >>> (e.g., "int emulate_mwait") ?
> >>> 
> >>> Default would be 0 (no emulation). 1 would mean "emulate as nop", and
> >>> if anyone ever figures out how to do proper page-locking based
> >>> emulation we could use 2 to enable that, etc. ?
> >>> 
> >>> Not sure we'd want qemu to enable/disable it automatically, though...
> >>> 
> >>> What do you all think ?
> >> 
> >> I don't like module parameters - they're system global and there's a good chance you want to run non-osx in parallel ;).
> >> 
> >> I'd either link this to the cpuid bits or enable it forcefully through ENABLE_CAP per vcpu.
> >> 
> >> Alex
> > 
> > Point is that.
> > Paolo here thinks it's safe to just make it a NOP unconditionally.
> > so module parameter would be there as a debugging tool:
> > as a means for users to test with old kvm behaviour if they see breakage.
> > Which we don't expect, so no need to waste cycles creating a pretty
> > interface for it.
> 
> Both interfaces already exist, so where's the problem?

Hmm sorry which interfaces for enabling mwait nop emulation exist?

> I'm fine with making it always nop too though.
> 
> Gabriel was asking how to make it switchable - and the only thing I'd nak is a module parameter because it's not useful.
> 
> 
> Alex
> 

-- 
MST

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Alexander Graf]

> Am 02.06.2014 um 22:41 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> 
>> On Mon, Jun 02, 2014 at 10:35:56PM +0200, Alexander Graf wrote:
>> 
>> 
>>>> Am 02.06.2014 um 22:20 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>>>> 
>>>> On Mon, Jun 02, 2014 at 09:48:19PM +0200, Alexander Graf wrote:
>>>> 
>>>> 
>>>>>> Am 02.06.2014 um 21:25 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
>>>>>> 
>>>>>> On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
>>>>>> Treat monitor and mwait instructions as nop, which is architecturally
>>>>>> correct (but inefficient) behavior. We do this to prevent misbehaving
>>>>>> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
>>>>>> monitor/mwait availability via cpuid.
>>>>>> 
>>>>>> Since mwait-based idle loops relying on these nop-emulated instructions
>>>>>> would keep the host CPU pegged at 100%, do NOT advertise their presence
>>>>>> via cpuid, to prevent compliant guests from using them inadvertently.
>>>>>> 
>>>>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
>>>>>> ---
>>>>>> 
>>>>>> New in v2: remove invalid_op handler functions which were only used to
>>>>>>         handle exits caused by monitor and mwait
>>>>>> 
>>>>>>>> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
>>>>>>>> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
>>>>>>>> If we really want to be paranoid and worry about guests
>>>>>>>> that use this strange way to trigger invalid opcode,
>>>>>>>> we can make it possible for userspace to enable/disable
>>>>>>>> this hack, and teach qemu to set it.
>>>>>>>> 
>>>>>>>> That would make it even safer than it was.
>>>>>>>> 
>>>>>>>> Not sure it's worth it, just a thought.
>>>>>>> 
>>>>>>> Since we don't trap on non-exposed other instructions (new SSE and
>>>>>>> whatdoiknow) I don't think it's really bad to just expose
>>>>>>> MONITOR/MWAIT as nops.
>>>>> 
>>>>> Would it make sense to make this a module parameter,
>>>>> (e.g., "int emulate_mwait") ?
>>>>> 
>>>>> Default would be 0 (no emulation). 1 would mean "emulate as nop", and
>>>>> if anyone ever figures out how to do proper page-locking based
>>>>> emulation we could use 2 to enable that, etc. ?
>>>>> 
>>>>> Not sure we'd want qemu to enable/disable it automatically, though...
>>>>> 
>>>>> What do you all think ?
>>>> 
>>>> I don't like module parameters - they're system global and there's a good chance you want to run non-osx in parallel ;).
>>>> 
>>>> I'd either link this to the cpuid bits or enable it forcefully through ENABLE_CAP per vcpu.
>>>> 
>>>> Alex
>>> 
>>> Point is that.
>>> Paolo here thinks it's safe to just make it a NOP unconditionally.
>>> so module parameter would be there as a debugging tool:
>>> as a means for users to test with old kvm behaviour if they see breakage.
>>> Which we don't expect, so no need to waste cycles creating a pretty
>>> interface for it.
>> 
>> Both interfaces already exist, so where's the problem?
> 
> Hmm sorry which interfaces for enabling mwait nop emulation exist?

User space can force cpuid bits that kvm doesn't return as supported, so we do have a negative-by-default switch.

We also have an ENABLE_CAP ioctl. Enabling the monitor/mwait nop ability explicitly by that is a 5 line patch.

Either way is very flexible and not system wide.


Alex

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Mon, Jun 02, 2014 at 11:01:07PM +0200, Alexander Graf wrote:
> 
> 
> > Am 02.06.2014 um 22:41 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> > 
> >> On Mon, Jun 02, 2014 at 10:35:56PM +0200, Alexander Graf wrote:
> >> 
> >> 
> >>>> Am 02.06.2014 um 22:20 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> >>>> 
> >>>> On Mon, Jun 02, 2014 at 09:48:19PM +0200, Alexander Graf wrote:
> >>>> 
> >>>> 
> >>>>>> Am 02.06.2014 um 21:25 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> >>>>>> 
> >>>>>> On Wed, May 07, 2014 at 04:52:13PM -0400, Gabriel L. Somlo wrote:
> >>>>>> Treat monitor and mwait instructions as nop, which is architecturally
> >>>>>> correct (but inefficient) behavior. We do this to prevent misbehaving
> >>>>>> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
> >>>>>> monitor/mwait availability via cpuid.
> >>>>>> 
> >>>>>> Since mwait-based idle loops relying on these nop-emulated instructions
> >>>>>> would keep the host CPU pegged at 100%, do NOT advertise their presence
> >>>>>> via cpuid, to prevent compliant guests from using them inadvertently.
> >>>>>> 
> >>>>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> >>>>>> ---
> >>>>>> 
> >>>>>> New in v2: remove invalid_op handler functions which were only used to
> >>>>>>         handle exits caused by monitor and mwait
> >>>>>> 
> >>>>>>>> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
> >>>>>>>> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
> >>>>>>>> If we really want to be paranoid and worry about guests
> >>>>>>>> that use this strange way to trigger invalid opcode,
> >>>>>>>> we can make it possible for userspace to enable/disable
> >>>>>>>> this hack, and teach qemu to set it.
> >>>>>>>> 
> >>>>>>>> That would make it even safer than it was.
> >>>>>>>> 
> >>>>>>>> Not sure it's worth it, just a thought.
> >>>>>>> 
> >>>>>>> Since we don't trap on non-exposed other instructions (new SSE and
> >>>>>>> whatdoiknow) I don't think it's really bad to just expose
> >>>>>>> MONITOR/MWAIT as nops.
> >>>>> 
> >>>>> Would it make sense to make this a module parameter,
> >>>>> (e.g., "int emulate_mwait") ?
> >>>>> 
> >>>>> Default would be 0 (no emulation). 1 would mean "emulate as nop", and
> >>>>> if anyone ever figures out how to do proper page-locking based
> >>>>> emulation we could use 2 to enable that, etc. ?
> >>>>> 
> >>>>> Not sure we'd want qemu to enable/disable it automatically, though...
> >>>>> 
> >>>>> What do you all think ?
> >>>> 
> >>>> I don't like module parameters - they're system global and there's a good chance you want to run non-osx in parallel ;).
> >>>> 
> >>>> I'd either link this to the cpuid bits or enable it forcefully through ENABLE_CAP per vcpu.
> >>>> 
> >>>> Alex
> >>> 
> >>> Point is that.
> >>> Paolo here thinks it's safe to just make it a NOP unconditionally.
> >>> so module parameter would be there as a debugging tool:
> >>> as a means for users to test with old kvm behaviour if they see breakage.
> >>> Which we don't expect, so no need to waste cycles creating a pretty
> >>> interface for it.
> >> 
> >> Both interfaces already exist, so where's the problem?
> > 
> > Hmm sorry which interfaces for enabling mwait nop emulation exist?
> 
> User space can force cpuid bits that kvm doesn't return as supported, so we do have a negative-by-default switch.
> 
> We also have an ENABLE_CAP ioctl. Enabling the monitor/mwait nop ability explicitly by that is a 5 line patch.
> 
> Either way is very flexible and not system wide.

W.r.t. monitor/mwait, a guest can do one of the following:

1. Never check CPUID, and never use monitor/mwait
	- This is great, we don't have to do anything about these

2. Check CPUID for mwait, use it to idle in preference over hlt
	- Linux, Windows, and Mavericks (10.9) do this
	- we never want to have CPUID say "yes" to these, since
	  monitor/mwait support will be clunky in the best case,
	  and hlt is overwhelmingly preferable! [*]

3. Never check CPUID, use monitor/mwait with abandon
	- OS X 10.6 .. 10.8 does this
	- emulating monitor/mwait here allows us to boot the guest
	  and use it, and perform sysadmin surgery to force a hlt
	  based idle

4. Check CPUID, panic if unavailable
	- OS X 10.5 did this, IIRC.
	- whether I can do kext surgery and get it to stop checking
	  CPUID *in addition to* falling back to hlt-based idle is
	  TBD.
	- emulating monitor/mwait allows us to boot this type of
	  guest, BUT WE ALSO HAVE TO ADVERTISE IT VIA CPUID !!!

I like telling qemu on the command line "do monitor = mwait = nop;
for this guest only", and having qemu pass that on to KVM for only the
VCPUs associated with this guest, optionally, for cases 3 and 4 only
(everyone else gets the invalid opcode fault behavior as before).


[*] I think we've been over this a few times already, but here's a
    quick recap:

	- monitor == mwait == NOP is correct (albeitwasteful) behavior
		- mwait MUST expect and deal with spurious wakeups
		  (per the Intel manual)
		- mwait == nop is an INSTANT spurious wakeup (hence
		  works OK with any correctly written program) !
		- monitor == nop won't "arm" anything, but that
		  doesn't matter if mwait always immediately wakes up !

		- this pegs the host CPU to 100%, so MUCH worse than
		  hlt, shouldn't do it unless we ABSOLUTELY HAVE TO !!!

	- guest-mode mwait should NEVER be allowed to stop the host CPU
	  (and, according to the Intel manual, it's HARD to try and
	  make it do so, which I think is on purpose !)

	- instead, guest-mode mwait should map to a host-side
	  condition-wait (where a write to a monitor-ed area
	  acts as condition-signal).

		- the most likely way to implement something like that
		  would be to write-protect pages and handle write faults
		- and I never got it working *properly* (but I'm a n00b,
		  so that ain't saying much :)
		- but the granularity would be all wrong compared to any
		  real CPU (1 page >> typical monitored area size)
		- but I still don't see it being any better than
		  hlt-based idle, even if we *did* get it to work correctly !!!



I'll look into ENABLE_CAP, and how to expose that on the qemu command
line (I think I might need both methods mentioned by Alex in tandem,
but I'll have to study existing examples before I can say anything
useful here). Any extra words of wisdom on how to do that, what
examples might be best to study for inspiration, etc, much appreciated !!!

Thanks,
--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Paolo Bonzini]

Il 02/06/2014 21:25, Gabriel L. Somlo ha scritto:
> Would it make sense to make this a module parameter,
> (e.g., "int emulate_mwait") ?
>
> Default would be 0 (no emulation). 1 would mean "emulate as nop", and
> if anyone ever figures out how to do proper page-locking based
> emulation we could use 2 to enable that, etc. ?
>
> Not sure we'd want qemu to enable/disable it automatically, though...
>
> What do you all think ?

I think it's fine as it is now. :)

Paolo

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Tue, Jun 03, 2014 at 11:17:48AM +0200, Paolo Bonzini wrote:
> 
> I think it's fine as it is now. :)

On Mon, Jun 02, 2014 at 09:55:18PM -0400, Gabriel L. Somlo wrote:
> 
> W.r.t. monitor/mwait, a guest can do one of the following:
> 
> 1. Never check CPUID, and never use monitor/mwait
> 	- This is great, we don't have to do anything about these
> 
> 2. Check CPUID for mwait, use it to idle in preference over hlt
> 	- Linux, Windows, and Mavericks (10.9) do this
> 	- we never want to have CPUID say "yes" to these, since
> 	  monitor/mwait support will be clunky in the best case,
> 	  and hlt is overwhelmingly preferable! [*]
> 
> 3. Never check CPUID, use monitor/mwait with abandon
> 	- OS X 10.6 .. 10.8 does this
> 	- emulating monitor/mwait here allows us to boot the guest
> 	  and use it, and perform sysadmin surgery to force a hlt
> 	  based idle
> 
> 4. Check CPUID, panic if unavailable
> 	- OS X 10.5 did this, IIRC.
> 	- whether I can do kext surgery and get it to stop checking
> 	  CPUID *in addition to* falling back to hlt-based idle is
> 	  TBD.
> 	- emulating monitor/mwait allows us to boot this type of
> 	  guest, BUT WE ALSO HAVE TO ADVERTISE IT VIA CPUID !!!

As it is right now, #4 is not being addressed (and we can't just
advertise mwait via cpuid, or we'd be screwing up #2).

I also feel a bit weird about the "undocumented feature" aspect
of NOT generating an invalid opcode for something that *should*
be an invalid opcode according to the feature set advertised via
cpuid...

So if there's a way to make it so we can tell QEMU/KVM to
"--enable-mwait" on a per-guest basis, I think that'd be better
than an always-on "undocumented" behavior...

But then again, I'm most likely missing something about the big
picture... :)

Thanks much,
--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Alexander Graf]

On 06/03/2014 04:21 PM, Gabriel L. Somlo wrote:
> On Tue, Jun 03, 2014 at 11:17:48AM +0200, Paolo Bonzini wrote:
>> I think it's fine as it is now. :)
> On Mon, Jun 02, 2014 at 09:55:18PM -0400, Gabriel L. Somlo wrote:
>> W.r.t. monitor/mwait, a guest can do one of the following:
>>
>> 1. Never check CPUID, and never use monitor/mwait
>> 	- This is great, we don't have to do anything about these
>>
>> 2. Check CPUID for mwait, use it to idle in preference over hlt
>> 	- Linux, Windows, and Mavericks (10.9) do this
>> 	- we never want to have CPUID say "yes" to these, since
>> 	  monitor/mwait support will be clunky in the best case,
>> 	  and hlt is overwhelmingly preferable! [*]
>>
>> 3. Never check CPUID, use monitor/mwait with abandon
>> 	- OS X 10.6 .. 10.8 does this
>> 	- emulating monitor/mwait here allows us to boot the guest
>> 	  and use it, and perform sysadmin surgery to force a hlt
>> 	  based idle
>>
>> 4. Check CPUID, panic if unavailable
>> 	- OS X 10.5 did this, IIRC.
>> 	- whether I can do kext surgery and get it to stop checking
>> 	  CPUID *in addition to* falling back to hlt-based idle is
>> 	  TBD.
>> 	- emulating monitor/mwait allows us to boot this type of
>> 	  guest, BUT WE ALSO HAVE TO ADVERTISE IT VIA CPUID !!!
> As it is right now, #4 is not being addressed (and we can't just
> advertise mwait via cpuid, or we'd be screwing up #2).

I think we should be able to handle #4 by doing -cpu core2duo,+monitor 
on the QEMU command line which should override the cpuid bits that the 
kernel tells us.


Alex

>
> I also feel a bit weird about the "undocumented feature" aspect
> of NOT generating an invalid opcode for something that *should*
> be an invalid opcode according to the feature set advertised via
> cpuid...
>
> So if there's a way to make it so we can tell QEMU/KVM to
> "--enable-mwait" on a per-guest basis, I think that'd be better
> than an always-on "undocumented" behavior...
>
> But then again, I'm most likely missing something about the big
> picture... :)
>
> Thanks much,
> --Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Tue, Jun 03, 2014 at 05:37:08PM +0200, Alexander Graf wrote:
> On 06/03/2014 04:21 PM, Gabriel L. Somlo wrote:
> >On Tue, Jun 03, 2014 at 11:17:48AM +0200, Paolo Bonzini wrote:
> >>I think it's fine as it is now. :)
> >On Mon, Jun 02, 2014 at 09:55:18PM -0400, Gabriel L. Somlo wrote:
> >>W.r.t. monitor/mwait, a guest can do one of the following:
> >>
> >>1. Never check CPUID, and never use monitor/mwait
> >>	- This is great, we don't have to do anything about these
> >>
> >>2. Check CPUID for mwait, use it to idle in preference over hlt
> >>	- Linux, Windows, and Mavericks (10.9) do this
> >>	- we never want to have CPUID say "yes" to these, since
> >>	  monitor/mwait support will be clunky in the best case,
> >>	  and hlt is overwhelmingly preferable! [*]
> >>
> >>3. Never check CPUID, use monitor/mwait with abandon
> >>	- OS X 10.6 .. 10.8 does this
> >>	- emulating monitor/mwait here allows us to boot the guest
> >>	  and use it, and perform sysadmin surgery to force a hlt
> >>	  based idle
> >>
> >>4. Check CPUID, panic if unavailable
> >>	- OS X 10.5 did this, IIRC.
> >>	- whether I can do kext surgery and get it to stop checking
> >>	  CPUID *in addition to* falling back to hlt-based idle is
> >>	  TBD.
> >>	- emulating monitor/mwait allows us to boot this type of
> >>	  guest, BUT WE ALSO HAVE TO ADVERTISE IT VIA CPUID !!!
> >As it is right now, #4 is not being addressed (and we can't just
> >advertise mwait via cpuid, or we'd be screwing up #2).
> 
> I think we should be able to handle #4 by doing -cpu core2duo,+monitor on
> the QEMU command line which should override the cpuid bits that the kernel
> tells us.

Using that (with kvm patched to do monitor = mwait = nop, but without
advertising it in cpuid), I still get a "Monitor feature not present"
panic with the 10.5.6 Leopard install DVD. Patching kvm to add F(MWAIT)
to kvm_supported_word4_x86_features gets me past that panic. Did you
mean "-cpu core2duo,+monitor" should work right now, or that we'd have
to add a quick patch to QEMU to make it happen ? (it doesn't complain
when I do that now, just doesn't work :)

Thx,
--Gabriel

> 
> 
> Alex
> 
> >
> >I also feel a bit weird about the "undocumented feature" aspect
> >of NOT generating an invalid opcode for something that *should*
> >be an invalid opcode according to the feature set advertised via
> >cpuid...
> >
> >So if there's a way to make it so we can tell QEMU/KVM to
> >"--enable-mwait" on a per-guest basis, I think that'd be better
> >than an always-on "undocumented" behavior...
> >
> >But then again, I'm most likely missing something about the big
> >picture... :)
> >
> >Thanks much,
> >--Gabriel
> 

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

Paolo,

I noticed the monitor=mwait=nop patch is making its way upstream, so
thanks !

I'm still interested in following up with something that would enable
this behavior only conditionally (e.g. following an ioctl call from
userspace to enable it only for the (set of) vcpu(s) belonging to one
guest VM at a time), which should then also include advertising the
feature in CPUID.

I grep-ed through the kvm sources for KVM_CAP for some inspiration,
and it looks more like KVM_CAP_* is a way to tell userspace what the
kernel supports, but nothing I saw showed me an example of a "tunable"
feature that userspace may ask to be turned on or off (e.g per-vcpu).

Is there something like that I could use as an example ?

Obviously, if you really like the current behavior better you can
always reject whatever patch I'll come up with, but I'd like to at
least try and see what it would look like :)

Thanks,
--Gabriel

On Tue, Jun 03, 2014 at 11:17:48AM +0200, Paolo Bonzini wrote:
> Il 02/06/2014 21:25, Gabriel L. Somlo ha scritto:
> >Would it make sense to make this a module parameter,
> >(e.g., "int emulate_mwait") ?
> >
> >Default would be 0 (no emulation). 1 would mean "emulate as nop", and
> >if anyone ever figures out how to do proper page-locking based
> >emulation we could use 2 to enable that, etc. ?
> >
> >Not sure we'd want qemu to enable/disable it automatically, though...
> >
> >What do you all think ?
> 
> I think it's fine as it is now. :)
> 
> Paolo

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Alexander Graf]

On 04.06.14 16:39, Gabriel L. Somlo wrote:
> Paolo,
>
> I noticed the monitor=mwait=nop patch is making its way upstream, so
> thanks !
>
> I'm still interested in following up with something that would enable
> this behavior only conditionally (e.g. following an ioctl call from
> userspace to enable it only for the (set of) vcpu(s) belonging to one
> guest VM at a time), which should then also include advertising the
> feature in CPUID.
>
> I grep-ed through the kvm sources for KVM_CAP for some inspiration,
> and it looks more like KVM_CAP_* is a way to tell userspace what the
> kernel supports, but nothing I saw showed me an example of a "tunable"
> feature that userspace may ask to be turned on or off (e.g per-vcpu).
>
> Is there something like that I could use as an example ?

Sure, we use it all over the place on PPC :).

> Obviously, if you really like the current behavior better you can
> always reject whatever patch I'll come up with, but I'd like to at
> least try and see what it would look like :)

I think it's perfectly fine to leave mwait always implemented as NOP - 
it's valid behavior.

As for the CPUID exposure, that should be a pure QEMU thing. If 
overriding CPUID bits the kernel mask tells us doesn't work today, we 
should just make it possible :).

Eventually I really think that -cpu foo,+mwait,+monitor or whatever the 
bits are should override any safety net that KVM gives us on features it 
thinks are safe to use.


Alex

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Wed, Jun 04, 2014 at 04:44:13PM +0200, Alexander Graf wrote:
> 
> On 04.06.14 16:39, Gabriel L. Somlo wrote:
> >Paolo,
> >
> >I noticed the monitor=mwait=nop patch is making its way upstream, so
> >thanks !
> >
> >I'm still interested in following up with something that would enable
> >this behavior only conditionally (e.g. following an ioctl call from
> >userspace to enable it only for the (set of) vcpu(s) belonging to one
> >guest VM at a time), which should then also include advertising the
> >feature in CPUID.
> >
> >I grep-ed through the kvm sources for KVM_CAP for some inspiration,
> >and it looks more like KVM_CAP_* is a way to tell userspace what the
> >kernel supports, but nothing I saw showed me an example of a "tunable"
> >feature that userspace may ask to be turned on or off (e.g per-vcpu).
> >
> >Is there something like that I could use as an example ?
> 
> Sure, we use it all over the place on PPC :).

Allright, I'll grep harder, then :)

> >Obviously, if you really like the current behavior better you can
> >always reject whatever patch I'll come up with, but I'd like to at
> >least try and see what it would look like :)
> 
> I think it's perfectly fine to leave mwait always implemented as NOP - it's
> valid behavior.

NOP is valid MWAIT behavior, *unless* MWAIT should generate an invalid
opcode (i.e., if CPUID says mwait not supported). In that respect,
we're cheating only to hook up guests which misbehave. I'd feel less
"dirty" if I could explicitly tell KVM "ok, just this once is OK, but
don't make a habit of it" :)

> As for the CPUID exposure, that should be a pure QEMU thing. If overriding
> CPUID bits the kernel mask tells us doesn't work today, we should just make
> it possible :).
> 
> Eventually I really think that -cpu foo,+mwait,+monitor or whatever the bits
> are should override any safety net that KVM gives us on features it thinks
> are safe to use.

I need to look at the qemu source, doing what you said
(+monitor,+mwait,+whatever) right now "works", doesn't generate an error,
but silently ignores you if it's not implemented. So I'd actually have to
generate a patch to make something happen when they're present on the
command line.

The part I'm unsure about is "how bad is it to cheat the way we do right
now", vs. "how much is it worth to be pedantic and require explicitly
enabling things, in both qemu and kvm"... I feel like I don't know
enough to 1. have a strong opinion either way, and 2. have my opinion
be *right* :) Which is why I won't let it go already (and thanks for
all your patience, BTW) :)

Thanks,
--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Alexander Graf]

On 04.06.14 17:05, Gabriel L. Somlo wrote:
> On Wed, Jun 04, 2014 at 04:44:13PM +0200, Alexander Graf wrote:
>> On 04.06.14 16:39, Gabriel L. Somlo wrote:
>>> Paolo,
>>>
>>> I noticed the monitor=mwait=nop patch is making its way upstream, so
>>> thanks !
>>>
>>> I'm still interested in following up with something that would enable
>>> this behavior only conditionally (e.g. following an ioctl call from
>>> userspace to enable it only for the (set of) vcpu(s) belonging to one
>>> guest VM at a time), which should then also include advertising the
>>> feature in CPUID.
>>>
>>> I grep-ed through the kvm sources for KVM_CAP for some inspiration,
>>> and it looks more like KVM_CAP_* is a way to tell userspace what the
>>> kernel supports, but nothing I saw showed me an example of a "tunable"
>>> feature that userspace may ask to be turned on or off (e.g per-vcpu).
>>>
>>> Is there something like that I could use as an example ?
>> Sure, we use it all over the place on PPC :).
> Allright, I'll grep harder, then :)
>
>>> Obviously, if you really like the current behavior better you can
>>> always reject whatever patch I'll come up with, but I'd like to at
>>> least try and see what it would look like :)
>> I think it's perfectly fine to leave mwait always implemented as NOP - it's
>> valid behavior.
> NOP is valid MWAIT behavior, *unless* MWAIT should generate an invalid
> opcode (i.e., if CPUID says mwait not supported). In that respect,
> we're cheating only to hook up guests which misbehave. I'd feel less
> "dirty" if I could explicitly tell KVM "ok, just this once is OK, but
> don't make a habit of it" :)

We don't limit instructions the guest can execute properly anyway. If 
CPUID doesn't expose AVX, but the host CPU supports AVX, the guest can 
still call AVX instructions.

So I think we're safe to always handle MWAIT :).

>
>> As for the CPUID exposure, that should be a pure QEMU thing. If overriding
>> CPUID bits the kernel mask tells us doesn't work today, we should just make
>> it possible :).
>>
>> Eventually I really think that -cpu foo,+mwait,+monitor or whatever the bits
>> are should override any safety net that KVM gives us on features it thinks
>> are safe to use.
> I need to look at the qemu source, doing what you said
> (+monitor,+mwait,+whatever) right now "works", doesn't generate an error,
> but silently ignores you if it's not implemented. So I'd actually have to
> generate a patch to make something happen when they're present on the
> command line.
>
> The part I'm unsure about is "how bad is it to cheat the way we do right
> now", vs. "how much is it worth to be pedantic and require explicitly
> enabling things, in both qemu and kvm"... I feel like I don't know
> enough to 1. have a strong opinion either way, and 2. have my opinion
> be *right* :) Which is why I won't let it go already (and thanks for
> all your patience, BTW) :)

I think it's sane behavior to not expose the MWAIT capability in the 
default CPUID mask (which comes from KVM) unless we can actually emulate 
it properly ;).

However, I think it's very important to be able to force CPUID bits to 
on from QEMU even when KVM says it doesn't support them. I actually 
thought we could do that already, but that code got refactored a number 
of times over the years, so maybe that ability got lost.

Basically KVM gives QEMU 2 ioctls:

   * get list of KVM supported CPUIDs
   * set guest exposed CPUIDs

Whether QEMU wants to only set CPUID bits that the kernel actually 
supports is up to its own implementation. Usually the "enforce" option 
is there to guarantee that all CPUID bits are actually supported. 
Apparently all unsupported bits just get dropped silently today. IMHO 
they shouldn't if they were specified through -cpu ...,+feature.


Alex

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Paolo Bonzini]

Il 04/06/2014 16:44, Alexander Graf ha scritto:
>
>
>> Obviously, if you really like the current behavior better you can
>> always reject whatever patch I'll come up with, but I'd like to at
>> least try and see what it would look like :)
>
> I think it's perfectly fine to leave mwait always implemented as NOP -
> it's valid behavior.
>
> As for the CPUID exposure, that should be a pure QEMU thing. If
> overriding CPUID bits the kernel mask tells us doesn't work today, we
> should just make it possible :).

That should be the purpose of KVM_GET_EMULATED_CPUID, so MWAIT could be 
added in __do_cpuid_ent_emulated.  However, the corresponding QEMU 
patches were never included.  Borislav, can you refresh them?

Paolo

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Wed, Jun 04, 2014 at 05:09:49PM +0200, Alexander Graf wrote:
> >>>
> >>>I grep-ed through the kvm sources for KVM_CAP for some inspiration,
> >>>and it looks more like KVM_CAP_* is a way to tell userspace what the
> >>>kernel supports, but nothing I saw showed me an example of a "tunable"
> >>>feature that userspace may ask to be turned on or off (e.g per-vcpu).
> >>>
> >>>Is there something like that I could use as an example ?
> >>Sure, we use it all over the place on PPC :).
> >Allright, I'll grep harder, then :)

Aah, I think I found it: KVM_ENABLE_CAP, currently only on ppc and s390.
I'd have to port this over to x86 before I could use it to enable mwait
on demand.

Would that be useful/desirable for any other use cases ?

> >>I think it's perfectly fine to leave mwait always implemented as NOP - it's
> >>valid behavior.
> >NOP is valid MWAIT behavior, *unless* MWAIT should generate an invalid
> >opcode (i.e., if CPUID says mwait not supported). In that respect,
> >we're cheating only to hook up guests which misbehave. I'd feel less
> >"dirty" if I could explicitly tell KVM "ok, just this once is OK, but
> >don't make a habit of it" :)
> 
> We don't limit instructions the guest can execute properly anyway. If CPUID
> doesn't expose AVX, but the host CPU supports AVX, the guest can still call
> AVX instructions.
> 
> So I think we're safe to always handle MWAIT :).
> 
> >
> >>As for the CPUID exposure, that should be a pure QEMU thing. If overriding
> >>CPUID bits the kernel mask tells us doesn't work today, we should just make
> >>it possible :).
> >>
> >>Eventually I really think that -cpu foo,+mwait,+monitor or whatever the bits
> >>are should override any safety net that KVM gives us on features it thinks
> >>are safe to use.
> >I need to look at the qemu source, doing what you said
> >(+monitor,+mwait,+whatever) right now "works", doesn't generate an error,
> >but silently ignores you if it's not implemented. So I'd actually have to
> >generate a patch to make something happen when they're present on the
> >command line.
> >
> >The part I'm unsure about is "how bad is it to cheat the way we do right
> >now", vs. "how much is it worth to be pedantic and require explicitly
> >enabling things, in both qemu and kvm"... I feel like I don't know
> >enough to 1. have a strong opinion either way, and 2. have my opinion
> >be *right* :) Which is why I won't let it go already (and thanks for
> >all your patience, BTW) :)
> 
> I think it's sane behavior to not expose the MWAIT capability in the default
> CPUID mask (which comes from KVM) unless we can actually emulate it properly
> ;).
> 
> However, I think it's very important to be able to force CPUID bits to on
> from QEMU even when KVM says it doesn't support them. I actually thought we
> could do that already, but that code got refactored a number of times over
> the years, so maybe that ability got lost.
> 
> Basically KVM gives QEMU 2 ioctls:
> 
>   * get list of KVM supported CPUIDs
>   * set guest exposed CPUIDs

Ah, so kvm_vcpu_ioctl_set_cpuid() and friends, morally similar to
kvm_vcpu_ioctl_enable_cap() on ppc, except it turns on cpuid flags
instead of entire kvm capabilities.

So we either have

	1 always-on but masked-by-default monitor/mwait as
	  nop, and enable just the cpuid flag on demand via the
	  existing ioctl_enable_cap() mechanism (and I have to
	  check out the qemu parser for cpuid command-line flags),

or

	2 off-by-default monitor/mwait/cpuid-flag, enabled via
	  ioctl_enable_cap(), which would have to first be ported
	  to x86, and would require somewhat more extensive qemu
	  hackery to take advantage of.

I think I sense a "path of least resistance" here, even though IMHO
#2 is still "The Right Thing To Do (TM)"  :) :)

Thanks,
--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Wed, Jun 04, 2014 at 01:07:21PM -0400, Gabriel L. Somlo wrote:
> On Wed, Jun 04, 2014 at 05:09:49PM +0200, Alexander Graf wrote:
> > >>>
> > >>>I grep-ed through the kvm sources for KVM_CAP for some inspiration,
> > >>>and it looks more like KVM_CAP_* is a way to tell userspace what the
> > >>>kernel supports, but nothing I saw showed me an example of a "tunable"
> > >>>feature that userspace may ask to be turned on or off (e.g per-vcpu).
> > >>>
> > >>>Is there something like that I could use as an example ?
> > >>Sure, we use it all over the place on PPC :).
> > >Allright, I'll grep harder, then :)
> 
> Aah, I think I found it: KVM_ENABLE_CAP, currently only on ppc and s390.
> I'd have to port this over to x86 before I could use it to enable mwait
> on demand.
> 
> Would that be useful/desirable for any other use cases ?
> 
> > >>I think it's perfectly fine to leave mwait always implemented as NOP - it's
> > >>valid behavior.
> > >NOP is valid MWAIT behavior, *unless* MWAIT should generate an invalid
> > >opcode (i.e., if CPUID says mwait not supported). In that respect,
> > >we're cheating only to hook up guests which misbehave. I'd feel less
> > >"dirty" if I could explicitly tell KVM "ok, just this once is OK, but
> > >don't make a habit of it" :)
> > 
> > We don't limit instructions the guest can execute properly anyway. If CPUID
> > doesn't expose AVX, but the host CPU supports AVX, the guest can still call
> > AVX instructions.
> > 
> > So I think we're safe to always handle MWAIT :).
> > 
> > >
> > >>As for the CPUID exposure, that should be a pure QEMU thing. If overriding
> > >>CPUID bits the kernel mask tells us doesn't work today, we should just make
> > >>it possible :).
> > >>
> > >>Eventually I really think that -cpu foo,+mwait,+monitor or whatever the bits
> > >>are should override any safety net that KVM gives us on features it thinks
> > >>are safe to use.
> > >I need to look at the qemu source, doing what you said
> > >(+monitor,+mwait,+whatever) right now "works", doesn't generate an error,
> > >but silently ignores you if it's not implemented. So I'd actually have to
> > >generate a patch to make something happen when they're present on the
> > >command line.
> > >
> > >The part I'm unsure about is "how bad is it to cheat the way we do right
> > >now", vs. "how much is it worth to be pedantic and require explicitly
> > >enabling things, in both qemu and kvm"... I feel like I don't know
> > >enough to 1. have a strong opinion either way, and 2. have my opinion
> > >be *right* :) Which is why I won't let it go already (and thanks for
> > >all your patience, BTW) :)
> > 
> > I think it's sane behavior to not expose the MWAIT capability in the default
> > CPUID mask (which comes from KVM) unless we can actually emulate it properly
> > ;).
> > 
> > However, I think it's very important to be able to force CPUID bits to on
> > from QEMU even when KVM says it doesn't support them. I actually thought we
> > could do that already, but that code got refactored a number of times over
> > the years, so maybe that ability got lost.
> > 
> > Basically KVM gives QEMU 2 ioctls:
> > 
> >   * get list of KVM supported CPUIDs
> >   * set guest exposed CPUIDs
> 
> Ah, so kvm_vcpu_ioctl_set_cpuid() and friends, morally similar to
> kvm_vcpu_ioctl_enable_cap() on ppc, except it turns on cpuid flags
> instead of entire kvm capabilities.
> 
> So we either have
> 
> 	1 always-on but masked-by-default monitor/mwait as
> 	  nop, and enable just the cpuid flag on demand via the
> 	  existing ioctl_enable_cap() mechanism (and I have to
> 	  check out the qemu parser for cpuid command-line flags),
> 
> or
> 
> 	2 off-by-default monitor/mwait/cpuid-flag, enabled via
> 	  ioctl_enable_cap(), which would have to first be ported
> 	  to x86, and would require somewhat more extensive qemu
> 	  hackery to take advantage of.
> 
> I think I sense a "path of least resistance" here, even though IMHO
> #2 is still "The Right Thing To Do (TM)"  :) :)
> 
> Thanks,
> --Gabriel

I think it's worng.
We really can't emulate mwait at the moment.
All we manage to do is a work-around for broken guests.

So let's not pretend that we can, just enable nop
unconditionally and be done with it.
Paolo already said it's OK with him, and I'll ack too.

Otherwise you are giving bad information to well-behaved guests,
so e.g. linux will try to use mwait. You don't want this.

The advantage is that if at some point CPUs can
actually support mwait in VMs, at that point
we will enable the CPUID bit, and userspace and guests
will be able to detect that and rely on that bit
to mean "mwait works and is efficient".

-- 
MST

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Wed, Jun 04, 2014 at 06:34:04PM +0200, Paolo Bonzini wrote:
> Il 04/06/2014 16:44, Alexander Graf ha scritto:
> >
> >
> >>Obviously, if you really like the current behavior better you can
> >>always reject whatever patch I'll come up with, but I'd like to at
> >>least try and see what it would look like :)
> >
> >I think it's perfectly fine to leave mwait always implemented as NOP -
> >it's valid behavior.
> >
> >As for the CPUID exposure, that should be a pure QEMU thing. If
> >overriding CPUID bits the kernel mask tells us doesn't work today, we
> >should just make it possible :).
> 
> That should be the purpose of KVM_GET_EMULATED_CPUID, so MWAIT could be
> added in __do_cpuid_ent_emulated.  However, the corresponding QEMU patches
> were never included.  Borislav, can you refresh them?
> 
> Paolo

I don't understand why would we want mwait bit set in CPUID.
The only reason we want the nop is because of broken guests which
don't check CPUID.

-- 
MST

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Nadav Amit]

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


On Jun 4, 2014, at 7:34 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/06/2014 16:44, Alexander Graf ha scritto:
>> 
>> 
>>> Obviously, if you really like the current behavior better you can
>>> always reject whatever patch I'll come up with, but I'd like to at
>>> least try and see what it would look like :)
>> 
>> I think it's perfectly fine to leave mwait always implemented as NOP -
>> it's valid behavior.
>> 
>> As for the CPUID exposure, that should be a pure QEMU thing. If
>> overriding CPUID bits the kernel mask tells us doesn't work today, we
>> should just make it possible :).
> 
> That should be the purpose of KVM_GET_EMULATED_CPUID, so MWAIT could be added in __do_cpuid_ent_emulated.  However, the corresponding QEMU patches were never included.  Borislav, can you refresh them?


Regardless to the whole discussion of what the guest is informed about, I think it might be better to implement mwait and monitor correctly according to the spec and let the instructions to be fully emulated.
Both mwait and monitor may encounter exceptions (#GP, #PF, regardless of #UD), so this behaviour should be correct.
If you want me, I’ll send my version which looks something like:

static int em_monitor(struct x86_emulate_ctxt *ctxt)
{
	int rc;
	struct segmented_address addr;
	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
	u64 rax = reg_read(ctxt, VCPU_REGS_RAX);
	u8 byte;

	rc = check_mwait_supported(ctxt);
	if (rc != X86EMUL_CONTINUE)
		return rc;

	if (ctxt->mode != X86EMUL_MODE_PROT64)
		rcx = (u32)rcx;

	if (rcx != 0)
		return emulate_gp(ctxt, 0);

	addr.seg = seg_override(ctxt);
	addr.ea = ctxt->ad_bytes == 8 ? rax : (u32)rax;

	rc = segmented_read(ctxt, addr, &byte, 1);
	if (rc != X86EMUL_CONTINUE)
		return rc;

	return X86EMUL_CONTINUE;
}

static int em_mwait(struct x86_emulate_ctxt *ctxt)
{
	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
	int rc = check_mwait_supported(ctxt);
	if (rc != X86EMUL_CONTINUE)
		return rc;
	if (ctxt->mode != X86EMUL_MODE_PROT64)
		rcx = (u32)rcx;

	if ((rcx & ~(u64)1) != 0)
		return emulate_gp(ctxt, 0);

	if (rcx & 1) {
		/* Interrupt as break event */
		u32 ebx, ecx, edx, eax;
		eax = 5;
		ecx = 0;
		ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
		if (!(ecx & 1))
			return emulate_gp(ctxt, 0);
	}
	return X86EMUL_CONTINUE;
}


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Wed, Jun 04, 2014 at 10:06:18PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 04, 2014 at 01:07:21PM -0400, Gabriel L. Somlo wrote:
> > Ah, so kvm_vcpu_ioctl_set_cpuid() and friends, morally similar to
> > kvm_vcpu_ioctl_enable_cap() on ppc, except it turns on cpuid flags
> > instead of entire kvm capabilities.
> > 
> > So we either have
> > 
> > 	1 always-on but masked-by-default monitor/mwait as
> > 	  nop, and enable just the cpuid flag on demand via the
> > 	  existing ioctl_enable_cap() mechanism (and I have to
> > 	  check out the qemu parser for cpuid command-line flags),
> > 
> > or
> > 
> > 	2 off-by-default monitor/mwait/cpuid-flag, enabled via
> > 	  ioctl_enable_cap(), which would have to first be ported
> > 	  to x86, and would require somewhat more extensive qemu
> > 	  hackery to take advantage of.
> > 
> > I think I sense a "path of least resistance" here, even though IMHO
> > #2 is still "The Right Thing To Do (TM)"  :) :)
> > 
> > Thanks,
> > --Gabriel
> 
> I think it's worng.
> We really can't emulate mwait at the moment.
> All we manage to do is a work-around for broken guests.
> 
> So let's not pretend that we can, just enable nop
> unconditionally and be done with it.
> Paolo already said it's OK with him, and I'll ack too.
> 
> Otherwise you are giving bad information to well-behaved guests,
> so e.g. linux will try to use mwait. You don't want this.

That's why I suggested having it default to disabled, and only
allowing it to be turned on per VM, explicitly.

> The advantage is that if at some point CPUs can
> actually support mwait in VMs, at that point
> we will enable the CPUID bit, and userspace and guests
> will be able to detect that and rely on that bit
> to mean "mwait works and is efficient".

OK, that makes sense. Thanks for helping me finally get over it ;)

--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Wed, Jun 04, 2014 at 10:08:12PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 04, 2014 at 06:34:04PM +0200, Paolo Bonzini wrote:
> > Il 04/06/2014 16:44, Alexander Graf ha scritto:
> > >
> > >
> > >>Obviously, if you really like the current behavior better you can
> > >>always reject whatever patch I'll come up with, but I'd like to at
> > >>least try and see what it would look like :)
> > >
> > >I think it's perfectly fine to leave mwait always implemented as NOP -
> > >it's valid behavior.
> > >
> > >As for the CPUID exposure, that should be a pure QEMU thing. If
> > >overriding CPUID bits the kernel mask tells us doesn't work today, we
> > >should just make it possible :).
> > 
> > That should be the purpose of KVM_GET_EMULATED_CPUID, so MWAIT could be
> > added in __do_cpuid_ent_emulated.  However, the corresponding QEMU patches
> > were never included.  Borislav, can you refresh them?
> > 
> > Paolo
> 
> I don't understand why would we want mwait bit set in CPUID.
> The only reason we want the nop is because of broken guests which
> don't check CPUID.

E.g., OS X 10.5 *does* check CPUID, and panics if it doesn't find it.
It needs the MONITOR cpuid flag to be on, *and* the actual
instructions to work.

HOWEVER: I really do NOT want us to bend over backwards to support it,
I think having 10.6 and up working is good enough. Besides, there are
other problems we'd run into with 10.5 if we got the mwait situation
taken care of, so even more reason to leave well enough alone.

Thanks,
--Gabriel

PS. Thanks again for everyone's patience while I wrapped my head
around the fine details :)

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Wed, Jun 04, 2014 at 03:24:06PM -0400, Gabriel L. Somlo wrote:
> On Wed, Jun 04, 2014 at 10:06:18PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 04, 2014 at 01:07:21PM -0400, Gabriel L. Somlo wrote:
> > > Ah, so kvm_vcpu_ioctl_set_cpuid() and friends, morally similar to
> > > kvm_vcpu_ioctl_enable_cap() on ppc, except it turns on cpuid flags
> > > instead of entire kvm capabilities.
> > > 
> > > So we either have
> > > 
> > > 	1 always-on but masked-by-default monitor/mwait as
> > > 	  nop, and enable just the cpuid flag on demand via the
> > > 	  existing ioctl_enable_cap() mechanism (and I have to
> > > 	  check out the qemu parser for cpuid command-line flags),
> > > 
> > > or
> > > 
> > > 	2 off-by-default monitor/mwait/cpuid-flag, enabled via
> > > 	  ioctl_enable_cap(), which would have to first be ported
> > > 	  to x86, and would require somewhat more extensive qemu
> > > 	  hackery to take advantage of.
> > > 
> > > I think I sense a "path of least resistance" here, even though IMHO
> > > #2 is still "The Right Thing To Do (TM)"  :) :)
> > > 
> > > Thanks,
> > > --Gabriel
> > 
> > I think it's worng.
> > We really can't emulate mwait at the moment.
> > All we manage to do is a work-around for broken guests.
> > 
> > So let's not pretend that we can, just enable nop
> > unconditionally and be done with it.
> > Paolo already said it's OK with him, and I'll ack too.
> > 
> > Otherwise you are giving bad information to well-behaved guests,
> > so e.g. linux will try to use mwait. You don't want this.
> 
> That's why I suggested having it default to disabled, and only
> allowing it to be turned on per VM, explicitly.

Turn on the bit in CPUID? Why is it useful?
I thought the point of the nop hack is work around
for broken guests.

> > The advantage is that if at some point CPUs can
> > actually support mwait in VMs, at that point
> > we will enable the CPUID bit, and userspace and guests
> > will be able to detect that and rely on that bit
> > to mean "mwait works and is efficient".
> 
> OK, that makes sense. Thanks for helping me finally get over it ;)
> 
> --Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Wed, Jun 04, 2014 at 03:33:38PM -0400, Gabriel L. Somlo wrote:
> On Wed, Jun 04, 2014 at 10:08:12PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 04, 2014 at 06:34:04PM +0200, Paolo Bonzini wrote:
> > > Il 04/06/2014 16:44, Alexander Graf ha scritto:
> > > >
> > > >
> > > >>Obviously, if you really like the current behavior better you can
> > > >>always reject whatever patch I'll come up with, but I'd like to at
> > > >>least try and see what it would look like :)
> > > >
> > > >I think it's perfectly fine to leave mwait always implemented as NOP -
> > > >it's valid behavior.
> > > >
> > > >As for the CPUID exposure, that should be a pure QEMU thing. If
> > > >overriding CPUID bits the kernel mask tells us doesn't work today, we
> > > >should just make it possible :).
> > > 
> > > That should be the purpose of KVM_GET_EMULATED_CPUID, so MWAIT could be
> > > added in __do_cpuid_ent_emulated.  However, the corresponding QEMU patches
> > > were never included.  Borislav, can you refresh them?
> > > 
> > > Paolo
> > 
> > I don't understand why would we want mwait bit set in CPUID.
> > The only reason we want the nop is because of broken guests which
> > don't check CPUID.
> 
> E.g., OS X 10.5 *does* check CPUID, and panics if it doesn't find it.
> It needs the MONITOR cpuid flag to be on, *and* the actual
> instructions to work.

Aha. I didn't realize this. I thought it used mwait without
checking CPUID.

> HOWEVER: I really do NOT want us to bend over backwards to support it,
> I think having 10.6 and up working is good enough. Besides, there are
> other problems we'd run into with 10.5 if we got the mwait situation
> taken care of, so even more reason to leave well enough alone.
> 
> Thanks,
> --Gabriel
> 
> PS. Thanks again for everyone's patience while I wrapped my head
> around the fine details :)

It's up to you of course. If some guests refuse to run without
the CPUID bit, then of course an option to set it might be useful.
Sorry about misunderstanding and making noise.

-- 
MST

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Wed, Jun 04, 2014 at 10:12:39PM +0300, Nadav Amit wrote:
> Regardless to the whole discussion of what the guest is informed about, I think it might be better to implement mwait and monitor correctly according to the spec and let the instructions to be fully emulated.
> Both mwait and monitor may encounter exceptions (#GP, #PF, regardless of #UD), so this behaviour should be correct.
> If you want me, I?ll send my version which looks something like:
> 
> static int em_monitor(struct x86_emulate_ctxt *ctxt)
> {
> 	int rc;
> 	struct segmented_address addr;
> 	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
> 	u64 rax = reg_read(ctxt, VCPU_REGS_RAX);
> 	u8 byte;
> 
> 	rc = check_mwait_supported(ctxt);
> 	if (rc != X86EMUL_CONTINUE)
> 		return rc;
> 
> 	if (ctxt->mode != X86EMUL_MODE_PROT64)
> 		rcx = (u32)rcx;
> 
> 	if (rcx != 0)
> 		return emulate_gp(ctxt, 0);
> 
> 	addr.seg = seg_override(ctxt);
> 	addr.ea = ctxt->ad_bytes == 8 ? rax : (u32)rax;
> 
> 	rc = segmented_read(ctxt, addr, &byte, 1);
> 	if (rc != X86EMUL_CONTINUE)
> 		return rc;
> 
> 	return X86EMUL_CONTINUE;
> }
> 
> static int em_mwait(struct x86_emulate_ctxt *ctxt)
> {
> 	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
> 	int rc = check_mwait_supported(ctxt);
> 	if (rc != X86EMUL_CONTINUE)
> 		return rc;
> 	if (ctxt->mode != X86EMUL_MODE_PROT64)
> 		rcx = (u32)rcx;
> 
> 	if ((rcx & ~(u64)1) != 0)
> 		return emulate_gp(ctxt, 0);
> 
> 	if (rcx & 1) {
> 		/* Interrupt as break event */
> 		u32 ebx, ecx, edx, eax;
> 		eax = 5;
> 		ecx = 0;
> 		ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> 		if (!(ecx & 1))
> 			return emulate_gp(ctxt, 0);
> 	}
> 	return X86EMUL_CONTINUE;
> }

I'd be curious how you're dealing with the "hidden" CPU state which
tells MWAIT to sleep until someone or something writes to the
monitored memory area set up by a corresponding MONITOR instruction.

Thanks,
--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Borislav Petkov]

On Wed, Jun 04, 2014 at 06:34:04PM +0200, Paolo Bonzini wrote:
> That should be the purpose of KVM_GET_EMULATED_CPUID, so MWAIT could be
> added in __do_cpuid_ent_emulated.  However, the corresponding QEMU patches
> were never included.  Borislav, can you refresh them?

/me goes and swaps in all the details from the discussion at that
time.... tries to decipher what the whole story was about...

Well, AFAIR - the operative word being "remember" - Eduardo had a
problem with diffentiating between emulated features and real hw
features and what was being filtered out... here's where the thread
starts:

http://lkml.kernel.org/r/20130923162856.GC7264@otherpad.lan.raisama.net

I think we remained at that someone, maybe he, would have to clean all
that discrepancy before we do the emulated gunk. Let me CC him.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Eduardo Habkost]

On Wed, Jun 04, 2014 at 10:44:48PM +0200, Borislav Petkov wrote:
> On Wed, Jun 04, 2014 at 06:34:04PM +0200, Paolo Bonzini wrote:
> > That should be the purpose of KVM_GET_EMULATED_CPUID, so MWAIT could be
> > added in __do_cpuid_ent_emulated.  However, the corresponding QEMU patches
> > were never included.  Borislav, can you refresh them?
> 
> /me goes and swaps in all the details from the discussion at that
> time.... tries to decipher what the whole story was about...
> 
> Well, AFAIR - the operative word being "remember" - Eduardo had a
> problem with diffentiating between emulated features and real hw
> features and what was being filtered out... here's where the thread
> starts:
> 
> http://lkml.kernel.org/r/20130923162856.GC7264@otherpad.lan.raisama.net
> 
> I think we remained at that someone, maybe he, would have to clean all
> that discrepancy before we do the emulated gunk. Let me CC him.

I didn't work on it because of other tasks, and because the
GET_SUPPORTED_CPUID code on QEMU needed some cleanup first. Now it
should be easy to implement GET_EMULATED_CPUID support in the same way
the "migratable" property was implemented[1]. I will send a patch today.

[1] See tree at
    https://github.com/ehabkost/qemu-hacks/tree/work/qom-cpu-fixes

-- 
Eduardo

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Eric Northup]

On Wed, May 7, 2014 at 1:52 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> Treat monitor and mwait instructions as nop, which is architecturally
> correct (but inefficient) behavior. We do this to prevent misbehaving
> guests (e.g. OS X <= 10.7) from crashing after they fail to check for
> monitor/mwait availability via cpuid.
>
> Since mwait-based idle loops relying on these nop-emulated instructions
> would keep the host CPU pegged at 100%, do NOT advertise their presence
> via cpuid, to prevent compliant guests from using them inadvertently.

If it's going to peg the host CPU at 100% anyway, why bother emulating
it?  Just let the guest run the mwait instruction!  Have a condition
that controls whether CPU_BASED_MWAIT_EXITING gets set in the VMCS
processor execution controls.  Go ahead and put it in CPUID, since
it's actually allowed.

>
> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> ---
>
> New in v2: remove invalid_op handler functions which were only used to
>            handle exits caused by monitor and mwait
>
> On Wed, May 07, 2014 at 08:31:27PM +0200, Alexander Graf wrote:
>> On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
>> >If we really want to be paranoid and worry about guests
>> >that use this strange way to trigger invalid opcode,
>> >we can make it possible for userspace to enable/disable
>> >this hack, and teach qemu to set it.
>> >
>> >That would make it even safer than it was.
>> >
>> >Not sure it's worth it, just a thought.
>>
>> Since we don't trap on non-exposed other instructions (new SSE and
>> whatdoiknow) I don't think it's really bad to just expose
>> MONITOR/MWAIT as nops.
>
> So AFAICT, linux prefers to use mwait for idling if cpuid tells it that
> it's available. If we keep telling everyone that we do NOT have monitor
> and mwait available, compliant guests will never end up using them, and
> this hack would remain completely invisible to them, which is good
> (better to use hlt-based idle loops when you're a vm guest, that would
> actually allow the host to relax while you're halted :)
>
> So the only time anyone would be able to tell we have this hack would be
> when they're about to receive an invalid opcode for using monitor/mwait
> in violation of what CPUID (would have) told them. That's what happens
> to OS X prior to 10.8, which is when I'm hypothesizing the Apple devs
> begain to seriously think about their OS running as a vm guest (on fusion
> and parallels)...
>
> Instead of killing the misbehaving guest with an invalid opcode, we'd
> allow them to peg the host CPU with their monitor == mwait == nop idle
> loop instead, which, at least on OS X, should be tolerable long enough
> to run 'rm -rf System/Library/Extensions/AppleIntelCPUPowerManagement.kext'
> and reboot the guest, after which things would settle down by reverting
> the guest to a hlt-based idle loop.
>
> The only reason I can think of to add functionality for enabling/disabling
> this hack would be to protect against a malicious guest which would use
> mwait *on purpose* to peg the host CPU. But a malicious guest could just
> run "for(;;);" in ring 0 and accomplish the same goal, so we wouldn't
> really gain anything in exchange for the added complexity...
>
> Thanks,
>   Gabriel
>
>  arch/x86/kvm/cpuid.c |  2 ++
>  arch/x86/kvm/svm.c   | 28 ++++++++++++++++++++--------
>  arch/x86/kvm/vmx.c   | 20 ++++++++++++++++----
>  3 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index f47a104..d094fc6 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -283,6 +283,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>                 0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
>         /* cpuid 1.ecx */
>         const u32 kvm_supported_word4_x86_features =
> +               /* NOTE: MONITOR (and MWAIT) are emulated as NOP,
> +                * but *not* advertised to guests via CPUID ! */
>                 F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
>                 0 /* DS-CPL, VMX, SMX, EST */ |
>                 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 7f4f9c2..0b7d58d 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2770,12 +2770,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
>         return 1;
>  }
>
> -static int invalid_op_interception(struct vcpu_svm *svm)
> -{
> -       kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> -       return 1;
> -}
> -
>  static int task_switch_interception(struct vcpu_svm *svm)
>  {
>         u16 tss_selector;
> @@ -3287,6 +3281,24 @@ static int pause_interception(struct vcpu_svm *svm)
>         return 1;
>  }
>
> +static int nop_interception(struct vcpu_svm *svm)
> +{
> +       skip_emulated_instruction(&(svm->vcpu));
> +       return 1;
> +}
> +
> +static int monitor_interception(struct vcpu_svm *svm)
> +{
> +       printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> +       return nop_interception(svm);
> +}
> +
> +static int mwait_interception(struct vcpu_svm *svm)
> +{
> +       printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> +       return nop_interception(svm);
> +}
> +
>  static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>         [SVM_EXIT_READ_CR0]                     = cr_interception,
>         [SVM_EXIT_READ_CR3]                     = cr_interception,
> @@ -3344,8 +3356,8 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>         [SVM_EXIT_CLGI]                         = clgi_interception,
>         [SVM_EXIT_SKINIT]                       = skinit_interception,
>         [SVM_EXIT_WBINVD]                       = emulate_on_interception,
> -       [SVM_EXIT_MONITOR]                      = invalid_op_interception,
> -       [SVM_EXIT_MWAIT]                        = invalid_op_interception,
> +       [SVM_EXIT_MONITOR]                      = monitor_interception,
> +       [SVM_EXIT_MWAIT]                        = mwait_interception,
>         [SVM_EXIT_XSETBV]                       = xsetbv_interception,
>         [SVM_EXIT_NPF]                          = pf_interception,
>  };
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 33e8c02..3ccbcb1 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5669,12 +5669,24 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>         return 1;
>  }
>
> -static int handle_invalid_op(struct kvm_vcpu *vcpu)
> +static int handle_nop(struct kvm_vcpu *vcpu)
>  {
> -       kvm_queue_exception(vcpu, UD_VECTOR);
> +       skip_emulated_instruction(vcpu);
>         return 1;
>  }
>
> +static int handle_mwait(struct kvm_vcpu *vcpu)
> +{
> +       printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> +       return handle_nop(vcpu);
> +}
> +
> +static int handle_monitor(struct kvm_vcpu *vcpu)
> +{
> +       printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> +       return handle_nop(vcpu);
> +}
> +
>  /*
>   * To run an L2 guest, we need a vmcs02 based on the L1-specified vmcs12.
>   * We could reuse a single VMCS for all the L2 guests, but we also want the
> @@ -6571,8 +6583,8 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
>         [EXIT_REASON_EPT_VIOLATION]           = handle_ept_violation,
>         [EXIT_REASON_EPT_MISCONFIG]           = handle_ept_misconfig,
>         [EXIT_REASON_PAUSE_INSTRUCTION]       = handle_pause,
> -       [EXIT_REASON_MWAIT_INSTRUCTION]       = handle_invalid_op,
> -       [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_invalid_op,
> +       [EXIT_REASON_MWAIT_INSTRUCTION]       = handle_mwait,
> +       [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
>         [EXIT_REASON_INVEPT]                  = handle_invept,
>  };
>
> --
> 1.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Thu, Jun 05, 2014 at 01:59:17PM -0700, Eric Northup wrote:
> On Wed, May 7, 2014 at 1:52 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> > Treat monitor and mwait instructions as nop, which is architecturally
> > correct (but inefficient) behavior. We do this to prevent misbehaving
> > guests (e.g. OS X <= 10.7) from crashing after they fail to check for
> > monitor/mwait availability via cpuid.
> >
> > Since mwait-based idle loops relying on these nop-emulated instructions
> > would keep the host CPU pegged at 100%, do NOT advertise their presence
> > via cpuid, to prevent compliant guests from using them inadvertently.
> 
> If it's going to peg the host CPU at 100% anyway, why bother emulating
> it?  Just let the guest run the mwait instruction!  Have a condition
> that controls whether CPU_BASED_MWAIT_EXITING gets set in the VMCS
> processor execution controls.  Go ahead and put it in CPUID, since
> it's actually allowed.

If a well-behaved guest *does* check CPUID, we want it to get a
negative answer and fall back to a HLT-based idle loop (rather than
use mwait and peg the host cpu to 100%).

We only emulate it so that guests which use it blindly, regardless of
what CPUID says (e.g., OS X 10.6 .. 10.8) would still run instead of
crashing with an invalid opcode fault. That is, work long enough for
someone to manually force a hlt-idle fallback on the guest-side by
removing System/Library/Extensions/AppleIntelCPUPowerManagement.kext :)

Causing a VM exit (as opposed to letting the MWAIT just be a NOP in VM
guest mode) is nice, since it allows us to try and do something more
fancy in the future (i.e. yield the host cpu instead of re-entering
immediately, or attempting to otherwise emulate the instruction in a
way that's "better").

Anyhow, my $0.02 :)

Cheers,
--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Michael S. Tsirkin]

On Tue, Jun 03, 2014 at 10:21:58AM -0400, Gabriel L. Somlo wrote:
> On Tue, Jun 03, 2014 at 11:17:48AM +0200, Paolo Bonzini wrote:
> > 
> > I think it's fine as it is now. :)
> 
> On Mon, Jun 02, 2014 at 09:55:18PM -0400, Gabriel L. Somlo wrote:
> > 
> > W.r.t. monitor/mwait, a guest can do one of the following:
> > 
> > 1. Never check CPUID, and never use monitor/mwait
> > 	- This is great, we don't have to do anything about these
> > 
> > 2. Check CPUID for mwait, use it to idle in preference over hlt
> > 	- Linux, Windows, and Mavericks (10.9) do this
> > 	- we never want to have CPUID say "yes" to these, since
> > 	  monitor/mwait support will be clunky in the best case,
> > 	  and hlt is overwhelmingly preferable! [*]
> > 
> > 3. Never check CPUID, use monitor/mwait with abandon
> > 	- OS X 10.6 .. 10.8 does this
> > 	- emulating monitor/mwait here allows us to boot the guest
> > 	  and use it, and perform sysadmin surgery to force a hlt
> > 	  based idle
> > 
> > 4. Check CPUID, panic if unavailable
> > 	- OS X 10.5 did this, IIRC.
> > 	- whether I can do kext surgery and get it to stop checking
> > 	  CPUID *in addition to* falling back to hlt-based idle is
> > 	  TBD.
> > 	- emulating monitor/mwait allows us to boot this type of
> > 	  guest, BUT WE ALSO HAVE TO ADVERTISE IT VIA CPUID !!!
> 
> As it is right now, #4 is not being addressed (and we can't just
> advertise mwait via cpuid, or we'd be screwing up #2).

Yes, I didn't understand 10.5 did #4.

> I also feel a bit weird about the "undocumented feature" aspect
> of NOT generating an invalid opcode for something that *should*
> be an invalid opcode according to the feature set advertised via
> cpuid...
> 
> So if there's a way to make it so we can tell QEMU/KVM to
> "--enable-mwait" on a per-guest basis, I think that'd be better
> than an always-on "undocumented" behavior...
> 
> But then again, I'm most likely missing something about the big
> picture... :)
> 
> Thanks much,
> --Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Nadav Amit]

On Jun 4, 2014, at 11:11 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:

> On Wed, Jun 04, 2014 at 11:01:50PM +0300, Nadav Amit wrote:
>> On Jun 4, 2014, at 10:43 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
>> 
>> My implementation still emulates the instruction as a NOP, but first checks for an exception.
> 
> [...] 
> 
>> Anyhow, if you want a real mwait emulation, you can write-protect the page of the monitored memory area in the EPT of the other VCPUs and set a callback once a write to the area takes place. You may want the host to cause a spurious wakeup after you do the write-protection, so you will not miss a write of another VCPU to the monitored area. After the spurious wake-up, the VM is likely to issue an additional mwait, using the same monitored cache-line.
>> 
>> Additional care for DMAs (emulated and paravirtual) might be needed with the assistance of QEMU. The complicated case is dealing with the DMAs of assigned devices due to the lack of support for I/O page-faules.
> 
> I took a stab at something like that a while ago:
> 
> http://www.contrib.andrew.cmu.edu/~somlo/OSXKVM/kvm-mwait-emu-20140205.patch
> 
> with limited success, probably due to my lack of familiarity with
> the fine details of the KVM code base... :)
> 
> My main interest was to get it working well enough to be useful for
> idle loops (which is the only thing I know of that either Linux or
> OS X use monitor and mwait for, currently -- so DMA wasn't a huge
> priority).
> 
> Even if we got it working well enough in the general case (any number
> of vcpus, etc) I think it would still suck for idle loops when
> compared to simply falling back to HLT (mainly due to all the TLB
> shootdowns required to make it work) :)

MWAIT is likely not to perform as good as HLT, but you may play with other techniques, like clearing the dirty flag of the page on other VCPUs and periodically check it.
You will still need TLB shootdowns at that case, and wakeup might be slower, but at least you will have fewer emulations and exits.
Anyhow, I would try to avoid using the emulator for your purpose (especially since your guest crashes), and try to use the MTF feature instead - at least to get the system to work.

Regards,
Nadav

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Gabriel L. Somlo]

On Wed, Jun 04, 2014 at 11:01:50PM +0300, Nadav Amit wrote:
> On Jun 4, 2014, at 10:43 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> 
> My implementation still emulates the instruction as a NOP, but first checks for an exception.

[...] 

> Anyhow, if you want a real mwait emulation, you can write-protect the page of the monitored memory area in the EPT of the other VCPUs and set a callback once a write to the area takes place. You may want the host to cause a spurious wakeup after you do the write-protection, so you will not miss a write of another VCPU to the monitored area. After the spurious wake-up, the VM is likely to issue an additional mwait, using the same monitored cache-line.
> 
> Additional care for DMAs (emulated and paravirtual) might be needed with the assistance of QEMU. The complicated case is dealing with the DMAs of assigned devices due to the lack of support for I/O page-faules.

I took a stab at something like that a while ago:

http://www.contrib.andrew.cmu.edu/~somlo/OSXKVM/kvm-mwait-emu-20140205.patch

with limited success, probably due to my lack of familiarity with
the fine details of the KVM code base... :)

My main interest was to get it working well enough to be useful for
idle loops (which is the only thing I know of that either Linux or
OS X use monitor and mwait for, currently -- so DMA wasn't a huge
priority).

Even if we got it working well enough in the general case (any number
of vcpus, etc) I think it would still suck for idle loops when
compared to simply falling back to HLT (mainly due to all the TLB
shootdowns required to make it work) :)

Thanks,
--Gabriel

Re: [PATCH v2] kvm: x86: emulate monitor and mwait instructions as nop

[Nadav Amit]

On Jun 4, 2014, at 10:43 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:

My implementation still emulates the instruction as a NOP, but first checks for an exception.
> On Wed, Jun 04, 2014 at 10:12:39PM +0300, Nadav Amit wrote:
> 
> I'd be curious how you're dealing with the "hidden" CPU state which
> tells MWAIT to sleep until someone or something writes to the
> monitored memory area set up by a corresponding MONITOR instruction.
>> Regardless to the whole discussion of what the guest is informed about, I think it might be better to implement mwait and monitor correctly according to the spec and let the instructions to be fully emulated.
>> Both mwait and monitor may encounter exceptions (#GP, #PF, regardless of #UD), so this behaviour should be correct.
>> If you want me, I?ll send my version which looks something like:
>> 
>> static int em_monitor(struct x86_emulate_ctxt *ctxt)
>> {
>> 	int rc;
>> 	struct segmented_address addr;
>> 	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
>> 	u64 rax = reg_read(ctxt, VCPU_REGS_RAX);
>> 	u8 byte;
>> 
>> 	rc = check_mwait_supported(ctxt);
>> 	if (rc != X86EMUL_CONTINUE)
>> 		return rc;
>> 
>> 	if (ctxt->mode != X86EMUL_MODE_PROT64)
>> 		rcx = (u32)rcx;
>> 
>> 	if (rcx != 0)
>> 		return emulate_gp(ctxt, 0);
>> 
>> 	addr.seg = seg_override(ctxt);
>> 	addr.ea = ctxt->ad_bytes == 8 ? rax : (u32)rax;
>> 
>> 	rc = segmented_read(ctxt, addr, &byte, 1);
>> 	if (rc != X86EMUL_CONTINUE)
>> 		return rc;
>> 
>> 	return X86EMUL_CONTINUE;
>> }
>> 
>> static int em_mwait(struct x86_emulate_ctxt *ctxt)
>> {
>> 	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
>> 	int rc = check_mwait_supported(ctxt);
>> 	if (rc != X86EMUL_CONTINUE)
>> 		return rc;
>> 	if (ctxt->mode != X86EMUL_MODE_PROT64)
>> 		rcx = (u32)rcx;
>> 
>> 	if ((rcx & ~(u64)1) != 0)
>> 		return emulate_gp(ctxt, 0);
>> 
>> 	if (rcx & 1) {
>> 		/* Interrupt as break event */
>> 		u32 ebx, ecx, edx, eax;
>> 		eax = 5;
>> 		ecx = 0;
>> 		ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
>> 		if (!(ecx & 1))
>> 			return emulate_gp(ctxt, 0);
>> 	}
>> 	return X86EMUL_CONTINUE;
>> }

Anyhow, if you want a real mwait emulation, you can write-protect the page of the monitored memory area in the EPT of the other VCPUs and set a callback once a write to the area takes place. You may want the host to cause a spurious wakeup after you do the write-protection, so you will not miss a write of another VCPU to the monitored area. After the spurious wake-up, the VM is likely to issue an additional mwait, using the same monitored cache-line.

Additional care for DMAs (emulated and paravirtual) might be needed with the assistance of QEMU. The complicated case is dealing with the DMAs of assigned devices due to the lack of support for I/O page-faules.

Nadav