| Message ID | 20140507181056.GD1655@ERROL.INI.CMU.EDU (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > --- > > 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 > 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 -- 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
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 -- 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
Il 07/05/2014 20:10, Gabriel L. Somlo ha scritto: > 1. I can't test svm.c (on AMD). As such, I'm not sure the > 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. Yes, it's necessary. > 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 ??? :) Nuke it. :) Thanks for working on OS X virtualization, weird guests are always the source of interesting quirks! Paolo -- 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
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, };