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 11:58:46 +0300 Message-ID: <20120806085846.GV27579@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> 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]:35752 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752897Ab2HFI6s (ORCPT ); Mon, 6 Aug 2012 04:58:48 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q768wlh6005685 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 6 Aug 2012 04:58:48 -0400 Content-Disposition: inline In-Reply-To: <501F854C.6070005@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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.