All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86emul: improve LOCK handling
@ 2016-08-16 13:51 Jan Beulich
  2016-08-16 15:01 ` Andrew Cooper
  0 siblings, 1 reply; 2+ messages in thread
From: Jan Beulich @ 2016-08-16 13:51 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper

[-- Attachment #1: Type: text/plain, Size: 4427 bytes --]

Certain opcodes would so far not have got #UD when a LOCK prefix was
present. Adjust this by
- moving the too early generic check into destination operand decoding,
  where DstNone and DstReg already have respective handling
- switching source and destination of TEST r,r/m, for it to be taken
  care of by aforementioned generic checks
- explicitly dealing with all forms of CMP, SHLD, SHRD, as well as
  TEST $imm,r/m

To make the handling of opcodes F6 and F7 more obvious, reduce the
amount of state set in the table, and adjust the respective switch()
statement accordingly.

Also eliminate the latent bug of the check in DstNone handling not
considering the opcode extension set.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This will only apply cleanly on top of
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01975.html.

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -110,7 +110,7 @@ static uint8_t opcode_table[256] = {
     /* 0x80 - 0x87 */
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImm|ModRM,
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImmByte|ModRM,
-    ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
+    ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM,
     ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
     /* 0x88 - 0x8F */
     ByteOp|DstMem|SrcReg|ModRM|Mov, DstMem|SrcReg|ModRM|Mov,
@@ -169,8 +169,7 @@ static uint8_t opcode_table[256] = {
     DstEax|SrcImplicit, DstEax|SrcImplicit, ImplicitOps, ImplicitOps,
     /* 0xF0 - 0xF7 */
     0, ImplicitOps, 0, 0,
-    ImplicitOps, ImplicitOps,
-    ByteOp|DstMem|SrcNone|ModRM, DstMem|SrcNone|ModRM,
+    ImplicitOps, ImplicitOps, ByteOp|ModRM, ModRM,
     /* 0xF8 - 0xFF */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps, ImplicitOps, ByteOp|DstMem|SrcNone|ModRM, DstMem|SrcNone|ModRM
@@ -1651,9 +1650,6 @@ x86_emulate(
         }
     }
 
-    /* Lock prefix is allowed only on RMW instructions. */
-    generate_exception_if((d & Mov) && lock_prefix, EXC_UD, -1);
-
     /* ModRM and SIB bytes. */
     if ( d & ModRM )
     {
@@ -1729,13 +1725,17 @@ x86_emulate(
                 switch ( modrm_reg & 7 )
                 {
                 case 0 ... 1: /* test */
-                    d = (d & ~SrcMask) | SrcImm;
+                    d |= DstMem | SrcImm;
+                    break;
+                case 2: /* not */
+                case 3: /* neg */
+                    d |= DstMem;
                     break;
                 case 4: /* mul */
                 case 5: /* imul */
                 case 6: /* div */
                 case 7: /* idiv */
-                    d = (d & (ByteOp | ModRM)) | DstEax | SrcMem;
+                    d |= DstEax | SrcMem;
                     break;
                 }
                 break;
@@ -1983,8 +1983,9 @@ x86_emulate(
          */
         generate_exception_if(
             lock_prefix &&
-            ((b < 0x20) || (b > 0x23)) && /* MOV CRn/DRn */
-            (b != 0xc7),                  /* CMPXCHG{8,16}B */
+            (ext != ext_0f ||
+             (((b < 0x20) || (b > 0x23)) && /* MOV CRn/DRn */
+              (b != 0xc7))),                /* CMPXCHG{8,16}B */
             EXC_UD, -1);
         dst.type = OP_NONE;
         break;
@@ -2062,6 +2063,8 @@ x86_emulate(
                 goto done;
             dst.orig_val = dst.val;
         }
+        else /* Lock prefix is allowed only on RMW instructions. */
+            generate_exception_if(lock_prefix, EXC_UD, -1);
         break;
     }
 
@@ -2111,6 +2114,7 @@ x86_emulate(
         break;
 
     case 0x38 ... 0x3d: cmp: /* cmp */
+        generate_exception_if(lock_prefix, EXC_UD, -1);
         emulate_2op_SrcV("cmp", src, dst, _regs.eflags);
         dst.type = OP_NONE;
         break;
@@ -3545,6 +3549,7 @@ x86_emulate(
             unsigned long u[2], v;
 
         case 0 ... 1: /* test */
+            generate_exception_if(lock_prefix, EXC_UD, -1);
             goto test;
         case 2: /* not */
             dst.val = ~dst.val;
@@ -4507,6 +4512,7 @@ x86_emulate(
     case 0xad: /* shrd %%cl,r,r/m */ {
         uint8_t shift, width = dst.bytes << 3;
 
+        generate_exception_if(lock_prefix, EXC_UD, -1);
         if ( b & 1 )
             shift = _regs.ecx;
         else



[-- Attachment #2: x86emul-lock-handling.patch --]
[-- Type: text/plain, Size: 4457 bytes --]

x86emul: improve LOCK handling

Certain opcodes would so far not have got #UD when a LOCK prefix was
present. Adjust this by
- moving the too early generic check into destination operand decoding,
  where DstNone and DstReg already have respective handling
- switching source and destination of TEST r,r/m, for it to be taken
  care of by aforementioned generic checks
- explicitly dealing with all forms of CMP, SHLD, SHRD, as well as
  TEST $imm,r/m

To make the handling of opcodes F6 and F7 more obvious, reduce the
amount of state set in the table, and adjust the respective switch()
statement accordingly.

Also eliminate the latent bug of the check in DstNone handling not
considering the opcode extension set.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This will only apply cleanly on top of
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01975.html.

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -110,7 +110,7 @@ static uint8_t opcode_table[256] = {
     /* 0x80 - 0x87 */
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImm|ModRM,
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImmByte|ModRM,
-    ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
+    ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM,
     ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
     /* 0x88 - 0x8F */
     ByteOp|DstMem|SrcReg|ModRM|Mov, DstMem|SrcReg|ModRM|Mov,
@@ -169,8 +169,7 @@ static uint8_t opcode_table[256] = {
     DstEax|SrcImplicit, DstEax|SrcImplicit, ImplicitOps, ImplicitOps,
     /* 0xF0 - 0xF7 */
     0, ImplicitOps, 0, 0,
-    ImplicitOps, ImplicitOps,
-    ByteOp|DstMem|SrcNone|ModRM, DstMem|SrcNone|ModRM,
+    ImplicitOps, ImplicitOps, ByteOp|ModRM, ModRM,
     /* 0xF8 - 0xFF */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps, ImplicitOps, ByteOp|DstMem|SrcNone|ModRM, DstMem|SrcNone|ModRM
@@ -1651,9 +1650,6 @@ x86_emulate(
         }
     }
 
-    /* Lock prefix is allowed only on RMW instructions. */
-    generate_exception_if((d & Mov) && lock_prefix, EXC_UD, -1);
-
     /* ModRM and SIB bytes. */
     if ( d & ModRM )
     {
@@ -1729,13 +1725,17 @@ x86_emulate(
                 switch ( modrm_reg & 7 )
                 {
                 case 0 ... 1: /* test */
-                    d = (d & ~SrcMask) | SrcImm;
+                    d |= DstMem | SrcImm;
+                    break;
+                case 2: /* not */
+                case 3: /* neg */
+                    d |= DstMem;
                     break;
                 case 4: /* mul */
                 case 5: /* imul */
                 case 6: /* div */
                 case 7: /* idiv */
-                    d = (d & (ByteOp | ModRM)) | DstEax | SrcMem;
+                    d |= DstEax | SrcMem;
                     break;
                 }
                 break;
@@ -1983,8 +1983,9 @@ x86_emulate(
          */
         generate_exception_if(
             lock_prefix &&
-            ((b < 0x20) || (b > 0x23)) && /* MOV CRn/DRn */
-            (b != 0xc7),                  /* CMPXCHG{8,16}B */
+            (ext != ext_0f ||
+             (((b < 0x20) || (b > 0x23)) && /* MOV CRn/DRn */
+              (b != 0xc7))),                /* CMPXCHG{8,16}B */
             EXC_UD, -1);
         dst.type = OP_NONE;
         break;
@@ -2062,6 +2063,8 @@ x86_emulate(
                 goto done;
             dst.orig_val = dst.val;
         }
+        else /* Lock prefix is allowed only on RMW instructions. */
+            generate_exception_if(lock_prefix, EXC_UD, -1);
         break;
     }
 
@@ -2111,6 +2114,7 @@ x86_emulate(
         break;
 
     case 0x38 ... 0x3d: cmp: /* cmp */
+        generate_exception_if(lock_prefix, EXC_UD, -1);
         emulate_2op_SrcV("cmp", src, dst, _regs.eflags);
         dst.type = OP_NONE;
         break;
@@ -3545,6 +3549,7 @@ x86_emulate(
             unsigned long u[2], v;
 
         case 0 ... 1: /* test */
+            generate_exception_if(lock_prefix, EXC_UD, -1);
             goto test;
         case 2: /* not */
             dst.val = ~dst.val;
@@ -4507,6 +4512,7 @@ x86_emulate(
     case 0xad: /* shrd %%cl,r,r/m */ {
         uint8_t shift, width = dst.bytes << 3;
 
+        generate_exception_if(lock_prefix, EXC_UD, -1);
         if ( b & 1 )
             shift = _regs.ecx;
         else

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
* Re: [PATCH] x86emul: improve LOCK handling
  2016-08-16 13:51 [PATCH] x86emul: improve LOCK handling Jan Beulich
@ 2016-08-16 15:01 ` Andrew Cooper
  0 siblings, 0 replies; 2+ messages in thread
From: Andrew Cooper @ 2016-08-16 15:01 UTC (permalink / raw)
  To: Jan Beulich, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 859 bytes --]

On 16/08/16 14:51, Jan Beulich wrote:
> Certain opcodes would so far not have got #UD when a LOCK prefix was
> present. Adjust this by
> - moving the too early generic check into destination operand decoding,
>   where DstNone and DstReg already have respective handling
> - switching source and destination of TEST r,r/m, for it to be taken
>   care of by aforementioned generic checks
> - explicitly dealing with all forms of CMP, SHLD, SHRD, as well as
>   TEST $imm,r/m
>
> To make the handling of opcodes F6 and F7 more obvious, reduce the
> amount of state set in the table, and adjust the respective switch()
> statement accordingly.
>
> Also eliminate the latent bug of the check in DstNone handling not
> considering the opcode extension set.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[-- Attachment #1.2: Type: text/html, Size: 1375 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-08-16 15:02 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-16 13:51 [PATCH] x86emul: improve LOCK handling Jan Beulich
2016-08-16 15:01 ` Andrew Cooper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.