[patch 0 of 2] Emulate CMPS instruction

[patch 0 of 2] Emulate CMPS instruction

[Guillaume Thouvenin]

Hello,

 This patch emulates the CMPS instruction. It should fix the openbsd
bug opened in sourceforge (it does on my computer). There are two
patches. The first one renames the REP prefix definition to be more
comprehensive and the second one is the emulation of the CMPS
instruction.

Regards,
Guillaume

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[patch 1 of 2] Rename REP prefixes

[Guillaume Thouvenin]

This patch renames REP prefix with more suitable name.

Signed-off-by:: Guillaume Thouvenin <guillaume.thouvenin-Z51IpKcfGtLk1uMJSBkQmQ@public.gmane.org> 
---

 drivers/kvm/x86_emulate.c |    4 ++--
 drivers/kvm/x86_emulate.h |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index ff7010c..cee60eb 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -829,10 +829,10 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 			c->lock_prefix = 1;
 			break;
 		case 0xf2:	/* REPNE/REPNZ */
-			c->rep_prefix = REPNE_REPNZ;
+			c->rep_prefix = REPNE_PREFIX;
 			break;
 		case 0xf3:	/* REP/REPE/REPZ */
-			c->rep_prefix = REP_REPE_REPZ;
+			c->rep_prefix = REPE_PREFIX;
 			break;
 		default:
 			goto done_prefixes;
diff --git a/drivers/kvm/x86_emulate.h b/drivers/kvm/x86_emulate.h
index 5ce4c0c..61762f9 100644
--- a/drivers/kvm/x86_emulate.h
+++ b/drivers/kvm/x86_emulate.h
@@ -162,8 +162,8 @@ struct x86_emulate_ctxt {
 };
 
 /* Repeat String Operation Prefix */
-#define REP_REPE_REPZ  1
-#define REPNE_REPNZ    2
+#define REPE_PREFIX	1
+#define REPNE_PREFIX	2
 
 /* Execution mode, passed to the emulator. */
 #define X86EMUL_MODE_REAL     0	/* Real mode.             */

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[patch 2 of 2] Emulate CMPS instruction

[Guillaume Thouvenin]

This patch emulates the CMPS instruction.

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin-Z51IpKcfGtLk1uMJSBkQmQ@public.gmane.org>
---

 drivers/kvm/x86_emulate.c |   54 +++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index cee60eb..db744cf 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -445,6 +445,29 @@ static u16 twobyte_table[256] = {
 		register_address_increment(c->eip, rel);		\
 	} while (0)
 
+/* Test if the repeat string operation prefix is REPE/REPZ or
+ * REPNE/REPNZ and if it's the case it tests the corresponding
+ * termination condition according to:
+ *     - if REPE/REPZ and ZF = 0 then done
+ *     - if REPNE/REPNZ and ZF = 1 then done
+ */
+#define handle_rep_prefix(c)                                            \
+	do {								\
+		if ((c->b == 0xa6) || (c->b == 0xa7) ||			\
+				(c->b == 0xae) || (c->b == 0xaf)) {	\
+			if ((c->rep_prefix == REPE_PREFIX) &&		\
+				((ctxt->eflags & EFLG_ZF) == 0)) {	\
+				ctxt->vcpu->rip = c->eip;		\
+				goto done;				\
+			}						\
+			if ((c->rep_prefix == REPNE_PREFIX) &&		\
+				((ctxt->eflags & EFLG_ZF) == EFLG_ZF)) {\
+				ctxt->vcpu->rip = c->eip;		\
+				goto done;				\
+			}						\
+		}							\
+	} while (0)
+
 static int do_fetch_insn_byte(struct x86_emulate_ctxt *ctxt,
 			      struct x86_emulate_ops *ops,
 			      unsigned long linear, u8 *dest)
