[PATCH] Correct management of REP prefix

[PATCH] Correct management of REP prefix

[Laurent Vivier]

This patch corrects some errors appearing when we have an emulation failure
on an operation using REP prefix.

When x86_emulate_insn() fails, saving EIP and ECX is not enough as emulation
should have modified other registers like RSI or RDI. Moreover, the emulation
can fail on the writeback, and in this case we are not able to restore 
registers.

This patch takes another approach: at the beginning of x86_emulate_insn() we 
restore state we have at end of x86_decode_insn(). To do that, we store EIP in
a new field in decode_cache, decode_eip. This field store the EIP as it is at
the end of x86_decode_insn(); and at beginning of x86_emulate_insn(), we restore
all registers as they are in vcpu. We can do that, because the x86_decode_insn()
doesn't modify registers (except EIP).

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
---
 drivers/kvm/x86_emulate.c |   71 +++++++++++++++++++++++---------------------
 drivers/kvm/x86_emulate.h |    1 +
 2 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 35069e3..3febb58 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -526,10 +526,11 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	int mode = ctxt->mode;
 	int index_reg = 0, base_reg = 0, scale, rip_relative = 0;
 
-	/* Shadow copy of register state. Committed on successful emulation. */
-
 	memset(c, 0, sizeof(struct decode_cache));
