[PATCH v3 0/4] x86/cpuid: enable new cpu features

[PATCH v3 0/4] x86/cpuid: enable new cpu features

[Yang Zhong]

The new cpu features in intel icelake: AVX512VBMI2/GFNI/VAES/
AVX512VNNI/AVX512BITALG/VPCLMULQDQ.


v2: adjust the patches sequence from Jan
v1: need implement x86 emulation for Legacy and VEX insns,
    EVEX insns in next time suggested by Jan


Yang Zhong (4):
  x86emul: Support GFNI insns
  x86emul: Support vpclmulqdq
  x86emul: Support vaes insns
  x86/cpuid: Enable new SSE/AVX/AVX512 cpu features

 docs/man/xl.cfg.pod.5.in                    |  3 ++-
 tools/libxl/libxl_cpuid.c                   |  6 +++++
 tools/misc/xen-cpuid.c                      | 13 +++++-----
 xen/arch/x86/x86_emulate/x86_emulate.c      | 37 ++++++++++++++++++++++++++++-
 xen/include/asm-x86/cpufeature.h            |  5 ++++
 xen/include/public/arch-x86/cpufeatureset.h |  6 +++++
 xen/tools/gen-cpuid.py                      |  3 ++-
 7 files changed, 64 insertions(+), 9 deletions(-)

-- 
1.9.1


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

[PATCH v3 1/4] x86emul: Support GFNI insns

