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

[Alexander Graf <agraf@suse.de>]

On 05/07/2014 08:15 PM, Michael S. Tsirkin wrote:
> On Wed, May 07, 2014 at 02:10:59PM -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 receiving invalid opcode faults after
>> failing 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, preventing compliant guests from ever using them inadvertently.
>>
>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> 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.


Alex

>
>> ---
>>
>> On Wed, May 07, 2014 at 05:30:47PM +0200, Paolo Bonzini wrote:
>>> Il 07/05/2014 17:05, Michael S. Tsirkin ha scritto:
>>>>>> 2. Emulate monitor and mwait as "nop", but continue to claim they are
>>>>>>     not supported via CPUID. That's the patch you cited. Not sure
>>>>>>     though whether that sort of "undocumented" functionality would be
>>>>>>     OK with the KVM crowd, though :)
>>>> I'd go for this one. It seems unlikely a guest wants to get
>>>> an exception intentionally.
>>>> Paolo?
>>> That's okay, but please add a printk_once the first time mwait is called.
>> OK, here's a first pass at an official submission. I have two questions:
>>
>>    1. I can't test svm.c (on AMD). As such, I'm not sure the

Joerg, mind to take a quick look here?


Alex

>>       skip_emulated_instruction() call in my own version of nop_interception()
>>       is necessary. If not, I could probably just call the already existing
>>       nop_on_interception() (line 1926 or thereabouts in svm.c), which
>>       just returns returns 1 without skipping anything.
>>
>>    2. I get "defined but not used" warnings on invalid_op_interception() (svm.c)
>>       and handle_invalid_op() (vmx.c). Apparently monitor/mwait are currently
>>       the only VM exit reasons which lead to an "invalid opcode" exception.
>>       Should my patch just nuke those functions (so that if anyone needs them
>>       in the future they'd have to re-add them), or comment them out, or
>>       call them after the "return 1;" statement in the monitor/mwait functions
>>       to shut up gcc, or ??? :)
>>
>> Thanks much,
>>     Gabriel
>>
>>   arch/x86/kvm/cpuid.c |  2 ++
>>   arch/x86/kvm/svm.c   | 22 ++++++++++++++++++++--
>>   arch/x86/kvm/vmx.c   | 22 ++++++++++++++++++++--
>>   3 files changed, 42 insertions(+), 4 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..1976488 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -3287,6 +3287,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 +3362,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..060b384 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -5669,6 +5669,24 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>>   	return 1;
>>   }
>>   
>> +static int handle_nop(struct kvm_vcpu *vcpu)
>> +{
>> +	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);
>> +}
>> +
>>   static int handle_invalid_op(struct kvm_vcpu *vcpu)
>>   {
>>   	kvm_queue_exception(vcpu, UD_VECTOR);
>> @@ -6571,8 +6589,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