Re: [PATCH v3] KVM: x86 emulator: fix REPZ/REPNZ termination condition

2010-08-19 Thread Gleb Natapov 
On Thu, Aug 19, 2010 at 02:34:08PM +0300, Avi Kivity wrote:
 EFLAGS.ZF needs to be checked after each iteration, not before.
 
 Signed-off-by: Avi Kivity a...@redhat.com
 ---
 
 v3: if restarting an instruction, don't advance rip
 
  arch/x86/kvm/emulate.c |   41 -
  1 files changed, 20 insertions(+), 21 deletions(-)
 
 diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
 index 729853a..4372dc5 100644
 --- a/arch/x86/kvm/emulate.c
 +++ b/arch/x86/kvm/emulate.c
 @@ -2782,28 +2782,10 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
   ctxt-restart = true;
   /* All REP prefixes have the same first termination condition */
   if (address_mask(c, c-regs[VCPU_REGS_RCX]) == 0) {
 - string_done:
   ctxt-restart = false;
   ctxt-eip = 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))
 - goto string_done;
 - if ((c-rep_prefix == REPNE_PREFIX) 
 - ((ctxt-eflags  EFLG_ZF) == EFLG_ZF))
 - goto string_done;
 - }
 - c-eip = ctxt-eip;
   }
  
   if ((c-src.type == OP_MEM)  !(c-d  NoAccess)) {
 @@ -3230,20 +3212,37 @@ writeback:
   if (c-rep_prefix  (c-d  String)) {
   struct read_cache *rc = ctxt-decode.io_read;
   register_address_increment(c, c-regs[VCPU_REGS_RCX], -1);
 + /* 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))
 +  (((c-rep_prefix == REPE_PREFIX) 
 +  ((ctxt-eflags  EFLG_ZF) == 0))
 + || ((c-rep_prefix == REPNE_PREFIX) 
 + ((ctxt-eflags  EFLG_ZF) == EFLG_ZF
 + ctxt-restart = false;
   /*
* Re-enter guest when pio read ahead buffer is empty or,
* if it is not used, after each 1024 iteration.
*/
 - if ((rc-end == 0  !(c-regs[VCPU_REGS_RCX]  0x3ff)) ||
 - (rc-end != 0  rc-end == rc-pos))
 + else if ((rc-end == 0  !(c-regs[VCPU_REGS_RCX]  0x3ff)) ||
 +  (rc-end != 0  rc-end == rc-pos)) {
   ctxt-restart = false;
 + c-eip = ctxt-eip;
 + }
   }
   /*
* reset read cache here in case string instruction is restared
* without decoding
*/
   ctxt-decode.mem_read.end = 0;
 - ctxt-eip = c-eip;
 + if (!ctxt-restart)
 + ctxt-eip = c-eip;
  
We will advance rip past instruction if exception is generated during
instruction emulation. Consequently exception will be injected at the
wrong rip. I think we should try different approach to fix this problem.

  done:
   return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
 -- 
 1.7.1

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] KVM: x86 emulator: fix REPZ/REPNZ termination condition

2010-08-19 Thread Avi Kivity 

 On 08/19/2010 03:09 PM, Gleb Natapov wrote:

On Thu, Aug 19, 2010 at 02:34:08PM +0300, Avi Kivity wrote:

EFLAGS.ZF needs to be checked after each iteration, not before.

Signed-off-by: Avi Kivitya...@redhat.com
---

v3: if restarting an instruction, don't advance rip

  arch/x86/kvm/emulate.c |   41 -
  1 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 729853a..4372dc5 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2782,28 +2782,10 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
ctxt-restart = true;
/* All REP prefixes have the same first termination condition */
if (address_mask(c, c-regs[VCPU_REGS_RCX]) == 0) {
-   string_done:
ctxt-restart = false;
ctxt-eip = 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))
-   goto string_done;
-   if ((c-rep_prefix == REPNE_PREFIX)
-   ((ctxt-eflags  EFLG_ZF) == EFLG_ZF))
-   goto string_done;
-   }
-   c-eip = ctxt-eip;
}

if ((c-src.type == OP_MEM)  !(c-d  NoAccess)) {
@@ -3230,20 +3212,37 @@ writeback:
if (c-rep_prefix  (c-d  String)) {
struct read_cache *rc =ctxt-decode.io_read;
register_address_increment(c,c-regs[VCPU_REGS_RCX], -1);
+   /* 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))
+ (((c-rep_prefix == REPE_PREFIX)
+((ctxt-eflags  EFLG_ZF) == 0))
+   || ((c-rep_prefix == REPNE_PREFIX)
+   ((ctxt-eflags  EFLG_ZF) == EFLG_ZF
+   ctxt-restart = false;
/*
 * Re-enter guest when pio read ahead buffer is empty or,
 * if it is not used, after each 1024 iteration.
 */
-   if ((rc-end == 0  !(c-regs[VCPU_REGS_RCX]  0x3ff)) ||
-   (rc-end != 0  rc-end == rc-pos))
+   else if ((rc-end == 0  !(c-regs[VCPU_REGS_RCX]  0x3ff)) ||
+(rc-end != 0  rc-end == rc-pos)) {
ctxt-restart = false;
+   c-eip = ctxt-eip;
+   }
}
/*
 * reset read cache here in case string instruction is restared
 * without decoding
 */
ctxt-decode.mem_read.end = 0;
-   ctxt-eip = c-eip;
+   if (!ctxt-restart)
+   ctxt-eip = c-eip;


We will advance rip past instruction if exception is generated during
instruction emulation. Consequently exception will be injected at the
wrong rip. I think we should try different approach to fix this problem.


Yes, I agree.  It's too confusing.

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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html