[PATCH 1/3] KVM: x86 emulator: Make one byte instruction emulation separate

[PATCH 1/3] KVM: x86 emulator: Make one byte instruction emulation separate

[Takuya Yoshikawa]

x86_emulate_insn() is too long and has many confusing goto statements.

This patch is the first part of a work which tries to split it into
a few meaningful functions: just encapsulates the switch statement for
the one byte instruction emulation as emulate_onebyte_insn().

Note that goto done with rc set to X86EMUL_UNHANDLEABLE will result in
returning EMULATION_FAILED which is defined as -1.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |  179 +++++++++++++++++++++++++++---------------------
 1 files changed, 100 insertions(+), 79 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index ad46239..18d7c07 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2945,88 +2945,14 @@ static bool string_insn_completed(struct x86_emulate_ctxt *ctxt)
 	return false;
 }
 
-int
-x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
+static int emulate_onebyte_insn(struct x86_emulate_ctxt *ctxt,
+				struct x86_emulate_ops *ops,
+				struct decode_cache *c,
+				bool *need_writeback)
 {
-	struct x86_emulate_ops *ops = ctxt->ops;
-	u64 msr_data;
-	struct decode_cache *c = &ctxt->decode;
 	int rc = X86EMUL_CONTINUE;
-	int saved_dst_type = c->dst.type;
 	int irq; /* Used for int 3, int, and into */
 
-	ctxt->decode.mem_read.pos = 0;
-
-	if (ctxt->mode == X86EMUL_MODE_PROT64 && (c->d & No64)) {
-		rc = emulate_ud(ctxt);
-		goto done;
-	}
-
-	/* LOCK prefix is allowed only with some instructions */
-	if (c->lock_prefix && (!(c->d & Lock) || c->dst.type != OP_MEM)) {
-		rc = emulate_ud(ctxt);
-		goto done;
-	}
-
-	if ((c->d & SrcMask) == SrcMemFAddr && c->src.type != OP_MEM) {
-		rc = emulate_ud(ctxt);
-		goto done;
-	}
-
-	/* Privileged instruction can be executed only in CPL=0 */
-	if ((c->d & Priv) && ops->cpl(ctxt->vcpu)) {
-		rc = emulate_gp(ctxt, 0);
-		goto done;
-	}
-
-	if (c->rep_prefix && (c->d & String)) {
-		/* All REP prefixes have the same first termination condition */
-		if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0) {
-			ctxt->eip = c->eip;
-			goto done;
-		}
-	}
-
-	if ((c->src.type == OP_MEM) && !(c->d & NoAccess)) {
-		rc = read_emulated(ctxt, ops, linear(ctxt, c->src.addr.mem),
-					c->src.valptr, c->src.bytes);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-		c->src.orig_val64 = c->src.val64;
-	}
-
-	if (c->src2.type == OP_MEM) {
-		rc = read_emulated(ctxt, ops, linear(ctxt, c->src2.addr.mem),
-					&c->src2.val, c->src2.bytes);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-	}
-
-	if ((c->d & DstMask) == ImplicitOps)
-		goto special_insn;
-
-
-	if ((c->dst.type == OP_MEM) && !(c->d & Mov)) {
-		/* optimisation - avoid slow emulated read if Mov */
-		rc = read_emulated(ctxt, ops, linear(ctxt, c->dst.addr.mem),
-				   &c->dst.val, c->dst.bytes);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-	}
-	c->dst.orig_val = c->dst.val;
-
-special_insn:
-
-	if (c->execute) {
-		rc = c->execute(ctxt);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-		goto writeback;
-	}
-
-	if (c->twobyte)
-		goto twobyte_insn;
-
 	switch (c->b) {
 	case 0x00 ... 0x05:
 	      add:		/* add */
@@ -3371,7 +3297,102 @@ special_insn:
 		goto cannot_emulate;
 	}
 