-	c->eip = ctxt->vcpu->rip;
+
+	/* decode_eip will contain EIP after decode phase */
+
+	c->decode_eip = ctxt->vcpu->rip;
 	memcpy(c->regs, ctxt->vcpu->regs, sizeof c->regs);
 
 	switch (mode) {
@@ -552,7 +553,7 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 
 	/* Legacy prefixes. */
 	for (;;) {
-		switch (c->b = insn_fetch(u8, 1, c->eip)) {
+		switch (c->b = insn_fetch(u8, 1, c->decode_eip)) {
 		case 0x66:	/* operand-size override */
 			c->op_bytes ^= 6;	/* switch between 2/4 bytes */
 			break;
@@ -620,7 +621,7 @@ done_prefixes:
 		/* Two-byte opcode? */
 		if (c->b == 0x0f) {
 			c->twobyte = 1;
-			c->b = insn_fetch(u8, 1, c->eip);
+			c->b = insn_fetch(u8, 1, c->decode_eip);
 			c->d = twobyte_table[c->b];
 		}
 
@@ -633,7 +634,7 @@ done_prefixes:
 
 	/* ModRM and SIB bytes. */
 	if (c->d & ModRM) {
-		c->modrm = insn_fetch(u8, 1, c->eip);
+		c->modrm = insn_fetch(u8, 1, c->decode_eip);
 		c->modrm_mod |= (c->modrm & 0xc0) >> 6;
 		c->modrm_reg |= (c->modrm & 0x38) >> 3;
 		c->modrm_rm |= (c->modrm & 0x07);
@@ -657,13 +658,14 @@ done_prefixes:
 			case 0:
 				if (c->modrm_rm == 6)
 					c->modrm_ea +=
-						insn_fetch(u16, 2, c->eip);
+					      insn_fetch(u16, 2, c->decode_eip);
 				break;
 			case 1:
-				c->modrm_ea += insn_fetch(s8, 1, c->eip);
+				c->modrm_ea += insn_fetch(s8, 1, c->decode_eip);
 				break;
 			case 2:
-				c->modrm_ea += insn_fetch(u16, 2, c->eip);
+				c->modrm_ea += insn_fetch(u16, 2,
+							  c->decode_eip);
 				break;
 			}
 			switch (c->modrm_rm) {
@@ -703,7 +705,7 @@ done_prefixes:
 			switch (c->modrm_rm) {
 			case 4:
 			case 12:
-				sib = insn_fetch(u8, 1, c->eip);
+				sib = insn_fetch(u8, 1, c->decode_eip);
 				index_reg |= (sib >> 3) & 7;
 				base_reg |= sib & 7;
 				scale = sib >> 6;
@@ -714,8 +716,8 @@ done_prefixes:
 						c->modrm_ea +=
 							c->regs[base_reg];
 					else
-						c->modrm_ea +=
-						    insn_fetch(s32, 4, c->eip);
+						c->modrm_ea += insn_fetch(s32,
+							      4, c->decode_eip);
 					break;
 				default:
 					c->modrm_ea += c->regs[base_reg];
@@ -742,14 +744,15 @@ done_prefixes:
 			switch (c->modrm_mod) {
 			case 0:
 				if (c->modrm_rm == 5)
-					c->modrm_ea +=
-						insn_fetch(s32, 4, c->eip);
+					c->modrm_ea += insn_fetch(s32, 4,
+								 c->decode_eip);
 				break;
 			case 1:
-				c->modrm_ea += insn_fetch(s8, 1, c->eip);
+				c->modrm_ea += insn_fetch(s8, 1, c->decode_eip);
 				break;
 			case 2:
-				c->modrm_ea += insn_fetch(s32, 4, c->eip);
+				c->modrm_ea += insn_fetch(s32, 4,
+							  c->decode_eip);
 				break;
 			}
 		}
@@ -764,7 +767,7 @@ done_prefixes:
 			c->modrm_ea += *c->override_base;
 
 		if (rip_relative) {
-			c->modrm_ea += c->eip;
+			c->modrm_ea += c->decode_eip;
 			switch (c->d & SrcMask) {
 			case SrcImmByte:
 				c->modrm_ea += 1;
@@ -837,28 +840,28 @@ done_prefixes:
 		break;
 	case SrcImm:
 		c->src.type = OP_IMM;
-		c->src.ptr = (unsigned long *)c->eip;
+		c->src.ptr = (unsigned long *)c->decode_eip;
 		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
 		if (c->src.bytes == 8)
 			c->src.bytes = 4;
 		/* NB. Immediates are sign-extended as necessary. */
 		switch (c->src.bytes) {
 		case 1:
-			c->src.val = insn_fetch(s8, 1, c->eip);
+			c->src.val = insn_fetch(s8, 1, c->decode_eip);
 			break;
 		case 2:
-			c->src.val = insn_fetch(s16, 2, c->eip);
+			c->src.val = insn_fetch(s16, 2, c->decode_eip);
 			break;
 		case 4:
-			c->src.val = insn_fetch(s32, 4, c->eip);
+			c->src.val = insn_fetch(s32, 4, c->decode_eip);
 			break;
 		}
 		break;
 	case SrcImmByte:
 		c->src.type = OP_IMM;
-		c->src.ptr = (unsigned long *)c->eip;
+		c->src.ptr = (unsigned long *)c->decode_eip;
 		c->src.bytes = 1;
-		c->src.val = insn_fetch(s8, 1, c->eip);
+		c->src.val = insn_fetch(s8, 1, c->decode_eip);
 		break;
 	}
 
@@ -1146,10 +1149,20 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 {
 	unsigned long cr2 = ctxt->cr2;
 	u64 msr_data;
-	unsigned long saved_rcx = 0, saved_eip = 0;
 	struct decode_cache *c = &ctxt->decode;
 	int rc = 0;
 
+	/* Shadow copy of register state. Committed on successful emulation.
+	 * NOTE: we can copy them from vcpu as x86_decode_insn() doesn't
+	 * modify them.
+	 */
+
+	memcpy(c->regs, ctxt->vcpu->regs, sizeof c->regs);
+
+	/* restore eip after decoding phase */
+
+	c->eip = c->decode_eip;
+
 	if ((c->d & ModRM) && (c->modrm_mod != 3))
 		cr2 = c->modrm_ea;
 
@@ -1441,8 +1454,6 @@ special_insn:
 			ctxt->vcpu->rip = c->eip;
 			goto done;
 		}
-		saved_rcx = c->regs[VCPU_REGS_RCX];
-		saved_eip = c->eip;
 		c->regs[VCPU_REGS_RCX]--;
 		c->eip = ctxt->vcpu->rip;
 	}
@@ -1459,10 +1470,6 @@ special_insn:
 					c->regs[VCPU_REGS_RSI]),
 					&c->dst.val,
 					c->dst.bytes, ctxt->vcpu)) != 0) {
-			if (c->rep_prefix) {
-				c->regs[VCPU_REGS_RCX] = saved_rcx;
-				c->eip = saved_eip;
-			}
 			goto done;
 		}
 		register_address_increment(c->regs[VCPU_REGS_RSI],
@@ -1491,10 +1498,6 @@ special_insn:
 		if ((rc = ops->read_emulated(cr2, &c->dst.val,
 					     c->dst.bytes,
 					     ctxt->vcpu)) != 0) {
-			if (c->rep_prefix) {
-				c->regs[VCPU_REGS_RCX] = saved_rcx;
-				c->eip = saved_eip;
-			}
 			goto done;
 		}
 		register_address_increment(c->regs[VCPU_REGS_RSI],
diff --git a/drivers/kvm/x86_emulate.h b/drivers/kvm/x86_emulate.h
index f03b128..9fd7bb2 100644
--- a/drivers/kvm/x86_emulate.h
+++ b/drivers/kvm/x86_emulate.h
@@ -131,6 +131,7 @@ struct decode_cache {
 	unsigned long *override_base;
 	unsigned int d;
 	unsigned long regs[NR_VCPU_REGS];
+	unsigned long decode_eip;
 	unsigned long eip;
 	/* modrm */
 	u8 modrm;
-- 
1.5.2.4


-------------------------------------------------------------------------
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] Correct management of REP prefix

[Avi Kivity]

Laurent Vivier wrote:
> This patch corrects some errors appearing when we have an emulation failure
> on an operation using REP prefix.
>
> When x86_emulate_insn() fails, saving EIP and ECX is not enough as emulation
> should have modified other registers like RSI or RDI. Moreover, the emulation
> can fail on the writeback, and in this case we are not able to restore 
> registers.
>
> This patch takes another approach: at the beginning of x86_emulate_insn() we 
> restore state we have at end of x86_decode_insn(). To do that, we store EIP in
> a new field in decode_cache, decode_eip. This field store the EIP as it is at
> the end of x86_decode_insn(); and at beginning of x86_emulate_insn(), we restore
> all registers as they are in vcpu. We can do that, because the x86_decode_insn()
> doesn't modify registers (except EIP).
>   

How about doing it slightly differently: keep c->eip at its current 
meaning, and add c->eip_orig to revert to? That will make the patch 
smaller and reduce the changes of something being missed.


-- 
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/

Re: [PATCH] Correct management of REP prefix

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 1379 bytes --]

Avi Kivity wrote:
> Laurent Vivier wrote:
>> This patch corrects some errors appearing when we have an emulation
>> failure
>> on an operation using REP prefix.
>>
>> When x86_emulate_insn() fails, saving EIP and ECX is not enough as
>> emulation
>> should have modified other registers like RSI or RDI. Moreover, the
>> emulation
>> can fail on the writeback, and in this case we are not able to restore
>> registers.
>>
>> This patch takes another approach: at the beginning of
>> x86_emulate_insn() we restore state we have at end of
>> x86_decode_insn(). To do that, we store EIP in
>> a new field in decode_cache, decode_eip. This field store the EIP as
>> it is at
>> the end of x86_decode_insn(); and at beginning of x86_emulate_insn(),
>> we restore
>> all registers as they are in vcpu. We can do that, because the
>> x86_decode_insn()
>> doesn't modify registers (except EIP).
>>   
> 
> How about doing it slightly differently: keep c->eip at its current
> meaning, and add c->eip_orig to revert to? That will make the patch
> smaller and reduce the changes of something being missed.

I didn't do like that because I was afraid to miss some points to restore orig_eip.

But a patch will follow...

Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 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/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

[PATCH] Correct management of REP prefix

[Laurent Vivier]

This patch corrects some errors appearing when we have an emulation failure
on an operation using REP prefix.

When x86_emulate_insn() fails, saving EIP and ECX is not enough as emulation
should have modified other registers like RSI or RDI. Moreover, the emulation
can fail on the writeback, and in this case we are not able to restore
registers.

At beginning of x86_emulate_insn(), we restore registers from vcpu as they were
not modified by x86d_decode_insn() and we save EIP to be able to restore it
in case of failure.

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
---
 drivers/kvm/x86_emulate.c |   35 +++++++++++++++++++++--------------
 1 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 35069e3..887de7d 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -1146,10 +1146,18 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 {
 	unsigned long cr2 = ctxt->cr2;
 	u64 msr_data;
-	unsigned long saved_rcx = 0, saved_eip = 0;
+	unsigned long saved_eip;
 	struct decode_cache *c = &ctxt->decode;
 	int rc = 0;
 
+	/* Shadow copy of register state. Committed on successful emulation.
+	 * NOTE: we can copy them from vcpu as x86_decode_insn() doesn't
+	 * modify them.
+	 */
+
+	memcpy(c->regs, ctxt->vcpu->regs, sizeof c->regs);
+	saved_eip = c->eip;
+
 	if ((c->d & ModRM) && (c->modrm_mod != 3))
 		cr2 = c->modrm_ea;
 
@@ -1354,7 +1362,11 @@ writeback:
 	ctxt->vcpu->rip = c->eip;
 
 done:
-	return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
+	if (rc == X86EMUL_UNHANDLEABLE) {
+		c->eip = saved_eip;
+		return -1;
+	}
+	return 0;
 
 special_insn:
 	if (c->twobyte)
@@ -1396,8 +1408,10 @@ special_insn:
 				register_address(ctxt->es_base,
 						 c->regs[VCPU_REGS_RDI]),
 				c->rep_prefix,
-				c->regs[VCPU_REGS_RDX]) == 0)
+				c->regs[VCPU_REGS_RDX]) == 0) {
+			c->eip = saved_eip;
 			return -1;
+		}
 		return 0;
 	case 0x6e:		/* outsb */
 	case 0x6f:		/* outsw/outsd */
