Re: [PATCHv5 4/4] KVM: emulator: optimize "rep ins" handling.

[Gleb Natapov <gleb@redhat.com>]

On Mon, Aug 06, 2012 at 11:50:20AM +0300, Avi Kivity wrote:
> On 07/30/2012 05:38 PM, Gleb Natapov wrote:
> > Optimize "rep ins" by allowing emulator to write back more than one
> > datum at a time. Introduce new operand type OP_MEM_STR which tells
> > writeback() that dst contains pointer to an array that should be written
> > back as opposite to just one data element.
> > 
> >  	}
> >  
> > -	memcpy(dest, rc->data + rc->pos, size);
> > -	rc->pos += size;
> > +	if (ctxt->rep_prefix && !(ctxt->eflags & EFLG_DF)) {
> > +		ctxt->dst.data = rc->data + rc->pos;
> > +		ctxt->dst.type = OP_MEM_STR;
> > +		ctxt->dst.count = (rc->end - rc->pos) / size;
> > +		rc->pos = rc->end;
> 
> Should take into account the segment limit.
> 
It does. During write back. pio_in_emulated() should linearize() address
before calculating page boundary, but this is (minor) bug unrelated to the patch
series.

> > +	} else {
> > +		memcpy(dest, rc->data + rc->pos, size);
> > +		rc->pos += size;
> > +	}
> >  	return 1;
> >  }
> >  
> > @@ -1500,6 +1507,14 @@ static int writeback(struct x86_emulate_ctxt *ctxt)
> >  		if (rc != X86EMUL_CONTINUE)
> >  			return rc;
> >  		break;
> > +	case OP_MEM_STR:
> > +		rc = segmented_write(ctxt,
> > +				ctxt->dst.addr.mem,
> > +				ctxt->dst.data,
> > +				ctxt->dst.bytes * ctxt->dst.count);
> > +		if (rc != X86EMUL_CONTINUE)
> > +			return rc;
> > +		break;
> >  	case OP_XMM:
> >  		write_sse_reg(ctxt, &ctxt->dst.vec_val, ctxt->dst.addr.xmm);
> >  		break;
> > @@ -2732,7 +2747,7 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
> >  static void string_addr_inc(struct x86_emulate_ctxt *ctxt, int reg,
> >  		struct operand *op)
> >  {
> > -	int df = (ctxt->eflags & EFLG_DF) ? -1 : 1;
> > +	int df = (ctxt->eflags & EFLG_DF) ? -op->count : op->count;
> >  
> >  	register_address_increment(ctxt, &ctxt->regs[reg], df * op->bytes);
> >  	op->addr.mem.ea = register_address(ctxt, ctxt->regs[reg]);
> > @@ -3672,7 +3687,7 @@ static struct opcode opcode_table[256] = {
> >  	I(DstReg | SrcMem | ModRM | Src2Imm, em_imul_3op),
> >  	I(SrcImmByte | Mov | Stack, em_push),
> >  	I(DstReg | SrcMem | ModRM | Src2ImmByte, em_imul_3op),
> > -	I2bvIP(DstDI | SrcDX | Mov | String, em_in, ins, check_perm_in), /* insb, insw/insd */
> > +	I2bvIP(DstDI | SrcDX | Mov | String | Unaligned, em_in, ins, check_perm_in), /* insb, insw/insd */
> 
> Eww.
This brings us back to the question what alignment check is doing in
linearize :)

> 
> >  	I2bvIP(SrcSI | DstDX | String, em_out, outs, check_perm_out), /* outsb, outsw/outsd */
> >  	/* 0x70 - 0x7F */
> >  	X16(D(SrcImmByte)),
> > @@ -3930,6 +3945,7 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
> >  			register_address(ctxt, ctxt->regs[VCPU_REGS_RDI]);
> >  		op->addr.mem.seg = VCPU_SREG_ES;
> >  		op->val = 0;
> > +		op->count = 1;
> >  		break;
> >  	case OpDX:
> >  		op->type = OP_REG;
> > @@ -3973,6 +3989,7 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
> >  			register_address(ctxt, ctxt->regs[VCPU_REGS_RSI]);
> >  		op->addr.mem.seg = seg_override(ctxt);
> >  		op->val = 0;
> > +		op->count = 1;
> >  		break;
> >  	case OpImmFAddr:
> >  		op->type = OP_IMM;
> > @@ -4513,8 +4530,14 @@ writeback:
> >  		string_addr_inc(ctxt, VCPU_REGS_RDI, &ctxt->dst);
> >  
> >  	if (ctxt->rep_prefix && (ctxt->d & String)) {
> > +		unsigned int count;
> >  		struct read_cache *r = &ctxt->io_read;
> > -		register_address_increment(ctxt, &ctxt->regs[VCPU_REGS_RCX], -1);
> > +		if ((ctxt->d & SrcMask) == SrcSI)
> > +			count = ctxt->src.count;
> > +		else
> > +			count = ctxt->dst.count;
> 
> Does this work correctly for 'rep movs' and friends?
> 
(src|dst).count is initialized to 1 during decode, so anything that does
not touch "count" behaves exactly like before.

--
			Gleb.