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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.02.18 at 17:13, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxx>
>> ---
>> 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.

Well - it's mostly the implication from this double walk which I'm
concerned about: The first walk is one for a read, which might
succeed when the second (write) walk fails. We'd then have done
a read which should have never occurred. But anyway this would
be follow-up work only, nothing to be added to the patch here.

>> -    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.

The read of the "destination" operand was skipped in case there
is a ->rmw hook (see the change to generic destination operand
processing ahead of the main switch()). This needs to be carried
out here now (and elsewhere when what is nominally the
destination operand really is a second source one).

>> @@ -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.

Consuming code (further down) ignores this for the register only
case. Only the body of "if ( state->rmw )" and a "case OP_MEM:"
look at this flag, and ...

>> +        if ( ops->rmw && dst.type == OP_MEM )
>> +        {
>> +            state->rmw = rmw_xchg;
>> +            break;
>> +        }

... state->rmw is set here with exactly the condition around it
that you ask for. I would certainly agree if it was the decode stage
where the flag is set, but we're in the execution phase already.

Jan


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