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

[Gleb Natapov <gleb@redhat.com>]

On Tue, Aug 17, 2010 at 12:20:34PM +0300, Avi Kivity wrote:
>  On 08/17/2010 12:13 PM, Gleb Natapov wrote:
> >On Tue, Aug 17, 2010 at 11:26:43AM +0300, Avi Kivity wrote:
> >>EFLAGS.ZF needs to be checked after each iteration, not before.
> >>
> >>Signed-off-by: Avi Kivity<avi@redhat.com>
> >>---
> >>  arch/x86/kvm/emulate.c |   42 +++++++++++++++++++++++-------------------
> >>  1 files changed, 23 insertions(+), 19 deletions(-)
> >>
> >>diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >>index 0c0ada9..a2edfb1 100644
> >>--- a/arch/x86/kvm/emulate.c
> >>+++ b/arch/x86/kvm/emulate.c
> >>@@ -2747,6 +2747,7 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
> >>  	int rc = X86EMUL_CONTINUE;
> >>  	int saved_dst_type = c->dst.type;
> >>  	int irq; /* Used for int 3, int, and into */
> >>+	ulong old_eip;
> >Is never used.
> >
> 
> Whoops.
> 
> >>@@ -3250,13 +3234,33 @@ 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)) {
> >>+			trace_printk("c->eip %lx ctxt->eip %lx\n",
> >>+				     c->eip, ctxt->eip);
> >>+			if (((c->rep_prefix == REPE_PREFIX)&&
> >>+			     ((ctxt->eflags&  EFLG_ZF) == 0))
> >>+			    || ((c->rep_prefix == REPNE_PREFIX)&&
> >>+				((ctxt->eflags&  EFLG_ZF) == EFLG_ZF))) {
> >>+				ctxt->restart = false;
> >Why not jump to string_done label here?
> 
> It does a 'goto done;' which skips a couple of things.
> 
The only thing it skips is:
ctxt->decode.mem_read.end = 0;
as far as I can see. And this is ok if instruction is completed.

> >>+			}
> >>+		}
> >>  		/*
> >>  		 * 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))
> >>+		    (rc->end != 0&&  rc->end == rc->pos)) {
> >>  			ctxt->restart = false;
> >>+			c->eip = ctxt->eip;
> >We can get here when instruction is completed by above "if", so same
> >instruction will reexecute once again.
> 
> Not good.  Will redo (and write tests).
> 
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.