@@ -1540,10 +1563,15 @@ special_insn:
 		break;
 	}
 	if (c->rep_prefix) {
+		/* All REP prefixes have the same first termination condition */
 		if (c->regs[VCPU_REGS_RCX] == 0) {
 			ctxt->vcpu->rip = c->eip;
 			goto done;
 		}
+		/* The second termination condition only applies for REPE
+		 * and REPNE. handle_rep_prefix() macro deals with that. 
+		 */
+		handle_rep_prefix(c);
 		c->regs[VCPU_REGS_RCX]--;
 		c->eip = ctxt->vcpu->rip;
 	}
@@ -1570,8 +1598,30 @@ special_insn:
 							   : c->dst.bytes);
 		break;
 	case 0xa6 ... 0xa7:	/* cmps */
-		DPRINTF("Urk! I don't handle CMPS.\n");
-		goto cannot_emulate;
+		c->src.type = OP_NONE;
+		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+		c->src.ptr = (unsigned long *)register_address(
+						   ctxt->ds_base,
+						   c->regs[VCPU_REGS_RDI]);
+
+		c->dst.type = OP_NONE;
+		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+		c->dst.ptr = (unsigned long *)register_address(
+						   ctxt->es_base,
+						   c->regs[VCPU_REGS_RSI]);
+
+		DPRINTF("cmps: mem1=0x%p mem2=0x%p\n", c->src.ptr, c->dst.ptr);
+		
+		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
+
+		register_address_increment(c->regs[VCPU_REGS_RDI],
+				       (ctxt->eflags & EFLG_DF) ? -c->dst.bytes
+				       			  : c->dst.bytes);
+
+		register_address_increment(c->regs[VCPU_REGS_RSI],
+				       (ctxt->eflags & EFLG_DF) ? -c->dst.bytes
+				       			  : c->dst.bytes);
+		break;
 	case 0xaa ... 0xab:	/* stos */
 		c->dst.type = OP_MEM;
 		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [patch 2 of 2] Emulate CMPS instruction

[Avi Kivity]

Guillaume Thouvenin wrote:
> This patch emulates the CMPS instruction.
>
> Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin-Z51IpKcfGtLk1uMJSBkQmQ@public.gmane.org>
> ---
>
>  drivers/kvm/x86_emulate.c |   54 +++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
> index cee60eb..db744cf 100644
> --- a/drivers/kvm/x86_emulate.c
> +++ b/drivers/kvm/x86_emulate.c
> @@ -445,6 +445,29 @@ static u16 twobyte_table[256] = {
>  		register_address_increment(c->eip, rel);		\
>  	} while (0)
>  
> +/* Test if the repeat string operation prefix is REPE/REPZ or
> + * REPNE/REPNZ and if it's the case it tests the corresponding
> + * termination condition according to:
> + *     - if REPE/REPZ and ZF = 0 then done
> + *     - if REPNE/REPNZ and ZF = 1 then done
> + */
> +#define handle_rep_prefix(c)                                            \
> +	do {								\
> +		if ((c->b == 0xa6) || (c->b == 0xa7) ||			\
> +				(c->b == 0xae) || (c->b == 0xaf)) {	\
> +			if ((c->rep_prefix == REPE_PREFIX) &&		\
> +				((ctxt->eflags & EFLG_ZF) == 0)) {	\
> +				ctxt->vcpu->rip = c->eip;		\
> +				goto done;				\
> +			}						\
> +			if ((c->rep_prefix == REPNE_PREFIX) &&		\
> +				((ctxt->eflags & EFLG_ZF) == EFLG_ZF)) {\
> +				ctxt->vcpu->rip = c->eip;		\
> +				goto done;				\
> +			}						\
> +		}							\
> +	} while (0)
> +
>   

No new macros in the emulator please. Just inline it at the callsite.