@@ -1412,8 +1426,10 @@ special_insn:
 							ctxt->ds_base,
 						 c->regs[VCPU_REGS_RSI]),
 				c->rep_prefix,
-				c->regs[VCPU_REGS_RDX]) == 0)
+				c->regs[VCPU_REGS_RDX]) == 0) {
+			c->eip = saved_eip;
 			return -1;
+		}
 		return 0;
 	case 0x70 ... 0x7f: /* jcc (short) */ {
 		int rel = insn_fetch(s8, 1, c->eip);
@@ -1441,8 +1457,6 @@ special_insn:
 			ctxt->vcpu->rip = c->eip;
 			goto done;
 		}
-		saved_rcx = c->regs[VCPU_REGS_RCX];
-		saved_eip = c->eip;
 		c->regs[VCPU_REGS_RCX]--;
 		c->eip = ctxt->vcpu->rip;
 	}
@@ -1459,10 +1473,6 @@ special_insn:
 					c->regs[VCPU_REGS_RSI]),
 					&c->dst.val,
 					c->dst.bytes, ctxt->vcpu)) != 0) {
-			if (c->rep_prefix) {
-				c->regs[VCPU_REGS_RCX] = saved_rcx;
-				c->eip = saved_eip;
-			}
 			goto done;
 		}
 		register_address_increment(c->regs[VCPU_REGS_RSI],
@@ -1491,10 +1501,6 @@ special_insn:
 		if ((rc = ops->read_emulated(cr2, &c->dst.val,
 					     c->dst.bytes,
 					     ctxt->vcpu)) != 0) {
-			if (c->rep_prefix) {
-				c->regs[VCPU_REGS_RCX] = saved_rcx;
-				c->eip = saved_eip;
-			}
 			goto done;
 		}
 		register_address_increment(c->regs[VCPU_REGS_RSI],
@@ -1762,5 +1768,6 @@ twobyte_special_insn:
 
 cannot_emulate:
 	DPRINTF("Cannot emulate %02x\n", c->b);
+	c->eip = saved_eip;
 	return -1;
 }
-- 
1.5.2.4


-------------------------------------------------------------------------
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] Correct management of REP prefix

