[PATCH 0/4] Vendor specific instructions and the emulator

[PATCH 0/4] Vendor specific instructions and the emulator

[Avi Kivity]

Currently we have some ad-hoc code in x86.c to restrict #UD emulation to
expected instructions (that is, vendor specific instructions).  This patchset
replaces the ad-hoc code with proper emulator support using decode tables.

Avi Kivity (4):
  KVM: Fix x86_decode_insn() return code check
  KVM: Simplify exit path on decode failure
  KVM: x86 emulator: vendor specific instructions
  KVM: Drop ad-hoc vendor specific instruction restriction

 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |   12 +++++++++---
 arch/x86/kvm/x86.c                 |   35 +++++------------------------------
 3 files changed, 15 insertions(+), 33 deletions(-)

[PATCH 1/4] KVM: Fix x86_decode_insn() return code check

[Avi Kivity]

x86_decode_insn() doesn't return X86EMUL_* values, it returns
EMULATION_* codes.  Adjust the check.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fa708c9..b20499d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4391,7 +4391,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 		vcpu->arch.emulate_ctxt.perm_ok = false;
 
 		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, insn, insn_len);
-		if (r == X86EMUL_PROPAGATE_FAULT)
+		if (r != EMULATION_OK)
 			goto done;
 
 		trace_kvm_emulate_insn_start(vcpu);
-- 
1.7.1

Re: [PATCH 1/4] KVM: Fix x86_decode_insn() return code check

[Avi Kivity]

On 01/04/2011 03:14 PM, Avi Kivity wrote:
> x86_decode_insn() doesn't return X86EMUL_* values, it returns
> EMULATION_* codes.  Adjust the check.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>
> ---
>   arch/x86/kvm/x86.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fa708c9..b20499d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4391,7 +4391,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>   		vcpu->arch.emulate_ctxt.perm_ok = false;
>
>   		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, insn, insn_len);
> -		if (r == X86EMUL_PROPAGATE_FAULT)
> +		if (r != EMULATION_OK)
>   			goto done;
>
>   		trace_kvm_emulate_insn_start(vcpu);

Strangely, this patch causes a failure in Windows XP.  Dropping until 
submitter investigates and fixes.

-- 
error compiling committee.c: too many arguments to function

[PATCH 2/4] KVM: Simplify exit path on decode failure

[Avi Kivity]

'goto done' leads to a maze of checks and actions, all pointless.  Bail
out immediately if we can't decode.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b20499d..b085ac3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4392,7 +4392,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 
 		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, insn, insn_len);
 		if (r != EMULATION_OK)
-			goto done;
+			return EMULATE_FAIL;
 
 		trace_kvm_emulate_insn_start(vcpu);
 
-- 
1.7.1

Re: [PATCH 2/4] KVM: Simplify exit path on decode failure

[Jan Kiszka]

Am 04.01.2011 14:14, Avi Kivity wrote:
> 'goto done' leads to a maze of checks and actions, all pointless.  Bail
> out immediately if we can't decode.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/x86.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b20499d..b085ac3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4392,7 +4392,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>  
>  		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, insn, insn_len);
>  		if (r != EMULATION_OK)
> -			goto done;
> +			return EMULATE_FAIL;

This leaves 'done' unused behind, and the compiler mourns.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 2/4] KVM: Simplify exit path on decode failure

[Avi Kivity]

On 01/10/2011 07:17 PM, Jan Kiszka wrote:
> Am 04.01.2011 14:14, Avi Kivity wrote:
> >  'goto done' leads to a maze of checks and actions, all pointless.  Bail
> >  out immediately if we can't decode.
> >
> >  Signed-off-by: Avi Kivity<avi@redhat.com>
> >  ---
> >   arch/x86/kvm/x86.c |    2 +-
> >   1 files changed, 1 insertions(+), 1 deletions(-)
> >
> >  diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >  index b20499d..b085ac3 100644
> >  --- a/arch/x86/kvm/x86.c
> >  +++ b/arch/x86/kvm/x86.c
> >  @@ -4392,7 +4392,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> >
> >   		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, insn, insn_len);
> >   		if (r != EMULATION_OK)
> >  -			goto done;
> >  +			return EMULATE_FAIL;
>
> This leaves 'done' unused behind, and the compiler mourns.

It's already in 'next'.  Marcelo can you amend it?

-- 
error compiling committee.c: too many arguments to function

[PATCH 3/4] KVM: x86 emulator: vendor specific instructions

[Avi Kivity]

