[PATCH] target-i386: enable cflushopt/clwb/pcommit instructions

[PATCH] target-i386: enable cflushopt/clwb/pcommit instructions

[Xiao Guangrong]

These instructions are used by NVDIMM drivers and the specification
locates at:
https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf

Let them be enabled on Broadwell on default

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 target-i386/cpu.c | 14 +++++++++-----
 target-i386/cpu.h |  3 +++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7a779b1..2129b3a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -260,8 +260,8 @@ static const char *svm_feature_name[] = {
 static const char *cpuid_7_0_ebx_feature_name[] = {
     "fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
     "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
-    "avx512f", NULL, "rdseed", "adx", "smap", NULL, NULL, NULL,
-    NULL, NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
+    "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", "clflushopt",
+    "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
 };
 
 static const char *cpuid_apm_edx_feature_name[] = {
@@ -346,7 +346,9 @@ static const char *cpuid_6_feature_name[] = {
 #define TCG_SVM_FEATURES 0
 #define TCG_KVM_FEATURES 0
 #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP | \
-          CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX)
+          CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX | \
+          CPUID_7_0_EBX_PCOMMIT | CPUID_7_0_EBX_CLFLUSHOPT |            \
+          CPUID_7_0_EBX_CLWB)
           /* missing:
           CPUID_7_0_EBX_FSGSBASE, CPUID_7_0_EBX_HLE, CPUID_7_0_EBX_AVX2,
           CPUID_7_0_EBX_ERMS, CPUID_7_0_EBX_INVPCID, CPUID_7_0_EBX_RTM,
@@ -1191,7 +1193,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP |
             CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID |
             CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX |
-            CPUID_7_0_EBX_SMAP,
+            CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_PCOMMIT |
+            CPUID_7_0_EBX_CLFLUSHOPT | CPUID_7_0_EBX_CLWB,
         .features[FEAT_XSAVE] =
             CPUID_XSAVE_XSAVEOPT,
         .features[FEAT_6_EAX] =
@@ -1229,7 +1232,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP |
             CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID |
             CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX |
-            CPUID_7_0_EBX_SMAP,
+            CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_PCOMMIT |
+            CPUID_7_0_EBX_CLFLUSHOPT | CPUID_7_0_EBX_CLWB,
         .features[FEAT_XSAVE] =
             CPUID_XSAVE_XSAVEOPT,
         .features[FEAT_6_EAX] =
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ead2832..cb65c05 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -572,6 +572,9 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EBX_RDSEED   (1U << 18)
 #define CPUID_7_0_EBX_ADX      (1U << 19)
 #define CPUID_7_0_EBX_SMAP     (1U << 20)
+#define CPUID_7_0_EBX_PCOMMIT  (1 << 22)
+#define CPUID_7_0_EBX_CLFLUSHOPT (1 << 23)
+#define CPUID_7_0_EBX_CLWB     (1 << 24)
 #define CPUID_7_0_EBX_AVX512PF (1U << 26) /* AVX-512 Prefetch */
 #define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */
 #define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */
-- 
2.4.3

Re: [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions

[Eduardo Habkost]

On Fri, Aug 21, 2015 at 01:05:12PM +0800, Xiao Guangrong wrote:
> These instructions are used by NVDIMM drivers and the specification
> locates at:
> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
> 
> Let them be enabled on Broadwell on default
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>

This breaks the ABI. If you change a CPU model you need machine compat
code (see the TYPE_X86_CPU entries in PC_COMPAT_2_3 for example).

And you can only change the CPU model in a new machine-type if that
doesn't affect the runnability of a VM. In other words:

If:
  -machine pc-i440fx-2.4 -cpu <model>,enforcec
is runnable in a given host, then
  -machine pc-i440fx-2.5 -cpu <model>,enforce
should be runnable too.

-- 
Eduardo

Re: [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions

[Xiao Guangrong]

On 08/22/2015 12:05 AM, Eduardo Habkost wrote:
> On Fri, Aug 21, 2015 at 01:05:12PM +0800, Xiao Guangrong wrote:
>> These instructions are used by NVDIMM drivers and the specification
>> locates at:
>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>>
>> Let them be enabled on Broadwell on default
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>
> This breaks the ABI. If you change a CPU model you need machine compat
> code (see the TYPE_X86_CPU entries in PC_COMPAT_2_3 for example).
>
> And you can only change the CPU model in a new machine-type if that
> doesn't affect the runnability of a VM. In other words:
>
> If:
>    -machine pc-i440fx-2.4 -cpu <model>,enforcec
> is runnable in a given host, then
>    -machine pc-i440fx-2.5 -cpu <model>,enforce
> should be runnable too.
>

Eduardo, thanks for your info. I will dig into the code and fix this
issue in the next version.

[PATCH] target-i386: enable cflushopt/clwb/pcommit instructions

[Xiao Guangrong]

These instructions are used by NVDIMM drivers and the specification
locates at:
https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf

There instructions are available on Skylake Server

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 target-i386/cpu.c | 8 +++++---
 target-i386/cpu.h | 3 +++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 05d7f26..ebecdb4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -259,8 +259,8 @@ static const char *svm_feature_name[] = {
 static const char *cpuid_7_0_ebx_feature_name[] = {
     "fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
     "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
-    "avx512f", NULL, "rdseed", "adx", "smap", NULL, NULL, NULL,
-    NULL, NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
+    "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", "clflushopt",
+    "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
 };
 
 static const char *cpuid_apm_edx_feature_name[] = {
@@ -345,7 +345,9 @@ static const char *cpuid_6_feature_name[] = {
 #define TCG_SVM_FEATURES 0
 #define TCG_KVM_FEATURES 0
 #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP | \
-          CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX)
+          CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX | \
+          CPUID_7_0_EBX_PCOMMIT | CPUID_7_0_EBX_CLFLUSHOPT |            \
+          CPUID_7_0_EBX_CLWB)
           /* missing:
           CPUID_7_0_EBX_FSGSBASE, CPUID_7_0_EBX_HLE, CPUID_7_0_EBX_AVX2,
           CPUID_7_0_EBX_ERMS, CPUID_7_0_EBX_INVPCID, CPUID_7_0_EBX_RTM,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 54d9d50..71ecb5c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -573,6 +573,9 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EBX_RDSEED   (1U << 18)
 #define CPUID_7_0_EBX_ADX      (1U << 19)
 #define CPUID_7_0_EBX_SMAP     (1U << 20)
+#define CPUID_7_0_EBX_PCOMMIT  (1U << 22) /* Persistent Commit */
+#define CPUID_7_0_EBX_CLFLUSHOPT (1U << 23) /* Flush a Cache Line Optimized */
+#define CPUID_7_0_EBX_CLWB     (1U << 24) /* Cache Line Write Back */
 #define CPUID_7_0_EBX_AVX512PF (1U << 26) /* AVX-512 Prefetch */
 #define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */
 #define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */
-- 
2.4.3

Re: [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions

[Richard Henderson]

On 11/04/2015 08:35 PM, Eduardo Habkost wrote:
> On Fri, Oct 30, 2015 at 01:54:33PM -0700, Richard Henderson wrote:
>> On 10/29/2015 12:31 AM, Xiao Guangrong wrote:
>>> These instructions are used by NVDIMM drivers and the specification
>>> locates at:
>>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>>>
>>> There instructions are available on Skylake Server
>>>
>>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>> ---
>>>   target-i386/cpu.c | 8 +++++---
>>>   target-i386/cpu.h | 3 +++
>>>   2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>
>> Although it would be nice to update the comments in translate.c to include the
>> new insns, since they overlap mfence and sfence.  At present we only check for
>> SSE enabled when accepting these; I suppose it's easiest to consider it invalid
>> to specify +clwb,-sse?
>
> I assume you want to add the extra SSE requirement to TCG code, not to
> generic x86 code, then I have no objections.

I don't really want to add any requirement, just point and laugh at anyone who 
reports an bug for the above condition.

> But in the case of clwb (/6 with a memory operand, modrm != 0xc0), we
> are not just requiring SSE2: we are rejecting the instruction unless
> modrm == 0xc0. That means TCG is rejecting the clwb instruction, so I
> believe we shouldn't add CLWB to TCG_7_0_EBX_FEATURES yet.

Hmm, yes.

I've cleaned up some of this code on a branch, but it didn't get enough testing 
or review this cycle, so it's going to wait for the next.  I see you've posted 
a patch for this, which should be good enough until then.


r~