Re: [PATCH 4/4] KVM: x86 emulator: Avoid pushing back ModRM byte in decode_opcode()

[Avi Kivity <avi@redhat.com>]

On 04/23/2012 06:37 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
>
> Although ModRM byte is read for group decoding, it is soon pushed back
> to make decode_modrm() fetch it later.
>
> We should consistently read it, only once, in decode_opcode() instead.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Cc: Takuya Yoshikawa <takuya.yoshikawa@gmail.com>
> ---
>  arch/x86/kvm/emulate.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index e87570e..8729773 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -972,7 +972,6 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
>  		ctxt->modrm_rm = base_reg = (ctxt->rex_prefix & 1) << 3; /* REG.B */
>  	}
>  
> -	ctxt->modrm = insn_fetch(u8, ctxt);
>  	ctxt->modrm_mod |= (ctxt->modrm & 0xc0) >> 6;
>  	ctxt->modrm_reg |= (ctxt->modrm & 0x38) >> 3;
>  	ctxt->modrm_rm |= (ctxt->modrm & 0x07);
> @@ -3969,6 +3968,7 @@ done:
>   * @ctxt: emulation context
>   *
>   * Decodes opcode bytes and reads opcode from table.
> + * The ModRM byte, if exists, is also fetched into ctxt->modrm.
>   *
>   * Returns X86EMUL_CONTINUE on success.
>   */
> @@ -3977,6 +3977,8 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
>  	int rc = X86EMUL_CONTINUE;
>  	int goffset, simd_prefix;
>  	struct opcode opcode;
> +	bool modrm_fetched = false;
> +	u64 gtype;
>  
>  	/* Two-byte opcode? */
>  	if (ctxt->b == 0x0f) {
> @@ -3988,17 +3990,18 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
>  
>  	ctxt->d = opcode.flags;
>  
> -	while (ctxt->d & GroupMask) {
> -		switch (ctxt->d & GroupMask) {
> -		case Group:
> +	while ((gtype = ctxt->d & GroupMask)) {
> +		if (!modrm_fetched && gtype != Prefix) {
>  			ctxt->modrm = insn_fetch(u8, ctxt);
> -			--ctxt->_eip;
> +			modrm_fetched = true;
> +		}
> +
> +		switch (gtype) {
> +		case Group:
>  			goffset = (ctxt->modrm >> 3) & 7;
>  			opcode = opcode.u.group[goffset];
>  			break;
>  		case GroupDual:
> -			ctxt->modrm = insn_fetch(u8, ctxt);
> -			--ctxt->_eip;
>  			goffset = (ctxt->modrm >> 3) & 7;
>  			if ((ctxt->modrm >> 6) == 3)
>  				opcode = opcode.u.gdual->mod3[goffset];
> @@ -4039,6 +4042,8 @@ static int decode_opcode(struct x86_emulate_ctxt *ctxt)
>  	ctxt->check_perm = opcode.check_perm;
>  	ctxt->intercept = opcode.intercept;
>  
> +	if (!modrm_fetched && (ctxt->d & ModRM))
> +		ctxt->modrm = insn_fetch(u8, ctxt);

Instead of adding yet another conditional, how about doing something like

   if ((c->d & ModRM) || (gtype == Group) || (gtype == GroupDual))
          ctxt->modrm = insn_fetch(u8, ctxt);

somewhere early?

In fact even that is too much.  All groups have ModRM somewhere in their
encoding; all we have to do is move it to the main tables (opcode_table
or twobyte_table) and just move the existing modrm fetch before group
parsing.

-- 
error compiling committee.c: too many arguments to function