Re: [PATCH v3 21/25] x86emul: add read-modify-write hook

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/12/17 14:16, Jan Beulich wrote:
> In order to correctly emulate read-modify-write insns, especially
> LOCKed ones, we should not issue reads and writes separately. Use a
> new hook to combine both, and don't uniformly read the memory
> destination anymore. Instead, DstMem opcodes without Mov now need to
> have done so in their respective case blocks.
>
> Also strip bogus _ prefixes from macro parameters when this only affects
> lines which are being changed anyway.
>
> In the test harness, besides some re-ordering to facilitate running a
> few tests twice (one without and a second time with the .rmw hook in
> place), tighten a few EFLAGS checks and add a test for NOT with memory
> operand (in particular to verify EFLAGS don't get altered there).
>
> For now make use of the hook optional for callers; eventually we may
> want to consider making this mandatory.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3: New.
> ---
> TBD: Do we want to also support non-lockable RMW insns in the new hook
>      and helper (SHL & friends, SHLD, SHRD)?

What would this achieve?  I suppose it would avoid a double pagewalk.

>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3356,35 +3388,83 @@ x86_emulate(
>          unsigned int i, n;
>          unsigned long dummy;
>  
> -    case 0x00 ... 0x05: add: /* add */
> -        emulate_2op_SrcV("add", src, dst, _regs.eflags);
> +    case 0x00: case 0x01: add: /* add reg,mem */
> +        if ( ops->rmw && dst.type == OP_MEM )
> +            state->rmw = rmw_add;
> +        else
> +        {
> +    case 0x02 ... 0x05: /* add */

I think it would help to identify reg,reg specifically in these comments.

> +            emulate_2op_SrcV("add", src, dst, _regs.eflags);
> +        }
>          break;
>  
> -    case 0x08 ... 0x0d: or:  /* or */
> -        emulate_2op_SrcV("or", src, dst, _regs.eflags);
> +    case 0x08: case 0x09: or: /* or reg,mem */
> +        if ( ops->rmw && dst.type == OP_MEM )
> +            state->rmw = rmw_or;
> +        else
> +        {
> +    case 0x0a ... 0x0d: /* or */
> +            emulate_2op_SrcV("or", src, dst, _regs.eflags);
> +        }
>          break;
>  
> -    case 0x10 ... 0x15: adc: /* adc */
> -        emulate_2op_SrcV("adc", src, dst, _regs.eflags);
> +    case 0x10: case 0x11: adc: /* adc reg,mem */
> +        if ( ops->rmw && dst.type == OP_MEM )
> +            state->rmw = rmw_adc;
> +        else
> +        {
> +    case 0x12 ... 0x15: /* adc */
> +            emulate_2op_SrcV("adc", src, dst, _regs.eflags);
> +        }
>          break;
>  
> -    case 0x18 ... 0x1d: sbb: /* sbb */
> -        emulate_2op_SrcV("sbb", src, dst, _regs.eflags);
> +    case 0x18: case 0x19: sbb: /* sbb reg,mem */
> +        if ( ops->rmw && dst.type == OP_MEM )
> +            state->rmw = rmw_sbb;
> +        else
> +        {
> +    case 0x1a ... 0x1d: /* sbb */
> +            emulate_2op_SrcV("sbb", src, dst, _regs.eflags);
> +        }
>          break;
>  
> -    case 0x20 ... 0x25: and: /* and */
> -        emulate_2op_SrcV("and", src, dst, _regs.eflags);
> +    case 0x20: case 0x21: and: /* and reg,mem */
> +        if ( ops->rmw && dst.type == OP_MEM )
> +            state->rmw = rmw_and;
> +        else
> +        {
> +    case 0x22 ... 0x25: /* and */
> +            emulate_2op_SrcV("and", src, dst, _regs.eflags);
> +        }
>          break;
>  
> -    case 0x28 ... 0x2d: sub: /* sub */
> -        emulate_2op_SrcV("sub", src, dst, _regs.eflags);
> +    case 0x28: case 0x29: sub: /* sub reg,mem */
> +        if ( ops->rmw && dst.type == OP_MEM )
> +            state->rmw = rmw_sub;
> +        else
> +        {
> +    case 0x2a ... 0x2d: /* sub */
> +            emulate_2op_SrcV("sub", src, dst, _regs.eflags);
> +        }
>          break;
>  
> -    case 0x30 ... 0x35: xor: /* xor */
> -        emulate_2op_SrcV("xor", src, dst, _regs.eflags);
> +    case 0x30: case 0x31: xor: /* xor reg,mem */
> +        if ( ops->rmw && dst.type == OP_MEM )
> +            state->rmw = rmw_xor;
> +        else
> +        {
> +    case 0x32 ... 0x35: /* xor */
> +            emulate_2op_SrcV("xor", src, dst, _regs.eflags);
> +        }
>          break;
>  
> -    case 0x38 ... 0x3d: cmp: /* cmp */
> +    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 )

Why does rmw matter here? cmp doesn't write to its operands.

> +            goto done;
> +        /* fall through */
> +    case 0x3a ... 0x3d: /* cmp */
>          generate_exception_if(lock_prefix, EXC_UD);
>          emulate_2op_SrcV("cmp", src, dst, _regs.eflags);
>          dst.type = OP_NONE;
> @@ -3700,6 +3780,13 @@ x86_emulate(
>          break;
>  
>      case 0x86 ... 0x87: xchg: /* xchg */
> +        /* The lock prefix is implied for this insn. */
> +        lock_prefix = 1;

Only for memory.  I.e. this should probably be inside an OP_MEM check.

~Andrew

> +        if ( ops->rmw && dst.type == OP_MEM )
> +        {
> +            state->rmw = rmw_xchg;
> +            break;
> +        }
>          /* Write back the register source. */
>          switch ( dst.bytes )
>          {


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel