[PATCH 0/2] x86emul: XSA-123 follow-up

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/2] x86emul: XSA-123 follow-up
@ 2015-03-10 16:30 Jan Beulich
  2015-03-10 16:35 ` [PATCH 1/2] x86emul: drop unused "bigval" fields from struct operand Jan Beulich
  2015-03-10 16:36 ` [PATCH 2/2] x86emul: simplify asm() constraints Jan Beulich
  0 siblings, 2 replies; 8+ messages in thread
From: Jan Beulich @ 2015-03-10 16:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Keir Fraser

1: drop unused "bigval" fields from struct operand
2: simplify asm() constraints

Signed-off-by: Jan Beulich <jbeulich@suse.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] x86emul: drop unused "bigval" fields from struct operand
  2015-03-10 16:30 [PATCH 0/2] x86emul: XSA-123 follow-up Jan Beulich
@ 2015-03-10 16:35 ` Jan Beulich
  2015-03-10 18:47   ` Andrew Cooper
  2015-03-10 16:36 ` [PATCH 2/2] x86emul: simplify asm() constraints Jan Beulich
  1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2015-03-10 16:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Keir Fraser

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

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
@@ -313,17 +313,11 @@ struct operand {
     enum { OP_REG, OP_MEM, OP_IMM, OP_NONE } type;
     unsigned int bytes;
 
-    /* Up to 128-byte operand value, addressable as ulong or uint32_t[]. */
-    union {
-        unsigned long val;
-        uint32_t bigval[4];
-    };
+    /* Operand value. */
+    unsigned long val;
 
-    /* Up to 128-byte operand value, addressable as ulong or uint32_t[]. */
-    union {
-        unsigned long orig_val;
-        uint32_t orig_bigval[4];
-    };
+    /* Original operand value. */
+    unsigned long orig_val;
 
     union {
         /* OP_REG: Pointer to register field. */




[-- Attachment #2: x86emul-drop-bigval.patch --]
[-- Type: text/plain, Size: 843 bytes --]

x86emul: drop unused "bigval" fields from struct operand

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
@@ -313,17 +313,11 @@ struct operand {
     enum { OP_REG, OP_MEM, OP_IMM, OP_NONE } type;
     unsigned int bytes;
 
-    /* Up to 128-byte operand value, addressable as ulong or uint32_t[]. */
-    union {
-        unsigned long val;
-        uint32_t bigval[4];
-    };
+    /* Operand value. */
+    unsigned long val;
 
-    /* Up to 128-byte operand value, addressable as ulong or uint32_t[]. */
-    union {
-        unsigned long orig_val;
-        uint32_t orig_bigval[4];
-    };
+    /* Original operand value. */
+    unsigned long orig_val;
 
     union {
         /* OP_REG: Pointer to register field. */

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

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] x86emul: drop unused "bigval" fields from struct operand
  2015-03-10 16:35 ` [PATCH 1/2] x86emul: drop unused "bigval" fields from struct operand Jan Beulich
@ 2015-03-10 18:47   ` Andrew Cooper
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2015-03-10 18:47 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Keir Fraser

On 10/03/15 16:35, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] x86emul: simplify asm() constraints
  2015-03-10 16:30 [PATCH 0/2] x86emul: XSA-123 follow-up Jan Beulich
  2015-03-10 16:35 ` [PATCH 1/2] x86emul: drop unused "bigval" fields from struct operand Jan Beulich
