[PATCH 0/2] x86emul: feature flag handling adjustments

[PATCH 0/2] x86emul: feature flag handling adjustments

[Jan Beulich]

1: x86emul: derive vcpu_must_have() from vcpu_has()
2: x86: drop cpu_has_sse{,2}

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


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

[PATCH 1/2] x86emul: derive vcpu_must_have() from vcpu_has()

[Jan Beulich]

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

... to avoid introducing further redundancy when adding further feature
flag checks, and to bring its use better in line with its host_and_*()
sibling.

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
@@ -1260,39 +1260,39 @@ static bool vcpu_has(
     return rc == X86EMUL_OKAY;
 }
 
-#define vcpu_has_clflush() vcpu_has(       1, EDX, 19, ctxt, ops)
-#define vcpu_has_lzcnt() vcpu_has(0x80000001, ECX,  5, ctxt, ops)
-#define vcpu_has_misalignsse() vcpu_has(0x80000001, ECX, 7, ctxt, ops)
-#define vcpu_has_bmi1()  vcpu_has(0x00000007, EBX,  3, ctxt, ops)
-#define vcpu_has_hle()   vcpu_has(0x00000007, EBX,  4, ctxt, ops)
-#define vcpu_has_rtm()   vcpu_has(0x00000007, EBX, 11, ctxt, ops)
-
-#define vcpu_must_have(leaf, reg, bit) \
-    generate_exception_if(!vcpu_has(leaf, reg, bit, ctxt, ops), EXC_UD)
-#define vcpu_must_have_fpu()  vcpu_must_have(0x00000001, EDX, 0)
-#define vcpu_must_have_cmov() vcpu_must_have(0x00000001, EDX, 15)
-#define vcpu_must_have_mmx()  vcpu_must_have(0x00000001, EDX, 23)
-#define vcpu_must_have_sse()  vcpu_must_have(0x00000001, EDX, 25)
-#define vcpu_must_have_sse2() vcpu_must_have(0x00000001, EDX, 26)
-#define vcpu_must_have_sse3() vcpu_must_have(0x00000001, ECX,  0)
-#define vcpu_must_have_cx16() vcpu_must_have(0x00000001, ECX, 13)
-#define vcpu_must_have_sse4_2() vcpu_must_have(0x00000001, ECX, 20)
-#define vcpu_must_have_movbe() vcpu_must_have(0x00000001, ECX, 22)
-#define vcpu_must_have_avx()  vcpu_must_have(0x00000001, ECX, 28)
+#define vcpu_has_fpu()         vcpu_has(         1, EDX,  0, ctxt, ops)
+#define vcpu_has_cmov()        vcpu_has(         1, EDX, 15, ctxt, ops)
+#define vcpu_has_clflush()     vcpu_has(         1, EDX, 19, ctxt, ops)
+#define vcpu_has_mmx()         vcpu_has(         1, EDX, 23, ctxt, ops)
+#define vcpu_has_sse()         vcpu_has(         1, EDX, 25, ctxt, ops)
+#define vcpu_has_sse2()        vcpu_has(         1, EDX, 26, ctxt, ops)
+#define vcpu_has_sse3()        vcpu_has(         1, ECX,  0, ctxt, ops)
+#define vcpu_has_cx16()        vcpu_has(         1, ECX, 13, ctxt, ops)
+#define vcpu_has_sse4_2()      vcpu_has(         1, ECX, 20, ctxt, ops)
+#define vcpu_has_movbe()       vcpu_has(         1, ECX, 22, ctxt, ops)
+#define vcpu_has_avx()         vcpu_has(         1, ECX, 28, ctxt, ops)
+#define vcpu_has_lzcnt()       vcpu_has(0x80000001, ECX,  5, ctxt, ops)
+#define vcpu_has_misalignsse() vcpu_has(0x80000001, ECX,  7, ctxt, ops)
+#define vcpu_has_bmi1()        vcpu_has(         7, EBX,  3, ctxt, ops)
+#define vcpu_has_hle()         vcpu_has(         7, EBX,  4, ctxt, ops)
+#define vcpu_has_rtm()         vcpu_has(         7, EBX, 11, ctxt, ops)
+
+#define vcpu_must_have(feat) \
+    generate_exception_if(!vcpu_has_##feat(), EXC_UD)
 
 #ifdef __XEN__
 /*
- * Note the difference between vcpu_must_have_<feature>() and
+ * Note the difference between vcpu_must_have(<feature>) and
  * host_and_vcpu_must_have(<feature>): The latter needs to be used when
  * emulation code is using the same instruction class for carrying out
  * the actual operation.
  */
 #define host_and_vcpu_must_have(feat) ({ \
     generate_exception_if(!cpu_has_##feat, EXC_UD); \
-    vcpu_must_have_##feat(); \
+    vcpu_must_have(feat); \
 })
 #else
