On 04/23/2012 06:37 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <[email protected]>
>
> 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 <[email protected]>
> Cc: Takuya Yoshikawa <[email protected]>
> ---
>  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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to