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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Aug 06, 2012 at 02:39:52PM +0300, Avi Kivity wrote:
> On 08/06/2012 02:05 PM, Gleb Natapov wrote:
> > On Mon, Aug 06, 2012 at 12:28:05PM +0300, Avi Kivity wrote:
> >> 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.
> >> 
> > Correct. And this can happen with or without the patch series.
> 
> No, it can't.  Ordinarily ins will trap inside the guest.
> 
We do not go to a guest for each iteration. In fact we will not go to a
guest for exactly "count" iterations.

> > 
> >> 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...
> >> 
> > It either check it in a wrong place or we need to mark all instructions
> > that do not care about alignment, so the patch is not "Eww" :)
> 
> If not there, where?
> 
During execution if instruction requires alignment? Why don't you like marking
instruction as Unaligned?
 
> 16-byte sse instructions, cmpxchg16b, fxsave/fxrstor all check for 16
> byte alignment.  There is also the #AC exception.  I couldn't find in
> the SDM whether linear or virtual addresses are checked, but I'm
> guessing linear.
> 
> Another way to work around this is to pass size/count separately.
> 
> > 
> >> 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.
> >> 
> > Execution state likely. String instruction works on segmented address
> > for instance (address increment/decrement). May be there are others.
> 
> Practically everything works on segmented addresses.
> 
Hmm, true. We can calculate liner address whenever it is needed and
cache it. If address changes cache is invalidated.

--
			Gleb.