Re: [PATCH 19/20] target/i386/tcg: mark APX as supported

[Zhao Liu <zhao1.liu@intel.com>]

> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e5ffb10d156..e966bd8049b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1005,7 +1005,7 @@ void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
>  #define TCG_7_1_EAX_FEATURES (CPUID_7_1_EAX_FZRM | CPUID_7_1_EAX_FSRS | \
>            CPUID_7_1_EAX_FSRC | CPUID_7_1_EAX_CMPCCXADD)
>  #define TCG_7_1_ECX_FEATURES 0
> -#define TCG_7_1_EDX_FEATURES 0
> +#define TCG_7_1_EDX_FEATURES CPUID_7_1_EDX_APXF

And NCI/NDD/NF is supported :-)

+#define TCG_29_0_EBX_FEATURES CPUID_29_0_EBX_APX_NCI_NDD_NF

>  #define TCG_7_2_EDX_FEATURES 0
>  #define TCG_APM_FEATURES 0
>  #define TCG_6_EAX_FEATURES CPUID_6_EAX_ARAT
> @@ -1554,7 +1554,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          },
>          .tcg_features = XSTATE_FP_MASK | XSTATE_SSE_MASK |
>              XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
> -            XSTATE_PKRU_MASK,
> +            XSTATE_PKRU_MASK | XSTATE_APX_MASK,
>          .migratable_flags = XSTATE_FP_MASK | XSTATE_SSE_MASK |
>              XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
>              XSTATE_OPMASK_MASK | XSTATE_ZMM_Hi256_MASK | XSTATE_Hi16_ZMM_MASK |
> @@ -9722,6 +9722,17 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>       * inside x86_cpu_parse_featurestr() too.
>       */
>      if (xcc->max_features) {
> +        /*
> +         * TCG supports both MPX and APX.  Since they they cannot be enabled together,
> +         * disable one---prefer APX if none was chosen explicitly.
> +         */
> +        if ((x86_cpu_get_supported_feature_word(cpu, FEAT_7_1_EDX) & CPUID_7_1_EDX_APXF) &&
> +            env->user_features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) {
> +            feature_word_info[FEAT_7_1_EDX].no_autoenable_flags |= CPUID_7_1_EDX_APXF;
> +        } else {
> +            feature_word_info[FEAT_7_0_EBX].no_autoenable_flags |= CPUID_7_0_EBX_MPX;
> +        }

I have some thoughts here:

1. we should check env->features[] instead of env->user_features[]
since the latter just indicates whether user touches this feature (maybe
on, or maybe off), in x86_cpu_set_bit_prop().

2. if accel doesn't supports APXF, then it goes to "else" and disable
MPX by default - I think this would hurt KVM.

3. changing feature_word_info seems not good - it is a global variable
and has longer life time than CPU object - so that changes to it will
affect multiple CPU instances (QMP's dummy instance).

For example,

 (QMP) query-cpu-model-expansion type=full model={"name":"max"}

 [mpx=off, apxf=on]

 (QMP) query-cpu-model-expansion type=full model={"name":"max","props":{"mpx":true}}

 [mpx=on, apxf=off]

 (QMP) query-cpu-model-expansion type=full model={"name":"max"}

 [mpx=off, apxf=off, NOTE: this gets the different result with 1st try!]

So, I think that if we're going to change a feature, we should change the
feature list that corresponds to the CPU lifetime, such as env->features[].

But I find it's still tricky to keep this check before FEATURE_WORDS
loop. So how about the following instead? 

-        /*
-         * TCG supports both MPX and APX.  Since they they cannot be enabled together,
-         * disable one---prefer APX if none was chosen explicitly.
-         */
-        if ((x86_cpu_get_supported_feature_word(cpu, FEAT_7_1_EDX) & CPUID_7_1_EDX_APXF) &&
-            env->user_features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) {
-            feature_word_info[FEAT_7_1_EDX].no_autoenable_flags |= CPUID_7_1_EDX_APXF;
-        } else {
-            feature_word_info[FEAT_7_0_EBX].no_autoenable_flags |= CPUID_7_0_EBX_MPX;
-        }
-
         for (w = 0; w < FEATURE_WORDS; w++) {
             /* Override only features that weren't set explicitly
              * by the user.
              */
             env->features[w] |=
                 x86_cpu_get_supported_feature_word(cpu, w) &
                 ~env->user_features[w] &
                 ~feature_word_info[w].no_autoenable_flags;
         }

+        /*
+         * TCG supports both MPX and APX.  Since they they cannot be enabled together,
+         * disable one---prefer APX if none was chosen explicitly. If both were
+         * requested explicitly, leave them alone and let the check below report the
+         * conflict.
+         */
+        if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) &&
+            (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_APXF)) {
+            if (!(env->user_features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX)) {
+                env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_MPX;
+            } else if (!(env->user_features[FEAT_7_1_EDX] & CPUID_7_1_EDX_APXF)) {
+                env->features[FEAT_7_1_EDX] &= ~CPUID_7_1_EDX_APXF;
+            }
+        }

Thanks,
Zhao