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

[Gleb Natapov <gleb@redhat.com>]

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.

>  
>  	ctxt->decode.mem_read.pos = 0;
>  
> @@ -2771,28 +2772,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) {
> @@ -3229,6 +3212,7 @@ special_insn:
>  	}
>  
>  writeback:
> +	old_eip = c->eip;
>  	rc = writeback(ctxt, ops);
>  	if (rc != X86EMUL_CONTINUE)
>  		goto done;
> @@ -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?

> +			}
> +		}
>  		/*
>  		 * 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.


> +		}
>  	}
>  	/*
>  	 * reset read cache here in case string instruction is restared
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.