Re: [Qemu-devel] [PATCH] target-i386: add AMD CPUID.1:edx aliases to x86_cpu_get_migratable_flags

[Eduardo Habkost]

On Thu, Apr 14, 2016 at 10:55:07PM +0200, Radim Krčmář wrote:
> QEMU complains about -cpu host on an AMD machine:
>   warning: host doesn't support requested feature: CPUID.80000001H:EDX [bit 0]
> For bits 0,1,3,4,5,6,7,8,9,12,13,14,15,16,17,23,24.
> 
> Host does support them, but x86_cpu_get_migratable_flags filters unnamed
> features and drops these bits without realizing that they are aliases to
> CPUID.1H:EDX and have their names there.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1326721 for details.
> 
> Signed-off-by: Radim Krčmář <address@hidden>
> ---
>  target-i386/cpu.c | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ddae932ee1b4..66bd9d0c4039 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -506,7 +506,7 @@ const char *get_register_name_32(unsigned int reg)
>   * Returns the set of feature flags that are supported and migratable by
>   * QEMU, for a given FeatureWord.
>   */
> -static uint32_t x86_cpu_get_migratable_flags(FeatureWord w)
> +static uint32_t x86_cpu_get_migratable_flags(FeatureWord w, bool is_amd)
>  {
>      FeatureWordInfo *wi = &feature_word_info[w];
>      uint32_t r = 0;
> @@ -514,12 +514,18 @@ static uint32_t 
> x86_cpu_get_migratable_flags(FeatureWord w)
>  
>      for (i = 0; i < 32; i++) {
>          uint32_t f = 1U << i;
> +        FeatureWordInfo *effective_wi = wi;
> +
> +        if (is_amd && w == FEAT_8000_0001_EDX && f & CPUID_EXT2_AMD_ALIASES) 
> {
> +            effective_wi = &feature_word_info[FEAT_1_EDX];
> +        }
> +
>          /* If the feature name is unknown, it is not supported by QEMU yet */
> -        if (!wi->feat_names[i]) {
> +        if (!effective_wi->feat_names[i]) {
>              continue;
>          }
>          /* Skip features known to QEMU, but explicitly marked as 
> unmigratable */
> -        if (wi->unmigratable_flags & f) {
> +        if (effective_wi->unmigratable_flags & f) {
>              continue;
>          }
>          r |= f;

I don't think we need that complexity to fix the problem. We even
have a similar hack in kvm_arch_get_supported_cpuid() to make
sure it handles the alias bits properly. Instead of hacking those
functions to copy CPUID[1] data, it's much easier to simply copy
the alias bits in realizefn after we call
x86_cpu_filter_features(), not before.

The following (untested) fix should be sufficient:

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ddae932..d0b5b69 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2897,6 +2897,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
         env->cpuid_level = 7;
     }
 
+    if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
+        error_setg(&local_err,
+                   kvm_enabled() ?
+                       "Host doesn't support requested features" :
+                       "TCG doesn't support requested features");
+        goto out;
+    }
+
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
      * CPUID[1].EDX.
      */
@@ -2907,14 +2915,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
     }
 
 
-    if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
-        error_setg(&local_err,
-                   kvm_enabled() ?
-                       "Host doesn't support requested features" :
-                       "TCG doesn't support requested features");
-        goto out;
-    }
-
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 


-- 
Eduardo