-	if (rc != X86EMUL_CONTINUE)
+	*need_writeback = (rc == X86EMUL_CONTINUE);
+	return rc;
+
+done:
+	*need_writeback = false;
+	return rc;
+
+cannot_emulate:
+	*need_writeback = false;
+	return X86EMUL_UNHANDLEABLE;
+}
+
+int
+x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
+{
+	struct x86_emulate_ops *ops = ctxt->ops;
+	u64 msr_data;
+	struct decode_cache *c = &ctxt->decode;
+	int rc = X86EMUL_CONTINUE;
+	int saved_dst_type = c->dst.type;
+	bool need_writeback;
+
+	ctxt->decode.mem_read.pos = 0;
+
+	if (ctxt->mode == X86EMUL_MODE_PROT64 && (c->d & No64)) {
+		rc = emulate_ud(ctxt);
+		goto done;
+	}
+
+	/* LOCK prefix is allowed only with some instructions */
+	if (c->lock_prefix && (!(c->d & Lock) || c->dst.type != OP_MEM)) {
+		rc = emulate_ud(ctxt);
+		goto done;
+	}
+
+	if ((c->d & SrcMask) == SrcMemFAddr && c->src.type != OP_MEM) {
+		rc = emulate_ud(ctxt);
+		goto done;
+	}
+
+	/* Privileged instruction can be executed only in CPL=0 */
+	if ((c->d & Priv) && ops->cpl(ctxt->vcpu)) {
+		rc = emulate_gp(ctxt, 0);
+		goto done;
+	}
+
+	if (c->rep_prefix && (c->d & String)) {
+		/* All REP prefixes have the same first termination condition */
+		if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0) {
+			ctxt->eip = c->eip;
+			goto done;
+		}
+	}
+
+	if ((c->src.type == OP_MEM) && !(c->d & NoAccess)) {
+		rc = read_emulated(ctxt, ops, linear(ctxt, c->src.addr.mem),
+					c->src.valptr, c->src.bytes);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		c->src.orig_val64 = c->src.val64;
+	}
+
+	if (c->src2.type == OP_MEM) {
+		rc = read_emulated(ctxt, ops, linear(ctxt, c->src2.addr.mem),
+					&c->src2.val, c->src2.bytes);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+	}
+
+	if ((c->d & DstMask) == ImplicitOps)
+		goto special_insn;
+
+
+	if ((c->dst.type == OP_MEM) && !(c->d & Mov)) {
+		/* optimisation - avoid slow emulated read if Mov */
+		rc = read_emulated(ctxt, ops, linear(ctxt, c->dst.addr.mem),
+				   &c->dst.val, c->dst.bytes);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+	}
+	c->dst.orig_val = c->dst.val;
+
+special_insn:
+
+	if (c->execute) {
+		rc = c->execute(ctxt);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		goto writeback;
+	}
+
+	if (c->twobyte)
+		goto twobyte_insn;
+
+	rc = emulate_onebyte_insn(ctxt, ops, c, &need_writeback);
+	if (!need_writeback)
 		goto done;
 
 writeback:
-- 
1.7.1

[PATCH 2/3] KVM: x86 emulator: Make two byte instruction emulation separate

[Takuya Yoshikawa]

This patch is the second part of a work which tries to split
x86_emulate_insn() into a few meaningful functions: just encapsulates
the switch statement for the two byte instruction emulation as
emulate_twobyte_insn().

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |  286 ++++++++++++++++++++++++++----------------------
 1 files changed, 153 insertions(+), 133 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 18d7c07..7aa93df 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3309,142 +3309,14 @@ cannot_emulate:
 	return X86EMUL_UNHANDLEABLE;
 }
 