@ 2015-03-10 16:36 ` Jan Beulich
  2015-03-10 19:48   ` Andrew Cooper
  2015-03-12 10:42   ` Tim Deegan
  1 sibling, 2 replies; 8+ messages in thread
From: Jan Beulich @ 2015-03-10 16:36 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Keir Fraser

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

Use + on outputs instead of = and a matching input. Allow not just
memory for the _eflags operand (it turns out that recent gcc produces
worse code when also doing this for _dst.val, so the latter is being
avoided).

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
@@ -428,7 +428,7 @@ typedef union {
 /* Before executing instruction: restore necessary bits in EFLAGS. */
 #define _PRE_EFLAGS(_sav, _msk, _tmp)                           \
 /* EFLAGS = (_sav & _msk) | (EFLAGS & ~_msk); _sav &= ~_msk; */ \
-"movl %"_sav",%"_LO32 _tmp"; "                                  \
+"movl %"_LO32 _sav",%"_LO32 _tmp"; "                            \
 "push %"_tmp"; "                                                \
 "push %"_tmp"; "                                                \
 "movl %"_msk",%"_LO32 _tmp"; "                                  \
@@ -448,7 +448,7 @@ typedef union {
 "pushf; "                                       \
 "pop  %"_tmp"; "                                \
 "andl %"_msk",%"_LO32 _tmp"; "                  \
-"orl  %"_LO32 _tmp",%"_sav"; "
+"orl  %"_LO32 _tmp",%"_LO32 _sav"; "
 
 /* Raw emulation: instruction has two explicit operands. */
 #define __emulate_2op_nobyte(_op,_src,_dst,_eflags,_wx,_wy,_lx,_ly,_qx,_qy)\
@@ -460,18 +460,16 @@ do{ unsigned long _tmp;                 
             _PRE_EFLAGS("0","4","2")                                       \
             _op"w %"_wx"3,%1; "                                            \
             _POST_EFLAGS("0","4","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : _wy ((_src).val), "i" (EFLAGS_MASK),                         \
-              "m" (_eflags), "m" ((_dst).val) );                           \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : _wy ((_src).val), "i" (EFLAGS_MASK) );                       \
         break;                                                             \
     case 4:                                                                \
         asm volatile (                                                     \
             _PRE_EFLAGS("0","4","2")                                       \
             _op"l %"_lx"3,%1; "                                            \
             _POST_EFLAGS("0","4","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : _ly ((_src).val), "i" (EFLAGS_MASK),                         \
-              "m" (_eflags), "m" ((_dst).val) );                           \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : _ly ((_src).val), "i" (EFLAGS_MASK) );                       \
         break;                                                             \
     case 8:                                                                \
         __emulate_2op_8byte(_op, _src, _dst, _eflags, _qx, _qy);           \
@@ -487,9 +485,8 @@ do{ unsigned long _tmp;                 
             _PRE_EFLAGS("0","4","2")                                       \
             _op"b %"_bx"3,%1; "                                            \
             _POST_EFLAGS("0","4","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : _by ((_src).val), "i" (EFLAGS_MASK),                         \
-              "m" (_eflags), "m" ((_dst).val) );                           \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : _by ((_src).val), "i" (EFLAGS_MASK) );                       \
         break;                                                             \
     default:                                                               \
         __emulate_2op_nobyte(_op,_src,_dst,_eflags,_wx,_wy,_lx,_ly,_qx,_qy);\
@@ -519,24 +516,24 @@ do{ unsigned long _tmp;                 
             _PRE_EFLAGS("0","3","2")                                       \
             _op"b %1; "                                                    \
             _POST_EFLAGS("0","3","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );        \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : "i" (EFLAGS_MASK) );                                         \
         break;                                                             \
     case 2:                                                                \
         asm volatile (                                                     \
             _PRE_EFLAGS("0","3","2")                                       \
             _op"w %1; "                                                    \
             _POST_EFLAGS("0","3","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );        \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : "i" (EFLAGS_MASK) );                                         \
         break;                                                             \
     case 4:                                                                \
         asm volatile (                                                     \
             _PRE_EFLAGS("0","3","2")                                       \
             _op"l %1; "                                                    \
             _POST_EFLAGS("0","3","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );        \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : "i" (EFLAGS_MASK) );                                         \
         break;                                                             \
     case 8:                                                                \
         __emulate_1op_8byte(_op, _dst, _eflags);                           \
@@ -551,17 +548,16 @@ do{ asm volatile (                      
         _PRE_EFLAGS("0","4","2")                                        \
         _op"q %"_qx"3,%1; "                                             \
         _POST_EFLAGS("0","4","2")                                       \
-        : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)               \
-        : _qy ((_src).val), "i" (EFLAGS_MASK),                          \
-          "m" (_eflags), "m" ((_dst).val) );                            \
+        : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)               \
+        : _qy ((_src).val), "i" (EFLAGS_MASK) );                        \
 } while (0)
 #define __emulate_1op_8byte(_op, _dst, _eflags)                         \
 do{ asm volatile (                                                      \
         _PRE_EFLAGS("0","3","2")                                        \
         _op"q %1; "                                                     \
         _POST_EFLAGS("0","3","2")                                       \
-        : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)               \
-        : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );         \
+        : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)               \
+        : "i" (EFLAGS_MASK) );                                          \
 } while (0)
 #elif defined(__i386__)
 #define __emulate_2op_8byte(_op, _src, _dst, _eflags, _qx, _qy)
@@ -806,9 +802,9 @@ static int read_ulong(
 static bool_t mul_dbl(unsigned long m[2])
 {
     bool_t rc;
-    asm ( "mul %4; seto %b2"
-          : "=a" (m[0]), "=d" (m[1]), "=q" (rc)
-          : "0" (m[0]), "1" (m[1]), "2" (0) );
+    asm ( "mul %1; seto %b2"
+          : "+a" (m[0]), "+d" (m[1]), "=q" (rc)
+          : "2" (0) );
     return rc;
 }
 
@@ -820,9 +816,9 @@ static bool_t mul_dbl(unsigned long m[2]
 static bool_t imul_dbl(unsigned long m[2])
 {
     bool_t rc;
-    asm ( "imul %4; seto %b2"
-          : "=a" (m[0]), "=d" (m[1]), "=q" (rc)
-          : "0" (m[0]), "1" (m[1]), "2" (0) );
+    asm ( "imul %1; seto %b2"
+          : "+a" (m[0]), "+d" (m[1]), "=q" (rc)
+          : "2" (0) );
     return rc;
 }
 
@@ -836,9 +832,7 @@ static bool_t div_dbl(unsigned long u[2]
 {
     if ( (v == 0) || (u[1] >= v) )
         return 1;
-    asm ( "div %4"
-          : "=a" (u[0]), "=d" (u[1])
-          : "0" (u[0]), "1" (u[1]), "r" (v) );
+    asm ( "div %2" : "+a" (u[0]), "+d" (u[1]) : "r" (v) );
     return 0;
 }
 



[-- Attachment #2: x86emul-asm-constraints.patch --]
[-- Type: text/plain, Size: 9116 bytes --]

x86emul: simplify asm() constraints

Use + on outputs instead of = and a matching input. Allow not just
memory for the _eflags operand (it turns out that recent gcc produces
worse code when also doing this for _dst.val, so the latter is being
avoided).

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
@@ -428,7 +428,7 @@ typedef union {
 /* Before executing instruction: restore necessary bits in EFLAGS. */
 #define _PRE_EFLAGS(_sav, _msk, _tmp)                           \
 /* EFLAGS = (_sav & _msk) | (EFLAGS & ~_msk); _sav &= ~_msk; */ \
-"movl %"_sav",%"_LO32 _tmp"; "                                  \
+"movl %"_LO32 _sav",%"_LO32 _tmp"; "                            \
 "push %"_tmp"; "                                                \
 "push %"_tmp"; "                                                \
 "movl %"_msk",%"_LO32 _tmp"; "                                  \
@@ -448,7 +448,7 @@ typedef union {
 "pushf; "                                       \
 "pop  %"_tmp"; "                                \
 "andl %"_msk",%"_LO32 _tmp"; "                  \
-"orl  %"_LO32 _tmp",%"_sav"; "
+"orl  %"_LO32 _tmp",%"_LO32 _sav"; "
 
 /* Raw emulation: instruction has two explicit operands. */
 #define __emulate_2op_nobyte(_op,_src,_dst,_eflags,_wx,_wy,_lx,_ly,_qx,_qy)\
@@ -460,18 +460,16 @@ do{ unsigned long _tmp;                 
             _PRE_EFLAGS("0","4","2")                                       \
             _op"w %"_wx"3,%1; "                                            \
             _POST_EFLAGS("0","4","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : _wy ((_src).val), "i" (EFLAGS_MASK),                         \
-              "m" (_eflags), "m" ((_dst).val) );                           \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : _wy ((_src).val), "i" (EFLAGS_MASK) );                       \
         break;                                                             \
     case 4:                                                                \
         asm volatile (                                                     \
             _PRE_EFLAGS("0","4","2")                                       \
             _op"l %"_lx"3,%1; "                                            \
             _POST_EFLAGS("0","4","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : _ly ((_src).val), "i" (EFLAGS_MASK),                         \
-              "m" (_eflags), "m" ((_dst).val) );                           \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : _ly ((_src).val), "i" (EFLAGS_MASK) );                       \
         break;                                                             \
     case 8:                                                                \
         __emulate_2op_8byte(_op, _src, _dst, _eflags, _qx, _qy);           \
@@ -487,9 +485,8 @@ do{ unsigned long _tmp;                 
             _PRE_EFLAGS("0","4","2")                                       \
             _op"b %"_bx"3,%1; "                                            \
             _POST_EFLAGS("0","4","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : _by ((_src).val), "i" (EFLAGS_MASK),                         \
-              "m" (_eflags), "m" ((_dst).val) );                           \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : _by ((_src).val), "i" (EFLAGS_MASK) );                       \
         break;                                                             \
     default:                                                               \
         __emulate_2op_nobyte(_op,_src,_dst,_eflags,_wx,_wy,_lx,_ly,_qx,_qy);\
@@ -519,24 +516,24 @@ do{ unsigned long _tmp;                 
             _PRE_EFLAGS("0","3","2")                                       \
             _op"b %1; "                                                    \
             _POST_EFLAGS("0","3","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );        \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : "i" (EFLAGS_MASK) );                                         \
         break;                                                             \
     case 2:                                                                \
         asm volatile (                                                     \
             _PRE_EFLAGS("0","3","2")                                       \
             _op"w %1; "                                                    \
             _POST_EFLAGS("0","3","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );        \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : "i" (EFLAGS_MASK) );                                         \
         break;                                                             \
     case 4:                                                                \
         asm volatile (                                                     \
             _PRE_EFLAGS("0","3","2")                                       \
             _op"l %1; "                                                    \
             _POST_EFLAGS("0","3","2")                                      \
-            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
-            : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );        \
+            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
+            : "i" (EFLAGS_MASK) );                                         \
         break;                                                             \
     case 8:                                                                \
         __emulate_1op_8byte(_op, _dst, _eflags);                           \
@@ -551,17 +548,16 @@ do{ asm volatile (                      
         _PRE_EFLAGS("0","4","2")                                        \
         _op"q %"_qx"3,%1; "                                             \
         _POST_EFLAGS("0","4","2")                                       \
-        : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)               \
-        : _qy ((_src).val), "i" (EFLAGS_MASK),                          \
-          "m" (_eflags), "m" ((_dst).val) );                            \
+        : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)               \
+        : _qy ((_src).val), "i" (EFLAGS_MASK) );                        \
 } while (0)
 #define __emulate_1op_8byte(_op, _dst, _eflags)                         \
 do{ asm volatile (                                                      \
         _PRE_EFLAGS("0","3","2")                                        \
         _op"q %1; "                                                     \
         _POST_EFLAGS("0","3","2")                                       \
-        : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)               \
-        : "i" (EFLAGS_MASK), "m" (_eflags), "m" ((_dst).val) );         \
+        : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)               \
+        : "i" (EFLAGS_MASK) );                                          \
 } while (0)
 #elif defined(__i386__)
 #define __emulate_2op_8byte(_op, _src, _dst, _eflags, _qx, _qy)
@@ -806,9 +802,9 @@ static int read_ulong(
 static bool_t mul_dbl(unsigned long m[2])
 {
     bool_t rc;
-    asm ( "mul %4; seto %b2"
-          : "=a" (m[0]), "=d" (m[1]), "=q" (rc)
-          : "0" (m[0]), "1" (m[1]), "2" (0) );
+    asm ( "mul %1; seto %b2"
+          : "+a" (m[0]), "+d" (m[1]), "=q" (rc)
+          : "2" (0) );
     return rc;
 }
 
@@ -820,9 +816,9 @@ static bool_t mul_dbl(unsigned long m[2]
 static bool_t imul_dbl(unsigned long m[2])
 {
     bool_t rc;
-    asm ( "imul %4; seto %b2"
-          : "=a" (m[0]), "=d" (m[1]), "=q" (rc)
-          : "0" (m[0]), "1" (m[1]), "2" (0) );
+    asm ( "imul %1; seto %b2"
+          : "+a" (m[0]), "+d" (m[1]), "=q" (rc)
+          : "2" (0) );
     return rc;
 }
 
@@ -836,9 +832,7 @@ static bool_t div_dbl(unsigned long u[2]
 {
     if ( (v == 0) || (u[1] >= v) )
         return 1;
-    asm ( "div %4"
-          : "=a" (u[0]), "=d" (u[1])
-          : "0" (u[0]), "1" (u[1]), "r" (v) );
+    asm ( "div %2" : "+a" (u[0]), "+d" (u[1]) : "r" (v) );
     return 0;
 }
 

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

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86emul: simplify asm() constraints
  2015-03-10 16:36 ` [PATCH 2/2] x86emul: simplify asm() constraints Jan Beulich
