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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Aug 06, 2012 at 03:08:28PM +0300, Avi Kivity wrote:
> On 08/06/2012 02:49 PM, Gleb Natapov wrote:
> > 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.
> 
> Ok.
> 
> If we linearize and translate pre-execution we can keep track of the
> remaining space available in the segment/page and do this correctly.
> 
Absolutely.

> >> >> > 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? 
> 
> Too many (all sse) instructions require alignment.
> 
But most do not. Although they do not work on big chunk of data either.

> > Why don't you like marking
> > instruction as Unaligned?
> 
> Because it's a workaround for a side effect of the implementation.  At a
> minimum it needs a comment.
> 
So add a comment and resend? Any other pre-requests before the series is
accepted?

> >> >> 
> >> > 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.
> 
> The correct thing is to check before, like the processor does.  For
Are you sure processor checks before? If you do "in" to an address that
lays outside of a segment PIO will not happen before a fault?

> example linearize also checks write permissions, so for RMW it needs to
> check writes before performing the first read.
> 
> Also cmovcc performs the checks even though it might not perform the access.
> 

--
			Gleb.