-int
-x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
+static int emulate_twobyte_insn(struct x86_emulate_ctxt *ctxt,
+				struct x86_emulate_ops *ops,
+				struct decode_cache *c,
+				bool *need_writeback)
 {
-	struct x86_emulate_ops *ops = ctxt->ops;
-	u64 msr_data;
-	struct decode_cache *c = &ctxt->decode;
 	int rc = X86EMUL_CONTINUE;
-	int saved_dst_type = c->dst.type;
-	bool need_writeback;
-
-	ctxt->decode.mem_read.pos = 0;
-
-	if (ctxt->mode == X86EMUL_MODE_PROT64 && (c->d & No64)) {
-		rc = emulate_ud(ctxt);
-		goto done;
-	}
-
-	/* LOCK prefix is allowed only with some instructions */
-	if (c->lock_prefix && (!(c->d & Lock) || c->dst.type != OP_MEM)) {
-		rc = emulate_ud(ctxt);
-		goto done;
-	}
-
-	if ((c->d & SrcMask) == SrcMemFAddr && c->src.type != OP_MEM) {
-		rc = emulate_ud(ctxt);
-		goto done;
-	}
-
-	/* Privileged instruction can be executed only in CPL=0 */
-	if ((c->d & Priv) && ops->cpl(ctxt->vcpu)) {
-		rc = emulate_gp(ctxt, 0);
-		goto done;
-	}
-
-	if (c->rep_prefix && (c->d & String)) {
-		/* All REP prefixes have the same first termination condition */
-		if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0) {
-			ctxt->eip = c->eip;
-			goto done;
-		}
-	}
-
-	if ((c->src.type == OP_MEM) && !(c->d & NoAccess)) {
-		rc = read_emulated(ctxt, ops, linear(ctxt, c->src.addr.mem),
-					c->src.valptr, c->src.bytes);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-		c->src.orig_val64 = c->src.val64;
-	}
-
-	if (c->src2.type == OP_MEM) {
-		rc = read_emulated(ctxt, ops, linear(ctxt, c->src2.addr.mem),
-					&c->src2.val, c->src2.bytes);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-	}
-
-	if ((c->d & DstMask) == ImplicitOps)
-		goto special_insn;
-
-
-	if ((c->dst.type == OP_MEM) && !(c->d & Mov)) {
-		/* optimisation - avoid slow emulated read if Mov */
-		rc = read_emulated(ctxt, ops, linear(ctxt, c->dst.addr.mem),
-				   &c->dst.val, c->dst.bytes);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-	}
-	c->dst.orig_val = c->dst.val;
-
-special_insn:
-
-	if (c->execute) {
-		rc = c->execute(ctxt);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-		goto writeback;
-	}
-
-	if (c->twobyte)
-		goto twobyte_insn;
-
-	rc = emulate_onebyte_insn(ctxt, ops, c, &need_writeback);
-	if (!need_writeback)
-		goto done;
-
-writeback:
-	rc = writeback(ctxt, ops);
-	if (rc != X86EMUL_CONTINUE)
-		goto done;
-
-	/*
-	 * restore dst type in case the decoding will be reused
-	 * (happens for string instruction )
-	 */
-	c->dst.type = saved_dst_type;
-
-	if ((c->d & SrcMask) == SrcSI)
-		string_addr_inc(ctxt, seg_override(ctxt, ops, c),
-				VCPU_REGS_RSI, &c->src);
-
-	if ((c->d & DstMask) == DstDI)
-		string_addr_inc(ctxt, VCPU_SREG_ES, VCPU_REGS_RDI,
-				&c->dst);
-
-	if (c->rep_prefix && (c->d & String)) {
-		struct read_cache *r = &ctxt->decode.io_read;
-		register_address_increment(c, &c->regs[VCPU_REGS_RCX], -1);
-
-		if (!string_insn_completed(ctxt)) {
-			/*
-			 * Re-enter guest when pio read ahead buffer is empty
-			 * or, if it is not used, after each 1024 iteration.
-			 */
-			if ((r->end != 0 || c->regs[VCPU_REGS_RCX] & 0x3ff) &&
-			    (r->end == 0 || r->end != r->pos)) {
-				/*
-				 * Reset read cache. Usually happens before
-				 * decode, but since instruction is restarted
-				 * we have to do it here.
-				 */
-				ctxt->decode.mem_read.end = 0;
-				return EMULATION_RESTART;
-			}
-			goto done; /* skip rip writeback */
-		}
-	}
-
-	ctxt->eip = c->eip;
-
-done:
-	if (rc == X86EMUL_PROPAGATE_FAULT)
-		ctxt->have_exception = true;
-	return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK;
+	u64 msr_data;
 
