[RFC Patch 0/5] x86_emulator: emulate shld and shrd instruction

[RFC Patch 0/5] x86_emulator: emulate shld and shrd instruction

[Guillaume Thouvenin]

This series of patches emulate instructions shld and shrd. As those
instructions have three operands we introduce a decode set for the Src2
operand. By doing this, the opcode descriptor needs to be extend to 32
bit.

So this series of patches:
 [1/5] extend the opcode descriptor to 32 bits
 [2/5] add Src2 decode set
 [3/5] add a new "implied 1" Src decode type
 [4/5] add the assembler code for three operands
 [5/5] add the emulation of shld and shrd instructions

[Patch 1/5] x86_emulator: Extend the opcode descriptor

[Guillaume Thouvenin]

Extend the opcode descriptor to 32 bits. This is needed by
the introduction of a new Src2 operand type.

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

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 8f60ace..f0c7620 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -76,7 +76,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 +195,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 +253,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 +297,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,
-- 
1.6.0.3.514.g2f91b

On Mon, 3 Nov 2008 15:42:49 +0100
Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net> wrote:

> This series of patches emulate instructions of shld and shrd
> instructions. As those instructions have three operands we introduce a
> decode set for the Src2 operand. By doing this, the opcode descriptor
> needs to be extend to 32 bit.
> 
> So this series of patches:
>  [1/5] extend the opcode descr

[Patch 2/5] 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 Src2ImmByte, Src2One
to support shld/shrd and we will expand it later.

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

diff --git a/arch/x86/include/asm/kvm_x86_emulate.h b/arch/x86/include/asm/kvm_x86_emulate.h
index 16a0026..6a15973 100644
--- a/arch/x86/include/asm/kvm_x86_emulate.h
+++ b/arch/x86/include/asm/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;
diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index f0c7620..4e8ef29 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -70,6 +70,12 @@
 #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 Src2One     (3<<29)
+#define Src2Mask    (7<<29)
 
 enum {
 	Group1_80, Group1_81, Group1_82, Group1_83,
@@ -1043,6 +1049,29 @@ 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.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;
+	case Src2One:
+		c->src2.bytes = 1;
+		c->src2.val = 1;
+		break;
+	}
+
 	/* Decode and fetch the destination operand: register or memory. */
 	switch (c->d & DstMask) {
 	case ImplicitOps:
-- 
1.6.0.3.514.g2f91b

[Patch 3/5] x86_emulator: add a new "implied 1" Src decode type

[Guillaume Thouvenin]

Add SrcOne operand type when we need to decode an
implied '1' like with regular shift instruction

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
---
 arch/x86/kvm/x86_emulate.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 4e8ef29..6e23e9c 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)
@@ -1047,6 +1048,10 @@ 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;
 	}
 
 	/*
-- 
1.6.0.3.514.g2f91b

[Patch 5/5] x86_emulator: add the emulation of shld and shrd instructions

[Guillaume Thouvenin]

Add emulation of shld and shrd instructions

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
---
 arch/x86/kvm/x86_emulate.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index b6108ad..3652658 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -237,9 +237,14 @@ static u32 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 | Src2ImmByte | ModRM,
+	DstMem | SrcReg | Src2CL | ModRM, 0, 0,
 	/* 0xA8 - 0xAF */
-	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, ModRM, 0,
+	0, 0, 0, DstMem | SrcReg | ModRM | BitOp,
+	DstMem | SrcReg | Src2ImmByte | ModRM,
+	DstMem | SrcReg | Src2CL | ModRM,
+	ModRM, 0,
 	/* 0xB0 - 0xB7 */
 	ByteOp | DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM, 0,
 	    DstMem | SrcReg | ModRM | BitOp,
@@ -2056,12 +2061,20 @@ 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 */
+		emulate_3op("shld", c->src, c->src2, c->dst, ctxt->eflags);
+		break;
 	case 0xab:
 	      bts:		/* bts */
 		/* only subword offset */
 		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bts", c->src, c->dst, ctxt->eflags);
 		break;
+	case 0xac: /* shrd imm8, r, r/m */
+	case 0xad: /* shrd cl, r, r/m */
+		emulate_3op("shrd", c->src, c->src2, c->dst, ctxt->eflags);
+		break;
 	case 0xae:              /* clflush */
 		break;
 	case 0xb0 ... 0xb1:	/* cmpxchg */
