[PATCH 0/4] KVM: x86 emulator: Move em_grp from switch statement to decode table

[PATCH 0/4] KVM: x86 emulator: Move em_grp from switch statement to decode table

[Takuya Yoshikawa]

I removed em_grp1a() and em_grp9() but kept em_grp2() and em_grp45()
because they would produce a lot of trivial functions.

Though I think it's almost done about the conversions, I do not have
strong opinion about how to treat the remaining instructions: define
em_default() and put those into it may be one way.

Anyway, now, rather readable than before.

Thanks,
	Takuya

[PATCH 1/4] KVM: x86 emulator: Use opcode::execute for Group 2 instructions

[Takuya Yoshikawa]

Group 2: C0, C1, D0, D1, D2, D3

According to the SDM, the case 6 of em_grp2() should be treated as
undefined and the opcode D2/D3 should be decoded using the SrcCL.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f641201..87e7616 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -101,6 +101,7 @@
 #define SrcAcc      (OpAcc << SrcShift)
 #define SrcImmU16   (OpImmU16 << SrcShift)
 #define SrcDX       (OpDX << SrcShift)
+#define SrcCL       (OpCL << SrcShift)
 #define SrcMask     (OpMask << SrcShift)
 #define BitOp       (1<<11)
 #define MemAbs      (1<<12)      /* Memory operand is absolute displacement */
@@ -1696,7 +1697,6 @@ static int em_grp2(struct x86_emulate_ctxt *ctxt)
 		emulate_2op_SrcB(ctxt, "rcr");
 		break;
 	case 4:	/* sal/shl */
-	case 6:	/* sal/shl */
 		emulate_2op_SrcB(ctxt, "sal");
 		break;
 	case 5:	/* shr */
@@ -3206,6 +3206,12 @@ static struct opcode group1A[] = {
 	D(DstMem | SrcNone | ModRM | Mov | Stack), N, N, N, N, N, N, N,
 };
 
