Re: [Qemu-devel] [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
On Thu, Nov 05, 2015 at 08:51:24AM +0100, Richard Henderson wrote: > 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 > >>>--- > >>> target-i386/cpu.c | 8 +--- > >>> target-i386/cpu.h | 3 +++ > >>> 2 files changed, 8 insertions(+), 3 deletions(-) > >> > >>Reviewed-by: Richard Henderson > >> > >>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. I will apply this patch without the TCG_*_FEATURES changes until we change TCG, then. That's OK? About the TCG patches I have sent, please let me know if they look good and appropriate for 2.5. This is the first time I have touched TCG code. -- Eduardo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
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 --- target-i386/cpu.c | 8 +--- target-i386/cpu.h | 3 +++ 2 files changed, 8 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson 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~ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
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 > > --- > > target-i386/cpu.c | 8 +--- > > target-i386/cpu.h | 3 +++ > > 2 files changed, 8 insertions(+), 3 deletions(-) > > Reviewed-by: Richard Henderson > > 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. Your conclusion seems to be right for pcommit and clflushopt, if I checked the opcodes and encoding properly. In the case of pcommit (/7, modrm == 0xf8), we check for SSE; in the case of clflushopt (/7 with a memory operand, modrm != 0xf8), we check for CLFLUSH. 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. -- Eduardo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
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 > --- > target-i386/cpu.c | 8 +--- > target-i386/cpu.h | 3 +++ > 2 files changed, 8 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson 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? r~ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
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 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 ,enforcec is runnable in a given host, then -machine pc-i440fx-2.5 -cpu ,enforce should be runnable too. Eduardo, thanks for your info. I will dig into the code and fix this issue in the next version. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
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 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 ,enforcec is runnable in a given host, then -machine pc-i440fx-2.5 -cpu ,enforce should be runnable too. -- Eduardo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] target-i386: enable cflushopt/clwb/pcommit instructions
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