-- 
1.6.0.3.514.g2f91b

[Patch 4/5] x86_emulator: add the assembler code for three operands

[Guillaume Thouvenin]

Add the assembler code for three operands

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
---
 arch/x86/kvm/x86_emulate.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 6e23e9c..b6108ad 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -366,6 +366,35 @@ static u32 group2_table[] = {
 	"andl %"_msk",%"_LO32 _tmp"; "		\
 	"orl  %"_LO32 _tmp",%"_sav"; "
 
+/* Instruction has three operands */
+/* In the switch we only implement case 4 because we know that for shld instruction
+ * bytes are equal to 4. When eveything will be fine, we will add others cases.
+ */
+#define __emulate_3op(_op,_src,_src2,_dst,_eflags,_by,_bx,_wx,_wy,_lx,_ly,_qx,_qy) 	\
+	do {										\
+		unsigned long _tmp;							\
+											\
+		switch((_dst).bytes) {							\
+			case 4:								\
+				__asm__ __volatile__ (					\
+				_PRE_EFLAGS("0", "5", "2")				\
+				"mov %4, %%rcx \n\t"					\
+				_op" %%cl,%3,%1; \n\t"					\
+				_POST_EFLAGS("0", "5", "2")				\
+				: "=m" (_eflags), "=m" ((_dst).val),			\
+				  "=&r" (_tmp)						\
+				: _ly ((_src).val) , _ly ((_src2).val), "i" (EFLAGS_MASK) \
+				: "%rcx" ); 						\
+				break;							\
+			default:							\
+				printk(KERN_INFO "__emulate_3op: case need to be added \n"); \
+		}									\
+	} while (0)
+
+#define emulate_3op(_op, _src, _src2, _dst, _eflags)	\
+ __emulate_3op(_op, _src, _src2, _dst, _eflags,		\
+               "b", "c", "b", "c", "b", "r", "b", "c")
+
 /* Raw emulation: instruction has two explicit operands. */
 #define __emulate_2op_nobyte(_op,_src,_dst,_eflags,_wx,_wy,_lx,_ly,_qx,_qy) \
 	do { 								    \
-- 
1.6.0.3.514.g2f91b

Re: [Patch 4/5] x86_emulator: add the assembler code for three operands

[Avi Kivity]

Guillaume Thouvenin wrote:
> Add the assembler code for three operands
>
>  
> +/* Instruction has three operands */
> +/* In the switch we only implement case 4 because we know that for shld instruction
> + * bytes are equal to 4. When eveything will be fine, we will add others cases.
>   

No, shld is defined for 16, 32, and 64 bit operands.  Need to implement 
those too.

> +#define __emulate_3op(_op,_src,_src2,_dst,_eflags,_by,_bx,_wx,_wy,_lx,_ly,_qx,_qy) 	\
Since this is not a generic 3 operand implementation, it's better to 
call this something like __emulate_2op_cl() to give attention to this fact.


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

Re: [Patch 4/5] x86_emulator: add the assembler code for three operands

[Guillaume Thouvenin]

On Tue, 04 Nov 2008 12:21:30 +0200
Avi Kivity <avi@redhat.com> wrote:

> Guillaume Thouvenin wrote:
> > Add the assembler code for three operands
> >
> >  
> > +/* Instruction has three operands */
> > +/* In the switch we only implement case 4 because we know that for shld instruction
> > + * bytes are equal to 4. When eveything will be fine, we will add others cases.
> >   
> 
> No, shld is defined for 16, 32, and 64 bit operands.  Need to implement 
> those too.

I tried something like:

+/* Instruction has three operands */
+/* In the switch we only implement case 4 because we know that for shld instruction
+ * bytes are equal to 4. When eveything will be fine, we will add others cases.
+ */
+#define __emulate_2op_cl(_op,_src,_src2,_dst,_eflags,_by,_bx,_wx,_wy,_lx,_ly,_qx,_qy)  \
+       do {                                                                            \
+               unsigned long _tmp;                                                     \
+                                                                                       \
+               switch((_dst).bytes) {                                                  \
+                       case 2:                                                         \
+                               __asm__ __volatile__ (                                  \
+                               _PRE_EFLAGS("0", "5", "2")                              \
+                               "mov %4, %%rcx \n\t"                                    \
+                               _op"w %%cl,%3,%1; \n\t"                                 \
+                               _POST_EFLAGS("0", "5", "2")                             \
+                               : "=m" (_eflags), "=m" ((_dst).val),                    \
+                                 "=&r" (_tmp)                                          \
+                               : _wy ((_src).val) , _wy ((_src2).val), "i" (EFLAGS_MASK) \
+                               : "%rcx" );                                             \
+                               break;                                                  \
+                       case 4:                                                         \
+                               __asm__ __volatile__ (                                  \
+                               _PRE_EFLAGS("0", "5", "2")                              \
+                               "mov %4, %%rcx \n\t"                                    \
+                               _op"l %%cl,%3,%1; \n\t"                                 \
+                               _POST_EFLAGS("0", "5", "2")                             \
+                               : "=m" (_eflags), "=m" ((_dst).val),                    \
+                                 "=&r" (_tmp)                                          \
+                               : _ly ((_src).val) , _ly ((_src2).val), "i" (EFLAGS_MASK) \
+                               : "%rcx" );                                             \
+                               break;                                                  \
+                       case 8:                                                         \
+                               __asm__ __volatile__ (                                  \
+                               _PRE_EFLAGS("0", "5", "2")                              \
+                               "mov %4, %%rcx \n\t"                                    \
+                               _op"q %%cl,%3,%1; \n\t"                                 \
+                               _POST_EFLAGS("0", "5", "2")                             \
+                               : "=m" (_eflags), "=m" ((_dst).val),                    \
+                                 "=&r" (_tmp)                                          \
+                               : _ly ((_src).val) , _ly ((_src2).val), "i" (EFLAGS_MASK) \
+                               : "%rcx" );                                             \
+                               break;                                                  \
+               }                                                                       \
+       } while (0)
+
+#define emulate_2op_cl(_op, _src, _src2, _dst, _eflags)        \
+ __emulate_2op_cl(_op, _src, _src2, _dst, _eflags,     \
+               "b", "r", "b", "r", "b", "r", "b", "r")
+

but it doesn't work because shld can not be used with suffix 'l' or 'w'
etc... Is the solution is to have a single case for all operand size like:

+#define __emulate_2op_cl(_op,_src,_src2,_dst,_eflags,_wx,_wy)  \
+       do {                                                                            \
+               unsigned long _tmp;                                                     \
+                                                                             \
+               __asm__ __volatile__ (                                  \
+                               _PRE_EFLAGS("0", "5", "2")                              \
+                               "mov %4, %%rcx \n\t"                                    \
+                               _op" %%cl,%3,%1; \n\t"                                 \
+                               _POST_EFLAGS("0", "5", "2")                             \
+                               : "=m" (_eflags), "=m" ((_dst).val),                    \
+                                 "=&r" (_tmp)                                          \
+                               : _wy ((_src).val) , _wy ((_src2).val), "i" (EFLAGS_MASK) \
+                               : "%rcx" );                                             \
+       } while (0)

I tested the code and it seems to work.

Excuse me for the delay of this answer,
Regards,
Guillaume

Re: [Patch 4/5] x86_emulator: add the assembler code for three operands

[Avi Kivity]

Guillaume Thouvenin wrote:
> On Tue, 04 Nov 2008 12:21:30 +0200
> Avi Kivity <avi@redhat.com> wrote:
>
>   
>> Guillaume Thouvenin wrote:
>>     
>>> Add the assembler code for three operands
>>>
>>>  
>>> +/* Instruction has three operands */
>>> +/* In the switch we only implement case 4 because we know that for shld instruction
>>> + * bytes are equal to 4. When eveything will be fine, we will add others cases.
>>>   
>>>       
>> No, shld is defined for 16, 32, and 64 bit operands.  Need to implement 
>> those too.
>>     
>
> I tried something like:
>
> +/* Instruction has three operands */
> +/* In the switch we only implement case 4 because we know that for shld instruction
> + * bytes are equal to 4. When eveything will be fine, we will add others cases.
> + */
> +#define __emulate_2op_cl(_op,_src,_src2,_dst,_eflags,_by,_bx,_wx,_wy,_lx,_ly,_qx,_qy)  \
> +       do {                                                                            \
> +               unsigned long _tmp;                                                     \
> +                                                                                       \
> +               switch((_dst).bytes) {                                                  \
> +                       case 2:                                                         \
> +                               __asm__ __volatile__ (                                  \
> +                               _PRE_EFLAGS("0", "5", "2")                              \
> +                               "mov %4, %%rcx \n\t"                                    \
> +                               _op"w %%cl,%3,%1; \n\t"                                 \
> +                               _POST_EFLAGS("0", "5", "2")                             \
> +                               : "=m" (_eflags), "=m" ((_dst).val),                    \
> +                                 "=&r" (_tmp)                                          \
> +                               : _wy ((_src).val) , _wy ((_src2).val), "i" (EFLAGS_MASK) \
> +                               : "%rcx" );                                             \
> +                               break;                                                  \
> +                       case 4:                                                         \
> +                               __asm__ __volatile__ (                                  \
> +                               _PRE_EFLAGS("0", "5", "2")                              \
> +                               "mov %4, %%rcx \n\t"                                    \
> +                               _op"l %%cl,%3,%1; \n\t"                                 \
> +                               _POST_EFLAGS("0", "5", "2")                             \
> +                               : "=m" (_eflags), "=m" ((_dst).val),                    \
> +                                 "=&r" (_tmp)                                          \
> +                               : _ly ((_src).val) , _ly ((_src2).val), "i" (EFLAGS_MASK) \
> +                               : "%rcx" );                                             \
> +                               break;                                                  \
> +                       case 8:                                                         \
> +                               __asm__ __volatile__ (                                  \
> +                               _PRE_EFLAGS("0", "5", "2")                              \
> +                               "mov %4, %%rcx \n\t"                                    \
> +                               _op"q %%cl,%3,%1; \n\t"                                 \
> +                               _POST_EFLAGS("0", "5", "2")                             \
> +                               : "=m" (_eflags), "=m" ((_dst).val),                    \
> +                                 "=&r" (_tmp)                                          \
> +                               : _ly ((_src).val) , _ly ((_src2).val), "i" (EFLAGS_MASK) \
> +                               : "%rcx" );                                             \
> +                               break;                                                  \
> +               }                                                                       \
> +       } while (0)
> +
> +#define emulate_2op_cl(_op, _src, _src2, _dst, _eflags)        \
> + __emulate_2op_cl(_op, _src, _src2, _dst, _eflags,     \
> +               "b", "r", "b", "r", "b", "r", "b", "r")
> +
>
> but it doesn't work because shld can not be used with suffix 'l' or 'w'
> etc... Is the solution is to have a single case for all operand size like:
>
>   

It does work, perhaps the variables were defined with the wrong size.

This works:

int shld(void)
{
unsigned short w1 = 3, w2 = 5;
unsigned int l1 = 3, l2 = 5;
unsigned long q1 = 3, q2 = 5;

asm("shldw %1, %0" : "+r"(w1) : "r"(w2));
asm("shldl %1, %0" : "+r"(l1) : "r"(l2));
asm("shldq %1, %0" : "+r"(q1) : "r"(q2));

return w1 + l1 + q1;
}



> +#define __emulate_2op_cl(_op,_src,_src2,_dst,_eflags,_wx,_wy)  \
> +       do {                                                                            \
> +               unsigned long _tmp;                                                     \
> +                                                                             \
> +               __asm__ __volatile__ (                                  \
> +                               _PRE_EFLAGS("0", "5", "2")                              \
> +                               "mov %4, %%rcx \n\t"                                    \
> +                               _op" %%cl,%3,%1; \n\t"                                 \
> +                               _POST_EFLAGS("0", "5", "2")                             \
> +                               : "=m" (_eflags), "=m" ((_dst).val),                    \
> +                                 "=&r" (_tmp)                                          \
> +                               : _wy ((_src).val) , _wy ((_src2).val), "i" (EFLAGS_MASK) \
> +                               : "%rcx" );                                             \
> +       } while (0)
>
> I tested the code and it seems to work.
>   

That's actually better and could be used for the other emulations. 
Please disassemble x86_emulate.o and verify that there are three 
different shld instructions, one for each register size.

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

Re: [Patch 4/5] x86_emulator: add the assembler code for three operands

[Guillaume Thouvenin]

On Tue, 25 Nov 2008 16:59:00 +0200
Avi Kivity <avi@redhat.com> wrote:

> 
> 
> > +#define __emulate_2op_cl(_op,_src,_src2,_dst,_eflags,_wx,_wy)  \
> > +       do {                                                                            \
> > +               unsigned long _tmp;                                                     \
> > +                                                                             \
> > +               __asm__ __volatile__ (                                  \
> > +                               _PRE_EFLAGS("0", "5", "2")                              \
> > +                               "mov %4, %%rcx \n\t"                                    \
> > +                               _op" %%cl,%3,%1; \n\t"                                 \
> > +                               _POST_EFLAGS("0", "5", "2")                             \
> > +                               : "=m" (_eflags), "=m" ((_dst).val),                    \
> > +                                 "=&r" (_tmp)                                          \
> > +                               : _wy ((_src).val) , _wy ((_src2).val), "i" (EFLAGS_MASK) \
> > +                               : "%rcx" );                                             \
> > +       } while (0)
> >
> > I tested the code and it seems to work.
> >   
> 
> That's actually better and could be used for the other emulations. 
> Please disassemble x86_emulate.o and verify that there are three 
> different shld instructions, one for each register size.
> 

I tried with the following code (it's nearly the same as above):

+               __asm__ __volatile__ (                                  \
+                       _PRE_EFLAGS("0", "5", "2")                              \
+                       "mov %4, %%rcx \n\t"                                    \
+                       _op" %3,%1; \n\t"                                       \
+                       _POST_EFLAGS("0", "5", "2")                             \
+                       : "=m" (_eflags), "+r" ((_dst).val),                    \
+                         "=&r" (_tmp)                                          \
+                       : _x ((_src).val) , _y ((_src2).val), "i" (EFLAGS_MASK) \
+                       : "%rcx" );                                             \

When I disassemble x86_emulate.o I can see the following produced code:

    4787:       ...
    4788:       41 8f 44 24 08          popq   0x8(%r12)
    478d:       48 89 d1                mov    %rdx,%rcx
    4790:       48 0f a5 f0             shld   %cl,%rsi,%rax
    4794:       9c                      pushfq
    4795:       ... 

It's the only code with shld instruction. I don't see how you can have
three different shld instructions here. I'm sure that I'm missing
something here because for me, when we emulate the shld instruction, the
code produced is the same. I mean that src.val and dst.val are always
unsigned long and I don't see why register size are important here. In
fact I don't understand why we need to use the switch ((_dst).bytes)
for the other emulations.

Guillaume

Re: [Patch 4/5] x86_emulator: add the assembler code for three operands

[Avi Kivity]

Guillaume Thouvenin wrote:
> It's the only code with shld instruction. I don't see how you can have
> three different shld instructions here. I'm sure that I'm missing
> something here because for me, when we emulate the shld instruction, the
> code produced is the same. I mean that src.val and dst.val are always
> unsigned long and I don't see why register size are important here. In
> fact I don't understand why we need to use the switch ((_dst).bytes)
> for the other emulations.
>   

The flags vary according to operand size.  For example, shld $1, %bx, 
%ax where %ax == 0x8000 will set eflags.cf, but shld $1, %ebx, %eax , 
with the same arguments, will not.

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

Re: [Patch 4/5] x86_emulator: add the assembler code for three operands

[Guillaume Thouvenin]

On Tue, 25 Nov 2008 16:59:00 +0200
Avi Kivity <avi@redhat.com> wrote:

> That's actually better and could be used for the other emulations. 
> Please disassemble x86_emulate.o and verify that there are three 
> different shld instructions, one for each register size.

For generating the three shld instructions I need to do the following:

 
+/* Instruction has three operands like shld */
+#define __emulate_2op_cl(_op,_shift, _src, _dst,_eflags,_suffix) 		\
+	do {									\
+		unsigned long _tmp;						\
+		__asm__ __volatile__ (						\
+			_PRE_EFLAGS("0", "5", "2")				\
+			_op _suffix" %4,%1 \n\t"				\
+			_POST_EFLAGS("0", "5", "2")				\
+			: "=m" (_eflags), "+r" (_dst),			\
+			  "=&r" (_tmp)						\
+			: "c" (_shift) , "r" (_src), "i" (EFLAGS_MASK) \
+			); 							\
+	} while (0)
+
+#define emulate_2op_cl(_op, _shift, _src, _dst, _eflags)	\
+	do {								\
+		switch ((_dst).bytes) {					\
+		case 2:	{						\
+			  unsigned short shift = (_shift).val; \
+			  unsigned short src = (_src).val; \
+			  unsigned short dst = (_dst).val; \
+			__emulate_2op_cl(_op,shift, src,dst,_eflags,"w"); \
+			break;						\
+			} \
+		case 4:	{						\
+			  unsigned int shift = (_shift).val; \
+			  unsigned int src = (_src).val; \
+			  unsigned int dst = (_dst).val; \
+			__emulate_2op_cl(_op,shift, src,dst,_eflags,"l"); \
+			break;						\
+			} \
+		case 8:	{						\
+			  unsigned long shift = (_shift).val; \
+			  unsigned long src = (_src).val; \
+			  unsigned long dst = (_dst).val; \
+			ON64(__emulate_2op_cl(_op,shift, src,dst,_eflags,"q")); \
+			break;						\
+			} \
+		}							\
+	} while (0)
+

Otherwise the code for "case 4:" cannot be generated. I've got:

Error: Incorrect register `%rax' used with `l' suffix

Is it correct? If yes I will repost patches that emulate shld/shrd instructions.

Guillaume

Re: [RFC Patch 0/5] x86_emulator: emulate shld and shrd instruction

[Avi Kivity]

Guillaume Thouvenin wrote:
> This series of patches emulate instructions shld and shrd. As those
> instructions have three operands we introduce a decode set for the Src2
> operand. By doing this, the opcode descriptor needs to be extend to 32
> bit.
>   

Other than the missing operand sizes, looks good.

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

[Patch 0/5] x86_emulator: emulate shld and shrd instructions

[Guillaume Thouvenin]

This series of patches emulate instructions shld and shrd. As those
instructions have three operands we introduce a decode set for the Src2
operand. By doing this, the opcode descriptor needs to be extend to 32
bit.

So this series of patches:
 [1/5] extend the opcode descriptor to 32 bits
 [2/5] add Src2 decode set
 [3/5] add a new "implied 1" Src decode type
 [4/5] add the assembler code for three operands (one operand is stored
in EXC) 
 [5/5] add the emulation of shld and shrd instructions

[Patch 4/5] x86_emulator: add the assembler code for three operands

[Guillaume Thouvenin]

Add the assembler code for instruction with three operands and one
operand is stored in ECX register

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
---
 arch/x86/kvm/x86_emulate.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 0c75306..9ae6d5b 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -431,6 +431,45 @@ static u32 group2_table[] = {
 	__emulate_2op_nobyte(_op, _src, _dst, _eflags,			\
 			     "w", "r", _LO32, "r", "", "r")
 
+/* Instruction has three operands and one operand is stored in ECX register */
+#define __emulate_2op_cl(_op, _cl, _src, _dst, _eflags, _suffix, _type) 	\
+	do {									\
+		unsigned long _tmp;						\
+		_type _clv  = (_cl).val;  					\
+		_type _srcv = (_src).val;    					\
+		_type _dstv = (_dst).val;					\
+										\
+		__asm__ __volatile__ (						\
+			_PRE_EFLAGS("0", "5", "2")				\
+			_op _suffix " %4,%1 \n"					\
+			_POST_EFLAGS("0", "5", "2")				\
+			: "=m" (_eflags), "+r" (_dstv), "=&r" (_tmp)		\
+			: "c" (_clv) , "r" (_srcv), "i" (EFLAGS_MASK)		\
+			); 							\
+										\
+		(_cl).val  = (unsigned long) _clv;				\
+		(_src).val = (unsigned long) _srcv;				\
+		(_dst).val = (unsigned long) _dstv;				\
+	} while (0)
+
+#define emulate_2op_cl(_op, _cl, _src, _dst, _eflags)				\
+	do {									\
+		switch ((_dst).bytes) {						\
+		case 2:								\
+			__emulate_2op_cl(_op, _cl, _src, _dst, _eflags,  	\
+						"w", unsigned short);         	\
+			break;							\
+		case 4: 							\
+			__emulate_2op_cl(_op, _cl, _src, _dst, _eflags,  	\
+						"l", unsigned int);           	\
+			break;							\
+		case 8:								\
+			ON64(__emulate_2op_cl(_op, _cl, _src, _dst, _eflags,	\
+						"q", unsigned long));  		\
+			break;							\
+		}								\
+	} while (0)
+
 #define __emulate_1op(_op, _dst, _eflags, _suffix)			\
 	do {								\
 		unsigned long _tmp;					\
-- 
1.6.0.4.623.g171d7