-twobyte_insn:
 	switch (c->b) {
 	case 0x01: /* lgdt, lidt, lmsw */
 		switch (c->modrm_reg) {
@@ -3748,9 +3620,157 @@ twobyte_insn:
 		goto cannot_emulate;
 	}
 
+	*need_writeback = (rc == X86EMUL_CONTINUE);
+	return rc;
+
+done:
+	*need_writeback = false;
+	return rc;
+
+cannot_emulate:
+	*need_writeback = false;
+	return X86EMUL_UNHANDLEABLE;
+}
+
+int
+x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
+{
+	struct x86_emulate_ops *ops = ctxt->ops;
+	struct decode_cache *c = &ctxt->decode;
+	int rc = X86EMUL_CONTINUE;
+	int saved_dst_type = c->dst.type;
+	bool need_writeback;
+
+	ctxt->decode.mem_read.pos = 0;
+
+	if (ctxt->mode == X86EMUL_MODE_PROT64 && (c->d & No64)) {
+		rc = emulate_ud(ctxt);
+		goto done;
+	}
+
+	/* LOCK prefix is allowed only with some instructions */
+	if (c->lock_prefix && (!(c->d & Lock) || c->dst.type != OP_MEM)) {
+		rc = emulate_ud(ctxt);
+		goto done;
+	}
+
+	if ((c->d & SrcMask) == SrcMemFAddr && c->src.type != OP_MEM) {
+		rc = emulate_ud(ctxt);
+		goto done;
+	}
+
+	/* Privileged instruction can be executed only in CPL=0 */
+	if ((c->d & Priv) && ops->cpl(ctxt->vcpu)) {
+		rc = emulate_gp(ctxt, 0);
+		goto done;
+	}
+
+	if (c->rep_prefix && (c->d & String)) {
+		/* All REP prefixes have the same first termination condition */
+		if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0) {
+			ctxt->eip = c->eip;
+			goto done;
+		}
+	}
+
+	if ((c->src.type == OP_MEM) && !(c->d & NoAccess)) {
+		rc = read_emulated(ctxt, ops, linear(ctxt, c->src.addr.mem),
+					c->src.valptr, c->src.bytes);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		c->src.orig_val64 = c->src.val64;
+	}
+
+	if (c->src2.type == OP_MEM) {
+		rc = read_emulated(ctxt, ops, linear(ctxt, c->src2.addr.mem),
+					&c->src2.val, c->src2.bytes);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+	}
+
+	if ((c->d & DstMask) == ImplicitOps)
+		goto special_insn;
+
+
+	if ((c->dst.type == OP_MEM) && !(c->d & Mov)) {
+		/* optimisation - avoid slow emulated read if Mov */
+		rc = read_emulated(ctxt, ops, linear(ctxt, c->dst.addr.mem),
+				   &c->dst.val, c->dst.bytes);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+	}
+	c->dst.orig_val = c->dst.val;
+
+special_insn:
+
+	if (c->execute) {
+		rc = c->execute(ctxt);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+		goto writeback;
+	}
+
+	if (c->twobyte)
+		goto twobyte_insn;
+
+	rc = emulate_onebyte_insn(ctxt, ops, c, &need_writeback);
+	if (!need_writeback)
+		goto done;
+
+writeback:
+	rc = writeback(ctxt, ops);
 	if (rc != X86EMUL_CONTINUE)
 		goto done;
 
+	/*
+	 * restore dst type in case the decoding will be reused
+	 * (happens for string instruction )
+	 */
+	c->dst.type = saved_dst_type;
+
+	if ((c->d & SrcMask) == SrcSI)
+		string_addr_inc(ctxt, seg_override(ctxt, ops, c),
+				VCPU_REGS_RSI, &c->src);
+
+	if ((c->d & DstMask) == DstDI)
+		string_addr_inc(ctxt, VCPU_SREG_ES, VCPU_REGS_RDI,
+				&c->dst);
+
+	if (c->rep_prefix && (c->d & String)) {
+		struct read_cache *r = &ctxt->decode.io_read;
+		register_address_increment(c, &c->regs[VCPU_REGS_RCX], -1);
+
+		if (!string_insn_completed(ctxt)) {
+			/*
+			 * Re-enter guest when pio read ahead buffer is empty
+			 * or, if it is not used, after each 1024 iteration.
+			 */
+			if ((r->end != 0 || c->regs[VCPU_REGS_RCX] & 0x3ff) &&
+			    (r->end == 0 || r->end != r->pos)) {
+				/*
+				 * Reset read cache. Usually happens before
+				 * decode, but since instruction is restarted
+				 * we have to do it here.
+				 */
+				ctxt->decode.mem_read.end = 0;
+				return EMULATION_RESTART;
+			}
+			goto done; /* skip rip writeback */
+		}
+	}
+
+	ctxt->eip = c->eip;
+
+done:
+	if (rc == X86EMUL_PROPAGATE_FAULT)
+		ctxt->have_exception = true;
+	return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK;
+
+twobyte_insn:
+	rc = emulate_twobyte_insn(ctxt, ops, c, &need_writeback);
+	if (!need_writeback)
+		goto done;
+
 	goto writeback;
 
 cannot_emulate:
