Re: [PATCH v10 2/9] x86emul: rework CMP and TEST emulation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/05/2020 15:26, Jan Beulich wrote:
> Unlike similarly encoded insns these don't write their memory operands,

"write to their".

> and hence x86_is_mem_write() should return false for them. However,
> rather than adding special logic there, rework how their emulation gets
> done, by making decoding attributes properly describe the r/o nature of
> their memory operands.

Describe how?  I see you've change the order of operands encoding, but
then override it back?

~Andrew

> Note how this also allows dropping custom LOCK prefix checks.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v10: New.
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -84,7 +84,7 @@ static const opcode_desc_t opcode_table[
>      ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM,
>      ByteOp|DstEax|SrcImm, DstEax|SrcImm, 0, ImplicitOps,
>      /* 0x38 - 0x3F */
> -    ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
> +    ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM,
>      ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM,
>      ByteOp|DstEax|SrcImm, DstEax|SrcImm, 0, ImplicitOps,
>      /* 0x40 - 0x4F */
> @@ -2481,7 +2481,6 @@ x86_decode_onebyte(
>      case 0x60: /* pusha */
>      case 0x61: /* popa */
>      case 0x62: /* bound */
> -    case 0x82: /* Grp1 (x86/32 only) */
>      case 0xc4: /* les */
>      case 0xc5: /* lds */
>      case 0xce: /* into */
> @@ -2491,6 +2490,14 @@ x86_decode_onebyte(
>          state->not_64bit = true;
>          break;
>  
> +    case 0x82: /* Grp1 (x86/32 only) */
> +        state->not_64bit = true;
> +        /* fall through */
> +    case 0x80: case 0x81: case 0x83: /* Grp1 */
> +        if ( (modrm_reg & 7) == 7 ) /* cmp */
> +            state->desc = (state->desc & ByteOp) | DstNone | SrcMem;
> +        break;
> +
>      case 0x90: /* nop / pause */
>          if ( repe_prefix() )
>              ctxt->opcode |= X86EMUL_OPC_F3(0, 0);
> @@ -2521,6 +2528,11 @@ x86_decode_onebyte(
>          imm2 = insn_fetch_type(uint8_t);
>          break;
>  
> +    case 0xf6: case 0xf7: /* Grp3 */
> +        if ( !(modrm_reg & 6) ) /* test */
> +            state->desc = (state->desc & ByteOp) | DstNone | SrcMem;
> +        break;
> +
>      case 0xff: /* Grp5 */
>          switch ( modrm_reg & 7 )
>          {
> @@ -3928,13 +3940,11 @@ x86_emulate(
>          break;
>  
>      case 0x38: case 0x39: cmp: /* cmp reg,mem */
> -        if ( ops->rmw && dst.type == OP_MEM &&
> -             (rc = read_ulong(dst.mem.seg, dst.mem.off, &dst.val,
> -                              dst.bytes, ctxt, ops)) != X86EMUL_OKAY )
> -            goto done;
> -        /* fall through */
> +        emulate_2op_SrcV("cmp", dst, src, _regs.eflags);
> +        dst.type = OP_NONE;
> +        break;
> +
>      case 0x3a ... 0x3d: /* cmp */
> -        generate_exception_if(lock_prefix, EXC_UD);
>          emulate_2op_SrcV("cmp", src, dst, _regs.eflags);
>          dst.type = OP_NONE;
>          break;
> @@ -4239,7 +4249,9 @@ x86_emulate(
>          case 4: goto and;
>          case 5: goto sub;
>          case 6: goto xor;
> -        case 7: goto cmp;
> +        case 7:
> +            dst.val = imm1;
> +            goto cmp;
>          }
>          break;
>  
> @@ -5233,11 +5245,8 @@ x86_emulate(
>              unsigned long u[2], v;
>  
>          case 0 ... 1: /* test */
> -            generate_exception_if(lock_prefix, EXC_UD);
> -            if ( ops->rmw && dst.type == OP_MEM &&
> -                 (rc = read_ulong(dst.mem.seg, dst.mem.off, &dst.val,
> -                                  dst.bytes, ctxt, ops)) != X86EMUL_OKAY )
> -                goto done;
> +            dst.val = imm1;
> +            dst.bytes = src.bytes;
>              goto test;
>          case 2: /* not */
>              if ( ops->rmw && dst.type == OP_MEM )
>