[Avi Kivity]

Laurent Vivier wrote:
> This patch corrects some errors appearing when we have an emulation failure
> on an operation using REP prefix.
>
> When x86_emulate_insn() fails, saving EIP and ECX is not enough as emulation
> should have modified other registers like RSI or RDI. Moreover, the emulation
> can fail on the writeback, and in this case we are not able to restore
> registers.
>
> At beginning of x86_emulate_insn(), we restore registers from vcpu as they were
> not modified by x86d_decode_insn() and we save EIP to be able to restore it
> in case of failure.
>
>   


Applied, thanks.

-- 
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] Correct management of REP prefix

[Avi Kivity]

Laurent Vivier wrote:
> Avi Kivity wrote:
>   
>> Laurent Vivier wrote:
>>     
>>> This patch corrects some errors appearing when we have an emulation
>>> failure
>>> on an operation using REP prefix.
>>>
>>> When x86_emulate_insn() fails, saving EIP and ECX is not enough as
>>> emulation
>>> should have modified other registers like RSI or RDI. Moreover, the
>>> emulation
>>> can fail on the writeback, and in this case we are not able to restore
>>> registers.
>>>
>>> This patch takes another approach: at the beginning of
>>> x86_emulate_insn() we restore state we have at end of
>>> x86_decode_insn(). To do that, we store EIP in
>>> a new field in decode_cache, decode_eip. This field store the EIP as
>>> it is at
>>> the end of x86_decode_insn(); and at beginning of x86_emulate_insn(),
>>> we restore
>>> all registers as they are in vcpu. We can do that, because the
>>> x86_decode_insn()
>>> doesn't modify registers (except EIP).
>>>   
>>>       
>> How about doing it slightly differently: keep c->eip at its current
>> meaning, and add c->eip_orig to revert to? That will make the patch
>> smaller and reduce the changes of something being missed.
>>     
>
> I didn't do like that because I was afraid to miss some points to restore orig_eip.
>   


