Re: [PATCH] kvm: x86: emulate monitor and mwait instructions as nop
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm: x86: emulate monitor and mwait instructions as nop
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
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]
Re: [PATCH] kvm: x86: emulate monitor and mwait instructions as nop
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
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_inte
