Re: [Xen-devel] [PATCH v5 08/14] x86emul: add read-modify-write hook

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/03/18 14:46, Jan Beulich wrote:
>>>> On 15.03.18 at 15:19, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 15/03/18 13:08, Jan Beulich wrote:
>>> @@ -8517,6 +8690,142 @@ x86_emulate(
>>>  #undef vex
>>>  #undef ea
>>>  
>>> +int x86_emul_rmw(
>>> +    void *ptr,
>>> +    unsigned int bytes,
>>> +    uint32_t *eflags,
>>> +    struct x86_emulate_state *state,
>>> +    struct x86_emulate_ctxt *ctxt)
>>> +{
>>> +    unsigned long *dst = ptr;
>>> +
>>> +    ASSERT(bytes == state->op_bytes);
>>> +
>>> +#ifdef __x86_64__
>>> +# define JCXZ "jrcxz"
>>> +#else
>>> +# define JCXZ "jecxz"
>>> +#endif
>>> +
>>> +#define COND_LOCK(op) \
>>> +    JCXZ " .L" #op "%=\n\t" \
>>> +    "lock\n" \
>>> +    ".L" #op "%=:\n\t" \
>>> +    #op
>> I'd forgotten that these encoding of jmp existed, but various ORMs
>> suggest that it is far slower to execute than other jumps.
>>
>> Irrespective of the instruction latency argument, you'll get better code
>> generation with cond_op() looking rather more like:
>>
>> cmpb $0, %[lock]
>> je 1f
>> lock
>> 1: #op
>>
>> which will most likely cause the cmp to be encoded with a memory
>> operand, rather than forcing it the value into %ecx.
> You're missing the main point of having chosen J{E,R}CXZ (arguably
> I should have added a comment, and I now will): This operation sits
> inside the range where we have loaded guest EFLAGS (the status
> ones) into the hardware EFLAGS register. For ADC, SBB, RCL, and
> RCR to function we must not alter CF here, and for flags not
> modified by an insn (e.g. everything other than CF for BT*) we
> must not alter any of the other status flags.

Ah ok - very good justification.  Subject to a suitable code comment, my
ack still stands.

~Andrew

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