This could be done using something like

emulate(ctxt)
{
    decode_cache save = ctxt->decode_cache;

    r = __emulate(ctxt);
    if (r != success)
        ctxt->decode_cache = save;
    return r;
}

or in some similar way.

-- 
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] Correct management of REP prefix

[Kamble, Nitin A]

Hi Laurent,
This patch looks much cleaner to me. 

I see you are saving the regs like this in the patch.
 memcpy(c->regs, ctxt->vcpu->regs, sizeof c->regs);

But I don't see any place in the patch these regs getting restored after
failure.

Is it taken care of the code outside of the patch?


Thanks & Regards,
Nitin
Linux Open Source Technology Center, Intel Corporation
------------------------------------------------------------------------
--------
The Mind is like a parachute; it works much better when it's open.

-----Original Message-----
From: Laurent Vivier [mailto:Laurent.Vivier-6ktuUTfB/bM@public.gmane.org] 
Sent: Monday, October 01, 2007 2:01 AM
To: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Cc: Kamble, Nitin A; avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org; Laurent Vivier
Subject: [PATCH] Correct management of REP prefix

This patch corrects some errors appearing when we have an emulation
failure
on an operation using REP prefix.

When x86_emulate_insn() fails, saving EIP and ECX is not enough as
emulation
should have modified other registers like RSI or RDI. Moreover, the
emulation
can fail on the writeback, and in this case we are not able to restore
registers.

At beginning of x86_emulate_insn(), we restore registers from vcpu as
they were
not modified by x86d_decode_insn() and we save EIP to be able to restore
it
in case of failure.

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
---
 drivers/kvm/x86_emulate.c |   35 +++++++++++++++++++++--------------
 1 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 35069e3..887de7d 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -1146,10 +1146,18 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt,
struct x86_emulate_ops *ops)
 {
 	unsigned long cr2 = ctxt->cr2;
 	u64 msr_data;
-	unsigned long saved_rcx = 0, saved_eip = 0;
+	unsigned long saved_eip;
 	struct decode_cache *c = &ctxt->decode;
 	int rc = 0;
 
+	/* Shadow copy of register state. Committed on successful
emulation.
+	 * NOTE: we can copy them from vcpu as x86_decode_insn() doesn't
+	 * modify them.
+	 */
+
+	memcpy(c->regs, ctxt->vcpu->regs, sizeof c->regs);
+	saved_eip = c->eip;
+
 	if ((c->d & ModRM) && (c->modrm_mod != 3))
 		cr2 = c->modrm_ea;
 
@@ -1354,7 +1362,11 @@ writeback:
 	ctxt->vcpu->rip = c->eip;
 
 done:
-	return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
+	if (rc == X86EMUL_UNHANDLEABLE) {
+		c->eip = saved_eip;
+		return -1;
+	}
+	return 0;
 
 special_insn:
 	if (c->twobyte)
@@ -1396,8 +1408,10 @@ special_insn:
 				register_address(ctxt->es_base,
 
c->regs[VCPU_REGS_RDI]),
 				c->rep_prefix,
-				c->regs[VCPU_REGS_RDX]) == 0)
+				c->regs[VCPU_REGS_RDX]) == 0) {
+			c->eip = saved_eip;
 			return -1;
+		}
 		return 0;
 	case 0x6e:		/* outsb */
 	case 0x6f:		/* outsw/outsd */