[Yang Zhong]

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 22 ++++++++++++++++++++++
 xen/include/asm-x86/cpufeature.h       |  3 +++
 2 files changed, 25 insertions(+)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 54a2756..c207f61 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -385,6 +385,7 @@ static const struct {
     [0x40] = { .simd_size = simd_packed_int },
     [0x41] = { .simd_size = simd_packed_int, .two_op = 1 },
     [0xc8 ... 0xcd] = { .simd_size = simd_other },
+    [0xcf] = { .simd_size = simd_other },
     [0xdb] = { .simd_size = simd_packed_int, .two_op = 1 },
     [0xdc ... 0xdf] = { .simd_size = simd_packed_int },
     [0xf0] = { .two_op = 1 },
@@ -421,6 +422,7 @@ static const struct {
     [0x4c] = { .simd_size = simd_packed_int, .four_op = 1 },
     [0x60 ... 0x63] = { .simd_size = simd_packed_int, .two_op = 1 },
     [0xcc] = { .simd_size = simd_other },
+    [0xce ... 0xcf] = { .simd_size = simd_other },
     [0xdf] = { .simd_size = simd_packed_int, .two_op = 1 },
     [0xf0] = {},
 };
@@ -1623,6 +1625,7 @@ static bool vcpu_has(
 #define vcpu_has_clflushopt()  vcpu_has(         7, EBX, 23, ctxt, ops)
 #define vcpu_has_clwb()        vcpu_has(         7, EBX, 24, ctxt, ops)
 #define vcpu_has_sha()         vcpu_has(         7, EBX, 29, ctxt, ops)
+#define vcpu_has_gfni()        vcpu_has(         7, ECX,  8, ctxt, ops)
 #define vcpu_has_rdpid()       vcpu_has(         7, ECX, 22, ctxt, ops)
 #define vcpu_has_clzero()      vcpu_has(0x80000008, EBX,  0, ctxt, ops)
 
@@ -7356,6 +7359,14 @@ x86_emulate(
         op_bytes = 16;
         goto simd_0f38_common;
 
+    case X86EMUL_OPC_66(0x0f38, 0xcf):       /* gf2p8mulb xmm/m128,xmm*/
+    case X86EMUL_OPC_VEX_66(0x0f38, 0xcf):   /* vgf2p8mulb xmm/m128,xmm*/
+        host_and_vcpu_must_have(gfni);
+        if ( vex.opcx == vex_none )
+            goto simd_0f38_common;
+        else
+            goto simd_0f_avx;
+
     case X86EMUL_OPC(0x0f38, 0xf0): /* movbe m,r */
     case X86EMUL_OPC(0x0f38, 0xf1): /* movbe r,m */
         vcpu_must_have(movbe);
@@ -7741,6 +7752,17 @@ x86_emulate(
         op_bytes = 16;
         goto simd_0f3a_common;
 
+    case X86EMUL_OPC_66(0x0f3a, 0xce):     /* gf2p8affineqb $imm8,xmm/m128,xmm*/
+    case X86EMUL_OPC_VEX_66(0x0f3a, 0xce): /* vgf2p8affineqb $imm8,xmm/m128,xmm*/
+    case X86EMUL_OPC_66(0x0f3a, 0xcf):     /* gf2p8affineinvqb $imm8,xmm/m128,xmm*/
+    case X86EMUL_OPC_VEX_66(0x0f3a, 0xcf): /* vgf2p8affineinvqb $imm8,xmm/m128,xmm*/
+        host_and_vcpu_must_have(gfni);
+        if ( vex.opcx == vex_none )
+            goto simd_0f3a_common;
+        else
+            goto simd_0f_imm8_avx;
+
+
     case X86EMUL_OPC_66(0x0f3a, 0xdf):     /* aeskeygenassist $imm8,xmm/m128,xmm */
     case X86EMUL_OPC_VEX_66(0x0f3a, 0xdf): /* vaeskeygenassist $imm8,xmm/m128,xmm */
         host_and_vcpu_must_have(aesni);
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 84cc51d..9c43cd8 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -97,6 +97,9 @@
 #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
 #define cpu_has_sha             boot_cpu_has(X86_FEATURE_SHA)
 
+/* CPUID level 0x00000007:0.ecx */
+#define cpu_has_gfni            boot_cpu_has(X86_FEATURE_GFNI)
+
 /* CPUID level 0x80000007.edx */
 #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
 
-- 
1.9.1


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

[PATCH v3 2/4] x86emul: Support vpclmulqdq

[Yang Zhong]

The previous vpclmulqdq only support AVX128.
Icelake added AVX256 support.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++++++-
 xen/include/asm-x86/cpufeature.h       | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index c207f61..559b387 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1626,6 +1626,7 @@ static bool vcpu_has(
 #define vcpu_has_clwb()        vcpu_has(         7, EBX, 24, ctxt, ops)
 #define vcpu_has_sha()         vcpu_has(         7, EBX, 29, ctxt, ops)
 #define vcpu_has_gfni()        vcpu_has(         7, ECX,  8, ctxt, ops)
+#define vcpu_has_vpclmulqdq()  vcpu_has(         7, ECX, 10, ctxt, ops)
 #define vcpu_has_rdpid()       vcpu_has(         7, ECX, 22, ctxt, ops)
 #define vcpu_has_clzero()      vcpu_has(0x80000008, EBX,  0, ctxt, ops)
 
@@ -7672,7 +7673,12 @@ x86_emulate(
         host_and_vcpu_must_have(pclmulqdq);
         if ( vex.opcx == vex_none )
             goto simd_0f3a_common;
-        generate_exception_if(vex.l, EXC_UD);
+        if ( !vex.l )
+        {
+            generate_exception_if(vex.l, EXC_UD);
+            goto simd_0f_imm8_avx;
+        }
+        host_and_vcpu_must_have(vpclmulqdq);
         goto simd_0f_imm8_avx;
 
     case X86EMUL_OPC_VEX_66(0x0f3a, 0x4a): /* vblendvps {x,y}mm,{x,y}mm/mem,{x,y}mm,{x,y}mm */
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 9c43cd8..3f24f06 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -99,6 +99,7 @@
 
 /* CPUID level 0x00000007:0.ecx */
 #define cpu_has_gfni            boot_cpu_has(X86_FEATURE_GFNI)
+#define cpu_has_vpclmulqdq      boot_cpu_has(X86_FEATURE_VPCLMULQDQ)
 
 /* CPUID level 0x80000007.edx */
 #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
-- 
1.9.1


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

[PATCH v3 3/4] x86emul: Support vaes insns

[Yang Zhong]

The previous aes insns only support legacy and AVX128.
Icelake added AVX256 support.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 7 +++++++
 xen/include/asm-x86/cpufeature.h       | 1 +
 2 files changed, 8 insertions(+)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 559b387..97cdf03 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1626,6 +1626,7 @@ static bool vcpu_has(
 #define vcpu_has_clwb()        vcpu_has(         7, EBX, 24, ctxt, ops)
 #define vcpu_has_sha()         vcpu_has(         7, EBX, 29, ctxt, ops)
 #define vcpu_has_gfni()        vcpu_has(         7, ECX,  8, ctxt, ops)
+#define vcpu_has_vaes()        vcpu_has(         7, ECX,  9, ctxt, ops)
 #define vcpu_has_vpclmulqdq()  vcpu_has(         7, ECX, 10, ctxt, ops)
 #define vcpu_has_rdpid()       vcpu_has(         7, ECX, 22, ctxt, ops)
 #define vcpu_has_clzero()      vcpu_has(0x80000008, EBX,  0, ctxt, ops)
@@ -7345,6 +7346,12 @@ x86_emulate(
         host_and_vcpu_must_have(aesni);
         if ( vex.opcx == vex_none )
             goto simd_0f38_common;
+        if ( vex.l )
+        {
+            host_and_vcpu_must_have(vaes);
+            goto simd_0f_avx;
+        }
+
         /* fall through */
     case X86EMUL_OPC_VEX_66(0x0f38, 0x41): /* vphminposuw xmm/m128,xmm,xmm */
         generate_exception_if(vex.l, EXC_UD);
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 3f24f06..ffa110e 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -99,6 +99,7 @@
 
 /* CPUID level 0x00000007:0.ecx */
 #define cpu_has_gfni            boot_cpu_has(X86_FEATURE_GFNI)
+#define cpu_has_vaes            boot_cpu_has(X86_FEATURE_VAES)
 #define cpu_has_vpclmulqdq      boot_cpu_has(X86_FEATURE_VPCLMULQDQ)
 
 /* CPUID level 0x80000007.edx */
-- 
1.9.1


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

[PATCH v3 4/4] x86/cpuid: Enable new SSE/AVX/AVX512 cpu features

[Yang Zhong]

Intel IceLake cpu has added new cpu features: AVX512VBMI2/GFNI/
VAES/AVX512VNNI/AVX512BITALG/VPCLMULQDQ. Those new cpu features
need expose to guest.

The bit definition:
CPUID.(EAX=7,ECX=0):ECX[bit 06] AVX512VBMI2
CPUID.(EAX=7,ECX=0):ECX[bit 08] GFNI
CPUID.(EAX=7,ECX=0):ECX[bit 09] VAES
CPUID.(EAX=7,ECX=0):ECX[bit 10] VPCLMULQDQ
CPUID.(EAX=7,ECX=0):ECX[bit 11] AVX512VNNI
CPUID.(EAX=7,ECX=0):ECX[bit 12] AVX512_BITALG

The release document ref below link:
https://software.intel.com/sites/default/files/managed/c5/15/\
architecture-instruction-set-extensions-programming-reference.pdf

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 docs/man/xl.cfg.pod.5.in                    |  3 ++-
 tools/libxl/libxl_cpuid.c                   |  6 ++++++
 tools/misc/xen-cpuid.c                      | 13 +++++++------
 xen/include/public/arch-x86/cpufeatureset.h |  6 ++++++
 xen/tools/gen-cpuid.py                      |  3 ++-
 5 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index b7b91d8..d056768 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1731,7 +1731,8 @@ perfctr_core perfctr_nb pge pku popcnt pse pse36 psn rdrand rdseed rdtscp rtm
 sha skinit smap smep smx ss sse sse2 sse3 sse4.1 sse4.2 sse4_1 sse4_2 sse4a
 ssse3 svm svm_decode svm_lbrv svm_npt svm_nrips svm_pausefilt svm_tscrate
 svm_vmcbclean syscall sysenter tbm tm tm2 topoext tsc tsc-deadline tsc_adjust
-umip vme vmx wdt x2apic xop xsave xtpr
+umip vme vmx wdt x2apic xop xsave xtpr avx512_vbmi2 gfni vaes vpclmulqdq
+avx512_vnni avx512_bitalg
 
 
 The xend syntax is a list of values in the form of
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index e692b61..614991f 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -199,6 +199,12 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
         {"umip",         0x00000007,  0, CPUID_REG_ECX,  2,  1},
         {"pku",          0x00000007,  0, CPUID_REG_ECX,  3,  1},
         {"ospke",        0x00000007,  0, CPUID_REG_ECX,  4,  1},
+        {"avx512_vbmi2", 0x00000007,  0, CPUID_REG_ECX,  6,  1},
+        {"gfni",         0x00000007,  0, CPUID_REG_ECX,  8,  1},
+        {"vaes",         0x00000007,  0, CPUID_REG_ECX,  9,  1},
+        {"vpclmulqdq",   0x00000007,  0, CPUID_REG_ECX, 10,  1},
+        {"avx512_vnni",  0x00000007,  0, CPUID_REG_ECX, 11,  1},
+        {"avx512_bitalg",0x00000007,  0, CPUID_REG_ECX, 12,  1},
 
         {"avx512-4vnniw",0x00000007,  0, CPUID_REG_EDX,  2,  1},
         {"avx512-4fmaps",0x00000007,  0, CPUID_REG_EDX,  3,  1},
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 106be0f..985deea 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -120,12 +120,13 @@ static const char *str_Da1[32] =
 
 static const char *str_7c0[32] =
 {
-    [ 0] = "prechwt1", [ 1] = "avx512vbmi",
-    [ 2] = "REZ",      [ 3] = "pku",
-    [ 4] = "ospke",
-
-    [5 ... 13] = "REZ",
-
+    [ 0] = "prechwt1",     [ 1] = "avx512vbmi",
+    [ 2] = "REZ",          [ 3] = "pku",
+    [ 4] = "ospke",        [ 5] = "REZ",
+    [ 6] = "avx512_vbmi2", [ 7] = "REZ",
+    [ 8] = "gfni",         [ 9] = "vaes",
+    [10] = "vpclmulqdq",   [11] = "avx512_vnni",
+    [12] = "avx512_bitalg",[13] = "REZ",
     [14] = "avx512_vpopcntdq",
 
     [15 ... 31] = "REZ",
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 0ee3ea3..bb24b79 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -228,6 +228,12 @@ XEN_CPUFEATURE(AVX512VBMI,    6*32+ 1) /*A  AVX-512 Vector Byte Manipulation Ins
 XEN_CPUFEATURE(UMIP,          6*32+ 2) /*S  User Mode Instruction Prevention */
 XEN_CPUFEATURE(PKU,           6*32+ 3) /*H  Protection Keys for Userspace */
 XEN_CPUFEATURE(OSPKE,         6*32+ 4) /*!  OS Protection Keys Enable */
+XEN_CPUFEATURE(AVX512_VBMI2,  6*32+ 6) /*A  addition AVX-512 VBMI Instructions */
+XEN_CPUFEATURE(GFNI,          6*32+ 8) /*A  Galois Field New Instructions */
+XEN_CPUFEATURE(VAES,          6*32+ 9) /*A  Vector AES instructions */
+XEN_CPUFEATURE(VPCLMULQDQ,    6*32+ 10) /*A  vector PCLMULQDQ instructions */
+XEN_CPUFEATURE(AVX512_VNNI,   6*32+ 11) /*A  Vector Neural Network Instructions */
+XEN_CPUFEATURE(AVX512_BITALG, 6*32+ 12) /*A  support for VPOPCNT[B,W] and VPSHUFBITQMB*/
 XEN_CPUFEATURE(AVX512_VPOPCNTDQ, 6*32+14) /*A  POPCNT for vectors of DW/QW */
 XEN_CPUFEATURE(RDPID,         6*32+22) /*A  RDPID instruction */
 
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 9ec4486..be8df48 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -255,7 +255,8 @@ def crunch_numbers(state):
         # top of AVX512F
         AVX512F: [AVX512DQ, AVX512IFMA, AVX512PF, AVX512ER, AVX512CD,
                   AVX512BW, AVX512VL, AVX512VBMI, AVX512_4VNNIW,
-                  AVX512_4FMAPS, AVX512_VPOPCNTDQ],
+                  AVX512_4FMAPS, AVX512_VPOPCNTDQ, AVX512_VBMI2,
+                  AVX512_VNNI, AVX512_BITALG],
     }
 
     deep_features = tuple(sorted(deps.keys()))
-- 
1.9.1


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

Re: [PATCH v3 0/4] x86/cpuid: enable new cpu features

[Jan Beulich]

>>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> The new cpu features in intel icelake: AVX512VBMI2/GFNI/VAES/
> AVX512VNNI/AVX512BITALG/VPCLMULQDQ.
> 
> 
> v2: adjust the patches sequence from Jan

I'm sorry, but please be a little more patient with sending new versions.
Allow for at least a couple of days, preferably a week, for other
comments to be made. This is the more that your series can't go in
right away anyway, as the tree is frozen.

Jan


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

Re: [PATCH v3 4/4] x86/cpuid: Enable new SSE/AVX/AVX512 cpu features

[Jan Beulich]

>>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> Intel IceLake cpu has added new cpu features: AVX512VBMI2/GFNI/
> VAES/AVX512VNNI/AVX512BITALG/VPCLMULQDQ. Those new cpu features
> need expose to guest.
> 
> The bit definition:
> CPUID.(EAX=7,ECX=0):ECX[bit 06] AVX512VBMI2
> CPUID.(EAX=7,ECX=0):ECX[bit 08] GFNI
> CPUID.(EAX=7,ECX=0):ECX[bit 09] VAES
> CPUID.(EAX=7,ECX=0):ECX[bit 10] VPCLMULQDQ
> CPUID.(EAX=7,ECX=0):ECX[bit 11] AVX512VNNI
> CPUID.(EAX=7,ECX=0):ECX[bit 12] AVX512_BITALG
> 
> The release document ref below link:
> https://software.intel.com/sites/default/files/managed/c5/15/\ 
> architecture-instruction-set-extensions-programming-reference.pdf
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>

Non-toolstack parts
Acked-by: Jan Beulich <jbeulich@suse.com>
(which you could have picked up from v2 if you hadn't been rushing v3)

Jan


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

Re: [PATCH v3 1/4] x86emul: Support GFNI insns

[Jan Beulich]

>>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>

First and foremost - did you try out your own patch? There not being
any (minimal) test added makes this at least questionable.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -385,6 +385,7 @@ static const struct {
>      [0x40] = { .simd_size = simd_packed_int },
>      [0x41] = { .simd_size = simd_packed_int, .two_op = 1 },
>      [0xc8 ... 0xcd] = { .simd_size = simd_other },
> +    [0xcf] = { .simd_size = simd_other },

Why simd_other? And if that's really the right choice, where do you
set op_bytes, which is required for this attribute due to

    if ( state->simd_size )
    {
        generate_exception_if(!op_bytes, EXC_UD);

?

> @@ -7356,6 +7359,14 @@ x86_emulate(
>          op_bytes = 16;
>          goto simd_0f38_common;
>  
> +    case X86EMUL_OPC_66(0x0f38, 0xcf):       /* gf2p8mulb xmm/m128,xmm*/
> +    case X86EMUL_OPC_VEX_66(0x0f38, 0xcf):   /* vgf2p8mulb xmm/m128,xmm*/

Please provide correct comments - the VEX variant comes with two
sizes. Also please add the missing blanks at the end.

> +        host_and_vcpu_must_have(gfni);
> +        if ( vex.opcx == vex_none )
> +            goto simd_0f38_common;
> +        else

Pointless else.

> +            goto simd_0f_avx;

vex.w needs to be checked before this goto.

> @@ -7741,6 +7752,17 @@ x86_emulate(
>          op_bytes = 16;
>          goto simd_0f3a_common;
>  
> +    case X86EMUL_OPC_66(0x0f3a, 0xce):     /* gf2p8affineqb $imm8,xmm/m128,xmm*/
> +    case X86EMUL_OPC_VEX_66(0x0f3a, 0xce): /* vgf2p8affineqb $imm8,xmm/m128,xmm*/
> +    case X86EMUL_OPC_66(0x0f3a, 0xcf):     /* gf2p8affineinvqb $imm8,xmm/m128,xmm*/
> +    case X86EMUL_OPC_VEX_66(0x0f3a, 0xcf): /* vgf2p8affineinvqb $imm8,xmm/m128,xmm*/
> +        host_and_vcpu_must_have(gfni);
> +        if ( vex.opcx == vex_none )
> +            goto simd_0f3a_common;
> +        else
> +            goto simd_0f_imm8_avx;

Similar comments apply here.

> +
> +

Please don't introduce double blank lines.

Jan


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

Re: [PATCH v3 2/4] x86emul: Support vpclmulqdq

[Jan Beulich]

>>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> @@ -7672,7 +7673,12 @@ x86_emulate(
>          host_and_vcpu_must_have(pclmulqdq);
>          if ( vex.opcx == vex_none )
>              goto simd_0f3a_common;
> -        generate_exception_if(vex.l, EXC_UD);
> +        if ( !vex.l )
> +        {
> +            generate_exception_if(vex.l, EXC_UD);

What's the point of this?

> +            goto simd_0f_imm8_avx;
> +        }
> +        host_and_vcpu_must_have(vpclmulqdq);

Why not simply

        if ( vex.l )
            host_and_vcpu_must_have(vpclmulqdq);

?

Jan


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

Re: [PATCH v3 3/4] x86emul: Support vaes insns

[Jan Beulich]

>>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1626,6 +1626,7 @@ static bool vcpu_has(
>  #define vcpu_has_clwb()        vcpu_has(         7, EBX, 24, ctxt, ops)
>  #define vcpu_has_sha()         vcpu_has(         7, EBX, 29, ctxt, ops)
>  #define vcpu_has_gfni()        vcpu_has(         7, ECX,  8, ctxt, ops)
> +#define vcpu_has_vaes()        vcpu_has(         7, ECX,  9, ctxt, ops)
>  #define vcpu_has_vpclmulqdq()  vcpu_has(         7, ECX, 10, ctxt, ops)
>  #define vcpu_has_rdpid()       vcpu_has(         7, ECX, 22, ctxt, ops)
>  #define vcpu_has_clzero()      vcpu_has(0x80000008, EBX,  0, ctxt, ops)
> @@ -7345,6 +7346,12 @@ x86_emulate(
>          host_and_vcpu_must_have(aesni);
>          if ( vex.opcx == vex_none )
>              goto simd_0f38_common;
> +        if ( vex.l )
> +        {
> +            host_and_vcpu_must_have(vaes);
> +            goto simd_0f_avx;
> +        }
> +
>          /* fall through */

Please move out the block no longer wanting this fall-through.
That'll also take care of vaesimc not becoming 256-bit enabled as
per the current version of the spec.

Jan


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

Re: [PATCH v3 3/4] x86emul: Support vaes insns

[Yang Zhong]

On Mon, Nov 27, 2017 at 09:30:15AM -0700, Jan Beulich wrote:
> >>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> > @@ -1626,6 +1626,7 @@ static bool vcpu_has(
> >  #define vcpu_has_clwb()        vcpu_has(         7, EBX, 24, ctxt, ops)
> >  #define vcpu_has_sha()         vcpu_has(         7, EBX, 29, ctxt, ops)
> >  #define vcpu_has_gfni()        vcpu_has(         7, ECX,  8, ctxt, ops)
> > +#define vcpu_has_vaes()        vcpu_has(         7, ECX,  9, ctxt, ops)
> >  #define vcpu_has_vpclmulqdq()  vcpu_has(         7, ECX, 10, ctxt, ops)
> >  #define vcpu_has_rdpid()       vcpu_has(         7, ECX, 22, ctxt, ops)
> >  #define vcpu_has_clzero()      vcpu_has(0x80000008, EBX,  0, ctxt, ops)
> > @@ -7345,6 +7346,12 @@ x86_emulate(
> >          host_and_vcpu_must_have(aesni);
> >          if ( vex.opcx == vex_none )
> >              goto simd_0f38_common;
> > +        if ( vex.l )
> > +        {
> > +            host_and_vcpu_must_have(vaes);
> > +            goto simd_0f_avx;
> > +        }
> > +
> >          /* fall through */
> 
> Please move out the block no longer wanting this fall-through.
> That'll also take care of vaesimc not becoming 256-bit enabled as
> per the current version of the spec.
> 
  Hello Jan,

  Thanks for review patch!
  
  The new patch will like below:

      host_and_vcpu_must_have(aesni);
      if ( vex.opcx == vex_none )
          goto simd_0f38_common;
+     if ( !vex.l )
+         goto simd_0f_avx;  -----> this will deal with vaesimc insn
+     host_and_vcpu_must_have(vaes);
+     goto simd_0f_ymm;    ------> this will deal with 256 VEX insn

  I will also remove / * fall through */ comments here, thanks!

  Regards,

  Yang
  
> Jan

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

Re: [PATCH v3 2/4] x86emul: Support vpclmulqdq

[Yang Zhong]

On Mon, Nov 27, 2017 at 08:53:24AM -0700, Jan Beulich wrote:
> >>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> > @@ -7672,7 +7673,12 @@ x86_emulate(
> >          host_and_vcpu_must_have(pclmulqdq);
> >          if ( vex.opcx == vex_none )
> >              goto simd_0f3a_common;
> > -        generate_exception_if(vex.l, EXC_UD);
> > +        if ( !vex.l )
> > +        {
> > +            generate_exception_if(vex.l, EXC_UD);
> 
> What's the point of this?

  Jan, thanks for your help!
  generate_exception_if(vex.l, EXC_UD) in here is not any useful,
  i will remove it, thanks!

> 
> > +            goto simd_0f_imm8_avx;
> > +        }
> > +        host_and_vcpu_must_have(vpclmulqdq);
> 
> Why not simply
> 
>         if ( vex.l )
>             host_and_vcpu_must_have(vpclmulqdq);
> 
  The new patch will like below:

      host_and_vcpu_must_have(pclmulqdq);
      if ( vex.opcx == vex_none )
          goto simd_0f3a_common;
+     if ( !vex.l )
+         goto simd_0f_imm8_avx;
+     host_and_vcpu_must_have(vpclmulqdq);
      goto simd_0f_imm8_ymm;

      simd_0f_imm8_ymm will be new created, which like simd_of_ymm did in the code.
> 
> Jan

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

Re: [PATCH v3 1/4] x86emul: Support GFNI insns

[Yang Zhong]

On Mon, Nov 27, 2017 at 08:42:29AM -0700, Jan Beulich wrote:
> >>> On 10.11.17 at 11:36, <yang.zhong@intel.com> wrote:
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> 
> First and foremost - did you try out your own patch? There not being
> any (minimal) test added makes this at least questionable.
> 
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> > @@ -385,6 +385,7 @@ static const struct {
> >      [0x40] = { .simd_size = simd_packed_int },
> >      [0x41] = { .simd_size = simd_packed_int, .two_op = 1 },
> >      [0xc8 ... 0xcd] = { .simd_size = simd_other },
> > +    [0xcf] = { .simd_size = simd_other },
> 
> Why simd_other? And if that's really the right choice, where do you
> set op_bytes, which is required for this attribute due to
> 
>     if ( state->simd_size )
>     {
>         generate_exception_if(!op_bytes, EXC_UD);
> 
>?

  Hello Jan,

  Thanks for review patch!
  I checked the Galois Field related theorem, this should be simd_size = simd_packed_int.

  I will change this, thanks!

  Regards,

  Yang

> 
> > @@ -7356,6 +7359,14 @@ x86_emulate(
> >          op_bytes = 16;
> >          goto simd_0f38_common;
> >  
> > +    case X86EMUL_OPC_66(0x0f38, 0xcf):       /* gf2p8mulb xmm/m128,xmm*/
> > +    case X86EMUL_OPC_VEX_66(0x0f38, 0xcf):   /* vgf2p8mulb xmm/m128,xmm*/
> 
> Please provide correct comments - the VEX variant comes with two
> sizes. Also please add the missing blanks at the end.

  I will change those comments, thanks!
> 
> > +        host_and_vcpu_must_have(gfni);
> > +        if ( vex.opcx == vex_none )
> > +            goto simd_0f38_common;
> > +        else
> 
> Pointless else.

  Thanks, this else is not useful.
> 
> > +            goto simd_0f_avx;
> 
> vex.w needs to be checked before this goto.

  I checked the document again, there is no vex.w exception to check.
  If vex.w is necessary, i will add this check in here. thanks!

> 
> > @@ -7741,6 +7752,17 @@ x86_emulate(
> >          op_bytes = 16;
> >          goto simd_0f3a_common;
> >  
> > +    case X86EMUL_OPC_66(0x0f3a, 0xce):     /* gf2p8affineqb $imm8,xmm/m128,xmm*/
> > +    case X86EMUL_OPC_VEX_66(0x0f3a, 0xce): /* vgf2p8affineqb $imm8,xmm/m128,xmm*/
> > +    case X86EMUL_OPC_66(0x0f3a, 0xcf):     /* gf2p8affineinvqb $imm8,xmm/m128,xmm*/
> > +    case X86EMUL_OPC_VEX_66(0x0f3a, 0xcf): /* vgf2p8affineinvqb $imm8,xmm/m128,xmm*/
> > +        host_and_vcpu_must_have(gfni);
> > +        if ( vex.opcx == vex_none )
> > +            goto simd_0f3a_common;
> > +        else
> > +            goto simd_0f_imm8_avx;
> 
> Similar comments apply here.

  I will remove else and add vex.w check here as above, thanks!
> 
> > +
> > +
> 
> Please don't introduce double blank lines.

  thanks, i will be care of this.
> 
> Jan

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