diff mbox series

[v6,1/6] x86/cpu: Rename XSAVE related feature words.

Message ID 20201013051935.6052-2-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series [v6,1/6] x86/cpu: Rename XSAVE related feature words. | expand

Commit Message

Yang, Weijiang Oct. 13, 2020, 5:19 a.m. UTC 
With more components in XSS being developed on Intel platform,
it's necessary to clean up existing XSAVE related feature words to
make the name clearer. It's to prepare for adding CET related support
in following patches.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 target/i386/cpu.c        | 60 ++++++++++++++++++++--------------------
 target/i386/cpu.h        |  6 ++--
 target/i386/fpu_helper.c |  2 +-
 target/i386/translate.c  |  2 +-
 4 files changed, 35 insertions(+), 35 deletions(-)

Comments

Sean Christopherson Oct. 14, 2020, 12:08 a.m. UTC | #1 
On Tue, Oct 13, 2020 at 01:19:30PM +0800, Yang Weijiang wrote:
> With more components in XSS being developed on Intel platform,
> it's necessary to clean up existing XSAVE related feature words to
> make the name clearer. It's to prepare for adding CET related support
> in following patches.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  target/i386/cpu.c        | 60 ++++++++++++++++++++--------------------
>  target/i386/cpu.h        |  6 ++--
>  target/i386/fpu_helper.c |  2 +-
>  target/i386/translate.c  |  2 +-
>  4 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 588f32e136..e2891740f1 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1050,7 +1050,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          .tcg_features = 0,
>          .unmigratable_flags = 0,
>      },
> -    [FEAT_XSAVE] = {
> +    [FEAT_XSAVE_INSTRUCTION] = {

The COMP->XCRO change is great, but I don't think XSAVE->XSAVE_INSTRUCTION
makes sense.  There is no guarantee the word will only be used for
instructions; it already blurs the line, e.g. XSAVEC also changes the behavior
of XRSTOR, and XSAVES also means the XSS MSR is available.

FWIW, I also don't find FEAT_XSAVE to be confusing.

>          .type = CPUID_FEATURE_WORD,
>          .feat_names = {
>              "xsaveopt", "xsavec", "xgetbv1", "xsaves",
> @@ -1084,7 +1084,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          .cpuid = { .eax = 6, .reg = R_EAX, },
>          .tcg_features = TCG_6_EAX_FEATURES,
>      },
> -    [FEAT_XSAVE_COMP_LO] = {
> +    [FEAT_XSAVE_XCR0_LO] = {
>          .type = CPUID_FEATURE_WORD,
>          .cpuid = {
>              .eax = 0xD,
Yang, Weijiang Oct. 15, 2020, 2:20 a.m. UTC | #2 
On Tue, Oct 13, 2020 at 05:08:54PM -0700, Sean Christopherson wrote:
> On Tue, Oct 13, 2020 at 01:19:30PM +0800, Yang Weijiang wrote:
> > With more components in XSS being developed on Intel platform,
> > it's necessary to clean up existing XSAVE related feature words to
> > make the name clearer. It's to prepare for adding CET related support
> > in following patches.
> > 
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > ---
> >  target/i386/cpu.c        | 60 ++++++++++++++++++++--------------------
> >  target/i386/cpu.h        |  6 ++--
> >  target/i386/fpu_helper.c |  2 +-
> >  target/i386/translate.c  |  2 +-
> >  4 files changed, 35 insertions(+), 35 deletions(-)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 588f32e136..e2891740f1 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1050,7 +1050,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> >          .tcg_features = 0,
> >          .unmigratable_flags = 0,
> >      },
> > -    [FEAT_XSAVE] = {
> > +    [FEAT_XSAVE_INSTRUCTION] = {
> 
> The COMP->XCRO change is great, but I don't think XSAVE->XSAVE_INSTRUCTION
> makes sense.  There is no guarantee the word will only be used for
> instructions; it already blurs the line, e.g. XSAVEC also changes the behavior
> of XRSTOR, and XSAVES also means the XSS MSR is available.
> 
> FWIW, I also don't find FEAT_XSAVE to be confusing.
>
Thanks for the feedback! I also found it's hard to rename it with a
precise one, just wanted to make all XSAVE related feature words more
specific. I'll revert it in next version.

> >          .type = CPUID_FEATURE_WORD,
> >          .feat_names = {
> >              "xsaveopt", "xsavec", "xgetbv1", "xsaves",
> > @@ -1084,7 +1084,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> >          .cpuid = { .eax = 6, .reg = R_EAX, },
> >          .tcg_features = TCG_6_EAX_FEATURES,
> >      },
> > -    [FEAT_XSAVE_COMP_LO] = {
> > +    [FEAT_XSAVE_XCR0_LO] = {
> >          .type = CPUID_FEATURE_WORD,
> >          .cpuid = {
> >              .eax = 0xD,
diff mbox series

Patch

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 588f32e136..e2891740f1 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1050,7 +1050,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         .tcg_features = 0,
         .unmigratable_flags = 0,
     },
-    [FEAT_XSAVE] = {
+    [FEAT_XSAVE_INSTRUCTION] = {
         .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "xsaveopt", "xsavec", "xgetbv1", "xsaves",
@@ -1084,7 +1084,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         .cpuid = { .eax = 6, .reg = R_EAX, },
         .tcg_features = TCG_6_EAX_FEATURES,
     },
-    [FEAT_XSAVE_COMP_LO] = {
+    [FEAT_XSAVE_XCR0_LO] = {
         .type = CPUID_FEATURE_WORD,
         .cpuid = {
             .eax = 0xD,
@@ -1097,7 +1097,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             XSTATE_OPMASK_MASK | XSTATE_ZMM_Hi256_MASK | XSTATE_Hi16_ZMM_MASK |
             XSTATE_PKRU_MASK,
     },
-    [FEAT_XSAVE_COMP_HI] = {
+    [FEAT_XSAVE_XCR0_HI] = {
         .type = CPUID_FEATURE_WORD,
         .cpuid = {
             .eax = 0xD,
@@ -1369,7 +1369,7 @@  static FeatureDep feature_dependencies[] = {
         .to = { FEAT_VMX_SECONDARY_CTLS,    ~0ull },
     },
     {
-        .from = { FEAT_XSAVE,               CPUID_XSAVE_XSAVES },
+        .from = { FEAT_XSAVE_INSTRUCTION,   CPUID_XSAVE_XSAVES },
         .to = { FEAT_VMX_SECONDARY_CTLS,    VMX_SECONDARY_EXEC_XSAVES },
     },
     {
@@ -1502,8 +1502,8 @@  static inline bool accel_uses_host_cpuid(void)
 
 static inline uint64_t x86_cpu_xsave_components(X86CPU *cpu)
 {
-    return ((uint64_t)cpu->env.features[FEAT_XSAVE_COMP_HI]) << 32 |
-           cpu->env.features[FEAT_XSAVE_COMP_LO];
+    return ((uint64_t)cpu->env.features[FEAT_XSAVE_XCR0_HI]) << 32 |
+           cpu->env.features[FEAT_XSAVE_XCR0_LO];
 }
 
 const char *get_register_name_32(unsigned int reg)
@@ -2390,7 +2390,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_EXT2_SYSCALL,
         .features[FEAT_8000_0001_ECX] =
             CPUID_EXT3_LAHF_LM,
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT,
         .features[FEAT_6_EAX] =
             CPUID_6_EAX_ARAT,
@@ -2479,7 +2479,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_EXT2_SYSCALL,
         .features[FEAT_8000_0001_ECX] =
             CPUID_EXT3_LAHF_LM,
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT,
         .features[FEAT_6_EAX] =
             CPUID_6_EAX_ARAT,
@@ -2574,7 +2574,7 @@  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,
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT,
         .features[FEAT_6_EAX] =
             CPUID_6_EAX_ARAT,
@@ -2704,7 +2704,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
             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,
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT,
         .features[FEAT_6_EAX] =
             CPUID_6_EAX_ARAT,
@@ -2834,7 +2834,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
          * and the only one defined in Skylake (processor tracing)
          * probably will block migration anyway.
          */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -2954,7 +2954,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
          * and the only one defined in Skylake (processor tracing)
          * probably will block migration anyway.
          */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -3089,7 +3089,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
                 * and the only one defined in Skylake (processor tracing)
                 * probably will block migration anyway.
                 */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -3228,7 +3228,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
          * and the only one defined in Skylake (processor tracing)
          * probably will block migration anyway.
          */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -3331,7 +3331,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
                 * and the only one defined in Skylake (processor tracing)
                 * probably will block migration anyway.
                 */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -3448,7 +3448,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
                 * and the only one defined in Skylake (processor tracing)
                 * probably will block migration anyway.
                 */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -3581,7 +3581,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
          * and the only one defined in Skylake (processor tracing)
          * probably will block migration anyway.
          */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC | CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
             CPUID_6_EAX_ARAT,
@@ -3712,7 +3712,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
          * and the only one defined in Skylake (processor tracing)
          * probably will block migration anyway.
          */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -3814,7 +3814,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_7_0_ECX_AVX512_VPOPCNTDQ,
         .features[FEAT_7_0_EDX] =
             CPUID_7_0_EDX_AVX512_4VNNIW | CPUID_7_0_EDX_AVX512_4FMAPS,
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT,
         .features[FEAT_6_EAX] =
             CPUID_6_EAX_ARAT,
@@ -3985,7 +3985,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
             CPUID_7_0_EBX_ADX | CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT |
             CPUID_7_0_EBX_SHA_NI,
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -4063,7 +4063,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
          * including v4.1 to v4.12).
          * KVM doesn't yet expose any XSAVES state save component.
          */
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1,
         .features[FEAT_6_EAX] =
@@ -4113,7 +4113,7 @@  static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_7_0_EBX_SHA_NI | CPUID_7_0_EBX_CLWB,
         .features[FEAT_7_0_ECX] =
             CPUID_7_0_ECX_UMIP | CPUID_7_0_ECX_RDPID,
-        .features[FEAT_XSAVE] =
+        .features[FEAT_XSAVE_INSTRUCTION] =
             CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
             CPUID_XSAVE_XGETBV1 | CPUID_XSAVE_XSAVES,
         .features[FEAT_6_EAX] =
@@ -4689,8 +4689,8 @@  static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
     /* XSAVE components are automatically enabled by other features,
      * so return the original feature name instead
      */
-    if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
-        int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
+    if (w == FEAT_XSAVE_XCR0_LO || w == FEAT_XSAVE_XCR0_HI) {
+        int comp = (w == FEAT_XSAVE_XCR0_HI) ? bitnr + 32 : bitnr;
 
         if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
             x86_ext_save_areas[comp].bits) {
@@ -5736,8 +5736,8 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
 
         if (count == 0) {
             *ecx = xsave_area_size(x86_cpu_xsave_components(cpu));
-            *eax = env->features[FEAT_XSAVE_COMP_LO];
-            *edx = env->features[FEAT_XSAVE_COMP_HI];
+            *eax = env->features[FEAT_XSAVE_XCR0_LO];
+            *edx = env->features[FEAT_XSAVE_XCR0_HI];
             /*
              * The initial value of xcr0 and ebx == 0, On host without kvm
              * commit 412a3c41(e.g., CentOS 6), the ebx's value always == 0
@@ -5746,7 +5746,7 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
              */
             *ebx = kvm_enabled() ? *ecx : xsave_area_size(env->xcr0);
         } else if (count == 1) {
-            *eax = env->features[FEAT_XSAVE];
+            *eax = env->features[FEAT_XSAVE_INSTRUCTION];
         } else if (count < ARRAY_SIZE(x86_ext_save_areas)) {
             if ((x86_cpu_xsave_components(cpu) >> count) & 1) {
                 const ExtSaveArea *esa = &x86_ext_save_areas[count];
@@ -6307,8 +6307,8 @@  static void x86_cpu_enable_xsave_components(X86CPU *cpu)
         }
     }
 
-    env->features[FEAT_XSAVE_COMP_LO] = mask;
-    env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
+    env->features[FEAT_XSAVE_XCR0_LO] = mask;
+    env->features[FEAT_XSAVE_XCR0_HI] = mask >> 32;
 }
 
 /***** Steps involved on loading and filtering CPUID data
@@ -6423,7 +6423,7 @@  static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         x86_cpu_adjust_feat_level(cpu, FEAT_8000_0008_EBX);
         x86_cpu_adjust_feat_level(cpu, FEAT_C000_0001_EDX);
         x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
-        x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
+        x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE_INSTRUCTION);
 
         /* Intel Processor Trace requires CPUID[0x14] */
         if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT)) {
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e1a5c174dc..b86eb8dba0 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -525,10 +525,10 @@  typedef enum FeatureWord {
     FEAT_HV_RECOMM_EAX, /* CPUID[4000_0004].EAX */
     FEAT_HV_NESTED_EAX, /* CPUID[4000_000A].EAX */
     FEAT_SVM,           /* CPUID[8000_000A].EDX */
-    FEAT_XSAVE,         /* CPUID[EAX=0xd,ECX=1].EAX */
+    FEAT_XSAVE_INSTRUCTION, /* CPUID[EAX=0xd,ECX=1].EAX */
     FEAT_6_EAX,         /* CPUID[6].EAX */
-    FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
-    FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
+    FEAT_XSAVE_XCR0_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
+    FEAT_XSAVE_XCR0_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
     FEAT_ARCH_CAPABILITIES,
     FEAT_CORE_CAPABILITY,
     FEAT_PERF_CAPABILITIES,
diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index 4ea73874d8..a104467a18 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -2891,7 +2891,7 @@  uint64_t helper_xgetbv(CPUX86State *env, uint32_t ecx)
     case 0:
         return env->xcr0;
     case 1:
-        if (env->features[FEAT_XSAVE] & CPUID_XSAVE_XGETBV1) {
+        if (env->features[FEAT_XSAVE_INSTRUCTION] & CPUID_XSAVE_XGETBV1) {
             return env->xcr0 & get_xinuse(env);
         }
         break;
diff --git a/target/i386/translate.c b/target/i386/translate.c
index caea6f5fb1..8510300ae6 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -8481,7 +8481,7 @@  static void i386_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
     dc->cpuid_ext2_features = env->features[FEAT_8000_0001_EDX];
     dc->cpuid_ext3_features = env->features[FEAT_8000_0001_ECX];
     dc->cpuid_7_0_ebx_features = env->features[FEAT_7_0_EBX];
-    dc->cpuid_xsave_features = env->features[FEAT_XSAVE];
+    dc->cpuid_xsave_features = env->features[FEAT_XSAVE_INSTRUCTION];
 #ifdef TARGET_X86_64
     dc->lma = (flags >> HF_LMA_SHIFT) & 1;
     dc->code64 = (flags >> HF_CS64_SHIFT) & 1;