Emulation of shld instruction doesn't work

Emulation of shld instruction doesn't work

[Guillaume Thouvenin]

Hello,

 I need to emulate the shld instruction (needed when enabling invalid
guest state framework).

 I added the code that emulates shld in kvm (see the end of this email)
and I also made a test case in user/test/x86/realmode.c. I think that
the code that emulated the instruction is fine but when I run kvmctl
with file realmode.flat it failed. Here is the test that I added in
realmode.c:

diff --git a/user/test/x86/realmode.c b/user/test/x86/realmode.c
index 69ded37..8533240 100644
--- a/user/test/x86/realmode.c
+++ b/user/test/x86/realmode.c
@@ -141,6 +141,22 @@ int regs_equal(const struct regs *r1, const struct regs *r2, int ignore)
                );                                 \
        extern u8 insn_##name[], insn_##name##_end[]
 
+void test_shld(const struct regs *inregs, struct regs *outregs)
+{
+       MK_INSN(shld_test, "mov $0xbe, %ebx\n\t"
+                          "mov $0xef000000, %ecx\n\t"
+                          "shld $8,%ecx,%ebx\n\t");
+
+       exec_in_big_real_mode(inregs, outregs,
+                             insn_shld_test,
+                             insn_shld_test_end - insn_shld_test);
+
+       if ((outregs->ebx != 0xbeef) && (outregs->ecx != 0xef000000))
+               print_serial("shld: failed\n");
+       else
+               print_serial("shld: succeded\n");
+}
+
 void test_mov_imm(const struct regs *inregs, struct regs *outregs)
 {
        MK_INSN(mov_r32_imm_1, "mov $1234567890, %eax");
@@ -342,6 +358,7 @@ void start(void)
        test_call(&inregs, &outregs);
        test_mov_imm(&inregs, &outregs);
        test_cmp_imm(&inregs, &outregs);
+       test_shld(&inregs, &outregs);
        test_io(&inregs, &outregs);
        test_eflags_insn(&inregs, &outregs);
        exit(0);

When I launch the test, I have an emulation failure that depends of the
count operand. With count == 4 (shld $4,...) I have:

  emulation failed (emulation failure) rip 1aa 04 87 06 c0

If count == 12 I have

 emulation failed (emulation failure) rip 1aa 0c 87 06 c0

and if count == 8 
 
 emulation failed (emulation failure) rip 1b1 1e c4 21 66


If I disassemble the code of realmode.flat I can see 
 
     198:       dc 21                   fsubl  (%ecx)
        ...
     1aa:       66 87 06                xchg   %ax,(%esi)
     1ad:       c0 21 66                shlb   $0x66,(%ecx)
     1b0:       87 1e                   xchg   %ebx,(%esi)
     1b2:       c4 21                   les    (%ecx),%esp

So rip values reported during the emulation failure are strange. I also
notice that the problem occurs after test_shld() is called. If the test
is the last one the program exit normally. I don't see what is wrong in
the emulation of shld instruction so any hints are welcome.



Best Regards,
Guillaume

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index a391e21..93eea5f 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -230,7 +230,8 @@ static u16 twobyte_table[256] = {
 	/* 0x90 - 0x9F */
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 	/* 0xA0 - 0xA7 */
-	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, 0, 0,
+	0, 0, 0, DstMem | SrcReg | ModRM | BitOp,
+	DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM, 0, 0,
 	/* 0xA8 - 0xAF */
 	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, ModRM, 0,
 	/* 0xB0 - 0xB7 */
@@ -501,6 +502,16 @@ static u16 group2_table[] = {
 	(_type)_x;							\
 })
 
+/*
+ * Reflects the parity of the given argument. It is set if
+ * the number of ones is even.
+ */
+static int even_parity(uint8_t v)
+{
+	asm ( "test %b0,%b0; setp %b0" : "=a" (v) : "0" (v) );
+	return v;
+}
+
 static inline unsigned long ad_mask(struct decode_cache *c)
 {
 	return (1UL << (c->ad_bytes << 3)) - 1;
@@ -1993,6 +2004,46 @@ twobyte_insn:
 		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bt", c->src, c->dst, ctxt->eflags);
 		break;
+	case 0xa4: /* shld imm8, r, r/m */
+	case 0xa5: /* shld cl, r, r/m */ {
+		uint8_t size;
+		uint8_t count;
+
+		size = c->dst.bytes << 3;
+		count = (c->b & 1) ? (uint8_t) c->regs[VCPU_REGS_RCX] : insn_fetch(u8, 1, c->eip);
+
+		if (count == 0)	/* No operation */
+			break;
+
+		if (count > size) {
+			printk(KERN_INFO "shld: bad parameters \n");
+			break;
+		}
+
+		c->dst.orig_val = c->dst.val;
+		c->dst.type = OP_REG;
+		c->dst.val <<= count;
+		c->dst.val |= ((c->src.val >> (size - count)) & ((1ull << count) - 1));
+
+		/* Flags affected */
+		ctxt->eflags &= ~(EFLG_OF|EFLG_SF|EFLG_ZF|EFLG_PF|EFLG_CF);
+
+		/* Set CF with left bit shifted if count is 1 or greater */
+		if ((c->dst.orig_val >> (size - count)) & 1)
+			ctxt->eflags |= EFLG_CF;
+		
+		/* For 1-bit shit, OF is set if a sign change occured, otherwise it
+		 * is cleared. For shift greater than 1 it is undefined */
+		if (((c->dst.orig_val ^ c->dst.val) >> (size - 1)) & 1)
+			ctxt->eflags |= EFLG_OF;
+
+		/* SF, ZF, and PF flags are set according to the value of the result */
+		ctxt->eflags |= ((c->dst.val >> (size - 1)) & 1) ? EFLG_SF : 0;
+		ctxt->eflags |= (c->dst.val == 0) ? EFLG_ZF : 0;
+		ctxt->eflags |= even_parity(c->dst.val) ? EFLG_PF : 0;
+
+		break;
+	}	
 	case 0xab:
 	      bts:		/* bts */
 		/* only subword offset */

Re: Emulation of shld instruction doesn't work

[Guillaume Thouvenin]

On Wed, 8 Oct 2008 14:52:45 +0200
Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net> wrote:

> When I launch the test, I have an emulation failure that depends of the
> count operand. With count == 4 (shld $4,...) I have:
> 
>   emulation failed (emulation failure) rip 1aa 04 87 06 c0
> 
> If count == 12 I have
> 
>  emulation failed (emulation failure) rip 1aa 0c 87 06 c0
> 
> and if count == 8 
>  
>  emulation failed (emulation failure) rip 1b1 1e c4 21 66

Just to be more accurate, I added trace in shld to display information
and I can see that destination operand is ok, the failure occurs after
test_shld(). 

Regards,
Guillaume

Re: Emulation of shld instruction doesn't work

[Avi Kivity]

Guillaume Thouvenin wrote:
> Hello,
>
>  I need to emulate the shld instruction (needed when enabling invalid
> guest state framework).
>
>  I added the code that emulates shld in kvm (see the end of this email)
> and I also made a test case in user/test/x86/realmode.c. I think that
> the code that emulated the instruction is fine but when I run kvmctl
> with file realmode.flat it failed. Here is the test that I added in
> realmode.c:
>   

Instructions that modify memory are best tested in emulator.c rather 
than realmode.c.

> diff --git a/user/test/x86/realmode.c b/user/test/x86/realmode.c
> index 69ded37..8533240 100644
> --- a/user/test/x86/realmode.c
> +++ b/user/test/x86/realmode.c
> @@ -141,6 +141,22 @@ int regs_equal(const struct regs *r1, const struct regs *r2, int ignore)
>                 );                                 \
>         extern u8 insn_##name[], insn_##name##_end[]
>  
> +void test_shld(const struct regs *inregs, struct regs *outregs)
> +{
> +       MK_INSN(shld_test, "mov $0xbe, %ebx\n\t"
> +                          "mov $0xef000000, %ecx\n\t"
>   

Do the assignments in C, modifying the inregs parameter rather than in 
assembly.


> @@ -230,7 +230,8 @@ static u16 twobyte_table[256] = {
>  	/* 0x90 - 0x9F */
>  	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>  	/* 0xA0 - 0xA7 */
> -	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, 0, 0,
> +	0, 0, 0, DstMem | SrcReg | ModRM | BitOp,
> +	DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM, 0, 0,
>  	/* 0xA8 - 0xAF */
>  	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, ModRM, 0,
>  	/* 0xB0 - 0xB7 */
> @@ -501,6 +502,16 @@ static u16 group2_table[] = {
>  	(_type)_x;							\
>  })
>  
> +/*
> + * Reflects the parity of the given argument. It is set if
> + * the number of ones is even.
> + */
> +static int even_parity(uint8_t v)
> +{
> +	asm ( "test %b0,%b0; setp %b0" : "=a" (v) : "0" (v) );
> +	return v;
> +}
> +
>  static inline unsigned long ad_mask(struct decode_cache *c)
>  {
>  	return (1UL << (c->ad_bytes << 3)) - 1;
> @@ -1993,6 +2004,46 @@ twobyte_insn:
>  		c->src.val &= (c->dst.bytes << 3) - 1;
>  		emulate_2op_SrcV_nobyte("bt", c->src, c->dst, ctxt->eflags);
>  		break;
> +	case 0xa4: /* shld imm8, r, r/m */
> +	case 0xa5: /* shld cl, r, r/m */ {
> +		uint8_t size;
> +		uint8_t count;
> +
> +		size = c->dst.bytes << 3;
> +		count = (c->b & 1) ? (uint8_t) c->regs[VCPU_REGS_RCX] : insn_fetch(u8, 1, c->eip);
>   

You need to fetch during the decode phase rather than the execution 
phase.  This is probably the source of the problem.

shld has three operands, so we need to add s Src2 decode set.  I guess 
we can start with Src2One, Src2CL, and Src2Imm8 to support shld and 
expand it later.

> +
> +		if (count == 0)	/* No operation */
> +			break;
> +
> +		if (count > size) {
> +			printk(KERN_INFO "shld: bad parameters \n");
> +			break;
> +		}
>   

The parameters aren't bad; the processor ignores excess bits.

> +
> +		c->dst.orig_val = c->dst.val;
> +		c->dst.type = OP_REG;
> +		c->dst.val <<= count;
> +		c->dst.val |= ((c->src.val >> (size - count)) & ((1ull << count) - 1));
> +
> +		/* Flags affected */
> +		ctxt->eflags &= ~(EFLG_OF|EFLG_SF|EFLG_ZF|EFLG_PF|EFLG_CF);
> +
> +		/* Set CF with left bit shifted if count is 1 or greater */
> +		if ((c->dst.orig_val >> (size - count)) & 1)
> +			ctxt->eflags |= EFLG_CF;
> +		
> +		/* For 1-bit shit, OF is set if a sign change occured, otherwise it
> +		 * is cleared. For shift greater than 1 it is undefined */
> +		if (((c->dst.orig_val ^ c->dst.val) >> (size - 1)) & 1)
> +			ctxt->eflags |= EFLG_OF;
> +
> +		/* SF, ZF, and PF flags are set according to the value of the result */
> +		ctxt->eflags |= ((c->dst.val >> (size - 1)) & 1) ? EFLG_SF : 0;
> +		ctxt->eflags |= (c->dst.val == 0) ? EFLG_ZF : 0;
> +		ctxt->eflags |= even_parity(c->dst.val) ? EFLG_PF : 0;
> +
>   

This needs to be implemented like the rest of the instructions, with the 
processor calculating the flags and result.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[PATCH] x86 emulator: Add Src2 decode set

[Guillaume Thouvenin]

Instruction like shld has three operands, so we need to add a Src2
decode set. We start with Src2None, Src2CL, and Src2Imm8 to support
shld and we will expand it later.

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
---
 arch/x86/kvm/x86_emulate.c        |   47 ++++++++++++++++++++++++++++----------
 include/asm-x86/kvm_x86_emulate.h |    1 
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index a391e21..c9ef2da 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -59,16 +59,21 @@
 #define SrcImm      (5<<4)	/* Immediate operand. */
 #define SrcImmByte  (6<<4)	/* 8-bit sign-extended immediate operand. */
 #define SrcMask     (7<<4)
+/* Source 2 operand type */
+#define Src2None    (0<<7)
+#define Src2CL      (1<<7)
+#define Src2Imm8    (2<<7)
+#define Src2Mask    (7<<7)
 /* Generic ModRM decode. */
-#define ModRM       (1<<7)
+#define ModRM       (1<<24)
 /* Destination is only written; never read. */
-#define Mov         (1<<8)
-#define BitOp       (1<<9)
-#define MemAbs      (1<<10)      /* Memory operand is absolute displacement */
-#define String      (1<<12)     /* String instruction (rep capable) */
-#define Stack       (1<<13)     /* Stack instruction (push/pop) */
-#define Group       (1<<14)     /* Bits 3:5 of modrm byte extend opcode */
-#define GroupDual   (1<<15)     /* Alternate decoding of mod == 3 */
+#define Mov         (1<<25)
+#define BitOp       (1<<26)
+#define MemAbs      (1<<27)     /* Memory operand is absolute displacement */
+#define String      (1<<28)     /* String instruction (rep capable) */
+#define Stack       (1<<29)     /* Stack instruction (push/pop) */
+#define Group       (1<<30)     /* Bits 3:5 of modrm byte extend opcode */
+#define GroupDual   (1<<31)     /* Alternate decoding of mod == 3 */
 #define GroupMask   0xff        /* Group number stored in bits 0:7 */
 
 enum {
@@ -76,7 +81,7 @@ enum {
 	Group1A, Group3_Byte, Group3, Group4, Group5, Group7,
 };
 
-static u16 opcode_table[256] = {
+static u32 opcode_table[256] = {
 	/* 0x00 - 0x07 */
 	ByteOp | DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM,
 	ByteOp | DstReg | SrcMem | ModRM, DstReg | SrcMem | ModRM,
@@ -195,7 +200,7 @@ static u16 opcode_table[256] = {
 	ImplicitOps, ImplicitOps, Group | Group4, Group | Group5,
 };
 
-static u16 twobyte_table[256] = {
+static u32 twobyte_table[256] = {
 	/* 0x00 - 0x0F */
 	0, Group | GroupDual | Group7, 0, 0, 0, 0, ImplicitOps, 0,
 	ImplicitOps, ImplicitOps, 0, 0, 0, ImplicitOps | ModRM, 0, 0,
@@ -253,7 +258,7 @@ static u16 twobyte_table[256] = {
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
 };
 
-static u16 group_table[] = {
+static u32 group_table[] = {
 	[Group1_80*8] =
 	ByteOp | DstMem | SrcImm | ModRM, ByteOp | DstMem | SrcImm | ModRM,
 	ByteOp | DstMem | SrcImm | ModRM, ByteOp | DstMem | SrcImm | ModRM,
@@ -297,7 +302,7 @@ static u16 group_table[] = {
 	SrcMem16 | ModRM | Mov, SrcMem | ModRM | ByteOp,
 };
 
-static u16 group2_table[] = {
+static u32 group2_table[] = {
 	[Group7*8] =
 	SrcNone | ModRM, 0, 0, 0,
 	SrcNone | ModRM | DstMem | Mov, 0,
@@ -1043,6 +1048,24 @@ done_prefixes:
 		break;
 	}
 
+	/*
+	 * Decode and fetch the second source operand: register, memory
+	 * or immediate.
+	 */
+	switch (c->d & Src2Mask) {
+	case Src2None:
+		break;
+	case Src2CL:
+		c->src2.val = c->regs[VCPU_REGS_RCX];
+		break;
+	case Src2Imm8:
+		c->src2.type = OP_IMM;
+		c->src2.ptr = (unsigned long *)c->eip;
+		c->src2.bytes = 1;
+		c->src2.val = insn_fetch(u8, 1, c->eip);
+		break;
+	}
+
 	/* Decode and fetch the destination operand: register or memory. */
 	switch (c->d & DstMask) {
 	case ImplicitOps:
diff --git a/include/asm-x86/kvm_x86_emulate.h b/include/asm-x86/kvm_x86_emulate.h
index 4e8c1e4..00de896 100644
--- a/include/asm-x86/kvm_x86_emulate.h
+++ b/include/asm-x86/kvm_x86_emulate.h
@@ -123,6 +123,7 @@ struct decode_cache {
 	u8 ad_bytes;
 	u8 rex_prefix;
 	struct operand src;
+	struct operand src2;
 	struct operand dst;
 	bool has_seg_override;
 	u8 seg_override;

Re: [PATCH] x86 emulator: Add Src2 decode set

[Avi Kivity]

Guillaume Thouvenin wrote:
> Instruction like shld has three operands, so we need to add a Src2
> decode set. We start with Src2None, Src2CL, and Src2Imm8 to support
> shld and we will expand it later.
>
>   

Please add Src2One (implied '1') as well, so we can switch the existing
shift operators to Src2 later.

> Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
> ---
>  arch/x86/kvm/x86_emulate.c        |   47 ++++++++++++++++++++++++++++----------
>  include/asm-x86/kvm_x86_emulate.h |    1 
>  2 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
> index a391e21..c9ef2da 100644
> --- a/arch/x86/kvm/x86_emulate.c
> +++ b/arch/x86/kvm/x86_emulate.c
> @@ -59,16 +59,21 @@
>  #define SrcImm      (5<<4)	/* Immediate operand. */
>  #define SrcImmByte  (6<<4)	/* 8-bit sign-extended immediate operand. */
>  #define SrcMask     (7<<4)
> +/* Source 2 operand type */
> +#define Src2None    (0<<7)
> +#define Src2CL      (1<<7)
> +#define Src2Imm8    (2<<7)
> +#define Src2Mask    (7<<7)
>   

Please allocate bits for this at the end to avoid renumbering.

>  
> +	/*
> +	 * Decode and fetch the second source operand: register, memory
> +	 * or immediate.
> +	 */
> +	switch (c->d & Src2Mask) {
> +	case Src2None:
> +		break;
> +	case Src2CL:
> +		c->src2.val = c->regs[VCPU_REGS_RCX];
>   

Mask to a single byte; also set the operand length.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] x86 emulator: Add Src2 decode set

[Avi Kivity]

Avi Kivity wrote:
>>  #define SrcMask     (7<<4)
>> +/* Source 2 operand type */
>> +#define Src2None    (0<<7)
>> +#define Src2CL      (1<<7)
>> +#define Src2Imm8    (2<<7)
>>     


Src2ImmByte like SrcImmByte.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] x86 emulator: Add Src2 decode set

[Guillaume Thouvenin]

On Thu, 09 Oct 2008 10:11:57 +0200
Avi Kivity <avi@redhat.com> wrote:

> Guillaume Thouvenin wrote:
> > Instruction like shld has three operands, so we need to add a Src2
> > decode set. We start with Src2None, Src2CL, and Src2Imm8 to support
> > shld and we will expand it later.
> >
> >   
> 
> Please add Src2One (implied '1') as well, so we can switch the existing
> shift operators to Src2 later.

I will add Src2One but I don't understand exactly what you mean by
switching shift operators to Src2 later. I also applied other remarks,
thanks for your help. The patch follows.

Re: [PATCH] x86 emulator: Add Src2 decode set

[Avi Kivity]

Guillaume Thouvenin wrote:
> I will add Src2One but I don't understand exactly what you mean by
> switching shift operators to Src2 later. I also applied other remarks,
> thanks for your help. The patch follows.
>   

The regular shift instructions (shl, rcl, etc) come in three varieties:
shift by 1, shift by imm8, and shift by CL.  Right now they use
SrcImmByte and decode the implied '1' and CL by hand.  If we change them
to use Src2, they can reuse the Src2CL and Src2One support that you are
adding now.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] x86 emulator: Add Src2 decode set

[Guillaume Thouvenin]

On Thu, 09 Oct 2008 11:06:50 +0200
Avi Kivity <avi@redhat.com> wrote:

> 
> The regular shift instructions (shl, rcl, etc) come in three varieties:
> shift by 1, shift by imm8, and shift by CL.  Right now they use
> SrcImmByte and decode the implied '1' and CL by hand.  If we change them
> to use Src2, they can reuse the Src2CL and Src2One support that you are
> adding now.

Ok I see but shld, rcld, etc come only in two varieties: immediate and
CL. So maybe it could be better to replace SrcImplicit (that is not
really useful) by SrcOne? and then we will have
 
 ...
 case SrcOne:
	c->src.val = 1;
        break;
 ...

and we can reuse Src2CL as well.

Re: [PATCH] x86 emulator: Add a Src2 decode set and SrcOne operand type

[Guillaume Thouvenin]

 Instruction like shld has three operands, so we need to add a Src2
decode set. We start with Src2None, Src2CL, and Src2Imm8 to support
shld and we will expand it later. Operand type of Src2 are placed at
the end of the set to avoid renumbering. For Src2CL we mask to a single
byte and set the operand length.

 This patch also added SrcOne operand type when we need to decode an
implied '1' like with regular shift instruction. 

 If needed I can split this patch into two patches, one for Src2 decode
set and another one for SrcOne. 


Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
---
 arch/x86/kvm/x86_emulate.c        |   37 +++++++++++++++++++++++++++++++++----
 include/asm-x86/kvm_x86_emulate.h |    1 +
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index a391e21..b5d7bc8 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -58,6 +58,7 @@
 #define SrcMem32    (4<<4)	/* Memory operand (32-bit). */
 #define SrcImm      (5<<4)	/* Immediate operand. */
 #define SrcImmByte  (6<<4)	/* 8-bit sign-extended immediate operand. */
+#define SrcOne      (7<<4)	/* Implied '1' */
 #define SrcMask     (7<<4)
 /* Generic ModRM decode. */
 #define ModRM       (1<<7)
@@ -70,13 +71,18 @@
 #define Group       (1<<14)     /* Bits 3:5 of modrm byte extend opcode */
 #define GroupDual   (1<<15)     /* Alternate decoding of mod == 3 */
 #define GroupMask   0xff        /* Group number stored in bits 0:7 */
+/* Source 2 operand type */
+#define Src2None    (0<<29)
+#define Src2CL      (1<<29)
+#define Src2ImmByte (2<<29)
+#define Src2Mask    (7<<29)
 
 enum {
 	Group1_80, Group1_81, Group1_82, Group1_83,
 	Group1A, Group3_Byte, Group3, Group4, Group5, Group7,
 };
 
-static u16 opcode_table[256] = {
+static u32 opcode_table[256] = {
 	/* 0x00 - 0x07 */
 	ByteOp | DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM,
 	ByteOp | DstReg | SrcMem | ModRM, DstReg | SrcMem | ModRM,
@@ -195,7 +201,7 @@ static u16 opcode_table[256] = {
 	ImplicitOps, ImplicitOps, Group | Group4, Group | Group5,
 };
 
-static u16 twobyte_table[256] = {
+static u32 twobyte_table[256] = {
 	/* 0x00 - 0x0F */
 	0, Group | GroupDual | Group7, 0, 0, 0, 0, ImplicitOps, 0,
 	ImplicitOps, ImplicitOps, 0, 0, 0, ImplicitOps | ModRM, 0, 0,
@@ -253,7 +259,7 @@ static u16 twobyte_table[256] = {
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
 };
 
-static u16 group_table[] = {
+static u32 group_table[] = {
 	[Group1_80*8] =
 	ByteOp | DstMem | SrcImm | ModRM, ByteOp | DstMem | SrcImm | ModRM,
 	ByteOp | DstMem | SrcImm | ModRM, ByteOp | DstMem | SrcImm | ModRM,
@@ -297,7 +303,7 @@ static u16 group_table[] = {
 	SrcMem16 | ModRM | Mov, SrcMem | ModRM | ByteOp,
 };
 
-static u16 group2_table[] = {
+static u32 group2_table[] = {
 	[Group7*8] =
 	SrcNone | ModRM, 0, 0, 0,
 	SrcNone | ModRM | DstMem | Mov, 0,
@@ -1041,6 +1047,29 @@ done_prefixes:
 		c->src.bytes = 1;
 		c->src.val = insn_fetch(s8, 1, c->eip);
 		break;
+	case SrcOne:
+		c->src.bytes = 1;
+		c->src.val = 1;
+		break;
+	}
+
+	/*
+	 * Decode and fetch the second source operand: register, memory
+	 * or immediate.
+	 */
+	switch (c->d & Src2Mask) {
+	case Src2None:
+		break;
+	case Src2CL:
+		c->src2.bytes = 1;
+		c->src2.val = c->regs[VCPU_REGS_RCX] & 0x8;
+		break;
+	case Src2ImmByte:
+		c->src2.type = OP_IMM;
+		c->src2.ptr = (unsigned long *)c->eip;
+		c->src2.bytes = 1;
+		c->src2.val = insn_fetch(u8, 1, c->eip);
+		break;
 	}
 
 	/* Decode and fetch the destination operand: register or memory. */
diff --git a/include/asm-x86/kvm_x86_emulate.h b/include/asm-x86/kvm_x86_emulate.h
index 4e8c1e4..00de896 100644
--- a/include/asm-x86/kvm_x86_emulate.h
+++ b/include/asm-x86/kvm_x86_emulate.h
@@ -123,6 +123,7 @@ struct decode_cache {
 	u8 ad_bytes;
 	u8 rex_prefix;
 	struct operand src;
+	struct operand src2;
 	struct operand dst;
 	bool has_seg_override;
 	u8 seg_override;

Re: [PATCH] x86 emulator: Add a Src2 decode set and SrcOne operand type

[Avi Kivity]

Guillaume Thouvenin wrote:
>  Instruction like shld has three operands, so we need to add a Src2
> decode set. We start with Src2None, Src2CL, and Src2Imm8 to support
> shld and we will expand it later. Operand type of Src2 are placed at
> the end of the set to avoid renumbering. For Src2CL we mask to a single
> byte and set the operand length.
>
>  This patch also added SrcOne operand type when we need to decode an
> implied '1' like with regular shift instruction. 
>
>   

Looks fine; I will apply when shld/shrd are ready.

>  If needed I can split this patch into two patches, one for Src2 decode
> set and another one for SrcOne. 
>   

Maybe even three: extend the opcode descriptor to 32 bits, add SrcOne, 
add Src2*.

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