From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCHv5 4/4] KVM: emulator: optimize "rep ins" handling. Date: Mon, 6 Aug 2012 14:49:53 +0300 Message-ID: <20120806114953.GZ27579@redhat.com> References: <1343659101-24877-1-git-send-email-gleb@redhat.com> <1343659101-24877-5-git-send-email-gleb@redhat.com> <501F854C.6070005@redhat.com> <20120806085846.GV27579@redhat.com> <501F8E25.5010107@redhat.com> <20120806110546.GY27579@redhat.com> <501FAD08.5020001@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, mtosatti@redhat.com To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:48961 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755998Ab2HFLtz (ORCPT ); Mon, 6 Aug 2012 07:49:55 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q76BntP2018018 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 6 Aug 2012 07:49:55 -0400 Content-Disposition: inline In-Reply-To: <501FAD08.5020001@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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.