From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH 7/8] KVM: x86 emulator: simplify instruction decode
 flags for opcodes E0-FF
Date: Mon, 23 Aug 2010 16:56:09 +0300
Message-ID: <20100823135609.GR10499@redhat.com>
References: <1282561577-24491-1-git-send-email-avi@redhat.com>
 <1282561577-24491-8-git-send-email-avi@redhat.com>
 <20100823133303.GQ10499@redhat.com>
 <4C727B99.40408@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:1026 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751993Ab0HWN4L (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 23 Aug 2010 09:56:11 -0400
Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o7NDuAIW015039
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <kvm@vger.kernel.org>; Mon, 23 Aug 2010 09:56:11 -0400
Content-Disposition: inline
In-Reply-To: <4C727B99.40408@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Mon, Aug 23, 2010 at 04:46:01PM +0300, Avi Kivity wrote:
>  On 08/23/2010 04:33 PM, Gleb Natapov wrote:
> >On Mon, Aug 23, 2010 at 02:06:16PM +0300, Avi Kivity wrote:
> >>Use the new byte/word dual opcode decode.
> >>
> >>Signed-off-by: Avi Kivity<avi@redhat.com>
> >>---
> >>  arch/x86/kvm/emulate.c |    6 ++----
> >>  1 files changed, 2 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >>index c61f73d..6cb5663 100644
> >>--- a/arch/x86/kvm/emulate.c
> >>+++ b/arch/x86/kvm/emulate.c
> >>@@ -2464,13 +2464,11 @@ static struct opcode opcode_table[256] = {
> >>  	N, N, N, N, N, N, N, N,
> >>  	/* 0xE0 - 0xE7 */
> >>  	X3(D(SrcImmByte)), N,
> >>-	D(ByteOp | SrcImmUByte | DstAcc), D(SrcImmUByte | DstAcc),
> >>-	D(ByteOp | SrcAcc | DstImmUByte), D(SrcAcc | DstImmUByte),
> >>+	D2bv(SrcImmUByte | DstAcc), D2bv(SrcAcc | DstImmUByte),
> >>  	/* 0xE8 - 0xEF */
> >>  	D(SrcImm | Stack), D(SrcImm | ImplicitOps),
> >>  	D(SrcImmFAddr | No64), D(SrcImmByte | ImplicitOps),
> >>-	D(SrcNone | ByteOp | DstAcc), D(SrcNone | DstAcc),
> >>-	D(ByteOp | SrcAcc | ImplicitOps), D(SrcAcc | ImplicitOps),
> >>+	D2bv(SrcNone | DstAcc),	D2bv(SrcAcc | ImplicitOps),
> >>  	/* 0xF0 - 0xF7 */
> >>  	N, N, N, N,
> >>  	D(ImplicitOps | Priv), D(ImplicitOps), G(ByteOp, group3), G(0, group3),
> >Hmm. I actually have a patch that makes encoding of those to be different.
> >
> >===
> >     Some instructions has 32 bit operand even in long mode.
> >
> >     Decode this through decode table instead of during instruction
> >     emulation.
> >
> 
> This can still work.
> 
> >@@ -2750,6 +2751,9 @@ done_prefixes:
> >  			c->op_bytes = 4;
> >  	}
> >
> >+	if (mode == X86EMUL_MODE_PROT64&&  (c->d&  Op32in64))
> >+		c->op_bytes = 4;
> 
> Just check that it isn't a ByteOp.
> 
Hmm. It seams that even that is not needed. We have
 c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
everywhere.

> btw, shouldn't that be
> 
>     if (c->op_bytes == 8 && (c->d & Op32in64))
> 
> ?  to account for c->op_bytes = 2 initially.
> 
> 
Initially it will be 4 in X86EMUL_MODE_PROT64, but it may be overridden
to 2, so yes we should check c->op_bytes == 8 not X86EMUL_MODE_PROT64.

--
			Gleb.