-#define host_and_vcpu_must_have(feat) vcpu_must_have_##feat()
+#define host_and_vcpu_must_have(feat) vcpu_must_have(feat)
 #endif
 
 static int
@@ -3651,7 +3651,7 @@ x86_emulate(
         case 0xc8 ... 0xcf: /* fcmove %stN */
         case 0xd0 ... 0xd7: /* fcmovbe %stN */
         case 0xd8 ... 0xdf: /* fcmovu %stN */
-            vcpu_must_have_cmov();
+            vcpu_must_have(cmov);
             emulate_fpu_insn_stub_eflags(0xda, modrm);
             break;
         case 0xe9:          /* fucompp */
@@ -3704,7 +3704,7 @@ x86_emulate(
         case 0xd8 ... 0xdf: /* fcmovnu %stN */
         case 0xe8 ... 0xef: /* fucomi %stN */
         case 0xf0 ... 0xf7: /* fcomi %stN */
-            vcpu_must_have_cmov();
+            vcpu_must_have(cmov);
             emulate_fpu_insn_stub_eflags(0xdb, modrm);
             break;
         case 0xe0: /* fneni - 8087 only, ignored by 287 */
@@ -3927,7 +3927,7 @@ x86_emulate(
             break;
         case 0xe8 ... 0xef: /* fucomip %stN */
         case 0xf0 ... 0xf7: /* fcomip %stN */
-            vcpu_must_have_cmov();
+            vcpu_must_have(cmov);
             emulate_fpu_insn_stub_eflags(0xdf, modrm);
             break;
         case 0xc0 ... 0xc7: /* ffreep %stN */
@@ -4824,7 +4824,7 @@ x86_emulate(
     }
 
     case X86EMUL_OPC(0x0f, 0x40) ... X86EMUL_OPC(0x0f, 0x4f): /* cmovcc */
-        vcpu_must_have_cmov();
+        vcpu_must_have(cmov);
         if ( test_cc(b, _regs.eflags) )
             dst.val = src.val;
         break;
@@ -5280,7 +5280,7 @@ x86_emulate(
 
     case X86EMUL_OPC(0x0f, 0xc3): /* movnti */
         /* Ignore the non-temporal hint for now. */
-        vcpu_must_have_sse2();
+        vcpu_must_have(sse2);
         generate_exception_if(dst.bytes <= 2, EXC_UD);
         dst.val = src.val;
         break;
@@ -5352,7 +5352,7 @@ x86_emulate(
 
     case X86EMUL_OPC(0x0f38, 0xf0): /* movbe m,r */
     case X86EMUL_OPC(0x0f38, 0xf1): /* movbe r,m */
-        vcpu_must_have_movbe();
+        vcpu_must_have(movbe);
         switch ( op_bytes )
         {
         case 2:



[-- Attachment #2: x86emul-vcpu_must_have.patch --]
[-- Type: text/plain, Size: 5574 bytes --]

x86emul: derive vcpu_must_have() from vcpu_has()

... to avoid introducing further redundancy when adding further feature
flag checks, and to bring its use better in line with its host_and_*()
sibling.

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
@@ -1260,39 +1260,39 @@ static bool vcpu_has(
     return rc == X86EMUL_OKAY;
 }
 
-#define vcpu_has_clflush() vcpu_has(       1, EDX, 19, ctxt, ops)
-#define vcpu_has_lzcnt() vcpu_has(0x80000001, ECX,  5, ctxt, ops)
-#define vcpu_has_misalignsse() vcpu_has(0x80000001, ECX, 7, ctxt, ops)
-#define vcpu_has_bmi1()  vcpu_has(0x00000007, EBX,  3, ctxt, ops)
-#define vcpu_has_hle()   vcpu_has(0x00000007, EBX,  4, ctxt, ops)
-#define vcpu_has_rtm()   vcpu_has(0x00000007, EBX, 11, ctxt, ops)
-
-#define vcpu_must_have(leaf, reg, bit) \
-    generate_exception_if(!vcpu_has(leaf, reg, bit, ctxt, ops), EXC_UD)
-#define vcpu_must_have_fpu()  vcpu_must_have(0x00000001, EDX, 0)
-#define vcpu_must_have_cmov() vcpu_must_have(0x00000001, EDX, 15)
-#define vcpu_must_have_mmx()  vcpu_must_have(0x00000001, EDX, 23)
-#define vcpu_must_have_sse()  vcpu_must_have(0x00000001, EDX, 25)
-#define vcpu_must_have_sse2() vcpu_must_have(0x00000001, EDX, 26)
-#define vcpu_must_have_sse3() vcpu_must_have(0x00000001, ECX,  0)
-#define vcpu_must_have_cx16() vcpu_must_have(0x00000001, ECX, 13)
-#define vcpu_must_have_sse4_2() vcpu_must_have(0x00000001, ECX, 20)
-#define vcpu_must_have_movbe() vcpu_must_have(0x00000001, ECX, 22)
-#define vcpu_must_have_avx()  vcpu_must_have(0x00000001, ECX, 28)
+#define vcpu_has_fpu()         vcpu_has(         1, EDX,  0, ctxt, ops)
+#define vcpu_has_cmov()        vcpu_has(         1, EDX, 15, ctxt, ops)
+#define vcpu_has_clflush()     vcpu_has(         1, EDX, 19, ctxt, ops)
+#define vcpu_has_mmx()         vcpu_has(         1, EDX, 23, ctxt, ops)
+#define vcpu_has_sse()         vcpu_has(         1, EDX, 25, ctxt, ops)
+#define vcpu_has_sse2()        vcpu_has(         1, EDX, 26, ctxt, ops)
+#define vcpu_has_sse3()        vcpu_has(         1, ECX,  0, ctxt, ops)
+#define vcpu_has_cx16()        vcpu_has(         1, ECX, 13, ctxt, ops)
+#define vcpu_has_sse4_2()      vcpu_has(         1, ECX, 20, ctxt, ops)
+#define vcpu_has_movbe()       vcpu_has(         1, ECX, 22, ctxt, ops)
+#define vcpu_has_avx()         vcpu_has(         1, ECX, 28, ctxt, ops)
+#define vcpu_has_lzcnt()       vcpu_has(0x80000001, ECX,  5, ctxt, ops)
+#define vcpu_has_misalignsse() vcpu_has(0x80000001, ECX,  7, ctxt, ops)
+#define vcpu_has_bmi1()        vcpu_has(         7, EBX,  3, ctxt, ops)
+#define vcpu_has_hle()         vcpu_has(         7, EBX,  4, ctxt, ops)
+#define vcpu_has_rtm()         vcpu_has(         7, EBX, 11, ctxt, ops)
+
+#define vcpu_must_have(feat) \
+    generate_exception_if(!vcpu_has_##feat(), EXC_UD)
 
 #ifdef __XEN__
 /*
- * Note the difference between vcpu_must_have_<feature>() and
+ * Note the difference between vcpu_must_have(<feature>) and
  * host_and_vcpu_must_have(<feature>): The latter needs to be used when
  * emulation code is using the same instruction class for carrying out
  * the actual operation.
  */
 #define host_and_vcpu_must_have(feat) ({ \
     generate_exception_if(!cpu_has_##feat, EXC_UD); \
-    vcpu_must_have_##feat(); \
+    vcpu_must_have(feat); \
 })
 #else
-#define host_and_vcpu_must_have(feat) vcpu_must_have_##feat()
+#define host_and_vcpu_must_have(feat) vcpu_must_have(feat)
 #endif
 
 static int
@@ -3651,7 +3651,7 @@ x86_emulate(
         case 0xc8 ... 0xcf: /* fcmove %stN */
         case 0xd0 ... 0xd7: /* fcmovbe %stN */
         case 0xd8 ... 0xdf: /* fcmovu %stN */
-            vcpu_must_have_cmov();
+            vcpu_must_have(cmov);
             emulate_fpu_insn_stub_eflags(0xda, modrm);
             break;
         case 0xe9:          /* fucompp */
@@ -3704,7 +3704,7 @@ x86_emulate(
         case 0xd8 ... 0xdf: /* fcmovnu %stN */
         case 0xe8 ... 0xef: /* fucomi %stN */
         case 0xf0 ... 0xf7: /* fcomi %stN */
-            vcpu_must_have_cmov();
+            vcpu_must_have(cmov);
             emulate_fpu_insn_stub_eflags(0xdb, modrm);
             break;
         case 0xe0: /* fneni - 8087 only, ignored by 287 */
@@ -3927,7 +3927,7 @@ x86_emulate(
             break;
         case 0xe8 ... 0xef: /* fucomip %stN */
         case 0xf0 ... 0xf7: /* fcomip %stN */
-            vcpu_must_have_cmov();
+            vcpu_must_have(cmov);
             emulate_fpu_insn_stub_eflags(0xdf, modrm);
             break;
         case 0xc0 ... 0xc7: /* ffreep %stN */
@@ -4824,7 +4824,7 @@ x86_emulate(
     }
 
     case X86EMUL_OPC(0x0f, 0x40) ... X86EMUL_OPC(0x0f, 0x4f): /* cmovcc */
-        vcpu_must_have_cmov();
+        vcpu_must_have(cmov);
         if ( test_cc(b, _regs.eflags) )
             dst.val = src.val;
         break;
@@ -5280,7 +5280,7 @@ x86_emulate(
 
     case X86EMUL_OPC(0x0f, 0xc3): /* movnti */
         /* Ignore the non-temporal hint for now. */
-        vcpu_must_have_sse2();
+        vcpu_must_have(sse2);
         generate_exception_if(dst.bytes <= 2, EXC_UD);
         dst.val = src.val;
         break;
@@ -5352,7 +5352,7 @@ x86_emulate(
 
     case X86EMUL_OPC(0x0f38, 0xf0): /* movbe m,r */
     case X86EMUL_OPC(0x0f38, 0xf1): /* movbe r,m */
-        vcpu_must_have_movbe();
+        vcpu_must_have(movbe);
         switch ( op_bytes )
         {
         case 2:

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

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

Re: [PATCH 1/2] x86emul: derive vcpu_must_have() from vcpu_has()

[Andrew Cooper]

On 09/12/16 11:53, Jan Beulich wrote:
> ... to avoid introducing further redundancy when adding further feature
> flag checks, and to bring its use better in line with its host_and_*()
> sibling.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

[PATCH 2/2] x86: drop cpu_has_sse{,2}

[Jan Beulich]

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

Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
added them - these features are always available on 64-bit CPUs. (Let's
not assume this for MMX though in at least the insn emulator.)

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
@@ -4691,9 +4691,9 @@ x86_emulate(
         if ( vex.opcx == vex_none )
         {
             if ( vex.pfx & VEX_PREFIX_DOUBLE_MASK )
-                host_and_vcpu_must_have(sse2);
+                vcpu_must_have(sse2);
             else
-                host_and_vcpu_must_have(sse);
+                vcpu_must_have(sse);
             ea.bytes = 16;
             SET_SSE_PREFIX(buf[0], vex.pfx);
             get_fpu(X86EMUL_FPU_xmm, &fic);
@@ -4957,7 +4957,7 @@ x86_emulate(
             {
             case vex_66:
             case vex_f3:
-                host_and_vcpu_must_have(sse2);
+                vcpu_must_have(sse2);
                 /* Converting movdqu to movdqa here: Our buffer is aligned. */
                 buf[0] = 0x66;
                 get_fpu(X86EMUL_FPU_xmm, &fic);
@@ -4967,7 +4967,7 @@ x86_emulate(
                 if ( b != 0xe7 )
                     host_and_vcpu_must_have(mmx);
                 else
-                    host_and_vcpu_must_have(sse);
+                    vcpu_must_have(sse);
                 get_fpu(X86EMUL_FPU_mmx, &fic);
                 ea.bytes = 8;
                 break;
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -59,8 +59,6 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPIN
 #define cpu_has_sep		boot_cpu_has(X86_FEATURE_SEP)
 #define cpu_has_mtrr		1
 #define cpu_has_mmx		1
-#define cpu_has_sse		boot_cpu_has(X86_FEATURE_SSE)
-#define cpu_has_sse2		boot_cpu_has(X86_FEATURE_SSE2)
 #define cpu_has_sse3		boot_cpu_has(X86_FEATURE_SSE3)
 #define cpu_has_sse4_2		boot_cpu_has(X86_FEATURE_SSE4_2)
 #define cpu_has_htt		boot_cpu_has(X86_FEATURE_HTT)




[-- Attachment #2: x86-SSE-SSE2-always.patch --]
[-- Type: text/plain, Size: 2062 bytes --]

x86: drop cpu_has_sse{,2}

Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
added them - these features are always available on 64-bit CPUs. (Let's
not assume this for MMX though in at least the insn emulator.)

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
@@ -4691,9 +4691,9 @@ x86_emulate(
         if ( vex.opcx == vex_none )
         {
             if ( vex.pfx & VEX_PREFIX_DOUBLE_MASK )
-                host_and_vcpu_must_have(sse2);
+                vcpu_must_have(sse2);
             else
-                host_and_vcpu_must_have(sse);
+                vcpu_must_have(sse);
             ea.bytes = 16;
             SET_SSE_PREFIX(buf[0], vex.pfx);
             get_fpu(X86EMUL_FPU_xmm, &fic);
@@ -4957,7 +4957,7 @@ x86_emulate(
             {
             case vex_66:
             case vex_f3:
-                host_and_vcpu_must_have(sse2);
+                vcpu_must_have(sse2);
                 /* Converting movdqu to movdqa here: Our buffer is aligned. */
                 buf[0] = 0x66;
                 get_fpu(X86EMUL_FPU_xmm, &fic);
@@ -4967,7 +4967,7 @@ x86_emulate(
                 if ( b != 0xe7 )
                     host_and_vcpu_must_have(mmx);
                 else
-                    host_and_vcpu_must_have(sse);
+                    vcpu_must_have(sse);
                 get_fpu(X86EMUL_FPU_mmx, &fic);
                 ea.bytes = 8;
                 break;
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -59,8 +59,6 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPIN
 #define cpu_has_sep		boot_cpu_has(X86_FEATURE_SEP)
 #define cpu_has_mtrr		1
 #define cpu_has_mmx		1
-#define cpu_has_sse		boot_cpu_has(X86_FEATURE_SSE)
-#define cpu_has_sse2		boot_cpu_has(X86_FEATURE_SSE2)
 #define cpu_has_sse3		boot_cpu_has(X86_FEATURE_SSE3)
 #define cpu_has_sse4_2		boot_cpu_has(X86_FEATURE_SSE4_2)
 #define cpu_has_htt		boot_cpu_has(X86_FEATURE_HTT)

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

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

Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Andrew Cooper]

On 09/12/16 11:54, Jan Beulich wrote:
> Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
> added them - these features are always available on 64-bit CPUs. (Let's
> not assume this for MMX though in at least the insn emulator.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This isn't necessarily true when compiled for 32bit in the userspace
harness.

What about just defining #define cpu_has_sse{,2} as a literal 1?

~Andrew

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

Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Jan Beulich]

>>> On 09.12.16 at 14:00, <andrew.cooper3@citrix.com> wrote:
> On 09/12/16 11:54, Jan Beulich wrote:
>> Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
>> added them - these features are always available on 64-bit CPUs. (Let's
>> not assume this for MMX though in at least the insn emulator.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This isn't necessarily true when compiled for 32bit in the userspace
> harness.

In the test harness vcpu_has_* == cpu_has_*, as also
demonstrated by

#define host_and_vcpu_must_have(feat) vcpu_must_have(feat)

. The change (as its title is trying to say) really only affects the
hypervisor.

Jan


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

Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Andrew Cooper]

On 09/12/16 13:28, Jan Beulich wrote:
>>>> On 09.12.16 at 14:00, <andrew.cooper3@citrix.com> wrote:
>> On 09/12/16 11:54, Jan Beulich wrote:
>>> Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
>>> added them - these features are always available on 64-bit CPUs. (Let's
>>> not assume this for MMX though in at least the insn emulator.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> This isn't necessarily true when compiled for 32bit in the userspace
>> harness.
> In the test harness vcpu_has_* == cpu_has_*, as also
> demonstrated by
>
> #define host_and_vcpu_must_have(feat) vcpu_must_have(feat)
>
> . The change (as its title is trying to say) really only affects the
> hypervisor.

Right, but this change is still contrary to the written requirement.

/*                                                                                                                                                                                      

 * Note the difference between vcpu_must_have_<feature>()
and                                                                                                                           

 * host_and_vcpu_must_have(<feature>): The latter needs to be used
when                                                                                                                 

 * emulation code is using the same instruction class for carrying
out                                                                                                                  

 * the actual
operation.                                                                                                                                                                

 */

We are using SSE and SSE2 instructions for carrying out that emulation,
so should still be using the host_and_vcpu check.

Swapping the hypervisor defines to being 1 will cause the
generate_exception_if() clause to become dead and get dropped, turning
host_and_vcpu_must_have() into just vcpu_must_have_##feat

~Andrew

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

Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Jan Beulich]

>>> On 09.12.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
> On 09/12/16 13:28, Jan Beulich wrote:
>>>>> On 09.12.16 at 14:00, <andrew.cooper3@citrix.com> wrote:
>>> On 09/12/16 11:54, Jan Beulich wrote:
>>>> Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
>>>> added them - these features are always available on 64-bit CPUs. (Let's
>>>> not assume this for MMX though in at least the insn emulator.)
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> This isn't necessarily true when compiled for 32bit in the userspace
>>> harness.
>> In the test harness vcpu_has_* == cpu_has_*, as also
>> demonstrated by
>>
>> #define host_and_vcpu_must_have(feat) vcpu_must_have(feat)
>>
>> . The change (as its title is trying to say) really only affects the
>> hypervisor.
> 
> Right, but this change is still contrary to the written requirement.

I don't understand.

> /*                                                                           
>  * Note the difference between vcpu_must_have_<feature>() and                                                                          
>  * host_and_vcpu_must_have(<feature>): The latter needs to be used when                                                                         
>  * emulation code is using the same instruction class for carrying out                                                                          
>  * the actual operation.                                                                   
>  */

This comment sits inside #ifdef __XEN__.

> We are using SSE and SSE2 instructions for carrying out that emulation,
> so should still be using the host_and_vcpu check.

Xen only running on x86-64, and x86-64 taking SSE and SSE2 as
given (all math operations other than 80-bit ones are using these
instead of the x87), there's no need to look at any flags here
(and as you can see from the patch, the flags also already haven't
been used anywhere outside the insn emulator, despite us using
SSE2 insns, e.g. MOVNTI in the page copying and clearing code).

> Swapping the hypervisor defines to being 1 will cause the
> generate_exception_if() clause to become dead and get dropped, turning
> host_and_vcpu_must_have() into just vcpu_must_have_##feat

Which is fine for the hypervisor. And for the test harness the
vcpu_must_have() is sufficient, as explained before.

Jan


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

Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Andrew Cooper]

On 09/12/16 14:33, Jan Beulich wrote:
>>>> On 09.12.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
>> On 09/12/16 13:28, Jan Beulich wrote:
>>>>>> On 09.12.16 at 14:00, <andrew.cooper3@citrix.com> wrote:
>>>> On 09/12/16 11:54, Jan Beulich wrote:
>>>>> Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
>>>>> added them - these features are always available on 64-bit CPUs. (Let's
>>>>> not assume this for MMX though in at least the insn emulator.)
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> This isn't necessarily true when compiled for 32bit in the userspace
>>>> harness.
>>> In the test harness vcpu_has_* == cpu_has_*, as also
>>> demonstrated by
>>>
>>> #define host_and_vcpu_must_have(feat) vcpu_must_have(feat)
>>>
>>> . The change (as its title is trying to say) really only affects the
>>> hypervisor.
>> Right, but this change is still contrary to the written requirement.
> I don't understand.
>
>> /*                                                                           
>>  * Note the difference between vcpu_must_have_<feature>() and                                                                          
>>  * host_and_vcpu_must_have(<feature>): The latter needs to be used when                                                                         
>>  * emulation code is using the same instruction class for carrying out                                                                          
>>  * the actual operation.                                                                   
>>  */
> This comment sits inside #ifdef __XEN__.

Yes, which means it applies to code running in the hypervisor.

>
>> We are using SSE and SSE2 instructions for carrying out that emulation,
>> so should still be using the host_and_vcpu check.
> Xen only running on x86-64, and x86-64 taking SSE and SSE2 as
> given (all math operations other than 80-bit ones are using these
> instead of the x87), there's no need to look at any flags here
> (and as you can see from the patch, the flags also already haven't
> been used anywhere outside the insn emulator, despite us using
> SSE2 insns, e.g. MOVNTI in the page copying and clearing code).

I agree that 64bit code may take SSE and SSE2 as a given, which is why I
have suggested this:

diff --git a/xen/include/asm-x86/cpufeature.h
b/xen/include/asm-x86/cpufeature.h
index c7c8520..c62dacd 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -59,8 +59,8 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPINTS+0)*32+ 11)
/* SMAP gets used by Xen i
 #define cpu_has_sep            boot_cpu_has(X86_FEATURE_SEP)
 #define cpu_has_mtrr           1
 #define cpu_has_mmx            1
-#define cpu_has_sse            boot_cpu_has(X86_FEATURE_SSE)
-#define cpu_has_sse2           boot_cpu_has(X86_FEATURE_SSE2)
+#define cpu_has_sse            1
+#define cpu_has_sse2           1
 #define cpu_has_sse3           boot_cpu_has(X86_FEATURE_SSE3)
 #define cpu_has_sse4_2         boot_cpu_has(X86_FEATURE_SSE4_2)
 #define cpu_has_htt            boot_cpu_has(X86_FEATURE_HTT)

This is fine, as we only support 64bit in the hypervisor.

>> Swapping the hypervisor defines to being 1 will cause the
>> generate_exception_if() clause to become dead and get dropped, turning
>> host_and_vcpu_must_have() into just vcpu_must_have_##feat
> Which is fine for the hypervisor. And for the test harness the
> vcpu_must_have() is sufficient, as explained before.

It is a layering violation to remove the host_has_* part of the check
from the emulator.

The purpose of the host_has_* part of the check is to be present when we
are using instructions from a certain instruction class, and we are
still using SSE/SSE2 instructions in the emulator.  Untill we drop the
32bit build of x86_emulate.c, it is wrong to drop the SSE/SSE2 check.

With the above diff, host_and_vcpu_must_have(SSE) will be simplified to
vcpu_must_have(SSE) by the compiler, which achieves your goal of
removing a tautological check.

~Andrew

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

Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Jan Beulich]

>>> On 09.12.16 at 15:56, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -59,8 +59,8 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPINTS+0)*32+ 11)
> /* SMAP gets used by Xen i
>  #define cpu_has_sep            boot_cpu_has(X86_FEATURE_SEP)
>  #define cpu_has_mtrr           1
>  #define cpu_has_mmx            1
> -#define cpu_has_sse            boot_cpu_has(X86_FEATURE_SSE)
> -#define cpu_has_sse2           boot_cpu_has(X86_FEATURE_SSE2)
> +#define cpu_has_sse            1
> +#define cpu_has_sse2           1

Such defines are a complete waste of space and compiler parsing
bandwidth. I'd really prefer not having to introduce them; I did
propose in the past to make e.g. cpu_has_fpu and cpu_has_mmx
actual checks again, as there certainly are signs of either or both
not having a permanent place in the ISA. Stuff like cpu_has_mtrr
would imo also better either go away (if we mean to tie ourselves
to present MTRRs) or be an actual check.

>>> Swapping the hypervisor defines to being 1 will cause the
>>> generate_exception_if() clause to become dead and get dropped, turning
>>> host_and_vcpu_must_have() into just vcpu_must_have_##feat
>> Which is fine for the hypervisor. And for the test harness the
>> vcpu_must_have() is sufficient, as explained before.
> 
> It is a layering violation to remove the host_has_* part of the check
> from the emulator.

This is from a very, very abstract perspective. The fact that
host_and_vcpu_*() and vcpu_*() are the same in the harness
should allow for them to be used interchangeably when only the
harness is affected.

Jan


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

Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Jan Beulich]

>>> On 09.12.16 at 16:19, <JBeulich@suse.com> wrote:
>>>> On 09.12.16 at 15:56, <andrew.cooper3@citrix.com> wrote:
>> It is a layering violation to remove the host_has_* part of the check
>> from the emulator.
> 
> This is from a very, very abstract perspective. The fact that
> host_and_vcpu_*() and vcpu_*() are the same in the harness
> should allow for them to be used interchangeably when only the
> harness is affected.

I should add that in a subsequent patch adding emulation of
LDDQU (an SSE3 insn) this is going to be done by slightly
extending the existing MOVDQ{U,A} etc block, i.e. using an
SSE2 insn. If I was to follow your directions, I'd have to add
separate host_has(sse2) and vcpu_has(sse3) checks. That's
just going to become insane. I have just the vcpu check in
there, and I really hope you won't force me to uglify that.

Jan


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

Ping: Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Jan Beulich]

>>> On 09.12.16 at 16:27, <JBeulich@suse.com> wrote:
>>>> On 09.12.16 at 16:19, <JBeulich@suse.com> wrote:
>>>>> On 09.12.16 at 15:56, <andrew.cooper3@citrix.com> wrote:
>>> It is a layering violation to remove the host_has_* part of the check
>>> from the emulator.
>> 
>> This is from a very, very abstract perspective. The fact that
>> host_and_vcpu_*() and vcpu_*() are the same in the harness
>> should allow for them to be used interchangeably when only the
>> harness is affected.
> 
> I should add that in a subsequent patch adding emulation of
> LDDQU (an SSE3 insn) this is going to be done by slightly
> extending the existing MOVDQ{U,A} etc block, i.e. using an
> SSE2 insn. If I was to follow your directions, I'd have to add
> separate host_has(sse2) and vcpu_has(sse3) checks. That's
> just going to become insane. I have just the vcpu check in
> there, and I really hope you won't force me to uglify that.

And one more which did stall. Meanwhile I've submitted patches
falling into the above category, which should help making a
judgment here.

Jan


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

Re: Ping: Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Andrew Cooper]

On 15/12/16 09:57, Jan Beulich wrote:
>>>> On 09.12.16 at 16:27, <JBeulich@suse.com> wrote:
>>>>> On 09.12.16 at 16:19, <JBeulich@suse.com> wrote:
>>>>>> On 09.12.16 at 15:56, <andrew.cooper3@citrix.com> wrote:
>>>> It is a layering violation to remove the host_has_* part of the check
>>>> from the emulator.
>>> This is from a very, very abstract perspective. The fact that
>>> host_and_vcpu_*() and vcpu_*() are the same in the harness
>>> should allow for them to be used interchangeably when only the
>>> harness is affected.
>> I should add that in a subsequent patch adding emulation of
>> LDDQU (an SSE3 insn) this is going to be done by slightly
>> extending the existing MOVDQ{U,A} etc block, i.e. using an
>> SSE2 insn. If I was to follow your directions, I'd have to add
>> separate host_has(sse2) and vcpu_has(sse3) checks. That's
>> just going to become insane. I have just the vcpu check in
>> there, and I really hope you won't force me to uglify that.
> And one more which did stall. Meanwhile I've submitted patches
> falling into the above category, which should help making a
> judgment here.

I am sorry, but I remain unconvinced by these arguments.

The rule governing the use host_and_vcpu_*() is a) correct, and b) very
clear when it comes to using instructions from certain classes as part
of emulation.

In particular, a 32bit build of the emulator should not crash even if
there is junk in the cpuid policy.

This means either, we drop support for 32bit builds of the emulator, or
retain the proper host_and_vcpu_*() checks.


An alternative which comes to mind and hasn't been discussed yet, would
be to formally require a 64bit cpu as a minimum requirement, even for
32bit builds.  In fact, I think we should have rather more written
documentation anyway (possibly at the head of x86_emulate.c), including
the instruction groups and behaviours we expect to be implemented, and
which we expect to currently be missing.

If we had a formal statement of requiring a 64bit-capable CPU for
running the emulator, I'd be happy to relax the checks based on the
assumptions we are permitted to make.

~Andrew

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

Re: Ping: Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Jan Beulich]

>>> On 15.12.16 at 13:03, <andrew.cooper3@citrix.com> wrote:
> On 15/12/16 09:57, Jan Beulich wrote:
>>>>> On 09.12.16 at 16:27, <JBeulich@suse.com> wrote:
>>>>>> On 09.12.16 at 16:19, <JBeulich@suse.com> wrote:
>>>>>>> On 09.12.16 at 15:56, <andrew.cooper3@citrix.com> wrote:
>>>>> It is a layering violation to remove the host_has_* part of the check
>>>>> from the emulator.
>>>> This is from a very, very abstract perspective. The fact that
>>>> host_and_vcpu_*() and vcpu_*() are the same in the harness
>>>> should allow for them to be used interchangeably when only the
>>>> harness is affected.
>>> I should add that in a subsequent patch adding emulation of
>>> LDDQU (an SSE3 insn) this is going to be done by slightly
>>> extending the existing MOVDQ{U,A} etc block, i.e. using an
>>> SSE2 insn. If I was to follow your directions, I'd have to add
>>> separate host_has(sse2) and vcpu_has(sse3) checks. That's
>>> just going to become insane. I have just the vcpu check in
>>> there, and I really hope you won't force me to uglify that.
>> And one more which did stall. Meanwhile I've submitted patches
>> falling into the above category, which should help making a
>> judgment here.
> 
> I am sorry, but I remain unconvinced by these arguments.
> 
> The rule governing the use host_and_vcpu_*() is a) correct, and b) very
> clear when it comes to using instructions from certain classes as part
> of emulation.
> 
> In particular, a 32bit build of the emulator should not crash even if
> there is junk in the cpuid policy.

And it doesn't. Once again - the comment you refer to is relevant
for the hypervisor only (which its placement tries to make obvious).
For the harness, the host and vcpu checks are equivalent.

Did you look at some of those newer patches, to estimate how odd it
would be to add separate vcpu_must_have() and host_must_have()
checks? Given the (growing) size of the part of the code base, I think
we should always strive for as concise code as possible - no
unnecessary clutter or anything. Much of my recent code folding was
motivated by that, and I appreciate your recent recognition of further
opportunities in that area.

> This means either, we drop support for 32bit builds of the emulator, or
> retain the proper host_and_vcpu_*() checks.
> 
> 
> An alternative which comes to mind and hasn't been discussed yet, would
> be to formally require a 64bit cpu as a minimum requirement, even for
> 32bit builds.  In fact, I think we should have rather more written
> documentation anyway (possibly at the head of x86_emulate.c), including
> the instruction groups and behaviours we expect to be implemented, and
> which we expect to currently be missing.
> 
> If we had a formal statement of requiring a 64bit-capable CPU for
> running the emulator, I'd be happy to relax the checks based on the
> assumptions we are permitted to make.

Well, if I really can't convince you, I wouldn't see any issue with
documenting such a requirement (albeit it's not really clear where
that would best go, as putting it at the top of some of the files'
would imo make it likely to be overlooked by anyone not specifically
searching for any such statement).

Jan


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