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

[Avi Kivity <avi@redhat.com>]

On 08/06/2012 11:58 AM, Gleb Natapov wrote:
> 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.

I see, yes, this problem preexists.

However, in normal conditions, non-repeating instructions will not reach
the emulator at all since they will fault in the guest (or in the shadow
mmu, which will reflect the fault to the guest).  Here, the first
iteration may fit in the segment but the second will not, so this will fail.

It's not a huge problem since no guest does this.

>> > @@ -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 :)

It's checking alignment...

Let's see how we would fix this mess.  We need to move linearization
(and virt->phys translation) to the decode stage, or perhaps the
execution state, but before instruction dispatch.  This would cause all
the various exceptions to be checked against before execution, and would
avoid double translation for RMW operands.


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

Ok.


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