[PATCH 0/3] Correct monitor-mwait emulation as nop

[PATCH 0/3] Correct monitor-mwait emulation as nop

[Nadav Amit]

KVM handles monitor-mwait as nop, but does not check any of the preconditions
for the instructions.  These instructions may generate all kind of exceptions
(#UD, #PF, #GP, #SS). They can also be executed in real-mode.  This patch-set
moves the handling of monitor-mwait to the emulator, to allow their execution
in either real-mode or protected-mode.  It tries to follow the SDM in checking
the preconditions and generating the necassary exceptions.

Thanks for reviewing the patch. Please try it with OS X to make sure it works
properly without generating unnecassary exception.

Nadav Amit (3):
  KVM: x86: Emulator flag for instruction with no big real mode
  KVM: x86: Emulator support for #UD on CPL>0
  KVM: x86: correct mwait and monitor emulation

 arch/x86/kvm/emulate.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++----
 arch/x86/kvm/svm.c     | 22 ++------------------
 arch/x86/kvm/vmx.c     | 27 ++++++++++---------------
 3 files changed, 64 insertions(+), 40 deletions(-)

-- 
1.9.1

[PATCH 1/3] KVM: x86: Emulator flag for instruction with no big real mode

[Nadav Amit]

Certain instructions, such as monitor and xsave do not support big real mode
and cause a #GP exception if any of the accessed bytes effective address are
not within [0, 0xffff].  This patch introduces a flag to mark these
instructions, including the necassary checks.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e4e833d..f90194d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -162,6 +162,7 @@
 #define NoWrite     ((u64)1 << 45)  /* No writeback */
 #define SrcWrite    ((u64)1 << 46)  /* Write back src operand */
 #define NoMod	    ((u64)1 << 47)  /* Mod field is ignored */
+#define NoBigReal   ((u64)1 << 48)  /* No big real mode */
 
 #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
 
@@ -651,7 +652,12 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 		if (!fetch && (desc.type & 8) && !(desc.type & 2))
 			goto bad;
 		lim = desc_limit_scaled(&desc);
-		if ((desc.type & 8) || !(desc.type & 4)) {
+		if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch &&
+		    (ctxt->d & NoBigReal)) {
+			/* la is between zero and 0xffff */
+			if (la > 0xffff || (u32)(la + size - 1) > 0xffff)
+				goto bad;
+		} else if ((desc.type & 8) || !(desc.type & 4)) {
 			/* expand-up segment */
 			if (addr.ea > lim || (u32)(addr.ea + size - 1) > lim)
 				goto bad;
-- 
1.9.1

[PATCH 2/3] KVM: x86: Emulator support for #UD on CPL>0

[Nadav Amit]

Certain instructions (e.g., mwait and monitor) cause a #UD exception when they
are executed in privilaged mode. This is in contrast to the regular privilaged
instructions which cause #GP. In order not to mess with SVM interception of
mwait and monitor which assumes privilage level assertions take place before
interception, a flag has been added.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f90194d..ef7a5a0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -163,6 +163,7 @@
 #define SrcWrite    ((u64)1 << 46)  /* Write back src operand */
 #define NoMod	    ((u64)1 << 47)  /* Mod field is ignored */
 #define NoBigReal   ((u64)1 << 48)  /* No big real mode */
+#define UDOnPriv    ((u64)1 << 49)  /* #UD instead of #GP on CPL > 0 */
 
 #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
 
@@ -4560,7 +4561,10 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 
 	/* Privileged instruction can be executed only in CPL=0 */
 	if ((ctxt->d & Priv) && ops->cpl(ctxt)) {
-		rc = emulate_gp(ctxt, 0);
+		if (ctxt->d & UDOnPriv)
+			rc = emulate_ud(ctxt);
+		else
+			rc = emulate_gp(ctxt, 0);
 		goto done;
 	}
 
-- 
1.9.1

Re: [PATCH 2/3] KVM: x86: Emulator support for #UD on CPL>0

[Paolo Bonzini]

Il 18/06/2014 16:19, Nadav Amit ha scritto:
> Certain instructions (e.g., mwait and monitor) cause a #UD exception when they
> are executed in privilaged mode.

It's actually "non-privileged mode" (Priv means the instruction is 
privileged, not the mode).  So I've renamed the flag to PrivUD.

Paolo

  This is in contrast to the regular privilaged
> instructions which cause #GP. In order not to mess with SVM interception of
> mwait and monitor which assumes privilage level assertions take place before
> interception, a flag has been added.
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f90194d..ef7a5a0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -163,6 +163,7 @@
>  #define SrcWrite    ((u64)1 << 46)  /* Write back src operand */
>  #define NoMod	    ((u64)1 << 47)  /* Mod field is ignored */
>  #define NoBigReal   ((u64)1 << 48)  /* No big real mode */
> +#define UDOnPriv    ((u64)1 << 49)  /* #UD instead of #GP on CPL > 0 */
>
>  #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
>
> @@ -4560,7 +4561,10 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
>
>  	/* Privileged instruction can be executed only in CPL=0 */
>  	if ((ctxt->d & Priv) && ops->cpl(ctxt)) {
> -		rc = emulate_gp(ctxt, 0);
> +		if (ctxt->d & UDOnPriv)
> +			rc = emulate_ud(ctxt);
> +		else
> +			rc = emulate_gp(ctxt, 0);
>  		goto done;
>  	}
>
>

[PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Nadav Amit]

mwait and monitor are currently handled as nop. Considering this behavior, they
should still be handled correctly, i.e., check execution conditions and generate
exceptions when required. mwait and monitor may also be executed in real-mode
and are not handled in that case.  This patch performs the emulation of
monitor-mwait according to Intel SDM (other than checking whether interrupt can
be used as a break event).

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 41 +++++++++++++++++++++++++++++++++++++++--
 arch/x86/kvm/svm.c     | 22 ++--------------------
 arch/x86/kvm/vmx.c     | 27 +++++++++++----------------
 3 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index ef7a5a0..424b58d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3344,6 +3344,43 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+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;
+
+	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;
+
+	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
+	return X86EMUL_CONTINUE;
+}
+
+static int em_mwait(struct x86_emulate_ctxt *ctxt)
+{
+	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
+
+	if (ctxt->mode != X86EMUL_MODE_PROT64)
+		rcx = (u32)rcx;
+	if ((rcx & ~1UL) != 0)
+		return emulate_gp(ctxt, 0);
+
+	/* Accepting interrupt as break event regardless to cpuid */
+	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
+	return X86EMUL_CONTINUE;
+}
+
 static bool valid_cr(int nr)
 {
 	switch (nr) {
@@ -3557,8 +3594,8 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
 		F2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
 
 static const struct opcode group7_rm1[] = {
-	DI(SrcNone | Priv, monitor),
-	DI(SrcNone | Priv, mwait),
+	II(SrcNone | Priv | NoBigReal | UDOnPriv, em_monitor, monitor),
+	II(SrcNone | Priv | NoBigReal | UDOnPriv, em_mwait, mwait),
 	N, N, N, N, N, N,
 };
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ec8366c..a524e04 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3274,24 +3274,6 @@ 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,
@@ -3349,8 +3331,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]			= monitor_interception,
-	[SVM_EXIT_MWAIT]			= mwait_interception,
+	[SVM_EXIT_MONITOR]			= emulate_on_interception,
+	[SVM_EXIT_MWAIT]			= emulate_on_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 801332e..7023e71 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5672,22 +5672,17 @@ static int handle_pause(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
-static int handle_nop(struct kvm_vcpu *vcpu)
+static int handle_emulate(struct kvm_vcpu *vcpu)
 {
-	skip_emulated_instruction(vcpu);
-	return 1;
-}
+	int err = emulate_instruction(vcpu, 0);
 
-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);
+	if (err != EMULATE_DONE) {
+		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+		vcpu->run->internal.ndata = 0;
+		return 0;
+	}
+	return 1;
 }
 
 /*
@@ -6651,8 +6646,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_mwait,
-	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
+	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_emulate,
+	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_emulate,
 	[EXIT_REASON_INVEPT]                  = handle_invept,
 };
 
-- 
1.9.1

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Paolo Bonzini]

Il 18/06/2014 16:19, Nadav Amit ha scritto:
> mwait and monitor are currently handled as nop. Considering this behavior, they
> should still be handled correctly, i.e., check execution conditions and generate
> exceptions when required. mwait and monitor may also be executed in real-mode
> and are not handled in that case.  This patch performs the emulation of
> monitor-mwait according to Intel SDM (other than checking whether interrupt can
> be used as a break event).
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  arch/x86/kvm/svm.c     | 22 ++--------------------
>  arch/x86/kvm/vmx.c     | 27 +++++++++++----------------
>  3 files changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index ef7a5a0..424b58d 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3344,6 +3344,43 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
>
> +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;
> +
> +	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;
> +
> +	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_mwait(struct x86_emulate_ctxt *ctxt)
> +{
> +	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
> +
> +	if (ctxt->mode != X86EMUL_MODE_PROT64)
> +		rcx = (u32)rcx;
> +	if ((rcx & ~1UL) != 0)
> +		return emulate_gp(ctxt, 0);
> +
> +	/* Accepting interrupt as break event regardless to cpuid */
> +	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> +	return X86EMUL_CONTINUE;
> +}
> +
>  static bool valid_cr(int nr)
>  {
>  	switch (nr) {
> @@ -3557,8 +3594,8 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
>  		F2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
>
>  static const struct opcode group7_rm1[] = {
> -	DI(SrcNone | Priv, monitor),
> -	DI(SrcNone | Priv, mwait),
> +	II(SrcNone | Priv | NoBigReal | UDOnPriv, em_monitor, monitor),
> +	II(SrcNone | Priv | NoBigReal | UDOnPriv, em_mwait, mwait),
>  	N, N, N, N, N, N,
>  };
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ec8366c..a524e04 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3274,24 +3274,6 @@ 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,
> @@ -3349,8 +3331,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]			= monitor_interception,
> -	[SVM_EXIT_MWAIT]			= mwait_interception,
> +	[SVM_EXIT_MONITOR]			= emulate_on_interception,
> +	[SVM_EXIT_MWAIT]			= emulate_on_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 801332e..7023e71 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5672,22 +5672,17 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>
> -static int handle_nop(struct kvm_vcpu *vcpu)
> +static int handle_emulate(struct kvm_vcpu *vcpu)
>  {
> -	skip_emulated_instruction(vcpu);
> -	return 1;
> -}
> +	int err = emulate_instruction(vcpu, 0);
>
> -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);
> +	if (err != EMULATE_DONE) {
> +		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +		vcpu->run->internal.ndata = 0;
> +		return 0;
> +	}
> +	return 1;
>  }
>
>  /*
> @@ -6651,8 +6646,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_mwait,
> -	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
> +	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_emulate,
> +	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_emulate,
>  	[EXIT_REASON_INVEPT]                  = handle_invept,
>  };
>
>

In VMX, handle_invd can be reused as handle_emulate.

Paolo

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Bandan Das]

Nadav Amit <namit@cs.technion.ac.il> writes:

> mwait and monitor are currently handled as nop. Considering this behavior, they
> should still be handled correctly, i.e., check execution conditions and generate
> exceptions when required. mwait and monitor may also be executed in real-mode

Is this necessary ? They are NOPs and kvm prints that out (correctly so) to dmesg.
Implementing them correctly is a different thing, but adding extra checks for NOPs 
just seems like adding extra cycles. 


> and are not handled in that case.  This patch performs the emulation of
> monitor-mwait according to Intel SDM (other than checking whether interrupt can
> be used as a break event).
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  arch/x86/kvm/svm.c     | 22 ++--------------------
>  arch/x86/kvm/vmx.c     | 27 +++++++++++----------------
>  3 files changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index ef7a5a0..424b58d 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3344,6 +3344,43 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
>  
> +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;
> +
> +	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;
> +
> +	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_mwait(struct x86_emulate_ctxt *ctxt)
> +{
> +	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
> +
> +	if (ctxt->mode != X86EMUL_MODE_PROT64)
> +		rcx = (u32)rcx;
> +	if ((rcx & ~1UL) != 0)
> +		return emulate_gp(ctxt, 0);
> +
> +	/* Accepting interrupt as break event regardless to cpuid */
> +	printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> +	return X86EMUL_CONTINUE;
> +}
> +
>  static bool valid_cr(int nr)
>  {
>  	switch (nr) {
> @@ -3557,8 +3594,8 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
>  		F2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
>  
>  static const struct opcode group7_rm1[] = {
> -	DI(SrcNone | Priv, monitor),
> -	DI(SrcNone | Priv, mwait),
> +	II(SrcNone | Priv | NoBigReal | UDOnPriv, em_monitor, monitor),
> +	II(SrcNone | Priv | NoBigReal | UDOnPriv, em_mwait, mwait),
>  	N, N, N, N, N, N,
>  };
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ec8366c..a524e04 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3274,24 +3274,6 @@ 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,
> @@ -3349,8 +3331,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]			= monitor_interception,
> -	[SVM_EXIT_MWAIT]			= mwait_interception,
> +	[SVM_EXIT_MONITOR]			= emulate_on_interception,
> +	[SVM_EXIT_MWAIT]			= emulate_on_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 801332e..7023e71 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5672,22 +5672,17 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> -static int handle_nop(struct kvm_vcpu *vcpu)
> +static int handle_emulate(struct kvm_vcpu *vcpu)
>  {
> -	skip_emulated_instruction(vcpu);
> -	return 1;
> -}
> +	int err = emulate_instruction(vcpu, 0);
>  
> -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);
> +	if (err != EMULATE_DONE) {
> +		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +		vcpu->run->internal.ndata = 0;
> +		return 0;
> +	}
> +	return 1;
>  }
>  
>  /*
> @@ -6651,8 +6646,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_mwait,
> -	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
> +	[EXIT_REASON_MWAIT_INSTRUCTION]	      = handle_emulate,
> +	[EXIT_REASON_MONITOR_INSTRUCTION]     = handle_emulate,
>  	[EXIT_REASON_INVEPT]                  = handle_invept,
>  };

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Paolo Bonzini]

Il 18/06/2014 18:43, Bandan Das ha scritto:
>> > mwait and monitor are currently handled as nop. Considering this behavior, they
>> > should still be handled correctly, i.e., check execution conditions and generate
>> > exceptions when required. mwait and monitor may also be executed in real-mode
> Is this necessary ? They are NOPs and kvm prints that out (correctly so) to dmesg.
> Implementing them correctly is a different thing, but adding extra checks for NOPs
> just seems like adding extra cycles.

Raising the correct exception is a good thing, though.  The guest is 
going to busy wait anyway, it doesn't matter how fast it does that. :)

Paolo

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 18/06/2014 18:43, Bandan Das ha scritto:
>>> > mwait and monitor are currently handled as nop. Considering this behavior, they
>>> > should still be handled correctly, i.e., check execution conditions and generate
>>> > exceptions when required. mwait and monitor may also be executed in real-mode
>> Is this necessary ? They are NOPs and kvm prints that out (correctly so) to dmesg.
>> Implementing them correctly is a different thing, but adding extra checks for NOPs
>> just seems like adding extra cycles.
>
> Raising the correct exception is a good thing, though.  The guest is
> going to busy wait anyway, it doesn't matter how fast it does that. :)

Thanks, makes sense. I was also thinking why to advertise it at all, but it's 
probably more common than I think it is.

> 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

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Eric Northup]

On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> mwait and monitor are currently handled as nop. Considering this behavior, they
> should still be handled correctly, i.e., check execution conditions and generate
> exceptions when required. mwait and monitor may also be executed in real-mode
> and are not handled in that case.  This patch performs the emulation of
> monitor-mwait according to Intel SDM (other than checking whether interrupt can
> be used as a break event).
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  arch/x86/kvm/svm.c     | 22 ++--------------------
>  arch/x86/kvm/vmx.c     | 27 +++++++++++----------------
>  3 files changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index ef7a5a0..424b58d 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3344,6 +3344,43 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
>         return X86EMUL_CONTINUE;
>  }
>
> +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;

I'd request:

u32 ebx, ecx, edx, eax = 1;
ctxt->opt->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
if (!(ecx & FFL(MWAIT)))
        return emulate_ud(ctxt);

and also in em_mwait.

> +
> +       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;
> +
> +       printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> +       return X86EMUL_CONTINUE;
> +}
> +
> +static int em_mwait(struct x86_emulate_ctxt *ctxt)
> +{
> +       u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
> +
> +       if (ctxt->mode != X86EMUL_MODE_PROT64)
> +               rcx = (u32)rcx;
> +       if ((rcx & ~1UL) != 0)
> +               return emulate_gp(ctxt, 0);
> +
> +       /* Accepting interrupt as break event regardless to cpuid */
> +       printk_once(KERN_WARNING "kvm: MWAIT instruction emulated as NOP!\n");
> +       return X86EMUL_CONTINUE;
> +}
> +
>  static bool valid_cr(int nr)
>  {
>         switch (nr) {
> @@ -3557,8 +3594,8 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
>                 F2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
>
>  static const struct opcode group7_rm1[] = {
> -       DI(SrcNone | Priv, monitor),
> -       DI(SrcNone | Priv, mwait),
> +       II(SrcNone | Priv | NoBigReal | UDOnPriv, em_monitor, monitor),
> +       II(SrcNone | Priv | NoBigReal | UDOnPriv, em_mwait, mwait),
>         N, N, N, N, N, N,
>  };
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ec8366c..a524e04 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3274,24 +3274,6 @@ 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,
> @@ -3349,8 +3331,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]                      = monitor_interception,
> -       [SVM_EXIT_MWAIT]                        = mwait_interception,
> +       [SVM_EXIT_MONITOR]                      = emulate_on_interception,
> +       [SVM_EXIT_MWAIT]                        = emulate_on_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 801332e..7023e71 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5672,22 +5672,17 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>         return 1;
>  }
>
> -static int handle_nop(struct kvm_vcpu *vcpu)
> +static int handle_emulate(struct kvm_vcpu *vcpu)
>  {
> -       skip_emulated_instruction(vcpu);
> -       return 1;
> -}
> +       int err = emulate_instruction(vcpu, 0);
>
> -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);
> +       if (err != EMULATE_DONE) {
> +               vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +               vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +               vcpu->run->internal.ndata = 0;
> +               return 0;
> +       }
> +       return 1;
>  }
>
>  /*
> @@ -6651,8 +6646,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_mwait,
> -       [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_monitor,
> +       [EXIT_REASON_MWAIT_INSTRUCTION]       = handle_emulate,
> +       [EXIT_REASON_MONITOR_INSTRUCTION]     = handle_emulate,
>         [EXIT_REASON_INVEPT]                  = handle_invept,
>  };
>
> --
> 1.9.1
>
> --
> 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 3/3] KVM: x86: correct mwait and monitor emulation

[Nadav Amit]

On 6/18/14, 8:59 PM, Eric Northup wrote:
> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
>> mwait and monitor are currently handled as nop. Considering this behavior, they
>> should still be handled correctly, i.e., check execution conditions and generate
>> exceptions when required. mwait and monitor may also be executed in real-mode
>> and are not handled in that case.  This patch performs the emulation of
>> monitor-mwait according to Intel SDM (other than checking whether interrupt can
>> be used as a break event).
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>>   arch/x86/kvm/emulate.c | 41 +++++++++++++++++++++++++++++++++++++++--
>>   arch/x86/kvm/svm.c     | 22 ++--------------------
>>   arch/x86/kvm/vmx.c     | 27 +++++++++++----------------
>>   3 files changed, 52 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index ef7a5a0..424b58d 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -3344,6 +3344,43 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
>>          return X86EMUL_CONTINUE;
>>   }
>>
>> +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;
>
> I'd request:
>
> u32 ebx, ecx, edx, eax = 1;
> ctxt->opt->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> if (!(ecx & FFL(MWAIT)))
>          return emulate_ud(ctxt);
>
> and also in em_mwait.
>

I had similar implementation on previous version, which also checked on 
mwait whether "interrupt as break event" matches ECX value. However, I 
was under the impression that it was decided that MWAIT will always be 
emulated as NOP to avoid misbehaving VMs that ignore CPUID (see the 
discussion at http://www.spinics.net/lists/kvm/msg102766.html ).

Nadav

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Eric Northup]

Quoting Gabriel's post http://www.spinics.net/lists/kvm/msg103792.html :

[...]

> 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.




On Wed, Jun 18, 2014 at 11:23 AM, Nadav Amit <nadav.amit@gmail.com> wrote:
> On 6/18/14, 8:59 PM, Eric Northup wrote:
>>
>> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il>
>> wrote:
>>>
>>> mwait and monitor are currently handled as nop. Considering this
>>> behavior, they
>>> should still be handled correctly, i.e., check execution conditions and
>>> generate
>>> exceptions when required. mwait and monitor may also be executed in
>>> real-mode
>>> and are not handled in that case.  This patch performs the emulation of
>>> monitor-mwait according to Intel SDM (other than checking whether
>>> interrupt can
>>> be used as a break event).
>>>
>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>> ---
>>>   arch/x86/kvm/emulate.c | 41 +++++++++++++++++++++++++++++++++++++++--
>>>   arch/x86/kvm/svm.c     | 22 ++--------------------
>>>   arch/x86/kvm/vmx.c     | 27 +++++++++++----------------
>>>   3 files changed, 52 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>>> index ef7a5a0..424b58d 100644
>>> --- a/arch/x86/kvm/emulate.c
>>> +++ b/arch/x86/kvm/emulate.c
>>> @@ -3344,6 +3344,43 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
>>>          return X86EMUL_CONTINUE;
>>>   }
>>>
>>> +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;
>>
>>
>> I'd request:
>>
>> u32 ebx, ecx, edx, eax = 1;
>> ctxt->opt->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
>> if (!(ecx & FFL(MWAIT)))
>>          return emulate_ud(ctxt);
>>
>> and also in em_mwait.
>>
>
> I had similar implementation on previous version, which also checked on
> mwait whether "interrupt as break event" matches ECX value. However, I was
> under the impression that it was decided that MWAIT will always be emulated
> as NOP to avoid misbehaving VMs that ignore CPUID (see the discussion at
> http://www.spinics.net/lists/kvm/msg102766.html ).
>
> Nadav

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Gabriel L. Somlo]

On Wed, Jun 18, 2014 at 11:30:07AM -0700, Eric Northup wrote:
> Quoting Gabriel's post http://www.spinics.net/lists/kvm/msg103792.html :
> 
> [...]
> 
> > 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.

That was an argument in favor of finding a mechanism to allow (qemu)
users to enable an otherwise default-off monitor cpuid flag.

We definitely don't want to advertise monitor/mwait availability to
guests which would otherwise sanely fail back to a hlt-based idle loop
when cpuid tells them monitor/mwait are not available :)

However, check my earlier proposal of backing out of monitor/mwait
entirely (as it turns out, there's a kernel command line to tell OS X
not to use monitor/mwait, which is IMHO vastly preferable to creating
"undocumented" KVM hacks :)

Thanks much,
--Gabriel

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Gabriel L. Somlo]

On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> > mwait and monitor are currently handled as nop. Considering this behavior, they
> > should still be handled correctly, i.e., check execution conditions and generate
> > exceptions when required. mwait and monitor may also be executed in real-mode
> > and are not handled in that case.  This patch performs the emulation of
> > monitor-mwait according to Intel SDM (other than checking whether interrupt can
> > be used as a break event).
> >
> > Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>

How about this instead (details in the commit log below) ? Please let
me know what you think, and if you'd prefer me to send it out as a
separate patch rather than a reply to this thread.

Thanks,
--Gabriel


>From 0375a0aceb54cdbc26a6c0e5b43c46324f830ec3 Mon Sep 17 00:00:00 2001
From: "Gabriel L. Somlo" <somlo@cmu.edu>
Date: Wed, 18 Jun 2014 14:39:15 -0400
Subject: [PATCH] kvm: x86: revert "emulate monitor and mwait instructions as nop"

This reverts commit 87c00572ba05aa8c9db118da75c608f47eb10b9e.

OS X <= 10.7.* are the only known guests which realistically required
this functionality. As it turns out, OS X can be told to forego using
monitor/mwait by passing it "idlehalt=0" as a kernel argument, so we're
better off removing this hack from KVM altogether.

Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
---
 arch/x86/kvm/cpuid.c |  2 --
 arch/x86/kvm/svm.c   |  8 +++-----
 arch/x86/kvm/vmx.c   | 10 ++++------
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 38a0afe..17b42fa 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -283,8 +283,6 @@ 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 ec8366c..0e8ef20 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3274,7 +3274,7 @@ static int pause_interception(struct vcpu_svm *svm)
 	return 1;
 }
 
-static int nop_interception(struct vcpu_svm *svm)
+static int invalid_op_interception(struct vcpu_svm *svm)
 {
 	skip_emulated_instruction(&(svm->vcpu));
 	return 1;
@@ -3282,14 +3282,12 @@ static int nop_interception(struct vcpu_svm *svm)
 
 static int monitor_interception(struct vcpu_svm *svm)
 {
-	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
-	return nop_interception(svm);
+	return invalid_op_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);
+	return invalid_op_interception(svm);
 }
 
 static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 801332e..577c7df 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5672,22 +5672,20 @@ static int handle_pause(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
-static int handle_nop(struct kvm_vcpu *vcpu)
+static int handle_invalid_op(struct kvm_vcpu *vcpu)
 {
-	skip_emulated_instruction(vcpu);
+	kvm_queue_exception(vcpu, UD_VECTOR);
 	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);
+	return handle_invalid_op(vcpu);
 }
 
 static int handle_monitor(struct kvm_vcpu *vcpu)
 {
-	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
-	return handle_nop(vcpu);
+	return handle_invalid_op(vcpu);
 }
 
 /*
-- 
1.9.3

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Bandan Das]

"Gabriel L. Somlo" <gsomlo@gmail.com> writes:

> On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
>> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
>> > mwait and monitor are currently handled as nop. Considering this behavior, they
>> > should still be handled correctly, i.e., check execution conditions and generate
>> > exceptions when required. mwait and monitor may also be executed in real-mode
>> > and are not handled in that case.  This patch performs the emulation of
>> > monitor-mwait according to Intel SDM (other than checking whether interrupt can
>> > be used as a break event).
>> >
>> > Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>
> How about this instead (details in the commit log below) ? Please let
> me know what you think, and if you'd prefer me to send it out as a
> separate patch rather than a reply to this thread.

I am not saying we should "punish" guests that don't do the right thing.
But if we can't emulate it the right way, we emulate it  ? 
And the fact that a workaround exists for the guest is all the more better :)

> Thanks,
> --Gabriel
>
>
> From 0375a0aceb54cdbc26a6c0e5b43c46324f830ec3 Mon Sep 17 00:00:00 2001
> From: "Gabriel L. Somlo" <somlo@cmu.edu>
> Date: Wed, 18 Jun 2014 14:39:15 -0400
> Subject: [PATCH] kvm: x86: revert "emulate monitor and mwait instructions as nop"
>
> This reverts commit 87c00572ba05aa8c9db118da75c608f47eb10b9e.
>
> OS X <= 10.7.* are the only known guests which realistically required
> this functionality. As it turns out, OS X can be told to forego using
> monitor/mwait by passing it "idlehalt=0" as a kernel argument, so we're
> better off removing this hack from KVM altogether.
>
> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> ---
>  arch/x86/kvm/cpuid.c |  2 --
>  arch/x86/kvm/svm.c   |  8 +++-----
>  arch/x86/kvm/vmx.c   | 10 ++++------
>  3 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 38a0afe..17b42fa 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -283,8 +283,6 @@ 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 ec8366c..0e8ef20 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3274,7 +3274,7 @@ static int pause_interception(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> -static int nop_interception(struct vcpu_svm *svm)
> +static int invalid_op_interception(struct vcpu_svm *svm)
>  {
>  	skip_emulated_instruction(&(svm->vcpu));
>  	return 1;
> @@ -3282,14 +3282,12 @@ static int nop_interception(struct vcpu_svm *svm)
>  
>  static int monitor_interception(struct vcpu_svm *svm)
>  {
> -	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> -	return nop_interception(svm);
> +	return invalid_op_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);
> +	return invalid_op_interception(svm);
>  }
>  
>  static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 801332e..577c7df 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5672,22 +5672,20 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> -static int handle_nop(struct kvm_vcpu *vcpu)
> +static int handle_invalid_op(struct kvm_vcpu *vcpu)
>  {
> -	skip_emulated_instruction(vcpu);
> +	kvm_queue_exception(vcpu, UD_VECTOR);
>  	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);
> +	return handle_invalid_op(vcpu);
>  }
>  
>  static int handle_monitor(struct kvm_vcpu *vcpu)
>  {
> -	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> -	return handle_nop(vcpu);
> +	return handle_invalid_op(vcpu);
>  }
>  
>  /*

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Michael S. Tsirkin]

On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
> On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
> > On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> > > mwait and monitor are currently handled as nop. Considering this behavior, they
> > > should still be handled correctly, i.e., check execution conditions and generate
> > > exceptions when required. mwait and monitor may also be executed in real-mode
> > > and are not handled in that case.  This patch performs the emulation of
> > > monitor-mwait according to Intel SDM (other than checking whether interrupt can
> > > be used as a break event).
> > >
> > > Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> 
> How about this instead (details in the commit log below) ? Please let
> me know what you think, and if you'd prefer me to send it out as a
> separate patch rather than a reply to this thread.
> 
> Thanks,
> --Gabriel

If there's an easy workaround, I'm inclined to agree.
We can always go back to Gabriel's patch (and then we'll need
Nadav's one too) but if we release a kernel with this
support it becomes an ABI and we can't go back.

So let's be careful here, and revert the hack for 3.16.


Acked-by: Michael S. Tsirkin <mst@redhat.com>



> 
> >From 0375a0aceb54cdbc26a6c0e5b43c46324f830ec3 Mon Sep 17 00:00:00 2001
> From: "Gabriel L. Somlo" <somlo@cmu.edu>
> Date: Wed, 18 Jun 2014 14:39:15 -0400
> Subject: [PATCH] kvm: x86: revert "emulate monitor and mwait instructions as nop"
> 
> This reverts commit 87c00572ba05aa8c9db118da75c608f47eb10b9e.
> 
> OS X <= 10.7.* are the only known guests which realistically required
> this functionality. As it turns out, OS X can be told to forego using
> monitor/mwait by passing it "idlehalt=0" as a kernel argument, so we're
> better off removing this hack from KVM altogether.
> 
> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> ---
>  arch/x86/kvm/cpuid.c |  2 --
>  arch/x86/kvm/svm.c   |  8 +++-----
>  arch/x86/kvm/vmx.c   | 10 ++++------
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 38a0afe..17b42fa 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -283,8 +283,6 @@ 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 ec8366c..0e8ef20 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3274,7 +3274,7 @@ static int pause_interception(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> -static int nop_interception(struct vcpu_svm *svm)
> +static int invalid_op_interception(struct vcpu_svm *svm)
>  {
>  	skip_emulated_instruction(&(svm->vcpu));
>  	return 1;
> @@ -3282,14 +3282,12 @@ static int nop_interception(struct vcpu_svm *svm)
>  
>  static int monitor_interception(struct vcpu_svm *svm)
>  {
> -	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> -	return nop_interception(svm);
> +	return invalid_op_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);
> +	return invalid_op_interception(svm);
>  }
>  
>  static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 801332e..577c7df 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5672,22 +5672,20 @@ static int handle_pause(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> -static int handle_nop(struct kvm_vcpu *vcpu)
> +static int handle_invalid_op(struct kvm_vcpu *vcpu)
>  {
> -	skip_emulated_instruction(vcpu);
> +	kvm_queue_exception(vcpu, UD_VECTOR);
>  	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);
> +	return handle_invalid_op(vcpu);
>  }
>  
>  static int handle_monitor(struct kvm_vcpu *vcpu)
>  {
> -	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");
> -	return handle_nop(vcpu);
> +	return handle_invalid_op(vcpu);
>  }
>  
>  /*
> -- 
> 1.9.3

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Gleb Natapov]

On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
> 
> On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
> >> On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
> >>> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> >>>> mwait and monitor are currently handled as nop. Considering this behavior, they
> >>>> should still be handled correctly, i.e., check execution conditions and generate
> >>>> exceptions when required. mwait and monitor may also be executed in real-mode
> >>>> and are not handled in that case.  This patch performs the emulation of
> >>>> monitor-mwait according to Intel SDM (other than checking whether interrupt can
> >>>> be used as a break event).
> >>>> 
> >>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> >> 
> >> How about this instead (details in the commit log below) ? Please let
> >> me know what you think, and if you'd prefer me to send it out as a
> >> separate patch rather than a reply to this thread.
> >> 
> >> Thanks,
> >> --Gabriel
> > 
> > If there's an easy workaround, I'm inclined to agree.
> > We can always go back to Gabriel's patch (and then we'll need
> > Nadav's one too) but if we release a kernel with this
> > support it becomes an ABI and we can't go back.
> > 
> > So let's be careful here, and revert the hack for 3.16.
> > 
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> Personally, I got a custom guest which requires mwait for executing correctly.
Can you elaborate on this guest a little bit. With nop implementation
for mwait the guest will hog a host cpu. Do you consider this to be
"executing correctly?"

--
			Gleb.

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Nadav Amit]

On 6/19/14, 2:23 PM, Gleb Natapov wrote:
> On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
>>
>> On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>>> On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
>>>> On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
>>>>> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
>>>>>> mwait and monitor are currently handled as nop. Considering this behavior, they
>>>>>> should still be handled correctly, i.e., check execution conditions and generate
>>>>>> exceptions when required. mwait and monitor may also be executed in real-mode
>>>>>> and are not handled in that case.  This patch performs the emulation of
>>>>>> monitor-mwait according to Intel SDM (other than checking whether interrupt can
>>>>>> be used as a break event).
>>>>>>
>>>>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>>>
>>>> How about this instead (details in the commit log below) ? Please let
>>>> me know what you think, and if you'd prefer me to send it out as a
>>>> separate patch rather than a reply to this thread.
>>>>
>>>> Thanks,
>>>> --Gabriel
>>>
>>> If there's an easy workaround, I'm inclined to agree.
>>> We can always go back to Gabriel's patch (and then we'll need
>>> Nadav's one too) but if we release a kernel with this
>>> support it becomes an ABI and we can't go back.
>>>
>>> So let's be careful here, and revert the hack for 3.16.
>>>
>>>
>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>> Personally, I got a custom guest which requires mwait for executing correctly.
> Can you elaborate on this guest a little bit. With nop implementation
> for mwait the guest will hog a host cpu. Do you consider this to be
> "executing correctly?"
>
> --

mwait is not as "clean" as it may appear. It encounters false wake-ups 
due to a variety of reasons, and any code need to recheck the wake-up 
condition afterwards. Actually, some CPUs had bugs that caused excessive 
wake-ups that degraded performance considerably (Nehalem, if I am not 
mistaken).
Therefore, handling mwait as nop is logically correct (although it may 
degrade performance).

For the reference, if you look at the SDM 8.10.4, you'll see:
"Multiple events other than a write to the triggering address range can 
cause a processor that executed MWAIT to wake up. These include events 
that would lead to voluntary or involuntary context switches, such as..."

Note the words "include" in the sentence "These include events". 
Software has no way of controlling whether it gets false wake-ups and 
cannot rely on the wake-up as indication to anything.

Nadav

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Michael S. Tsirkin]

On Thu, Jun 19, 2014 at 02:52:20PM +0300, Nadav Amit wrote:
> On 6/19/14, 2:23 PM, Gleb Natapov wrote:
> >On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
> >>
> >>On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >>>On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
> >>>>On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
> >>>>>On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> >>>>>>mwait and monitor are currently handled as nop. Considering this behavior, they
> >>>>>>should still be handled correctly, i.e., check execution conditions and generate
> >>>>>>exceptions when required. mwait and monitor may also be executed in real-mode
> >>>>>>and are not handled in that case.  This patch performs the emulation of
> >>>>>>monitor-mwait according to Intel SDM (other than checking whether interrupt can
> >>>>>>be used as a break event).
> >>>>>>
> >>>>>>Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> >>>>
> >>>>How about this instead (details in the commit log below) ? Please let
> >>>>me know what you think, and if you'd prefer me to send it out as a
> >>>>separate patch rather than a reply to this thread.
> >>>>
> >>>>Thanks,
> >>>>--Gabriel
> >>>
> >>>If there's an easy workaround, I'm inclined to agree.
> >>>We can always go back to Gabriel's patch (and then we'll need
> >>>Nadav's one too) but if we release a kernel with this
> >>>support it becomes an ABI and we can't go back.
> >>>
> >>>So let's be careful here, and revert the hack for 3.16.
> >>>
> >>>
> >>>Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>>
> >>Personally, I got a custom guest which requires mwait for executing correctly.
> >Can you elaborate on this guest a little bit. With nop implementation
> >for mwait the guest will hog a host cpu. Do you consider this to be
> >"executing correctly?"
> >
> >--
> 
> mwait is not as "clean" as it may appear. It encounters false wake-ups due
> to a variety of reasons, and any code need to recheck the wake-up condition
> afterwards. Actually, some CPUs had bugs that caused excessive wake-ups that
> degraded performance considerably (Nehalem, if I am not mistaken).
> Therefore, handling mwait as nop is logically correct (although it may
> degrade performance).
> 
> For the reference, if you look at the SDM 8.10.4, you'll see:
> "Multiple events other than a write to the triggering address range can
> cause a processor that executed MWAIT to wake up. These include events that
> would lead to voluntary or involuntary context switches, such as..."
> 
> Note the words "include" in the sentence "These include events". Software
> has no way of controlling whether it gets false wake-ups and cannot rely on
> the wake-up as indication to anything.
> 
> Nadav

It's a quality of implementation question.
It is correct in the same sense that
a NIC dropping each second packet is correct.

If we ship this hack we have to maintain it forever,
so there needs to be a compelling reason beyond
just "because we can".


-- 
MST

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Gleb Natapov]

On Thu, Jun 19, 2014 at 02:52:20PM +0300, Nadav Amit wrote:
> On 6/19/14, 2:23 PM, Gleb Natapov wrote:
> >On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
> >>
> >>On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >>>On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
> >>>>On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
> >>>>>On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> >>>>>>mwait and monitor are currently handled as nop. Considering this behavior, they
> >>>>>>should still be handled correctly, i.e., check execution conditions and generate
> >>>>>>exceptions when required. mwait and monitor may also be executed in real-mode
> >>>>>>and are not handled in that case.  This patch performs the emulation of
> >>>>>>monitor-mwait according to Intel SDM (other than checking whether interrupt can
> >>>>>>be used as a break event).
> >>>>>>
> >>>>>>Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> >>>>
> >>>>How about this instead (details in the commit log below) ? Please let
> >>>>me know what you think, and if you'd prefer me to send it out as a
> >>>>separate patch rather than a reply to this thread.
> >>>>
> >>>>Thanks,
> >>>>--Gabriel
> >>>
> >>>If there's an easy workaround, I'm inclined to agree.
> >>>We can always go back to Gabriel's patch (and then we'll need
> >>>Nadav's one too) but if we release a kernel with this
> >>>support it becomes an ABI and we can't go back.
> >>>
> >>>So let's be careful here, and revert the hack for 3.16.
> >>>
> >>>
> >>>Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>>
> >>Personally, I got a custom guest which requires mwait for executing correctly.
> >Can you elaborate on this guest a little bit. With nop implementation
> >for mwait the guest will hog a host cpu. Do you consider this to be
> >"executing correctly?"
> >
> >--
> 
> mwait is not as "clean" as it may appear. It encounters false wake-ups due
> to a variety of reasons, and any code need to recheck the wake-up condition
> afterwards. Actually, some CPUs had bugs that caused excessive wake-ups that
> degraded performance considerably (Nehalem, if I am not mistaken).
> Therefore, handling mwait as nop is logically correct (although it may
> degrade performance).
> 
> For the reference, if you look at the SDM 8.10.4, you'll see:
> "Multiple events other than a write to the triggering address range can
> cause a processor that executed MWAIT to wake up. These include events that
> would lead to voluntary or involuntary context switches, such as..."
> 
> Note the words "include" in the sentence "These include events". Software
> has no way of controlling whether it gets false wake-ups and cannot rely on
> the wake-up as indication to anything.
> 
That's all well and good and I didn't say that nop is not a valid
mwait implementation, it is, though there is a big difference between
"encounters false wake-ups" and never sleeps.  What I asked is do you
consider your guest hogging host cpu to be "executing correctly?". What
this guest is doing that such behaviour is tolerated and shouldn't it
be better to just poll for a condition you are waiting for instead of
executing expensive vmexits. This will also hog 100% host cpu, but will
be actually faster.

--
			Gleb.

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Nadav Amit]

On 6/19/14, 3:07 PM, Gleb Natapov wrote:
> On Thu, Jun 19, 2014 at 02:52:20PM +0300, Nadav Amit wrote:
>> On 6/19/14, 2:23 PM, Gleb Natapov wrote:
>>> On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
>>>>
>>>> On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>>> On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
>>>>>> On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
>>>>>>> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
>>>>>>>> mwait and monitor are currently handled as nop. Considering this behavior, they
>>>>>>>> should still be handled correctly, i.e., check execution conditions and generate
>>>>>>>> exceptions when required. mwait and monitor may also be executed in real-mode
>>>>>>>> and are not handled in that case.  This patch performs the emulation of
>>>>>>>> monitor-mwait according to Intel SDM (other than checking whether interrupt can
>>>>>>>> be used as a break event).
>>>>>>>>
>>>>>>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>>>>>
>>>>>> How about this instead (details in the commit log below) ? Please let
>>>>>> me know what you think, and if you'd prefer me to send it out as a
>>>>>> separate patch rather than a reply to this thread.
>>>>>>
>>>>>> Thanks,
>>>>>> --Gabriel
>>>>>
>>>>> If there's an easy workaround, I'm inclined to agree.
>>>>> We can always go back to Gabriel's patch (and then we'll need
>>>>> Nadav's one too) but if we release a kernel with this
>>>>> support it becomes an ABI and we can't go back.
>>>>>
>>>>> So let's be careful here, and revert the hack for 3.16.
>>>>>
>>>>>
>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>
>>>> Personally, I got a custom guest which requires mwait for executing correctly.
>>> Can you elaborate on this guest a little bit. With nop implementation
>>> for mwait the guest will hog a host cpu. Do you consider this to be
>>> "executing correctly?"
>>>
>>> --
>>
>> mwait is not as "clean" as it may appear. It encounters false wake-ups due
>> to a variety of reasons, and any code need to recheck the wake-up condition
>> afterwards. Actually, some CPUs had bugs that caused excessive wake-ups that
>> degraded performance considerably (Nehalem, if I am not mistaken).
>> Therefore, handling mwait as nop is logically correct (although it may
>> degrade performance).
>>
>> For the reference, if you look at the SDM 8.10.4, you'll see:
>> "Multiple events other than a write to the triggering address range can
>> cause a processor that executed MWAIT to wake up. These include events that
>> would lead to voluntary or involuntary context switches, such as..."
>>
>> Note the words "include" in the sentence "These include events". Software
>> has no way of controlling whether it gets false wake-ups and cannot rely on
>> the wake-up as indication to anything.
>>
> That's all well and good and I didn't say that nop is not a valid
> mwait implementation, it is, though there is a big difference between
> "encounters false wake-ups" and never sleeps.  What I asked is do you
> consider your guest hogging host cpu to be "executing correctly?". What
> this guest is doing that such behaviour is tolerated and shouldn't it
> be better to just poll for a condition you are waiting for instead of
> executing expensive vmexits. This will also hog 100% host cpu, but will
> be actually faster.
>
You are correct, but unfortunately I have no control over the guest 
workload. In this specific workload I do not care about performance but 
only about correctness.

Nadav

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Gleb Natapov]

On Thu, Jun 19, 2014 at 03:10:21PM +0300, Nadav Amit wrote:
> On 6/19/14, 3:07 PM, Gleb Natapov wrote:
> >On Thu, Jun 19, 2014 at 02:52:20PM +0300, Nadav Amit wrote:
> >>On 6/19/14, 2:23 PM, Gleb Natapov wrote:
> >>>On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
> >>>>
> >>>>On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>
> >>>>>On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
> >>>>>>On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
> >>>>>>>On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> >>>>>>>>mwait and monitor are currently handled as nop. Considering this behavior, they
> >>>>>>>>should still be handled correctly, i.e., check execution conditions and generate
> >>>>>>>>exceptions when required. mwait and monitor may also be executed in real-mode
> >>>>>>>>and are not handled in that case.  This patch performs the emulation of
> >>>>>>>>monitor-mwait according to Intel SDM (other than checking whether interrupt can
> >>>>>>>>be used as a break event).
> >>>>>>>>
> >>>>>>>>Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> >>>>>>
> >>>>>>How about this instead (details in the commit log below) ? Please let
> >>>>>>me know what you think, and if you'd prefer me to send it out as a
> >>>>>>separate patch rather than a reply to this thread.
> >>>>>>
> >>>>>>Thanks,
> >>>>>>--Gabriel
> >>>>>
> >>>>>If there's an easy workaround, I'm inclined to agree.
> >>>>>We can always go back to Gabriel's patch (and then we'll need
> >>>>>Nadav's one too) but if we release a kernel with this
> >>>>>support it becomes an ABI and we can't go back.
> >>>>>
> >>>>>So let's be careful here, and revert the hack for 3.16.
> >>>>>
> >>>>>
> >>>>>Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>Personally, I got a custom guest which requires mwait for executing correctly.
> >>>Can you elaborate on this guest a little bit. With nop implementation
> >>>for mwait the guest will hog a host cpu. Do you consider this to be
> >>>"executing correctly?"
> >>>
> >>>--
> >>
> >>mwait is not as "clean" as it may appear. It encounters false wake-ups due
> >>to a variety of reasons, and any code need to recheck the wake-up condition
> >>afterwards. Actually, some CPUs had bugs that caused excessive wake-ups that
> >>degraded performance considerably (Nehalem, if I am not mistaken).
> >>Therefore, handling mwait as nop is logically correct (although it may
> >>degrade performance).
> >>
> >>For the reference, if you look at the SDM 8.10.4, you'll see:
> >>"Multiple events other than a write to the triggering address range can
> >>cause a processor that executed MWAIT to wake up. These include events that
> >>would lead to voluntary or involuntary context switches, such as..."
> >>
> >>Note the words "include" in the sentence "These include events". Software
> >>has no way of controlling whether it gets false wake-ups and cannot rely on
> >>the wake-up as indication to anything.
> >>
> >That's all well and good and I didn't say that nop is not a valid
> >mwait implementation, it is, though there is a big difference between
> >"encounters false wake-ups" and never sleeps.  What I asked is do you
> >consider your guest hogging host cpu to be "executing correctly?". What
> >this guest is doing that such behaviour is tolerated and shouldn't it
> >be better to just poll for a condition you are waiting for instead of
> >executing expensive vmexits. This will also hog 100% host cpu, but will
> >be actually faster.
> >
> You are correct, but unfortunately I have no control over the guest
> workload. In this specific workload I do not care about performance but only
> about correctness.
> 
Fair enough. But can you at least hint what is this mysterious guest?

--
			Gleb.

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Michael S. Tsirkin]

On Thu, Jun 19, 2014 at 03:10:21PM +0300, Nadav Amit wrote:
> On 6/19/14, 3:07 PM, Gleb Natapov wrote:
> >On Thu, Jun 19, 2014 at 02:52:20PM +0300, Nadav Amit wrote:
> >>On 6/19/14, 2:23 PM, Gleb Natapov wrote:
> >>>On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
> >>>>
> >>>>On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>
> >>>>>On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
> >>>>>>On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
> >>>>>>>On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> >>>>>>>>mwait and monitor are currently handled as nop. Considering this behavior, they
> >>>>>>>>should still be handled correctly, i.e., check execution conditions and generate
> >>>>>>>>exceptions when required. mwait and monitor may also be executed in real-mode
> >>>>>>>>and are not handled in that case.  This patch performs the emulation of
> >>>>>>>>monitor-mwait according to Intel SDM (other than checking whether interrupt can
> >>>>>>>>be used as a break event).
> >>>>>>>>
> >>>>>>>>Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> >>>>>>
> >>>>>>How about this instead (details in the commit log below) ? Please let
> >>>>>>me know what you think, and if you'd prefer me to send it out as a
> >>>>>>separate patch rather than a reply to this thread.
> >>>>>>
> >>>>>>Thanks,
> >>>>>>--Gabriel
> >>>>>
> >>>>>If there's an easy workaround, I'm inclined to agree.
> >>>>>We can always go back to Gabriel's patch (and then we'll need
> >>>>>Nadav's one too) but if we release a kernel with this
> >>>>>support it becomes an ABI and we can't go back.
> >>>>>
> >>>>>So let's be careful here, and revert the hack for 3.16.
> >>>>>
> >>>>>
> >>>>>Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>Personally, I got a custom guest which requires mwait for executing correctly.
> >>>Can you elaborate on this guest a little bit. With nop implementation
> >>>for mwait the guest will hog a host cpu. Do you consider this to be
> >>>"executing correctly?"
> >>>
> >>>--
> >>
> >>mwait is not as "clean" as it may appear. It encounters false wake-ups due
> >>to a variety of reasons, and any code need to recheck the wake-up condition
> >>afterwards. Actually, some CPUs had bugs that caused excessive wake-ups that
> >>degraded performance considerably (Nehalem, if I am not mistaken).
> >>Therefore, handling mwait as nop is logically correct (although it may
> >>degrade performance).
> >>
> >>For the reference, if you look at the SDM 8.10.4, you'll see:
> >>"Multiple events other than a write to the triggering address range can
> >>cause a processor that executed MWAIT to wake up. These include events that
> >>would lead to voluntary or involuntary context switches, such as..."
> >>
> >>Note the words "include" in the sentence "These include events". Software
> >>has no way of controlling whether it gets false wake-ups and cannot rely on
> >>the wake-up as indication to anything.
> >>
> >That's all well and good and I didn't say that nop is not a valid
> >mwait implementation, it is, though there is a big difference between
> >"encounters false wake-ups" and never sleeps.  What I asked is do you
> >consider your guest hogging host cpu to be "executing correctly?". What
> >this guest is doing that such behaviour is tolerated and shouldn't it
> >be better to just poll for a condition you are waiting for instead of
> >executing expensive vmexits. This will also hog 100% host cpu, but will
> >be actually faster.
> >
> You are correct, but unfortunately I have no control over the guest
> workload. In this specific workload I do not care about performance but only
> about correctness.
> 
> Nadav

No one prevents you from patching your kernel to run this workload.  But
is this of use to anyone else? If yes why?

-- 
MST

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Nadav Amit]

On 6/19/14, 3:17 PM, Michael S. Tsirkin wrote:
> On Thu, Jun 19, 2014 at 03:10:21PM +0300, Nadav Amit wrote:
>> On 6/19/14, 3:07 PM, Gleb Natapov wrote:
>>> On Thu, Jun 19, 2014 at 02:52:20PM +0300, Nadav Amit wrote:
>>>> On 6/19/14, 2:23 PM, Gleb Natapov wrote:
>>>>> On Thu, Jun 19, 2014 at 01:53:36PM +0300, Nadav Amit wrote:
>>>>>>
>>>>>> On Jun 19, 2014, at 1:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>
>>>>>>> On Wed, Jun 18, 2014 at 02:46:01PM -0400, Gabriel L. Somlo wrote:
>>>>>>>> On Wed, Jun 18, 2014 at 10:59:14AM -0700, Eric Northup wrote:
>>>>>>>>> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
>>>>>>>>>> mwait and monitor are currently handled as nop. Considering this behavior, they
>>>>>>>>>> should still be handled correctly, i.e., check execution conditions and generate
>>>>>>>>>> exceptions when required. mwait and monitor may also be executed in real-mode
>>>>>>>>>> and are not handled in that case.  This patch performs the emulation of
>>>>>>>>>> monitor-mwait according to Intel SDM (other than checking whether interrupt can
>>>>>>>>>> be used as a break event).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>>>>>>>
>>>>>>>> How about this instead (details in the commit log below) ? Please let
>>>>>>>> me know what you think, and if you'd prefer me to send it out as a
>>>>>>>> separate patch rather than a reply to this thread.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> --Gabriel
>>>>>>>
>>>>>>> If there's an easy workaround, I'm inclined to agree.
>>>>>>> We can always go back to Gabriel's patch (and then we'll need
>>>>>>> Nadav's one too) but if we release a kernel with this
>>>>>>> support it becomes an ABI and we can't go back.
>>>>>>>
>>>>>>> So let's be careful here, and revert the hack for 3.16.
>>>>>>>
>>>>>>>
>>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>>
>>>>>> Personally, I got a custom guest which requires mwait for executing correctly.
>>>>> Can you elaborate on this guest a little bit. With nop implementation
>>>>> for mwait the guest will hog a host cpu. Do you consider this to be
>>>>> "executing correctly?"
>>>>>
>>>>> --
>>>>
>>>> mwait is not as "clean" as it may appear. It encounters false wake-ups due
>>>> to a variety of reasons, and any code need to recheck the wake-up condition
>>>> afterwards. Actually, some CPUs had bugs that caused excessive wake-ups that
>>>> degraded performance considerably (Nehalem, if I am not mistaken).
>>>> Therefore, handling mwait as nop is logically correct (although it may
>>>> degrade performance).
>>>>
>>>> For the reference, if you look at the SDM 8.10.4, you'll see:
>>>> "Multiple events other than a write to the triggering address range can
>>>> cause a processor that executed MWAIT to wake up. These include events that
>>>> would lead to voluntary or involuntary context switches, such as..."
>>>>
>>>> Note the words "include" in the sentence "These include events". Software
>>>> has no way of controlling whether it gets false wake-ups and cannot rely on
>>>> the wake-up as indication to anything.
>>>>
>>> That's all well and good and I didn't say that nop is not a valid
>>> mwait implementation, it is, though there is a big difference between
>>> "encounters false wake-ups" and never sleeps.  What I asked is do you
>>> consider your guest hogging host cpu to be "executing correctly?". What
>>> this guest is doing that such behaviour is tolerated and shouldn't it
>>> be better to just poll for a condition you are waiting for instead of
>>> executing expensive vmexits. This will also hog 100% host cpu, but will
>>> be actually faster.
>>>
>> You are correct, but unfortunately I have no control over the guest
>> workload. In this specific workload I do not care about performance but only
>> about correctness.
>>
>> Nadav
>
> No one prevents you from patching your kernel to run this workload.  But
> is this of use to anyone else? If yes why?
>
I do not say it should be the default behavior, and I can try to push to 
qemu some setting to turn it on by demand.

Anyhow, I believe there are cases you may want mwait support - either an 
OS X guest which was not modified to run without mwait, or for debugging 
the monitor-mwait flow of a guest OS.

I am not going to argue too much. Since I was under the impression there 
are needs for mwait, other than mine, I thought it would make all of our 
lives easier to have a better implementation.

Nadav

Re: [PATCH 3/3] KVM: x86: correct mwait and monitor emulation

[Paolo Bonzini]

Il 18/06/2014 19:59, Eric Northup ha scritto:
> On Wed, Jun 18, 2014 at 7:19 AM, Nadav Amit <namit@cs.technion.ac.il> wrote:
>> mwait and monitor are currently handled as nop. Considering this behavior, they
>> should still be handled correctly, i.e., check execution conditions and generate
>> exceptions when required. mwait and monitor may also be executed in real-mode
>> and are not handled in that case.  This patch performs the emulation of
>> monitor-mwait according to Intel SDM (other than checking whether interrupt can
>> be used as a break event).
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>>  arch/x86/kvm/emulate.c | 41 +++++++++++++++++++++++++++++++++++++++--
>>  arch/x86/kvm/svm.c     | 22 ++--------------------
>>  arch/x86/kvm/vmx.c     | 27 +++++++++++----------------
>>  3 files changed, 52 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index ef7a5a0..424b58d 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -3344,6 +3344,43 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
>>         return X86EMUL_CONTINUE;
>>  }
>>
>> +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;
>
> I'd request:
>
> u32 ebx, ecx, edx, eax = 1;
> ctxt->opt->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> if (!(ecx & FFL(MWAIT)))
>         return emulate_ud(ctxt);
>
> and also in em_mwait.

Ignoring the fact that this should never be true 
(KVM_GET_SUPPORTED_CPUID never reports the MWAIT bit), why should 
MONITOR and MWAIT be special?  We do not do this kind of check for SSE 
or AVX instructions.

An alternative is to record the address that was being waited on, and 
invoke PLE (kvm_vcpu_on_spin) if the current address matches the last 
one.  A VMEXIT + emulation takes a couple thousand cycles, which is the 
same order of magnitude as the PLE window.

Even if there is a workaround, I don't think reverting the patch is 
necessary.  The patch was there for a fringe case anyway (recent 
versions of Mac OS X get CPUID right), so I don't think the availability 
of a work around changes the assessment of how ugly/useful MONITOR/MWAIT is.

Paolo