>  static int do_fetch_insn_byte(struct x86_emulate_ctxt *ctxt,
>  			      struct x86_emulate_ops *ops,
>  			      unsigned long linear, u8 *dest)
> @@ -1540,10 +1563,15 @@ special_insn:
>  		break;
>  	}
>  	if (c->rep_prefix) {
> +		/* All REP prefixes have the same first termination condition */
>  		if (c->regs[VCPU_REGS_RCX] == 0) {
>  			ctxt->vcpu->rip = c->eip;
>  			goto done;
>  		}
> +		/* The second termination condition only applies for REPE
> +		 * and REPNE. handle_rep_prefix() macro deals with that. 
> +		 */
> +		handle_rep_prefix(c);
>  		c->regs[VCPU_REGS_RCX]--;
>  		c->eip = ctxt->vcpu->rip;
>  	}
> @@ -1570,8 +1598,30 @@ special_insn:
>  							   : c->dst.bytes);
>  		break;
>  	case 0xa6 ... 0xa7:	/* cmps */
> -		DPRINTF("Urk! I don't handle CMPS.\n");
> -		goto cannot_emulate;
> +		c->src.type = OP_NONE;
>   

Shouldn't this be OP_MEM?

> +		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
> +		c->src.ptr = (unsigned long *)register_address(
> +						   ctxt->ds_base,
> +						   c->regs[VCPU_REGS_RDI]);
> +
> +		c->dst.type = OP_NONE;
>   

And here?

> +		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
> +		c->dst.ptr = (unsigned long *)register_address(
> +						   ctxt->es_base,
> +						   c->regs[VCPU_REGS_RSI]);
> +
> +		DPRINTF("cmps: mem1=0x%p mem2=0x%p\n", c->src.ptr, c->dst.ptr);
> +		
>   

Where is the actual memory access?

> +		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
> +
> +		register_address_increment(c->regs[VCPU_REGS_RDI],
> +				       (ctxt->eflags & EFLG_DF) ? -c->dst.bytes
> +				       			  : c->dst.bytes);
> +
> +		register_address_increment(c->regs[VCPU_REGS_RSI],
> +				       (ctxt->eflags & EFLG_DF) ? -c->dst.bytes
> +				       			  : c->dst.bytes);
> +		break;
>  	case 0xaa ... 0xab:	/* stos */
>  		c->dst.type = OP_MEM;
>  		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2005.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>   


-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [patch 2 of 2] Emulate CMPS instruction

[Guillaume Thouvenin]

On Fri, 23 Nov 2007 19:54:46 +0200
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:

> 
> No new macros in the emulator please. Just inline it at the callsite.
>

Ok I make the modification.
 							   : c->dst.bytes);
> >  		break;
> >  	case 0xa6 ... 0xa7:	/* cmps */
> > -		DPRINTF("Urk! I don't handle CMPS.\n");
> > -		goto cannot_emulate;
> > +		c->src.type = OP_NONE;
> >   
> 
> Shouldn't this be OP_MEM?

As bytes to be compared are just read I use OP_NONE to disable writeback.  
 
> 
> > +		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
> > +		c->dst.ptr = (unsigned long *)register_address(
> > +						   ctxt->es_base,
> > +						   c->regs[VCPU_REGS_RSI]);
> > +
> > +		DPRINTF("cmps: mem1=0x%p mem2=0x%p\n", c->src.ptr, c->dst.ptr);
> > +		
> >   
> 
> Where is the actual memory access?

Oops I missed that point.... I fix that immediately.

Thanks for your help,
Guillaume

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [patch 2 of 2][rewritten] Emulate CMPS instruction

[Guillaume Thouvenin]