+static struct opcode group2[] = {
+	X6(I(DstMem | ModRM, em_grp2)),
+	N,
+	I(DstMem | ModRM, em_grp2),
+};
+
 static struct opcode group3[] = {
 	I(DstMem | SrcImm | ModRM, em_test),
 	I(DstMem | SrcImm | ModRM, em_test),
@@ -3358,7 +3364,7 @@ static struct opcode opcode_table[256] = {
 	/* 0xB8 - 0xBF */
 	X8(I(DstReg | SrcImm | Mov, em_mov)),
 	/* 0xC0 - 0xC7 */
-	D2bv(DstMem | SrcImmByte | ModRM),
+	G(ByteOp | SrcImmByte, group2), G(SrcImmByte, group2),
 	I(ImplicitOps | Stack | SrcImmU16, em_ret_near_imm),
 	I(ImplicitOps | Stack, em_ret),
 	I(DstReg | SrcMemFAddr | ModRM | No64 | Src2ES, em_lseg),
@@ -3369,7 +3375,8 @@ static struct opcode opcode_table[256] = {
 	D(ImplicitOps), DI(SrcImmByte, intn),
 	D(ImplicitOps | No64), II(ImplicitOps, em_iret, iret),
 	/* 0xD0 - 0xD7 */
-	D2bv(DstMem | SrcOne | ModRM), D2bv(DstMem | ModRM),
+	G(ByteOp | SrcOne, group2), G(SrcOne, group2),
+	G(ByteOp | SrcReg | SrcCL, group2), G(SrcReg | SrcCL, group2),
 	N, N, N, N,
 	/* 0xD8 - 0xDF */
 	N, N, N, N, N, N, N, N,
@@ -4046,9 +4053,6 @@ special_insn:
 		case 8: ctxt->dst.val = (s32)ctxt->dst.val; break;
 		}
 		break;
-	case 0xc0 ... 0xc1:
-		rc = em_grp2(ctxt);
-		break;
 	case 0xcc:		/* int3 */
 		rc = emulate_int(ctxt, 3);
 		break;
@@ -4059,13 +4063,6 @@ special_insn:
 		if (ctxt->eflags & EFLG_OF)
 			rc = emulate_int(ctxt, 4);
 		break;
-	case 0xd0 ... 0xd1:	/* Grp2 */
-		rc = em_grp2(ctxt);
-		break;
-	case 0xd2 ... 0xd3:	/* Grp2 */
-		ctxt->src.val = ctxt->regs[VCPU_REGS_RCX];
-		rc = em_grp2(ctxt);
-		break;
 	case 0xe9: /* jmp rel */
 	case 0xeb: /* jmp rel short */
 		jmp_rel(ctxt, ctxt->src.val);
-- 
1.7.5.4

Re: [PATCH 1/4] KVM: x86 emulator: Use opcode::execute for Group 2 instructions

[Avi Kivity]

On 12/06/2011 11:05 AM, Takuya Yoshikawa wrote:
> Group 2: C0, C1, D0, D1, D2, D3
>
> According to the SDM, the case 6 of em_grp2() should be treated as
> undefined and the opcode D2/D3 should be decoded using the SrcCL.
>
>
>  		emulate_2op_SrcB(ctxt, "rcr");
>  		break;
>  	case 4:	/* sal/shl */
> -	case 6:	/* sal/shl */
>  		emulate_2op_SrcB(ctxt, "sal");
>  		break;
>  	case 5:	/* shr */

AMD processors decode grp2 /6 as sal/shl.  In any case, please don't mix
functional changes with refactoring.


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

[PATCH 2/4] KVM: x86 emulator: Use opcode::execute for Group 1A instruction

[Takuya Yoshikawa]

Group 1A: 8F

Register em_pop() directly and remove em_grp1a().

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 87e7616..d49cde0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1676,11 +1676,6 @@ static int em_jmp_far(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
-static int em_grp1a(struct x86_emulate_ctxt *ctxt)
-{
-	return emulate_pop(ctxt, &ctxt->dst.val, ctxt->dst.bytes);
-}
-
 static int em_grp2(struct x86_emulate_ctxt *ctxt)
 {
 	switch (ctxt->modrm_reg) {
@@ -3203,7 +3198,7 @@ static struct opcode group1[] = {
 };
 
 static struct opcode group1A[] = {
-	D(DstMem | SrcNone | ModRM | Mov | Stack), N, N, N, N, N, N, N,
+	I(DstMem | SrcNone | ModRM | Mov | Stack, em_pop), N, N, N, N, N, N, N,
 };
 
 static struct opcode group2[] = {
@@ -4038,9 +4033,6 @@ special_insn:
 	case 0x8d: /* lea r16/r32, m */
 		ctxt->dst.val = ctxt->src.addr.mem.ea;
 		break;
-	case 0x8f:		/* pop (sole member of Grp1a) */
-		rc = em_grp1a(ctxt);
-		break;
 	case 0x90 ... 0x97: /* nop / xchg reg, rax */
 		if (ctxt->dst.addr.reg == &ctxt->regs[VCPU_REGS_RAX])
 			break;
-- 
1.7.5.4

[PATCH 3/4] KVM: x86 emulator: Use opcode::execute for Group 4/5 instructions

[Takuya Yoshikawa]

Group 4: FE
Group 5: FF

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index d49cde0..2df3a91 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3219,16 +3219,19 @@ static struct opcode group3[] = {
 };
 
 static struct opcode group4[] = {
-	D(ByteOp | DstMem | SrcNone | ModRM | Lock), D(ByteOp | DstMem | SrcNone | ModRM | Lock),
+	I(ByteOp | DstMem | SrcNone | ModRM | Lock, em_grp45),
+	I(ByteOp | DstMem | SrcNone | ModRM | Lock, em_grp45),
 	N, N, N, N, N, N,
 };
 
 static struct opcode group5[] = {
-	D(DstMem | SrcNone | ModRM | Lock), D(DstMem | SrcNone | ModRM | Lock),
-	D(SrcMem | ModRM | Stack),
+	I(DstMem | SrcNone | ModRM | Lock, em_grp45),
+	I(DstMem | SrcNone | ModRM | Lock, em_grp45),
+	I(SrcMem | ModRM | Stack, em_grp45),
 	I(SrcMemFAddr | ModRM | ImplicitOps | Stack, em_call_far),
-	D(SrcMem | ModRM | Stack), D(SrcMemFAddr | ModRM | ImplicitOps),
-	D(SrcMem | ModRM | Stack), N,
+	I(SrcMem | ModRM | Stack, em_grp45),
+	I(SrcMemFAddr | ModRM | ImplicitOps, em_grp45),
+	I(SrcMem | ModRM | Stack, em_grp45), N,
 };
 
 static struct opcode group6[] = {
@@ -4079,12 +4082,6 @@ special_insn:
 	case 0xfd: /* std */
 		ctxt->eflags |= EFLG_DF;
 		break;
-	case 0xfe: /* Grp4 */
-		rc = em_grp45(ctxt);
-		break;
-	case 0xff: /* Grp5 */
-		rc = em_grp45(ctxt);
-		break;
 	default:
 		goto cannot_emulate;
 	}
-- 
1.7.5.4

[PATCH 4/4] KVM: x86 emulator: Use opcode::execute for Group 9 instruction

[Takuya Yoshikawa]

Group 9: 0F C7

Rename em_grp9() to em_cmpxchg8b() and register it.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2df3a91..5882e30 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1784,7 +1784,7 @@ static int em_grp45(struct x86_emulate_ctxt *ctxt)
 	return rc;
 }
 
-static int em_grp9(struct x86_emulate_ctxt *ctxt)
+static int em_cmpxchg8b(struct x86_emulate_ctxt *ctxt)
 {
 	u64 old = ctxt->dst.orig_val64;
 
@@ -3267,7 +3267,7 @@ static struct opcode group8[] = {
 };
 
 static struct group_dual group9 = { {
-	N, D(DstMem64 | ModRM | Lock | PageTable), N, N, N, N, N, N,
+	N, I(DstMem64 | ModRM | Lock | PageTable, em_cmpxchg8b), N, N, N, N, N, N,
 }, {
 	N, N, N, N, N, N, N, N,
 } };
@@ -4199,9 +4199,6 @@ twobyte_insn:
 		ctxt->dst.val = (ctxt->op_bytes == 4) ? (u32) ctxt->src.val :
 							(u64) ctxt->src.val;
 		break;
-	case 0xc7:		/* Grp9 (cmpxchg8b) */
-		rc = em_grp9(ctxt);
-		break;
 	default:
 		goto cannot_emulate;
 	}
-- 
1.7.5.4

Re: [PATCH 0/4] KVM: x86 emulator: Move em_grp from switch statement to decode table

[Avi Kivity]

On 12/06/2011 11:04 AM, Takuya Yoshikawa wrote:
> I removed em_grp1a() and em_grp9() but kept em_grp2() and em_grp45()
> because they would produce a lot of trivial functions.

We could convert the various emulate_2op_blah() macros to generate
functions instead of statements, that would greatly reduce the boilerplate.

> Though I think it's almost done about the conversions, I do not have
> strong opinion about how to treat the remaining instructions: define
> em_default() and put those into it may be one way.

With em_insn() you have direct visibility as to what a decode table
entry describes.  That doesn't work with em_default() or em_grpX().


> Anyway, now, rather readable than before.
>

Yes.

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

Re: [PATCH 0/4] KVM: x86 emulator: Move em_grp from switch statement to decode table

[Avi Kivity]

On 12/07/2011 06:51 PM, Avi Kivity wrote:
> On 12/06/2011 11:04 AM, Takuya Yoshikawa wrote:
> > I removed em_grp1a() and em_grp9() but kept em_grp2() and em_grp45()
> > because they would produce a lot of trivial functions.
>
> We could convert the various emulate_2op_blah() macros to generate
> functions instead of statements, that would greatly reduce the boilerplate.
>
> > Though I think it's almost done about the conversions, I do not have
> > strong opinion about how to treat the remaining instructions: define
> > em_default() and put those into it may be one way.
>
> With em_insn() you have direct visibility as to what a decode table
> entry describes.  That doesn't work with em_default() or em_grpX().
>
>
> > Anyway, now, rather readable than before.
> >
>
> Yes.
>

Forgot to mention, I applied 2-4, thanks.

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