@ 2015-03-10 19:48   ` Andrew Cooper
  2015-03-11  8:08     ` Jan Beulich
  2015-03-12 10:42   ` Tim Deegan
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2015-03-10 19:48 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Keir Fraser


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

On 10/03/15 16:36, Jan Beulich wrote:
> Use + on outputs instead of = and a matching input. Allow not just
> memory for the _eflags operand (it turns out that recent gcc produces
> worse code when also doing this for _dst.val, so the latter is being
> avoided).
>
> 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
> @@ -428,7 +428,7 @@ typedef union {
>  /* Before executing instruction: restore necessary bits in EFLAGS. */
>  #define _PRE_EFLAGS(_sav, _msk, _tmp)                           \
>  /* EFLAGS = (_sav & _msk) | (EFLAGS & ~_msk); _sav &= ~_msk; */ \
> -"movl %"_sav",%"_LO32 _tmp"; "                                  \
> +"movl %"_LO32 _sav",%"_LO32 _tmp"; "                            \
>  "push %"_tmp"; "                                                \
>  "push %"_tmp"; "                                                \
>  "movl %"_msk",%"_LO32 _tmp"; "                                  \
> @@ -448,7 +448,7 @@ typedef union {
>  "pushf; "                                       \
>  "pop  %"_tmp"; "                                \
>  "andl %"_msk",%"_LO32 _tmp"; "                  \
> -"orl  %"_LO32 _tmp",%"_sav"; "
> +"orl  %"_LO32 _tmp",%"_LO32 _sav"; "
>  
>  /* Raw emulation: instruction has two explicit operands. */
>  #define __emulate_2op_nobyte(_op,_src,_dst,_eflags,_wx,_wy,_lx,_ly,_qx,_qy)\
> @@ -460,18 +460,16 @@ do{ unsigned long _tmp;                 
>              _PRE_EFLAGS("0","4","2")                                       \
>              _op"w %"_wx"3,%1; "                                            \
>              _POST_EFLAGS("0","4","2")                                      \
> -            : "=m" (_eflags), "=m" ((_dst).val), "=&r" (_tmp)              \
> -            : _wy ((_src).val), "i" (EFLAGS_MASK),                         \
> -              "m" (_eflags), "m" ((_dst).val) );                           \
> +            : "+g" (_eflags), "+m" ((_dst).val), "=&r" (_tmp)              \
> +            : _wy ((_src).val), "i" (EFLAGS_MASK) );                       \

I believe the old ASM was buggy, not just inefficient.

Having read the Extended ASM documentation quite carefully, the
following statement is relevant

"Only input operands may use numbers in constraints, and they must each
refer to an output operand. Only a number (or the symbolic assembler
name) in the constraint can guarantee that one operand is in the same
place as another. The mere fact tha|t 'foo' |||is the value of both
operands is not enough to guarantee that they are in the same place in
the generated assembler code."

Because the input operands do not use numbers, the asm must read from %5
and write to %0 to guarantee that the _eflags temporary is used properly.

I believe that this transformation does now make the asm correct, as the
output and input sides are now guaranteed to match the %0 used to
reference the _eflags temporary.

Did you observe any code changes simply from changing = constraints to
+, or did we get very lucky in with the generated code?

I think it might be a very wise idea to switch to using symbolic names. 
This code is very complicated and has many ways to go subtly wrong.

~Andrew

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

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

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86emul: simplify asm() constraints
  2015-03-10 19:48   ` Andrew Cooper
