diff mbox

[4/4] x86emul: use DstEax also for XCHG

Message ID 57B19B080200007800105CF0@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich Aug. 15, 2016, 8:35 a.m. UTC 
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@suse.com>
x86emul: use DstEax also for XCHG

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

--- 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,
     /* 0x98 - 0x9F */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
@@ -2491,16 +2493,14 @@ x86_emulate(
 
     case 0x90: /* nop / xchg %%r8,%%rax */
         if ( !(rex_prefix & 1) )
-            break; /* nop */
+            goto no_writeback; /* nop */
 
     case 0x91 ... 0x97: /* xchg reg,%%rax */
-        src.type = dst.type = OP_REG;
-        src.bytes = dst.bytes = op_bytes;
-        src.reg  = (unsigned long *)&_regs.eax;
-        src.val  = *src.reg;
-        dst.reg  = decode_register(
+        src.type = OP_REG;
+        src.bytes = op_bytes;
+        src.reg  = decode_register(
             (b & 7) | ((rex_prefix & 1) << 3), &_regs, 0);
-        dst.val  = *dst.reg;
+        src.val  = *src.reg;
         goto xchg;
 
     case 0x98: /* cbw/cwde/cdqe */

Comments

Andrew Cooper Aug. 16, 2016, 10:59 a.m. UTC | #1 
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@suse.com>
>
> --- 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.

>      /* 0x98 - 0x9F */
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
> @@ -2491,16 +2493,14 @@ x86_emulate(
>  
>      case 0x90: /* nop / xchg %%r8,%%rax */
>          if ( !(rex_prefix & 1) )
> -            break; /* nop */
> +            goto no_writeback; /* nop */

Could you add an explicit /* fallthrough */ here?  The only reason it
isn't currently a coverity defect is because of the /* nop */ comment.

With these two, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>  
>      case 0x91 ... 0x97: /* xchg reg,%%rax */
> -        src.type = dst.type = OP_REG;
> -        src.bytes = dst.bytes = op_bytes;
> -        src.reg  = (unsigned long *)&_regs.eax;
> -        src.val  = *src.reg;
> -        dst.reg  = decode_register(
> +        src.type = OP_REG;
> +        src.bytes = op_bytes;
> +        src.reg  = decode_register(
>              (b & 7) | ((rex_prefix & 1) << 3), &_regs, 0);
> -        dst.val  = *dst.reg;
> +        src.val  = *src.reg;
>          goto xchg;
>  
>      case 0x98: /* cbw/cwde/cdqe */
>
>
>
Jan Beulich Aug. 16, 2016, 11:31 a.m. UTC | #2 
>>> On 16.08.16 at 12:59, <andrew.cooper3@citrix.com> 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@suse.com>
>>
>> --- 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.)

>> @@ -2491,16 +2493,14 @@ x86_emulate(
>>  
>>      case 0x90: /* nop / xchg %%r8,%%rax */
>>          if ( !(rex_prefix & 1) )
>> -            break; /* nop */
>> +            goto no_writeback; /* nop */
> 
> Could you add an explicit /* fallthrough */ here?  The only reason it
> isn't currently a coverity defect is because of the /* nop */ comment.

Will do; I actually had considered that, but then thought the
present comment is enough to silence Coverity.

> With these two, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'll wait with using this until the first point above got clarified.

Jan
Andrew Cooper Aug. 16, 2016, 12:46 p.m. UTC | #3 
On 16/08/16 12:31, Jan Beulich wrote:
>>>> On 16.08.16 at 12:59, <andrew.cooper3@citrix.com> 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@suse.com>
>>>
>>> --- 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.

~Andrew
Jan Beulich Aug. 16, 2016, 1:12 p.m. UTC | #4 
>>> On 16.08.16 at 14:46, <andrew.cooper3@citrix.com> wrote:
> On 16/08/16 12:31, Jan Beulich wrote:
>>>>> On 16.08.16 at 12:59, <andrew.cooper3@citrix.com> 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@suse.com>
>>>>
>>>> --- 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
diff mbox

Patch

--- 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,
     /* 0x98 - 0x9F */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
@@ -2491,16 +2493,14 @@  x86_emulate(
 
     case 0x90: /* nop / xchg %%r8,%%rax */
         if ( !(rex_prefix & 1) )
-            break; /* nop */
+            goto no_writeback; /* nop */
 
     case 0x91 ... 0x97: /* xchg reg,%%rax */
-        src.type = dst.type = OP_REG;
-        src.bytes = dst.bytes = op_bytes;
-        src.reg  = (unsigned long *)&_regs.eax;
-        src.val  = *src.reg;
-        dst.reg  = decode_register(
+        src.type = OP_REG;
+        src.bytes = op_bytes;
+        src.reg  = decode_register(
             (b & 7) | ((rex_prefix & 1) << 3), &_regs, 0);
-        dst.val  = *dst.reg;
+        src.val  = *src.reg;
         goto xchg;
 
     case 0x98: /* cbw/cwde/cdqe */