Re: [Xen-devel] [PATCH 4/4] x86emul: use DstEax also for XCHG

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/08/16 09:35, Jan Beulich wrote:
> Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
> possible"): While it avoids just a few instructions, we should
> nevertheless make use of generic code as much as possible. Here we can
> arrange for that by simply swapping source and destination (as they're
> interchangeable).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -118,8 +118,10 @@ static uint8_t opcode_table[256] = {
>      DstMem|SrcReg|ModRM|Mov, DstReg|SrcNone|ModRM,
>      DstReg|SrcMem16|ModRM|Mov, DstMem|SrcNone|ModRM|Mov,
>      /* 0x90 - 0x97 */
> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,

Please add a comment explaining that DstEax is interchangeable with
SrcEax in the xchg case.  Otherwise, the decode table reads incorrectly.

>      /* 0x98 - 0x9F */
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
> @@ -2491,16 +2493,14 @@ x86_emulate(
>  
>      case 0x90: /* nop / xchg %%r8,%%rax */
>          if ( !(rex_prefix & 1) )
> -            break; /* nop */
> +            goto no_writeback; /* nop */

Could you add an explicit /* fallthrough */ here?  The only reason it
isn't currently a coverity defect is because of the /* nop */ comment.

With these two, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

>  
>      case 0x91 ... 0x97: /* xchg reg,%%rax */
> -        src.type = dst.type = OP_REG;
> -        src.bytes = dst.bytes = op_bytes;
> -        src.reg  = (unsigned long *)&_regs.eax;
> -        src.val  = *src.reg;
> -        dst.reg  = decode_register(
> +        src.type = OP_REG;
> +        src.bytes = op_bytes;
> +        src.reg  = decode_register(
>              (b & 7) | ((rex_prefix & 1) << 3), &_regs, 0);
> -        dst.val  = *dst.reg;
> +        src.val  = *src.reg;
>          goto xchg;
>  
>      case 0x98: /* cbw/cwde/cdqe */
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel