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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.16 at 14:46, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 16/08/16 12:31, Jan Beulich wrote:
>>>>> On 16.08.16 at 12:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> 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.
>> Do you mean me to do so even considering there's no SrcEax
>> (yet, it'll come with the not yet posted patch finally doing the
>> split off of the decode part)? (Nor can I see why the decode
>> table reads incorrectly the way it is above.)
> 
> xchg is explicitly specified to have SrcEax, so people comparing the
> instruction manuals to our implementation can be forgiven for thinking
> that our code is wrong if it has DstEax instead.
> 
> If SrcEax is imminent then it perhaps it doesn't matter too much.

Otoh I could obviously re-order this with the other one and
use SrcEax here, making for a slightly smaller overall change.
Or introduce SrcEax right here. Maybe that's the most natural
route to go.

Jan


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