-- 
1.7.1

[PATCH 3/3] KVM: x86 emulator: Remove unnecessary goto statements from x86_emulate_insn()

[Takuya Yoshikawa]

This patch is the last part of a work which tries to split
x86_emulate_insn() into a few meaningful functions: removes unnecessary
goto statements based on the former two patches.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 7aa93df..b5fefcb 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3632,8 +3632,7 @@ cannot_emulate:
 	return X86EMUL_UNHANDLEABLE;
 }
 
-int
-x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
+int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 {
 	struct x86_emulate_ops *ops = ctxt->ops;
 	struct decode_cache *c = &ctxt->decode;
@@ -3711,9 +3710,10 @@ special_insn:
 	}
 
 	if (c->twobyte)
-		goto twobyte_insn;
+		rc = emulate_twobyte_insn(ctxt, ops, c, &need_writeback);
+	else
+		rc = emulate_onebyte_insn(ctxt, ops, c, &need_writeback);
 
-	rc = emulate_onebyte_insn(ctxt, ops, c, &need_writeback);
 	if (!need_writeback)
 		goto done;
 
@@ -3765,14 +3765,4 @@ done:
 	if (rc == X86EMUL_PROPAGATE_FAULT)
 		ctxt->have_exception = true;
 	return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK;
-
-twobyte_insn:
-	rc = emulate_twobyte_insn(ctxt, ops, c, &need_writeback);
-	if (!need_writeback)
-		goto done;
-
-	goto writeback;
-
-cannot_emulate:
-	return -1;
 }
-- 
1.7.1

Re: [PATCH 1/3] KVM: x86 emulator: Make one byte instruction emulation separate

[Avi Kivity]

On 03/10/2011 09:35 AM, Takuya Yoshikawa wrote:
> x86_emulate_insn() is too long and has many confusing goto statements.
>
> This patch is the first part of a work which tries to split it into
> a few meaningful functions: just encapsulates the switch statement for
> the one byte instruction emulation as emulate_onebyte_insn().

I, too, dislike the switch statements.  So I started on a path to 
eliminate it completely - take a look at struct opcode::execute.  If 
present, it is executed instead of the code in the switch statements.

The plan is to migrate all of the contents of the switch statements into 
->execute() callbacks.  This way, all of the information about an 
instruction is present in the decode tables.

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

Re: [PATCH 1/3] KVM: x86 emulator: Make one byte instruction emulation separate

[Takuya Yoshikawa]

On Thu, 10 Mar 2011 11:05:38 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 03/10/2011 09:35 AM, Takuya Yoshikawa wrote:
> > x86_emulate_insn() is too long and has many confusing goto statements.
> >
> > This patch is the first part of a work which tries to split it into
> > a few meaningful functions: just encapsulates the switch statement for
> > the one byte instruction emulation as emulate_onebyte_insn().
> 
> I, too, dislike the switch statements.  So I started on a path to 
> eliminate it completely - take a look at struct opcode::execute.  If 
> present, it is executed instead of the code in the switch statements.
> 
> The plan is to migrate all of the contents of the switch statements into 
> ->execute() callbacks.  This way, all of the information about an 
> instruction is present in the decode tables.

I see.
I'm looking forward to the completion of the plan!

Thanks,
  Takuya

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


-- 
Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Re: [PATCH 1/3] KVM: x86 emulator: Make one byte instruction emulation separate

[Avi Kivity]

On 03/10/2011 11:26 AM, Takuya Yoshikawa wrote:
> >  The plan is to migrate all of the contents of the switch statements into
> >  ->execute() callbacks.  This way, all of the information about an
> >  instruction is present in the decode tables.
>
> I see.
> I'm looking forward to the completion of the plan!
>

I don't know if anyone is working on it, so feel free to send patches!

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

Re: [PATCH 1/3] KVM: x86 emulator: Make one byte instruction emulation separate

[Takuya Yoshikawa]

On Thu, 10 Mar 2011 11:27:30 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 03/10/2011 11:26 AM, Takuya Yoshikawa wrote:

> I don't know if anyone is working on it, so feel free to send patches!

Yes, I'm interested in it.  So I will take a look and try!

  I was doing some live migration tests using Kemari for some time.
  Now it's time to restart KVM code reading!

Thanks,
  Takuya