[PATCH 0/5] KVM: x86 emulator: Use single stage decoding -- Part 1

[PATCH 0/5] KVM: x86 emulator: Use single stage decoding -- Part 1

[Takuya Yoshikawa]

This work will continue until we can remove the ugly switch statements.

But I want to do this with enough care not to insert extra errors.
  -- For me, this is a good opportunity to read SDM well.

So the whole work will be done in a step by step manner!

Thanks,
  Takuya

[PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Takuya Yoshikawa]

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

ADD, OR, ADC, SBB, AND, SUB, XOR, CMP are converted using a new macro
I6ALU(_f, _e).

CMPS, SCAS will be converted later.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 14c5ad5..bd9572a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2325,6 +2325,70 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_add(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("add", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_or(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("or", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_adc(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("adc", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_sbb(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("sbb", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_and(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("and", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_sub(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("sub", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_xor(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("xor", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_cmp(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
+	return X86EMUL_CONTINUE;
+}
+
 #define D(_y) { .flags = (_y) }
 #define N    D(0)
 #define G(_f, _g) { .flags = ((_f) | Group), .u.group = (_g) }
@@ -2337,10 +2401,20 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
 #define D6ALU(_f) D2bv((_f) | DstMem | SrcReg | ModRM),			\
 		D2bv(((_f) | DstReg | SrcMem | ModRM) & ~Lock),		\
 		D2bv(((_f) & ~Lock) | DstAcc | SrcImm)
+#define I6ALU(_f, _e) I2bv((_f) | DstMem | SrcReg | ModRM, _e),		\
+		I2bv(((_f) | DstReg | SrcMem | ModRM) & ~Lock, _e),	\
+		I2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
 
 
 static struct opcode group1[] = {
-	X7(D(Lock)), N
+	I(Lock, em_add),
+	I(Lock, em_or),
+	I(Lock, em_adc),
+	I(Lock, em_sbb),
+	I(Lock, em_and),
+	I(Lock, em_sub),
+	I(Lock, em_xor),
+	I(0, em_cmp)
 };
 
 static struct opcode group1A[] = {
@@ -2396,25 +2470,25 @@ static struct opcode group11[] = {
 
 static struct opcode opcode_table[256] = {
 	/* 0x00 - 0x07 */
-	D6ALU(Lock),
+	I6ALU(Lock, em_add),
 	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
 	/* 0x08 - 0x0F */
-	D6ALU(Lock),
+	I6ALU(Lock, em_or),
 	D(ImplicitOps | Stack | No64), N,
 	/* 0x10 - 0x17 */
-	D6ALU(Lock),
+	I6ALU(Lock, em_adc),
 	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
 	/* 0x18 - 0x1F */
-	D6ALU(Lock),
+	I6ALU(Lock, em_sbb),
 	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
 	/* 0x20 - 0x27 */
-	D6ALU(Lock), N, N,
+	I6ALU(Lock, em_and), N, N,
 	/* 0x28 - 0x2F */
-	D6ALU(Lock), N, I(ByteOp | DstAcc | No64, em_das),
+	I6ALU(Lock, em_sub), N, I(ByteOp | DstAcc | No64, em_das),
 	/* 0x30 - 0x37 */
-	D6ALU(Lock), N, N,
+	I6ALU(Lock, em_xor), N, N,
 	/* 0x38 - 0x3F */
-	D6ALU(0), N, N,
+	I6ALU(0, em_cmp), N, N,
 	/* 0x40 - 0x4F */
 	X16(D(DstReg)),
 	/* 0x50 - 0x57 */
@@ -2568,6 +2642,7 @@ static struct opcode twobyte_table[256] = {
 #undef D2bv
 #undef I2bv
 #undef D6ALU
+#undef I6ALU
 
 static unsigned imm_size(struct decode_cache *c)
 {
@@ -3034,59 +3109,27 @@ special_insn:
 		goto twobyte_insn;
 
 	switch (c->b) {
-	case 0x00 ... 0x05:
-	      add:		/* add */
-		emulate_2op_SrcV("add", c->src, c->dst, ctxt->eflags);
-		break;
 	case 0x06:		/* push es */
 		emulate_push_sreg(ctxt, ops, VCPU_SREG_ES);
 		break;
 	case 0x07:		/* pop es */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_ES);
 		break;
-	case 0x08 ... 0x0d:
-	      or:		/* or */
-		emulate_2op_SrcV("or", c->src, c->dst, ctxt->eflags);
-		break;
 	case 0x0e:		/* push cs */
 		emulate_push_sreg(ctxt, ops, VCPU_SREG_CS);
 		break;
-	case 0x10 ... 0x15:
-	      adc:		/* adc */
-		emulate_2op_SrcV("adc", c->src, c->dst, ctxt->eflags);
-		break;
 	case 0x16:		/* push ss */
 		emulate_push_sreg(ctxt, ops, VCPU_SREG_SS);
 		break;
 	case 0x17:		/* pop ss */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_SS);
 		break;
-	case 0x18 ... 0x1d:
-	      sbb:		/* sbb */
-		emulate_2op_SrcV("sbb", c->src, c->dst, ctxt->eflags);
-		break;
 	case 0x1e:		/* push ds */
 		emulate_push_sreg(ctxt, ops, VCPU_SREG_DS);
 		break;
 	case 0x1f:		/* pop ds */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_DS);
 		break;
-	case 0x20 ... 0x25:
-	      and:		/* and */
-		emulate_2op_SrcV("and", c->src, c->dst, ctxt->eflags);
-		break;
-	case 0x28 ... 0x2d:
-	      sub:		/* sub */
-		emulate_2op_SrcV("sub", c->src, c->dst, ctxt->eflags);
-		break;
-	case 0x30 ... 0x35:
-	      xor:		/* xor */
-		emulate_2op_SrcV("xor", c->src, c->dst, ctxt->eflags);
-		break;
-	case 0x38 ... 0x3d:
-	      cmp:		/* cmp */
-		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
-		break;
 	case 0x40 ... 0x47: /* inc r16/r32 */
 		emulate_1op("inc", c->dst, ctxt->eflags);
 		break;
@@ -3121,26 +3164,6 @@ special_insn:
 		if (test_cc(c->b, ctxt->eflags))
 			jmp_rel(c, c->src.val);
 		break;
-	case 0x80 ... 0x83:	/* Grp1 */
-		switch (c->modrm_reg) {
-		case 0:
-			goto add;
-		case 1:
-			goto or;
-		case 2:
-			goto adc;
-		case 3:
-			goto sbb;
-		case 4:
-			goto and;
-		case 5:
-			goto sub;
-		case 6:
-			goto xor;
-		case 7:
-			goto cmp;
-		}
-		break;
 	case 0x84 ... 0x85:
 	test:
 		emulate_2op_SrcV("test", c->src, c->dst, ctxt->eflags);
@@ -3212,11 +3235,13 @@ special_insn:
 		break;
 	case 0xa6 ... 0xa7:	/* cmps */
 		c->dst.type = OP_NONE; /* Disable writeback. */
-		goto cmp;
+		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
+		break;
 	case 0xa8 ... 0xa9:	/* test ax, imm */
 		goto test;
 	case 0xae ... 0xaf:	/* scas */
-		goto cmp;
+		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
+		break;
 	case 0xc0 ... 0xc1:
 		emulate_grp2(ctxt);
 		break;
-- 
1.7.1

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Gleb Natapov]

On Mon, Mar 14, 2011 at 12:17:27AM +0900, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> 
> ADD, OR, ADC, SBB, AND, SUB, XOR, CMP are converted using a new macro
> I6ALU(_f, _e).
> 
> CMPS, SCAS will be converted later.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  arch/x86/kvm/emulate.c |  151 ++++++++++++++++++++++++++++--------------------
>  1 files changed, 88 insertions(+), 63 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 14c5ad5..bd9572a 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2325,6 +2325,70 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
>  
> +static int em_add(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("add", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_or(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("or", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_adc(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("adc", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_sbb(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("sbb", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_and(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("and", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_sub(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("sub", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_xor(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("xor", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
> +static int em_cmp(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct decode_cache *c = &ctxt->decode;
> +
> +	emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> +	return X86EMUL_CONTINUE;
> +}
> +
>  #define D(_y) { .flags = (_y) }
>  #define N    D(0)
>  #define G(_f, _g) { .flags = ((_f) | Group), .u.group = (_g) }
> @@ -2337,10 +2401,20 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
>  #define D6ALU(_f) D2bv((_f) | DstMem | SrcReg | ModRM),			\
>  		D2bv(((_f) | DstReg | SrcMem | ModRM) & ~Lock),		\
>  		D2bv(((_f) & ~Lock) | DstAcc | SrcImm)
> +#define I6ALU(_f, _e) I2bv((_f) | DstMem | SrcReg | ModRM, _e),		\
> +		I2bv(((_f) | DstReg | SrcMem | ModRM) & ~Lock, _e),	\
> +		I2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
>  
>  
>  static struct opcode group1[] = {
> -	X7(D(Lock)), N
> +	I(Lock, em_add),
> +	I(Lock, em_or),
> +	I(Lock, em_adc),
> +	I(Lock, em_sbb),
> +	I(Lock, em_and),
> +	I(Lock, em_sub),
> +	I(Lock, em_xor),
> +	I(0, em_cmp)
>  };
>  
>  static struct opcode group1A[] = {
> @@ -2396,25 +2470,25 @@ static struct opcode group11[] = {
>  
>  static struct opcode opcode_table[256] = {
>  	/* 0x00 - 0x07 */
> -	D6ALU(Lock),
> +	I6ALU(Lock, em_add),
>  	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
>  	/* 0x08 - 0x0F */
> -	D6ALU(Lock),
> +	I6ALU(Lock, em_or),
>  	D(ImplicitOps | Stack | No64), N,
>  	/* 0x10 - 0x17 */
> -	D6ALU(Lock),
> +	I6ALU(Lock, em_adc),
>  	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
>  	/* 0x18 - 0x1F */
> -	D6ALU(Lock),
> +	I6ALU(Lock, em_sbb),
>  	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
>  	/* 0x20 - 0x27 */
> -	D6ALU(Lock), N, N,
> +	I6ALU(Lock, em_and), N, N,
>  	/* 0x28 - 0x2F */
> -	D6ALU(Lock), N, I(ByteOp | DstAcc | No64, em_das),
> +	I6ALU(Lock, em_sub), N, I(ByteOp | DstAcc | No64, em_das),
>  	/* 0x30 - 0x37 */
> -	D6ALU(Lock), N, N,
> +	I6ALU(Lock, em_xor), N, N,
>  	/* 0x38 - 0x3F */
> -	D6ALU(0), N, N,
> +	I6ALU(0, em_cmp), N, N,
>  	/* 0x40 - 0x4F */
>  	X16(D(DstReg)),
>  	/* 0x50 - 0x57 */
> @@ -2568,6 +2642,7 @@ static struct opcode twobyte_table[256] = {
>  #undef D2bv
>  #undef I2bv
>  #undef D6ALU
> +#undef I6ALU
>  
>  static unsigned imm_size(struct decode_cache *c)
>  {
> @@ -3034,59 +3109,27 @@ special_insn:
>  		goto twobyte_insn;
>  
>  	switch (c->b) {
> -	case 0x00 ... 0x05:
> -	      add:		/* add */
> -		emulate_2op_SrcV("add", c->src, c->dst, ctxt->eflags);
> -		break;
>  	case 0x06:		/* push es */
>  		emulate_push_sreg(ctxt, ops, VCPU_SREG_ES);
>  		break;
>  	case 0x07:		/* pop es */
>  		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_ES);
>  		break;
> -	case 0x08 ... 0x0d:
> -	      or:		/* or */
> -		emulate_2op_SrcV("or", c->src, c->dst, ctxt->eflags);
> -		break;
>  	case 0x0e:		/* push cs */
>  		emulate_push_sreg(ctxt, ops, VCPU_SREG_CS);
>  		break;
> -	case 0x10 ... 0x15:
> -	      adc:		/* adc */
> -		emulate_2op_SrcV("adc", c->src, c->dst, ctxt->eflags);
> -		break;
>  	case 0x16:		/* push ss */
>  		emulate_push_sreg(ctxt, ops, VCPU_SREG_SS);
>  		break;
>  	case 0x17:		/* pop ss */
>  		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_SS);
>  		break;
> -	case 0x18 ... 0x1d:
> -	      sbb:		/* sbb */
> -		emulate_2op_SrcV("sbb", c->src, c->dst, ctxt->eflags);
> -		break;
>  	case 0x1e:		/* push ds */
>  		emulate_push_sreg(ctxt, ops, VCPU_SREG_DS);
>  		break;
>  	case 0x1f:		/* pop ds */
>  		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_DS);
>  		break;
> -	case 0x20 ... 0x25:
> -	      and:		/* and */
> -		emulate_2op_SrcV("and", c->src, c->dst, ctxt->eflags);
> -		break;
> -	case 0x28 ... 0x2d:
> -	      sub:		/* sub */
> -		emulate_2op_SrcV("sub", c->src, c->dst, ctxt->eflags);
> -		break;
> -	case 0x30 ... 0x35:
> -	      xor:		/* xor */
> -		emulate_2op_SrcV("xor", c->src, c->dst, ctxt->eflags);
> -		break;
> -	case 0x38 ... 0x3d:
> -	      cmp:		/* cmp */
> -		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> -		break;
>  	case 0x40 ... 0x47: /* inc r16/r32 */
>  		emulate_1op("inc", c->dst, ctxt->eflags);
>  		break;
> @@ -3121,26 +3164,6 @@ special_insn:
>  		if (test_cc(c->b, ctxt->eflags))
>  			jmp_rel(c, c->src.val);
>  		break;
> -	case 0x80 ... 0x83:	/* Grp1 */
> -		switch (c->modrm_reg) {
> -		case 0:
> -			goto add;
> -		case 1:
> -			goto or;
> -		case 2:
> -			goto adc;
> -		case 3:
> -			goto sbb;
> -		case 4:
> -			goto and;
> -		case 5:
> -			goto sub;
> -		case 6:
> -			goto xor;
> -		case 7:
> -			goto cmp;
> -		}
> -		break;
>  	case 0x84 ... 0x85:
>  	test:
>  		emulate_2op_SrcV("test", c->src, c->dst, ctxt->eflags);
> @@ -3212,11 +3235,13 @@ special_insn:
>  		break;
>  	case 0xa6 ... 0xa7:	/* cmps */
>  		c->dst.type = OP_NONE; /* Disable writeback. */
> -		goto cmp;
> +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
Why not call em_cmp() here?
> +		break;
>  	case 0xa8 ... 0xa9:	/* test ax, imm */
>  		goto test;
>  	case 0xae ... 0xaf:	/* scas */
> -		goto cmp;
> +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
And here?

> +		break;
>  	case 0xc0 ... 0xc1:
>  		emulate_grp2(ctxt);
>  		break;
> -- 
> 1.7.1

--
			Gleb.

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Takuya Yoshikawa]

On Mon, 14 Mar 2011 17:11:40 +0200
Gleb Natapov <gleb@redhat.com> wrote:

> > @@ -3212,11 +3235,13 @@ special_insn:
> >  		break;
> >  	case 0xa6 ... 0xa7:	/* cmps */
> >  		c->dst.type = OP_NONE; /* Disable writeback. */
> > -		goto cmp;
> > +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> Why not call em_cmp() here?

I thought that I needed to check of
	c->dst.type = OP_NONE; /* Disable writeback. */
later.

So I just decided to treat CMPS and SCAS in another patch.
I mean I may introduce em_cmps or em_scas later if needed.

You prefer to treat these in this patch?

> > +		break;
> >  	case 0xa8 ... 0xa9:	/* test ax, imm */
> >  		goto test;
> >  	case 0xae ... 0xaf:	/* scas */
> > -		goto cmp;
> > +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> And here?

What is the difference of CMPS and SCAS?


> 
> > +		break;
> >  	case 0xc0 ... 0xc1:
> >  		emulate_grp2(ctxt);
> >  		break;
> > -- 
> > 1.7.1
> 
> --
> 			Gleb.


-- 
Takuya Yoshikawa <takuya.yoshikawa@gmail.com>

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Gleb Natapov]

On Tue, Mar 15, 2011 at 06:32:32AM +0900, Takuya Yoshikawa wrote:
> On Mon, 14 Mar 2011 17:11:40 +0200
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > > @@ -3212,11 +3235,13 @@ special_insn:
> > >  		break;
> > >  	case 0xa6 ... 0xa7:	/* cmps */
> > >  		c->dst.type = OP_NONE; /* Disable writeback. */
> > > -		goto cmp;
> > > +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> > Why not call em_cmp() here?
> 
> I thought that I needed to check of
> 	c->dst.type = OP_NONE; /* Disable writeback. */
> later.
> 
I mean call em_cmp() after c->dst.type = OP_NONE line, not replacing it.
> So I just decided to treat CMPS and SCAS in another patch.
> I mean I may introduce em_cmps or em_scas later if needed.
> 
scas will likely just call em_cmp.

> You prefer to treat these in this patch?
> 
If there will be other patch for those instruction then it may be left
as is.

> > > +		break;
> > >  	case 0xa8 ... 0xa9:	/* test ax, imm */
> > >  		goto test;
> > >  	case 0xae ... 0xaf:	/* scas */
> > > -		goto cmp;
> > > +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> > And here?
> 
> What is the difference of CMPS and SCAS?
> 
> 
One compares to memory locations and another memory with AX register.

--
			Gleb.

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Takuya Yoshikawa]

On Tue, 15 Mar 2011 11:35:07 +0200
Gleb Natapov <gleb@redhat.com> wrote:

> > > Why not call em_cmp() here?
> > 
> > I thought that I needed to check of
> > 	c->dst.type = OP_NONE; /* Disable writeback. */
> > later.
> > 
> I mean call em_cmp() after c->dst.type = OP_NONE line, not replacing it.

I see the point!

> > So I just decided to treat CMPS and SCAS in another patch.
> > I mean I may introduce em_cmps or em_scas later if needed.
> > 
> scas will likely just call em_cmp.
> 
> > You prefer to treat these in this patch?
> > 
> If there will be other patch for those instruction then it may be left
> as is.

In my city, electric power supply may become restricted under control
from now, though only a few hours.  So please take the patch series as
is if possible!


> 
> > > > +		break;
> > > >  	case 0xa8 ... 0xa9:	/* test ax, imm */
> > > >  		goto test;
> > > >  	case 0xae ... 0xaf:	/* scas */
> > > > -		goto cmp;
> > > > +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> > > And here?
> > 
> > What is the difference of CMPS and SCAS?
> > 
> > 
> One compares to memory locations and another memory with AX register.

I wanted to know whether we should introduce em_cmps() or em_scas() later.

Probably we can eliminate introducing em_scas() because it should be
completely same as em_cmp().

But em_cmps() will be needed for inserting
  c->dst.type = OP_NONE;
before em_cmp().

Anyway, I will submit a patch for CMPS and SCAS conversion separately
if this patch can be applied.

Thanks,
  Takuya

> 
> --
> 			Gleb.

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Avi Kivity]

On 03/15/2011 04:06 PM, Takuya Yoshikawa wrote:
> >  >  So I just decided to treat CMPS and SCAS in another patch.
> >  >  I mean I may introduce em_cmps or em_scas later if needed.
> >  >
> >  scas will likely just call em_cmp.
> >
> >  >  You prefer to treat these in this patch?
> >  >
> >  If there will be other patch for those instruction then it may be left
> >  as is.
>
> In my city, electric power supply may become restricted under control
> from now, though only a few hours.  So please take the patch series as
> is if possible!

I prefer to have the patchset fully updated, even if it takes a while.  
Good luck with the recovery!

> >  >
> >  >  What is the difference of CMPS and SCAS?
> >  >
> >  >
> >  One compares to memory locations and another memory with AX register.
>
> I wanted to know whether we should introduce em_cmps() or em_scas() later.
>
> Probably we can eliminate introducing em_scas() because it should be
> completely same as em_cmp().

I agree.

> But em_cmps() will be needed for inserting
>    c->dst.type = OP_NONE;
> before em_cmp().

I think we can put this line into em_cmp().  In fact, it looks like CMP 
r/m, reg will now write back the data into memory, which is wrong.  So I 
recommend a first patch to add c->dst.type = OP_NONE before the cmp: 
label, so we have a fix patch followed by a refactoring patch.

Later we can have a ReadOnly opcode table bit, so we can disable 
writeback from the opcode tables, not the code.


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

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Takuya Yoshikawa]

On Tue, 22 Mar 2011 14:53:21 +0200
Avi Kivity <avi@redhat.com> wrote:

> 
> I prefer to have the patchset fully updated, even if it takes a while.  
> Good luck with the recovery!

Things already got back as usual, thanks.
I had expected much longer time.

BTW, is it better to wait until rc1 is released when we send
patches for the next merge window?

> > >  >  What is the difference of CMPS and SCAS?
> > >  >
> > >  >
> > >  One compares to memory locations and another memory with AX register.
> >
> > I wanted to know whether we should introduce em_cmps() or em_scas() later.
> >
> > Probably we can eliminate introducing em_scas() because it should be
> > completely same as em_cmp().
> 
> I agree.
> 
> > But em_cmps() will be needed for inserting
> >    c->dst.type = OP_NONE;
> > before em_cmp().
> 
> I think we can put this line into em_cmp().  In fact, it looks like CMP 
> r/m, reg will now write back the data into memory, which is wrong.  So I 
> recommend a first patch to add c->dst.type = OP_NONE before the cmp: 
> label, so we have a fix patch followed by a refactoring patch.

I'll update like that!

> 
> Later we can have a ReadOnly opcode table bit, so we can disable 
> writeback from the opcode tables, not the code.

OK, then we can remove the line at this timing.

Takuya

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Avi Kivity]

On 03/22/2011 05:35 PM, Takuya Yoshikawa wrote:
> On Tue, 22 Mar 2011 14:53:21 +0200
> Avi Kivity<avi@redhat.com>  wrote:
>
>> I prefer to have the patchset fully updated, even if it takes a while.
>> Good luck with the recovery!
> Things already got back as usual, thanks.
> I had expected much longer time.
>

Good to hear.

> BTW, is it better to wait until rc1 is released when we send
> patches for the next merge window?
>

No need - we (the maintainers) buffer patches in kvm.git master.

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Avi Kivity]

On 03/13/2011 05:17 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>
> ADD, OR, ADC, SBB, AND, SUB, XOR, CMP are converted using a new macro
> I6ALU(_f, _e).
>
> CMPS, SCAS will be converted later.

> @@ -2337,10 +2401,20 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
>   #define D6ALU(_f) D2bv((_f) | DstMem | SrcReg | ModRM),			\
>   		D2bv(((_f) | DstReg | SrcMem | ModRM)&  ~Lock),		\
>   		D2bv(((_f)&  ~Lock) | DstAcc | SrcImm)
> +#define I6ALU(_f, _e) I2bv((_f) | DstMem | SrcReg | ModRM, _e),		\
> +		I2bv(((_f) | DstReg | SrcMem | ModRM)&  ~Lock, _e),	\
> +		I2bv(((_f)&  ~Lock) | DstAcc | SrcImm, _e)
>

I think you can remove D6ALU, no?

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

Re: [PATCH 1/5] KVM: x86 emulator: Use single stage decoding for Group 1 instructions

[Takuya Yoshikawa]

On Tue, 22 Mar 2011 14:55:57 +0200
Avi Kivity <avi@redhat.com> wrote:

> > @@ -2337,10 +2401,20 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
> >   #define D6ALU(_f) D2bv((_f) | DstMem | SrcReg | ModRM),			\
> >   		D2bv(((_f) | DstReg | SrcMem | ModRM)&  ~Lock),		\
> >   		D2bv(((_f)&  ~Lock) | DstAcc | SrcImm)
> > +#define I6ALU(_f, _e) I2bv((_f) | DstMem | SrcReg | ModRM, _e),		\
> > +		I2bv(((_f) | DstReg | SrcMem | ModRM)&  ~Lock, _e),	\
> > +		I2bv(((_f)&  ~Lock) | DstAcc | SrcImm, _e)
> >
> 
> I think you can remove D6ALU, no?

Yes, I can. I'll remove in the next version.

Takuya

[PATCH 2/5] KVM: x86 emulator: Use single stage decoding for PUSH/POP XS instructions

[Takuya Yoshikawa]

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

PUSH ES/CS/SS/DS/FS/GS and POP ES/SS/DS/FS/GS are converted.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index bd9572a..fcc49ef 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2188,6 +2188,67 @@ static int em_push(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_push_es(struct x86_emulate_ctxt *ctxt)
+{
+	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_ES);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_push_cs(struct x86_emulate_ctxt *ctxt)
+{
+	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_CS);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_push_ss(struct x86_emulate_ctxt *ctxt)
+{
+	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_SS);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_push_ds(struct x86_emulate_ctxt *ctxt)
+{
+	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_DS);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_push_fs(struct x86_emulate_ctxt *ctxt)
+{
+	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_FS);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_push_gs(struct x86_emulate_ctxt *ctxt)
+{
+	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_GS);
+	return X86EMUL_CONTINUE;
+}
+
+static int em_pop_es(struct x86_emulate_ctxt *ctxt)
+{
+	return emulate_pop_sreg(ctxt, ctxt->ops, VCPU_SREG_ES);
+}
+
+static int em_pop_ss(struct x86_emulate_ctxt *ctxt)
+{
+	return emulate_pop_sreg(ctxt, ctxt->ops, VCPU_SREG_SS);
+}
+
+static int em_pop_ds(struct x86_emulate_ctxt *ctxt)
+{
+	return emulate_pop_sreg(ctxt, ctxt->ops, VCPU_SREG_DS);
+}
+
+static int em_pop_fs(struct x86_emulate_ctxt *ctxt)
+{
+	return emulate_pop_sreg(ctxt, ctxt->ops, VCPU_SREG_FS);
+}
+
+static int em_pop_gs(struct x86_emulate_ctxt *ctxt)
+{
+	return emulate_pop_sreg(ctxt, ctxt->ops, VCPU_SREG_GS);
+}
+
 static int em_das(struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -2471,16 +2532,19 @@ static struct opcode group11[] = {
 static struct opcode opcode_table[256] = {
 	/* 0x00 - 0x07 */
 	I6ALU(Lock, em_add),
-	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
+	I(ImplicitOps | Stack | No64, em_push_es),
+	I(ImplicitOps | Stack | No64, em_pop_es),
 	/* 0x08 - 0x0F */
 	I6ALU(Lock, em_or),
-	D(ImplicitOps | Stack | No64), N,
+	I(ImplicitOps | Stack | No64, em_push_cs), N,
 	/* 0x10 - 0x17 */
 	I6ALU(Lock, em_adc),
-	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
+	I(ImplicitOps | Stack | No64, em_push_ss),
+	I(ImplicitOps | Stack | No64, em_pop_ss),
 	/* 0x18 - 0x1F */
 	I6ALU(Lock, em_sbb),
-	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
+	I(ImplicitOps | Stack | No64, em_push_ds),
+	I(ImplicitOps | Stack | No64, em_pop_ds),
 	/* 0x20 - 0x27 */
 	I6ALU(Lock, em_and), N, N,
 	/* 0x28 - 0x2F */
@@ -2600,12 +2664,14 @@ static struct opcode twobyte_table[256] = {
 	/* 0x90 - 0x9F */
 	X16(D(ByteOp | DstMem | SrcNone | ModRM| Mov)),
 	/* 0xA0 - 0xA7 */
-	D(ImplicitOps | Stack), D(ImplicitOps | Stack),
+	I(ImplicitOps | Stack, em_push_fs),
+	I(ImplicitOps | Stack, em_pop_fs),
 	N, D(DstMem | SrcReg | ModRM | BitOp),
 	D(DstMem | SrcReg | Src2ImmByte | ModRM),
 	D(DstMem | SrcReg | Src2CL | ModRM), N, N,
 	/* 0xA8 - 0xAF */
-	D(ImplicitOps | Stack), D(ImplicitOps | Stack),
+	I(ImplicitOps | Stack, em_push_gs),
+	I(ImplicitOps | Stack, em_pop_gs),
 	N, D(DstMem | SrcReg | ModRM | BitOp | Lock),
 	D(DstMem | SrcReg | Src2ImmByte | ModRM),
 	D(DstMem | SrcReg | Src2CL | ModRM),
@@ -3109,27 +3175,6 @@ special_insn:
 		goto twobyte_insn;
 
 	switch (c->b) {
-	case 0x06:		/* push es */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_ES);
-		break;
-	case 0x07:		/* pop es */
-		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_ES);
-		break;
-	case 0x0e:		/* push cs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_CS);
-		break;
-	case 0x16:		/* push ss */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_SS);
-		break;
-	case 0x17:		/* pop ss */
-		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_SS);
-		break;
-	case 0x1e:		/* push ds */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_DS);
-		break;
-	case 0x1f:		/* pop ds */
-		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_DS);
-		break;
 	case 0x40 ... 0x47: /* inc r16/r32 */
 		emulate_1op("inc", c->dst, ctxt->eflags);
 		break;
@@ -3627,12 +3672,6 @@ twobyte_insn:
 	case 0x90 ... 0x9f:     /* setcc r/m8 */
 		c->dst.val = test_cc(c->b, ctxt->eflags);
 		break;
-	case 0xa0:	  /* push fs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_FS);
-		break;
-	case 0xa1:	 /* pop fs */
-		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_FS);
-		break;
 	case 0xa3:
 	      bt:		/* bt */
 		c->dst.type = OP_NONE;
@@ -3644,12 +3683,6 @@ twobyte_insn:
 	case 0xa5: /* shld cl, r, r/m */
 		emulate_2op_cl("shld", c->src2, c->src, c->dst, ctxt->eflags);
 		break;
-	case 0xa8:	/* push gs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_GS);
-		break;
-	case 0xa9:	/* pop gs */
-		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_GS);
-		break;
 	case 0xab:
 	      bts:		/* bts */
 		emulate_2op_SrcV_nobyte("bts", c->src, c->dst, ctxt->eflags);
-- 
1.7.1

Re: [PATCH 2/5] KVM: x86 emulator: Use single stage decoding for PUSH/POP XS instructions

[Avi Kivity]

On 03/13/2011 05:19 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>
> PUSH ES/CS/SS/DS/FS/GS and POP ES/SS/DS/FS/GS are converted.
>
>
> +static int em_push_es(struct x86_emulate_ctxt *ctxt)
> +{
> +	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_ES);
> +	return X86EMUL_CONTINUE;
> +}

I thought of adding generic sreg decoding, so we can use 
em_push()/em_pop() here.  It can be done later, though, and really 
there's no huge advantage here.

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

Re: [PATCH 2/5] KVM: x86 emulator: Use single stage decoding for PUSH/POP XS instructions

[Takuya Yoshikawa]

On Tue, 22 Mar 2011 15:03:11 +0200
Avi Kivity <avi@redhat.com> wrote:

> > +static int em_push_es(struct x86_emulate_ctxt *ctxt)
> > +{
> > +	emulate_push_sreg(ctxt, ctxt->ops, VCPU_SREG_ES);
> > +	return X86EMUL_CONTINUE;
> > +}
> 
> I thought of adding generic sreg decoding, so we can use 
> em_push()/em_pop() here.  It can be done later, though, and really 
> there's no huge advantage here.
> 

Then, I will drop this one from the next version.

Takuya

[PATCH 3/5] KVM: x86 emulator: Use single stage decoding for POP instructions

[Takuya Yoshikawa]

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

POP is converted.  RET will be converted later.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index fcc49ef..8295c50 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2224,6 +2224,13 @@ static int em_push_gs(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_pop(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	return emulate_pop(ctxt, ctxt->ops, &c->dst.val, c->op_bytes);
+}
+
 static int em_pop_es(struct x86_emulate_ctxt *ctxt)
 {
 	return emulate_pop_sreg(ctxt, ctxt->ops, VCPU_SREG_ES);
@@ -2558,7 +2565,7 @@ static struct opcode opcode_table[256] = {
 	/* 0x50 - 0x57 */
 	X8(I(SrcReg | Stack, em_push)),
 	/* 0x58 - 0x5F */
-	X8(D(DstReg | Stack)),
+	X8(I(DstReg | Stack, em_pop)),
 	/* 0x60 - 0x67 */
 	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
 	N, D(DstReg | SrcMem32 | ModRM | Mov) /* movsxd (x86/64) */ ,
@@ -3181,10 +3188,6 @@ special_insn:
 	case 0x48 ... 0x4f: /* dec r16/r32 */
 		emulate_1op("dec", c->dst, ctxt->eflags);
 		break;
-	case 0x58 ... 0x5f: /* pop reg */
-	pop_instruction:
-		rc = emulate_pop(ctxt, ops, &c->dst.val, c->op_bytes);
-		break;
 	case 0x60:	/* pusha */
 		rc = emulate_pusha(ctxt, ops);
 		break;
@@ -3294,7 +3297,8 @@ special_insn:
 		c->dst.type = OP_REG;
 		c->dst.addr.reg = &c->eip;
 		c->dst.bytes = c->op_bytes;
-		goto pop_instruction;
+		rc = emulate_pop(ctxt, ops, &c->dst.val, c->op_bytes);
+		break;
 	case 0xc4:		/* les */
 		rc = emulate_load_segment(ctxt, ops, VCPU_SREG_ES);
 		break;
-- 
1.7.1

Re: [PATCH 3/5] KVM: x86 emulator: Use single stage decoding for POP instructions

[Avi Kivity]

On 03/13/2011 05:20 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>
> POP is converted.  RET will be converted later.

There is also POP r/m (8F /0); could be done later.

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

Re: [PATCH 3/5] KVM: x86 emulator: Use single stage decoding for POP instructions

[Takuya Yoshikawa]

On Tue, 22 Mar 2011 15:06:33 +0200
Avi Kivity <avi@redhat.com> wrote:

> > POP is converted.  RET will be converted later.
> 
> There is also POP r/m (8F /0); could be done later.
> 

OK, I'll recheck.
I want to put related things into one patch if possible.

Takuya

[PATCH 4/5] KVM: x86 emulator: Use single stage decoding for PUSHA and POPA instructions

[Takuya Yoshikawa]

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

PUSHA and POPA are converted.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8295c50..4e16a55 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2224,6 +2224,11 @@ static int em_push_gs(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_pusha(struct x86_emulate_ctxt *ctxt)
+{
+	return emulate_pusha(ctxt, ctxt->ops);
+}
+
 static int em_pop(struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -2256,6 +2261,11 @@ static int em_pop_gs(struct x86_emulate_ctxt *ctxt)
 	return emulate_pop_sreg(ctxt, ctxt->ops, VCPU_SREG_GS);
 }
 
+static int em_popa(struct x86_emulate_ctxt *ctxt)
+{
+	return emulate_popa(ctxt, ctxt->ops);
+}
+
 static int em_das(struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -2567,7 +2577,8 @@ static struct opcode opcode_table[256] = {
 	/* 0x58 - 0x5F */
 	X8(I(DstReg | Stack, em_pop)),
 	/* 0x60 - 0x67 */
-	D(ImplicitOps | Stack | No64), D(ImplicitOps | Stack | No64),
+	I(ImplicitOps | Stack | No64, em_pusha),
+	I(ImplicitOps | Stack | No64, em_popa),
 	N, D(DstReg | SrcMem32 | ModRM | Mov) /* movsxd (x86/64) */ ,
 	N, N, N, N,
 	/* 0x68 - 0x6F */
@@ -3188,12 +3199,6 @@ special_insn:
 	case 0x48 ... 0x4f: /* dec r16/r32 */
 		emulate_1op("dec", c->dst, ctxt->eflags);
 		break;
-	case 0x60:	/* pusha */
-		rc = emulate_pusha(ctxt, ops);
-		break;
-	case 0x61:	/* popa */
-		rc = emulate_popa(ctxt, ops);
-		break;
 	case 0x63:		/* movsxd */
 		if (ctxt->mode != X86EMUL_MODE_PROT64)
 			goto cannot_emulate;
-- 
1.7.1

Re: [PATCH 4/5] KVM: x86 emulator: Use single stage decoding for PUSHA and POPA instructions

[Avi Kivity]

On 03/13/2011 05:21 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>
> PUSHA and POPA are converted.
>
> Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
> ---
>   arch/x86/kvm/emulate.c |   19 ++++++++++++-------
>   1 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 8295c50..4e16a55 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2224,6 +2224,11 @@ static int em_push_gs(struct x86_emulate_ctxt *ctxt)
>   	return X86EMUL_CONTINUE;
>   }
>
> +static int em_pusha(struct x86_emulate_ctxt *ctxt)
> +{
> +	return emulate_pusha(ctxt, ctxt->ops);
> +}
> +

You can simply rename/update emulate_pusha/emulate_popa, since they have 
no other callers.

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

Re: [PATCH 4/5] KVM: x86 emulator: Use single stage decoding for PUSHA and POPA instructions

[Takuya Yoshikawa]

On Tue, 22 Mar 2011 15:07:20 +0200
Avi Kivity <avi@redhat.com> wrote:

> > +static int em_pusha(struct x86_emulate_ctxt *ctxt)
> > +{
> > +	return emulate_pusha(ctxt, ctxt->ops);
> > +}
> > +
> 
> You can simply rename/update emulate_pusha/emulate_popa, since they have 
> no other callers.
> 

I intentionally left emulate_* in this version because I thought
there might be some reason for introducing new em_* naming.

OK, I'll remove useless old functions.

Takuya

Re: [PATCH 4/5] KVM: x86 emulator: Use single stage decoding for PUSHA and POPA instructions

[Avi Kivity]

On 03/22/2011 05:54 PM, Takuya Yoshikawa wrote:
> I intentionally left emulate_* in this version because I thought
> there might be some reason for introducing new em_* naming.
>

It's just that their signatures are all the same, and to conserve space 
in the decode tables.

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

[PATCH 5/5] KVM: x86 emulator: Use single stage decoding for PUSHF and POPF instructions

[Takuya Yoshikawa]

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

PUSHF and POPF are converted.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4e16a55..60182d3 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2229,6 +2229,15 @@ static int em_pusha(struct x86_emulate_ctxt *ctxt)
 	return emulate_pusha(ctxt, ctxt->ops);
 }
 
+static int em_pushf(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	c->src.val =  (unsigned long)ctxt->eflags;
+	emulate_push(ctxt, ctxt->ops);
+	return X86EMUL_CONTINUE;
+}
+
 static int em_pop(struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -2266,6 +2275,16 @@ static int em_popa(struct x86_emulate_ctxt *ctxt)
 	return emulate_popa(ctxt, ctxt->ops);
 }
 
+static int em_popf(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	c->dst.type = OP_REG;
+	c->dst.addr.reg = &ctxt->eflags;
+	c->dst.bytes = c->op_bytes;
+	return emulate_popf(ctxt, ctxt->ops, &c->dst.val, c->op_bytes);
+}
+
 static int em_das(struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -2606,7 +2625,8 @@ static struct opcode opcode_table[256] = {
 	/* 0x98 - 0x9F */
 	D(DstAcc | SrcNone), I(ImplicitOps | SrcAcc, em_cwd),
 	I(SrcImmFAddr | No64, em_call_far), N,
-	D(ImplicitOps | Stack), D(ImplicitOps | Stack), N, N,
+	I(ImplicitOps | Stack, em_pushf),
+	I(ImplicitOps | Stack, em_popf), N, N,
 	/* 0xA0 - 0xA7 */
 	I2bv(DstAcc | SrcMem | Mov | MemAbs, em_mov),
 	I2bv(DstMem | SrcAcc | Mov | MemAbs, em_mov),
@@ -3276,16 +3296,6 @@ special_insn:
 		case 8: c->dst.val = (s32)c->dst.val; break;
 		}
 		break;
-	case 0x9c: /* pushf */
-		c->src.val =  (unsigned long) ctxt->eflags;
-		emulate_push(ctxt, ops);
-		break;
-	case 0x9d: /* popf */
-		c->dst.type = OP_REG;
-		c->dst.addr.reg = &ctxt->eflags;
-		c->dst.bytes = c->op_bytes;
-		rc = emulate_popf(ctxt, ops, &c->dst.val, c->op_bytes);
-		break;
 	case 0xa6 ... 0xa7:	/* cmps */
 		c->dst.type = OP_NONE; /* Disable writeback. */
 		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
-- 
1.7.1