@ 2015-03-11  8:08     ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2015-03-11  8:08 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Keir Fraser

>>> On 10.03.15 at 20:48, <andrew.cooper3@citrix.com> wrote:
> Did you observe any code changes simply from changing = constraints to
> +, or did we get very lucky in with the generated code?

I think it's not really unexpected that there was no change, since
the operands were all memory-only ones (and generally gcc avoids
to address the same memory location in multiple different ways).

> I think it might be a very wise idea to switch to using symbolic names. 
> This code is very complicated and has many ways to go subtly wrong.

Let's save this for another day.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86emul: simplify asm() constraints
  2015-03-10 16:36 ` [PATCH 2/2] x86emul: simplify asm() constraints Jan Beulich
  2015-03-10 19:48   ` Andrew Cooper
@ 2015-03-12 10:42   ` Tim Deegan
  2015-03-12 10:55     ` Jan Beulich
  1 sibling, 1 reply; 8+ messages in thread
From: Tim Deegan @ 2015-03-12 10:42 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Andrew Cooper

At 16:36 +0000 on 10 Mar (1426001780), Jan Beulich wrote:
> @@ -806,9 +802,9 @@ static int read_ulong(
>  static bool_t mul_dbl(unsigned long m[2])
>  {
>      bool_t rc;
> -    asm ( "mul %4; seto %b2"
> -          : "=a" (m[0]), "=d" (m[1]), "=q" (rc)
> -          : "0" (m[0]), "1" (m[1]), "2" (0) );
> +    asm ( "mul %1; seto %b2"
> +          : "+a" (m[0]), "+d" (m[1]), "=q" (rc)
> +          : "2" (0) );

Would 'bool_t rc = 0' allow you to switch operand 2 to +q and drop the
last input operand as well?  Or did that also produce worse code?

Cheers,

Tim.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86emul: simplify asm() constraints
  2015-03-12 10:42   ` Tim Deegan
@ 2015-03-12 10:55     ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2015-03-12 10:55 UTC (permalink / raw)
  To: Tim Deegan; +Cc: Andrew Cooper, Keir Fraser, xen-devel

>>> On 12.03.15 at 11:42, <tim@xen.org> wrote:
> At 16:36 +0000 on 10 Mar (1426001780), Jan Beulich wrote:
>> @@ -806,9 +802,9 @@ static int read_ulong(
>>  static bool_t mul_dbl(unsigned long m[2])
>>  {
>>      bool_t rc;
>> -    asm ( "mul %4; seto %b2"
>> -          : "=a" (m[0]), "=d" (m[1]), "=q" (rc)
>> -          : "0" (m[0]), "1" (m[1]), "2" (0) );
>> +    asm ( "mul %1; seto %b2"
>> +          : "+a" (m[0]), "+d" (m[1]), "=q" (rc)
>> +          : "2" (0) );
> 
> Would 'bool_t rc = 0' allow you to switch operand 2 to +q and drop the
> last input operand as well?

Yes.

>  Or did that also produce worse code?

I didn't try yet, but I can't see why it would.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-03-12 10:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-10 16:30 [PATCH 0/2] x86emul: XSA-123 follow-up Jan Beulich
2015-03-10 16:35 ` [PATCH 1/2] x86emul: drop unused "bigval" fields from struct operand Jan Beulich
2015-03-10 18:47   ` Andrew Cooper
2015-03-10 16:36 ` [PATCH 2/2] x86emul: simplify asm() constraints Jan Beulich
2015-03-10 19:48   ` Andrew Cooper
2015-03-11  8:08     ` Jan Beulich
2015-03-12 10:42   ` Tim Deegan
2015-03-12 10:55     ` Jan Beulich

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.