Mark some instructions as vendor specific, and allow the caller to request
emulation only of vendor specific instructions.  This is useful in some
circumstances (responding to a #UD fault).

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |   12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 8e37deb..50ebc32 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -239,6 +239,7 @@ struct x86_emulate_ctxt {
 	int interruptibility;
 
 	bool perm_ok; /* do not check permissions if true */
+	bool only_vendor_specific_insn;
 
 	bool have_exception;
 	struct x86_exception exception;
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 02a0041..ad46239 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -76,6 +76,7 @@
 #define Group       (1<<14)     /* Bits 3:5 of modrm byte extend opcode */
 #define GroupDual   (1<<15)     /* Alternate decoding of mod == 3 */
 /* Misc flags */
+#define VendorSpecific (1<<22) /* Vendor specific instruction */
 #define NoAccess    (1<<23) /* Don't access memory (lea/invlpg/verr etc) */
 #define Op3264      (1<<24) /* Operand is 64b in long mode, 32b otherwise */
 #define Undefined   (1<<25) /* No Such Instruction */
@@ -2365,7 +2366,8 @@ static struct group_dual group7 = { {
 	D(SrcMem16 | ModRM | Mov | Priv),
 	D(SrcMem | ModRM | ByteOp | Priv | NoAccess),
 }, {
-	D(SrcNone | ModRM | Priv), N, N, D(SrcNone | ModRM | Priv),
+	D(SrcNone | ModRM | Priv | VendorSpecific), N,
+	N, D(SrcNone | ModRM | Priv | VendorSpecific),
 	D(SrcNone | ModRM | DstMem | Mov), N,
 	D(SrcMem16 | ModRM | Mov | Priv), N,
 } };
@@ -2489,7 +2491,7 @@ static struct opcode opcode_table[256] = {
 static struct opcode twobyte_table[256] = {
 	/* 0x00 - 0x0F */
 	N, GD(0, &group7), N, N,
-	N, D(ImplicitOps), D(ImplicitOps | Priv), N,
+	N, D(ImplicitOps | VendorSpecific), D(ImplicitOps | Priv), N,
 	D(ImplicitOps | Priv), D(ImplicitOps | Priv), N, N,
 	N, D(ImplicitOps | ModRM), N, N,
 	/* 0x10 - 0x1F */
@@ -2502,7 +2504,8 @@ static struct opcode twobyte_table[256] = {
 	/* 0x30 - 0x3F */
 	D(ImplicitOps | Priv), I(ImplicitOps, em_rdtsc),
 	D(ImplicitOps | Priv), N,
-	D(ImplicitOps), D(ImplicitOps | Priv), N, N,
+	D(ImplicitOps | VendorSpecific), D(ImplicitOps | Priv | VendorSpecific),
+	N, N,
 	N, N, N, N, N, N, N, N,
 	/* 0x40 - 0x4F */
 	X16(D(DstReg | SrcMem | ModRM | Mov)),
@@ -2741,6 +2744,9 @@ done_prefixes:
 	if (c->d == 0 || (c->d & Undefined))
 		return -1;
 
+	if (!(c->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
+		return -1;
+
 	if (mode == X86EMUL_MODE_PROT64 && (c->d & Stack))
 		c->op_bytes = 8;
 
-- 
1.7.1

[PATCH 4/4] KVM: Drop ad-hoc vendor specific instruction restriction

[Avi Kivity]

Use the new support in the emulator, and drop the ad-hoc code in x86.c.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |   31 +++----------------------------
 1 files changed, 3 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b085ac3..8652643 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4390,39 +4390,14 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 		vcpu->arch.emulate_ctxt.have_exception = false;
 		vcpu->arch.emulate_ctxt.perm_ok = false;
 
+		vcpu->arch.emulate_ctxt.only_vendor_specific_insn
+			= emulation_type & EMULTYPE_TRAP_UD;
+
 		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, insn, insn_len);
 		if (r != EMULATION_OK)
 			return EMULATE_FAIL;
 
 		trace_kvm_emulate_insn_start(vcpu);
-
-		/* Only allow emulation of specific instructions on #UD
-		 * (namely VMMCALL, sysenter, sysexit, syscall)*/
-		if (emulation_type & EMULTYPE_TRAP_UD) {
-			if (!c->twobyte)
-				return EMULATE_FAIL;
-			switch (c->b) {
-			case 0x01: /* VMMCALL */
-				if (c->modrm_mod != 3 || c->modrm_rm != 1)
-					return EMULATE_FAIL;
-				break;
-			case 0x34: /* sysenter */
-			case 0x35: /* sysexit */
-				if (c->modrm_mod != 0 || c->modrm_rm != 0)
-					return EMULATE_FAIL;
-				break;
-			case 0x05: /* syscall */
-				if (c->modrm_mod != 0 || c->modrm_rm != 0)
-					return EMULATE_FAIL;
-				break;
-			default:
-				return EMULATE_FAIL;
-			}
-
-			if (!(c->modrm_reg == 0 || c->modrm_reg == 3))
-				return EMULATE_FAIL;
-		}
-
 		++vcpu->stat.insn_emulation;
 		if (r)  {
 			if (reexecute_instruction(vcpu, cr2))
-- 
1.7.1

Re: [PATCH 0/4] Vendor specific instructions and the emulator

[Marcelo Tosatti]

On Tue, Jan 04, 2011 at 03:14:40PM +0200, Avi Kivity wrote:
> Currently we have some ad-hoc code in x86.c to restrict #UD emulation to
> expected instructions (that is, vendor specific instructions).  This patchset
> replaces the ad-hoc code with proper emulator support using decode tables.
> 
> Avi Kivity (4):
>   KVM: Fix x86_decode_insn() return code check
>   KVM: Simplify exit path on decode failure
>   KVM: x86 emulator: vendor specific instructions
>   KVM: Drop ad-hoc vendor specific instruction restriction
> 
>  arch/x86/include/asm/kvm_emulate.h |    1 +
>  arch/x86/kvm/emulate.c             |   12 +++++++++---
>  arch/x86/kvm/x86.c                 |   35 +++++------------------------------
>  3 files changed, 15 insertions(+), 33 deletions(-)

Applied, thanks.