This patch emulates the CMPS instruction. I made corrections requested
by Avi (removed macro and added the memory access). I fixed an
inversion between arguments VCPU_REGS_RSI and VCPU_REGS_RDI in function
register_address(). I also added the test if DS segment is overridden
with a segment override prefix (ES segment cannot be overriden).

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin-Z51IpKcfGtLk1uMJSBkQmQ@public.gmane.org>
---

 drivers/kvm/x86_emulate.c |   58 +++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index cee60eb..9eb9922 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -1540,10 +1540,31 @@ special_insn:
 		break;
 	}
 	if (c->rep_prefix) {
+		/* All REP prefixes have the same first termination condition */
 		if (c->regs[VCPU_REGS_RCX] == 0) {
 			ctxt->vcpu->rip = c->eip;
 			goto done;
 		}
+		/* The second termination condition only applies for REPE
+		 * and REPNE. Test if the repeat string operation prefix is 
+		 * REPE/REPZ or REPNE/REPNZ and if it's the case it tests the 
+		 * corresponding termination condition according to:
+		 * 	- if REPE/REPZ and ZF = 0 then done
+		 * 	- if REPNE/REPNZ and ZF = 1 then done
+		 */
+		if ((c->b == 0xa6) || (c->b == 0xa7) ||
+				(c->b == 0xae) || (c->b == 0xaf)) {
+			if ((c->rep_prefix == REPE_PREFIX) &&
+				((ctxt->eflags & EFLG_ZF) == 0)) {
+					ctxt->vcpu->rip = c->eip;
+					goto done;
+			}
+			if ((c->rep_prefix == REPNE_PREFIX) &&
+				((ctxt->eflags & EFLG_ZF) == EFLG_ZF)) {
+				ctxt->vcpu->rip = c->eip;
+				goto done;
+			}
+		}
 		c->regs[VCPU_REGS_RCX]--;
 		c->eip = ctxt->vcpu->rip;
 	}
@@ -1570,8 +1591,41 @@ special_insn:
 							   : c->dst.bytes);
 		break;
 	case 0xa6 ... 0xa7:	/* cmps */
-		DPRINTF("Urk! I don't handle CMPS.\n");
-		goto cannot_emulate;
+		c->src.type = OP_NONE; /* Disable writeback. */
+		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+		c->src.ptr = (unsigned long *)register_address(
+				c->override_base ? *c->override_base :
+						   ctxt->ds_base,
+						   c->regs[VCPU_REGS_RSI]);
+		if ((rc = ops->read_emulated((unsigned long)c->src.ptr, 
+						&c->src.val,
+						c->src.bytes, 
+						ctxt->vcpu)) != 0)
+			goto done;
+
+		c->dst.type = OP_NONE; /* Disable writeback. */
+		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+		c->dst.ptr = (unsigned long *)register_address(
+						   ctxt->es_base,
+						   c->regs[VCPU_REGS_RDI]);
+		if ((rc = ops->read_emulated((unsigned long)c->dst.ptr, 
+						&c->dst.val,
+						c->dst.bytes, 
+						ctxt->vcpu)) != 0)
+			goto done;
+
+		DPRINTF("cmps: mem1=0x%p mem2=0x%p\n", c->src.ptr, c->dst.ptr);
+		
+		emulate_2op_SrcV("cmp", c->src, c->dst, ctxt->eflags);
+
+		register_address_increment(c->regs[VCPU_REGS_RSI],
+				       (ctxt->eflags & EFLG_DF) ? -c->src.bytes
+				       			  : c->src.bytes);
+		register_address_increment(c->regs[VCPU_REGS_RDI],
+				       (ctxt->eflags & EFLG_DF) ? -c->dst.bytes
+				       			  : c->dst.bytes);
+
+		break;
 	case 0xaa ... 0xab:	/* stos */
 		c->dst.type = OP_MEM;
 		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [patch 2 of 2][rewritten] Emulate CMPS instruction

[Avi Kivity]

Guillaume Thouvenin wrote:
> This patch emulates the CMPS instruction. I made corrections requested
> by Avi (removed macro and added the memory access). I fixed an
> inversion between arguments VCPU_REGS_RSI and VCPU_REGS_RDI in function
> register_address(). I also added the test if DS segment is overridden
> with a segment override prefix (ES segment cannot be overriden).
>   

Applied, thanks.  It passes the emulator testsuite, but if I change m3 
to m2 kvm spins without entering the guest (probably due to two reads 
from mmio space in one instruction).

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/