@@ -1412,8 +1426,10 @@ special_insn:
 							ctxt->ds_base,
 
c->regs[VCPU_REGS_RSI]),
 				c->rep_prefix,
-				c->regs[VCPU_REGS_RDX]) == 0)
+				c->regs[VCPU_REGS_RDX]) == 0) {
+			c->eip = saved_eip;
 			return -1;
+		}
 		return 0;
 	case 0x70 ... 0x7f: /* jcc (short) */ {
 		int rel = insn_fetch(s8, 1, c->eip);
@@ -1441,8 +1457,6 @@ special_insn:
 			ctxt->vcpu->rip = c->eip;
 			goto done;
 		}
-		saved_rcx = c->regs[VCPU_REGS_RCX];
-		saved_eip = c->eip;
 		c->regs[VCPU_REGS_RCX]--;
 		c->eip = ctxt->vcpu->rip;
 	}
@@ -1459,10 +1473,6 @@ special_insn:
 					c->regs[VCPU_REGS_RSI]),
 					&c->dst.val,
 					c->dst.bytes, ctxt->vcpu)) != 0)
{
-			if (c->rep_prefix) {
-				c->regs[VCPU_REGS_RCX] = saved_rcx;
-				c->eip = saved_eip;
-			}
 			goto done;
 		}
 		register_address_increment(c->regs[VCPU_REGS_RSI],
@@ -1491,10 +1501,6 @@ special_insn:
 		if ((rc = ops->read_emulated(cr2, &c->dst.val,
 					     c->dst.bytes,
 					     ctxt->vcpu)) != 0) {
-			if (c->rep_prefix) {
-				c->regs[VCPU_REGS_RCX] = saved_rcx;
-				c->eip = saved_eip;
-			}
 			goto done;
 		}
 		register_address_increment(c->regs[VCPU_REGS_RSI],
@@ -1762,5 +1768,6 @@ twobyte_special_insn:
 
 cannot_emulate:
 	DPRINTF("Cannot emulate %02x\n", c->b);
+	c->eip = saved_eip;
 	return -1;
 }
-- 
1.5.2.4

-------------------------------------------------------------------------
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] Correct management of REP prefix

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 815 bytes --]

Kamble, Nitin A wrote:
> Hi Laurent,
> This patch looks much cleaner to me. 
> 
> I see you are saving the regs like this in the patch.
>  memcpy(c->regs, ctxt->vcpu->regs, sizeof c->regs);
> 
> But I don't see any place in the patch these regs getting restored after
> failure.
> 
> Is it taken care of the code outside of the patch?

In fact, during the emulation the function works on c->regs and copy them into
ctxt->vpcu->regs on success (see label "writeback"). The result is in vcpu not
in c->regs.
If the function fails, as we didn't modify ctxt->vcpu->regs, we can re-start
with original values by copying again ctxt->vcpu->regs to c->regs.

Regargs,
Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 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/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Correct management of REP prefix

[Kamble, Nitin A]

Hi Laurent,
   Fair enough. I think the patch can go in.


Thanks & Regards,
Nitin
Linux Open Source Technology Center, Intel Corporation
------------------------------------------------------------------------
--------
The Mind is like a parachute; it works much better when it's open.

-----Original Message-----
From: Laurent Vivier [mailto:Laurent.Vivier-6ktuUTfB/bM@public.gmane.org] 
Sent: Wednesday, October 03, 2007 12:24 AM
To: Kamble, Nitin A
Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org
Subject: Re: [PATCH] Correct management of REP prefix

Kamble, Nitin A wrote:
> Hi Laurent,
> This patch looks much cleaner to me. 
> 
> I see you are saving the regs like this in the patch.
>  memcpy(c->regs, ctxt->vcpu->regs, sizeof c->regs);
> 
> But I don't see any place in the patch these regs getting restored
after
> failure.
> 
> Is it taken care of the code outside of the patch?

In fact, during the emulation the function works on c->regs and copy
them into
ctxt->vpcu->regs on success (see label "writeback"). The result is in
vcpu not
in c->regs.
If the function fails, as we didn't modify ctxt->vcpu->regs, we can
re-start
with original values by copying again ctxt->vcpu->regs to c->regs.

Regargs,
Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/