[Qemu-devel] [PATCH 0/5] i386: cpu: remove duplicate feature names

[Qemu-devel] [PATCH 0/5] i386: cpu: remove duplicate feature names

[Eduardo Habkost]

The problem:

 - Some features are report at the same time on both CPUID[1].EDX and
   CPUID[8000_0001].EDX on AMD CPUs (e.g. fpu, tsc, msr, pae, mmx).
 - "-cpu <model>,+feature" should enable the bit only on CPUID[1] if
   it's not an AMD CPU, but it should enable the bit on both CPUID[1] and
   CPUID[8000_0001] if it's an AMD CPU.
 - The same should happen when implementing CPU properties: setting the
   property that enables a feature should set the duplicate CPUID[8000_0001].EDX
   bit only if CPU vendor is AMD.

Reference: http://article.gmane.org/gmane.comp.emulators.qemu/166024

The solution implemented by this series is:
 - On the CPU model table and while parsing CPU options/properties, set the bit
   only on CPUID[1] (the x86_def_t.features field).
 - When finishing initialization of the CPU cpuid fields, duplicate those
   feature bits on cpuid_ext2_features if and only if the CPU vendor is AMD.

This series depends on the "x86 CPU patches that didn't get into 1.2" series:
  http://article.gmane.org/gmane.comp.emulators.qemu/168633
  Message-Id: <1346877673-9136-1-git-send-email-ehabkost@redhat.com>


Eduardo Habkost (5):
  i386: kvm: bit 10 of CPUID[8000_0001].EDX is reserved
  i386: kvm: use a #define for the set of alias feature bits
  i386: cpu: replace EXT2_FEATURE_MASK with CPUID_EXT2_AMD_ALIASES
  i386: cpu: eliminate duplicate feature names
  i386: -cpu help: remove reference to specific CPUID leaves/registers

 target-i386/cpu.c | 59 +++++++++++++++++++++++++++++++++++--------------------
 target-i386/cpu.h | 12 +++++++++++
 target-i386/kvm.c |  2 +-
 3 files changed, 51 insertions(+), 22 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 1/5] i386: kvm: bit 10 of CPUID[8000_0001].EDX is reserved

[Eduardo Habkost]

Bit 10 of CPUID[8000_0001].EDX is not defined as an alias of
CPUID[1].EDX[10], so do not duplicate it on
kvm_arch_get_supported_cpuid().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-By: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ffc294e..294af5f 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -164,7 +164,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                      * so add missing bits according to the AMD spec:
                      */
                     cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
-                    ret |= cpuid_1_edx & 0x183f7ff;
+                    ret |= cpuid_1_edx & 0x183f3ff;
                     break;
                 }
                 break;
-- 
1.7.11.4

[Qemu-devel] [PATCH 2/5] i386: kvm: use a #define for the set of alias feature bits

[Eduardo Habkost]

Instea of using a hardcoded hex constant, define CPUID_EXT2_AMD_ALIASES
as the set of CPUID[8000_0001].EDX bits that on AMD are the same as the
bits of CPUID[1].EDX.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-By: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.h | 12 ++++++++++++
 target-i386/kvm.c |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index d7ea2f9..4995084 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -409,6 +409,7 @@
 #define CPUID_EXT_HYPERVISOR  (1 << 31)
 
 #define CPUID_EXT2_FPU     (1 << 0)
+#define CPUID_EXT2_VME     (1 << 1)
 #define CPUID_EXT2_DE      (1 << 2)
 #define CPUID_EXT2_PSE     (1 << 3)
 #define CPUID_EXT2_TSC     (1 << 4)
@@ -436,6 +437,17 @@
 #define CPUID_EXT2_3DNOWEXT (1 << 30)
 #define CPUID_EXT2_3DNOW   (1 << 31)
 
+/* CPUID[8000_0001].EDX bits that are aliase of CPUID[1].EDX bits on AMD CPUs */
+#define CPUID_EXT2_AMD_ALIASES (CPUID_EXT2_FPU | CPUID_EXT2_VME | \
+                                CPUID_EXT2_DE | CPUID_EXT2_PSE | \
+                                CPUID_EXT2_TSC | CPUID_EXT2_MSR | \
+                                CPUID_EXT2_PAE | CPUID_EXT2_MCE | \
+                                CPUID_EXT2_CX8 | CPUID_EXT2_APIC | \
+                                CPUID_EXT2_MTRR | CPUID_EXT2_PGE | \
+                                CPUID_EXT2_MCA | CPUID_EXT2_CMOV | \
+                                CPUID_EXT2_PAT | CPUID_EXT2_PSE36 | \
+                                CPUID_EXT2_MMX | CPUID_EXT2_FXSR)
+
 #define CPUID_EXT3_LAHF_LM (1 << 0)
 #define CPUID_EXT3_CMP_LEG (1 << 1)
 #define CPUID_EXT3_SVM     (1 << 2)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 294af5f..895d848 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -164,7 +164,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                      * so add missing bits according to the AMD spec:
                      */
                     cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
-                    ret |= cpuid_1_edx & 0x183f3ff;
+                    ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
                     break;
                 }
                 break;
-- 
1.7.11.4

[Qemu-devel] [PATCH 3/5] i386: cpu: replace EXT2_FEATURE_MASK with CPUID_EXT2_AMD_ALIASES

[Eduardo Habkost]

Both constants have the same value, but CPUID_EXT2_AMD_ALIASES is
defined without using magic numbers.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7c0953f..682895b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -258,7 +258,6 @@ typedef struct x86_def_t {
           CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
           CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
           CPUID_PAE | CPUID_SEP | CPUID_APIC)
-#define EXT2_FEATURE_MASK 0x0183F3FF
 
 #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \
           CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \
@@ -276,7 +275,7 @@ typedef struct x86_def_t {
           /* missing:
           CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_VMX, CPUID_EXT_EST,
           CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_XSAVE */
-#define TCG_EXT2_FEATURES ((TCG_FEATURES & EXT2_FEATURE_MASK) | \
+#define TCG_EXT2_FEATURES ((TCG_FEATURES & CPUID_EXT2_AMD_ALIASES) | \
           CPUID_EXT2_NX | CPUID_EXT2_MMXEXT | CPUID_EXT2_RDTSCP | \
           CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT)
           /* missing:
@@ -305,7 +304,7 @@ static x86_def_t builtin_x86_defs[] = {
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
             CPUID_PSE36,
         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16 | CPUID_EXT_POPCNT,
-        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
+        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
             CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
@@ -325,7 +324,7 @@ static x86_def_t builtin_x86_defs[] = {
             CPUID_PSE36 | CPUID_VME | CPUID_HT,
         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_CX16 |
             CPUID_EXT_POPCNT,
-        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
+        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX |
             CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT | CPUID_EXT2_MMXEXT |
             CPUID_EXT2_FFXSR | CPUID_EXT2_PDPE1GB | CPUID_EXT2_RDTSCP,
@@ -373,7 +372,7 @@ static x86_def_t builtin_x86_defs[] = {
         /* Missing: CPUID_EXT_POPCNT, CPUID_EXT_MONITOR */
         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16,
         /* Missing: CPUID_EXT2_PDPE1GB, CPUID_EXT2_RDTSCP */
-        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
+        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         /* Missing: CPUID_EXT3_LAHF_LM, CPUID_EXT3_CMP_LEG, CPUID_EXT3_EXTAPIC,
                     CPUID_EXT3_CR8LEG, CPUID_EXT3_ABM, CPUID_EXT3_SSE4A,
@@ -402,7 +401,7 @@ static x86_def_t builtin_x86_defs[] = {
         .features = PPRO_FEATURES |
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_PSE36,
         .ext_features = CPUID_EXT_SSE3,
-        .ext2_features = PPRO_FEATURES & EXT2_FEATURE_MASK,
+        .ext2_features = PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES,
         .ext3_features = 0,
         .xlevel = 0x80000008,
         .model_id = "Common 32-bit KVM processor"
@@ -467,8 +466,10 @@ static x86_def_t builtin_x86_defs[] = {
         .family = 6,
         .model = 2,
         .stepping = 3,
-        .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR | CPUID_MCA,
-        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) | CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
+        .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR |
+            CPUID_MCA,
+        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
+            CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
         .xlevel = 0x80000008,
     },
     {
@@ -484,7 +485,8 @@ static x86_def_t builtin_x86_defs[] = {
             /* Some CPUs got no CPUID_SEP */
         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
             CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR,
-        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) | CPUID_EXT2_NX,
+        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
+            CPUID_EXT2_NX,
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000000A,
         .model_id = "Intel(R) Atom(TM) CPU N270   @ 1.60GHz",
-- 
1.7.11.4

[Qemu-devel] [PATCH 4/5] i386: cpu: eliminate duplicate feature names

[Eduardo Habkost]

Instead of having duplicate feature names on the ext2_feature array for
the AMD feature bit aliases, we keep the feature names only on the
feature_name[] array, and copy the corresponding bits to
cpuid_ext2_features in case the CPU vendor is AMD.

This will:

- Make sure we don't set the feature bit aliases on Intel CPUs;
- Make it easier to convert feature bits to CPU properties, as now we
  have a single bit on the x86_def_t struct for each CPU feature.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 682895b..4b65b33 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -61,15 +61,19 @@ static const char *ext_feature_name[] = {
     "tsc-deadline", "aes", "xsave", "osxsave",
     "avx", NULL, NULL, "hypervisor",
 };
+/* Feature names that are already defined on feature_name[] but are set on
+ * CPUID[8000_0001].EDX on AMD CPUs don't have their names on
+ * ext2_feature_name[]. They are copied automatically to cpuid_ext2_features
+ * if and only if CPU vendor is AMD.
+ */
 static const char *ext2_feature_name[] = {
-    "fpu", "vme", "de", "pse",
-    "tsc", "msr", "pae", "mce",
-    "cx8" /* AMD CMPXCHG8B */, "apic", NULL, "syscall",
-    "mtrr", "pge", "mca", "cmov",
-    "pat", "pse36", NULL, NULL /* Linux mp */,
-    "nx|xd", NULL, "mmxext", "mmx",
-    "fxsr", "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
-    NULL, "lm|i64", "3dnowext", "3dnow",
+    NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
+    NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
+    NULL /* cx8 */ /* AMD CMPXCHG8B */, NULL /* apic */, NULL, "syscall",
+    NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
+    NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
+    "nx|xd", NULL, "mmxext", NULL /* mmx */,
+    NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
 };
 static const char *ext3_feature_name[] = {
     "lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD ExtApicSpace */,
@@ -1374,6 +1378,17 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     env->cpuid_xlevel2 = def->xlevel2;
     object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
                             "tsc-frequency", &error);
+
+    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
+     * CPUID[1].EDX.
+     */
+    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
+        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
+    }
+
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
-- 
1.7.11.4

[Qemu-devel] [PATCH 5/5] i386: -cpu help: remove reference to specific CPUID leaves/registers

[Eduardo Habkost]

The -cpu configuration interface is based on a list of feature names or
properties, on a single namespace, so there's no need to mention on
which CPUID leaf/register each flag is located.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4b65b33..ac12139 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1312,13 +1312,13 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     }
     (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
     listflags(buf, sizeof(buf), (uint32_t)~0, feature_name, 1);
-    (*cpu_fprintf)(f, "  f_edx: %s\n", buf);
+    (*cpu_fprintf)(f, "  %s\n", buf);
     listflags(buf, sizeof(buf), (uint32_t)~0, ext_feature_name, 1);
-    (*cpu_fprintf)(f, "  f_ecx: %s\n", buf);
+    (*cpu_fprintf)(f, "  %s\n", buf);
     listflags(buf, sizeof(buf), (uint32_t)~0, ext2_feature_name, 1);
-    (*cpu_fprintf)(f, "  extf_edx: %s\n", buf);
+    (*cpu_fprintf)(f, "  %s\n", buf);
     listflags(buf, sizeof(buf), (uint32_t)~0, ext3_feature_name, 1);
-    (*cpu_fprintf)(f, "  extf_ecx: %s\n", buf);
+    (*cpu_fprintf)(f, "  %s\n", buf);
 }
 
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 1/5] i386: kvm: bit 10 of CPUID[8000_0001].EDX is reserved

[Don Slutz]

On 09/06/12 16:05, Eduardo Habkost wrote:
> Bit 10 of CPUID[8000_0001].EDX is not defined as an alias of
> CPUID[1].EDX[10], so do not duplicate it on
> kvm_arch_get_supported_cpuid().
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-By: Igor Mammedov <imammedo@redhat.com>
> ---
>   target-i386/kvm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ffc294e..294af5f 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -164,7 +164,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>                        * so add missing bits according to the AMD spec:
>                        */
>                       cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
> -                    ret |= cpuid_1_edx & 0x183f7ff;
> +                    ret |= cpuid_1_edx & 0x183f3ff;
>                       break;
>                   }
>                   break;
Reviewed-by: Don Slutz <Don@CloudSwitch.com>

Re: [Qemu-devel] [PATCH 2/5] i386: kvm: use a #define for the set of alias feature bits

[Don Slutz]

On 09/06/12 16:05, Eduardo Habkost wrote:
> Instea of using a hardcoded hex constant, define CPUID_EXT2_AMD_ALIASES
> as the set of CPUID[8000_0001].EDX bits that on AMD are the same as the
> bits of CPUID[1].EDX.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-By: Igor Mammedov <imammedo@redhat.com>
> ---
>   target-i386/cpu.h | 12 ++++++++++++
>   target-i386/kvm.c |  2 +-
>   2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index d7ea2f9..4995084 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -409,6 +409,7 @@
>   #define CPUID_EXT_HYPERVISOR  (1 << 31)
>   
>   #define CPUID_EXT2_FPU     (1 << 0)
> +#define CPUID_EXT2_VME     (1 << 1)
>   #define CPUID_EXT2_DE      (1 << 2)
>   #define CPUID_EXT2_PSE     (1 << 3)
>   #define CPUID_EXT2_TSC     (1 << 4)
> @@ -436,6 +437,17 @@
>   #define CPUID_EXT2_3DNOWEXT (1 << 30)
>   #define CPUID_EXT2_3DNOW   (1 << 31)
>   
> +/* CPUID[8000_0001].EDX bits that are aliase of CPUID[1].EDX bits on AMD CPUs */
> +#define CPUID_EXT2_AMD_ALIASES (CPUID_EXT2_FPU | CPUID_EXT2_VME | \
> +                                CPUID_EXT2_DE | CPUID_EXT2_PSE | \
> +                                CPUID_EXT2_TSC | CPUID_EXT2_MSR | \
> +                                CPUID_EXT2_PAE | CPUID_EXT2_MCE | \
> +                                CPUID_EXT2_CX8 | CPUID_EXT2_APIC | \
> +                                CPUID_EXT2_MTRR | CPUID_EXT2_PGE | \
> +                                CPUID_EXT2_MCA | CPUID_EXT2_CMOV | \
> +                                CPUID_EXT2_PAT | CPUID_EXT2_PSE36 | \
> +                                CPUID_EXT2_MMX | CPUID_EXT2_FXSR)
> +
>   #define CPUID_EXT3_LAHF_LM (1 << 0)
>   #define CPUID_EXT3_CMP_LEG (1 << 1)
>   #define CPUID_EXT3_SVM     (1 << 2)
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 294af5f..895d848 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -164,7 +164,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>                        * so add missing bits according to the AMD spec:
>                        */
>                       cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
> -                    ret |= cpuid_1_edx & 0x183f3ff;
> +                    ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
>                       break;
>                   }
>                   break;
Reviewed-by: Don Slutz <Don@CloudSwitch.com>

Re: [Qemu-devel] [PATCH 3/5] i386: cpu: replace EXT2_FEATURE_MASK with CPUID_EXT2_AMD_ALIASES

[Don Slutz]

On 09/06/12 16:05, Eduardo Habkost wrote:
> Both constants have the same value, but CPUID_EXT2_AMD_ALIASES is
> defined without using magic numbers.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   target-i386/cpu.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7c0953f..682895b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -258,7 +258,6 @@ typedef struct x86_def_t {
>             CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
>             CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
>             CPUID_PAE | CPUID_SEP | CPUID_APIC)
> -#define EXT2_FEATURE_MASK 0x0183F3FF
>   
>   #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \
>             CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \
> @@ -276,7 +275,7 @@ typedef struct x86_def_t {
>             /* missing:
>             CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_VMX, CPUID_EXT_EST,
>             CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_XSAVE */
> -#define TCG_EXT2_FEATURES ((TCG_FEATURES & EXT2_FEATURE_MASK) | \
> +#define TCG_EXT2_FEATURES ((TCG_FEATURES & CPUID_EXT2_AMD_ALIASES) | \
>             CPUID_EXT2_NX | CPUID_EXT2_MMXEXT | CPUID_EXT2_RDTSCP | \
>             CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT)
>             /* missing:
> @@ -305,7 +304,7 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
>               CPUID_PSE36,
>           .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16 | CPUID_EXT_POPCNT,
> -        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
> +        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
>               CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>           .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
>               CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
> @@ -325,7 +324,7 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_PSE36 | CPUID_VME | CPUID_HT,
>           .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_CX16 |
>               CPUID_EXT_POPCNT,
> -        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
> +        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
>               CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX |
>               CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT | CPUID_EXT2_MMXEXT |
>               CPUID_EXT2_FFXSR | CPUID_EXT2_PDPE1GB | CPUID_EXT2_RDTSCP,
> @@ -373,7 +372,7 @@ static x86_def_t builtin_x86_defs[] = {
>           /* Missing: CPUID_EXT_POPCNT, CPUID_EXT_MONITOR */
>           .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16,
>           /* Missing: CPUID_EXT2_PDPE1GB, CPUID_EXT2_RDTSCP */
> -        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
> +        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
>               CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>           /* Missing: CPUID_EXT3_LAHF_LM, CPUID_EXT3_CMP_LEG, CPUID_EXT3_EXTAPIC,
>                       CPUID_EXT3_CR8LEG, CPUID_EXT3_ABM, CPUID_EXT3_SSE4A,
> @@ -402,7 +401,7 @@ static x86_def_t builtin_x86_defs[] = {
>           .features = PPRO_FEATURES |
>               CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_PSE36,
>           .ext_features = CPUID_EXT_SSE3,
> -        .ext2_features = PPRO_FEATURES & EXT2_FEATURE_MASK,
> +        .ext2_features = PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES,
>           .ext3_features = 0,
>           .xlevel = 0x80000008,
>           .model_id = "Common 32-bit KVM processor"
> @@ -467,8 +466,10 @@ static x86_def_t builtin_x86_defs[] = {
>           .family = 6,
>           .model = 2,
>           .stepping = 3,
> -        .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR | CPUID_MCA,
> -        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) | CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
> +        .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR |
> +            CPUID_MCA,
> +        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
> +            CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
>           .xlevel = 0x80000008,
>       },
>       {
> @@ -484,7 +485,8 @@ static x86_def_t builtin_x86_defs[] = {
>               /* Some CPUs got no CPUID_SEP */
>           .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
>               CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR,
> -        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) | CPUID_EXT2_NX,
> +        .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
> +            CPUID_EXT2_NX,
>           .ext3_features = CPUID_EXT3_LAHF_LM,
>           .xlevel = 0x8000000A,
>           .model_id = "Intel(R) Atom(TM) CPU N270   @ 1.60GHz",
Reviewed-by: Don Slutz <Don@CloudSwitch.com>

Re: [Qemu-devel] [PATCH 4/5] i386: cpu: eliminate duplicate feature names

[Don Slutz]

On 09/06/12 16:05, Eduardo Habkost wrote:
> Instead of having duplicate feature names on the ext2_feature array for
> the AMD feature bit aliases, we keep the feature names only on the
> feature_name[] array, and copy the corresponding bits to
> cpuid_ext2_features in case the CPU vendor is AMD.
>
> This will:
>
> - Make sure we don't set the feature bit aliases on Intel CPUs;
> - Make it easier to convert feature bits to CPU properties, as now we
>    have a single bit on the x86_def_t struct for each CPU feature.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   target-i386/cpu.c | 31 +++++++++++++++++++++++--------
>   1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 682895b..4b65b33 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -61,15 +61,19 @@ static const char *ext_feature_name[] = {
>       "tsc-deadline", "aes", "xsave", "osxsave",
>       "avx", NULL, NULL, "hypervisor",
>   };
> +/* Feature names that are already defined on feature_name[] but are set on
> + * CPUID[8000_0001].EDX on AMD CPUs don't have their names on
> + * ext2_feature_name[]. They are copied automatically to cpuid_ext2_features
> + * if and only if CPU vendor is AMD.
> + */
>   static const char *ext2_feature_name[] = {
> -    "fpu", "vme", "de", "pse",
> -    "tsc", "msr", "pae", "mce",
> -    "cx8" /* AMD CMPXCHG8B */, "apic", NULL, "syscall",
> -    "mtrr", "pge", "mca", "cmov",
> -    "pat", "pse36", NULL, NULL /* Linux mp */,
> -    "nx|xd", NULL, "mmxext", "mmx",
> -    "fxsr", "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
> -    NULL, "lm|i64", "3dnowext", "3dnow",
> +    NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
> +    NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
> +    NULL /* cx8 */ /* AMD CMPXCHG8B */, NULL /* apic */, NULL, "syscall",
> +    NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
> +    NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
> +    "nx|xd", NULL, "mmxext", NULL /* mmx */,
> +    NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
>   };
>   static const char *ext3_feature_name[] = {
>       "lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD ExtApicSpace */,
> @@ -1374,6 +1378,17 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>       env->cpuid_xlevel2 = def->xlevel2;
>       object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
>                               "tsc-frequency", &error);
> +
> +    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> +     * CPUID[1].EDX.
> +     */
> +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> +            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> +            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> +        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> +        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
> +    }
> +
>       if (!kvm_enabled()) {
>           env->cpuid_features &= TCG_FEATURES;
>           env->cpuid_ext_features &= TCG_EXT_FEATURES;
Reviewed-by: Don Slutz <Don@CloudSwitch.com>

Re: [Qemu-devel] [PATCH 5/5] i386: -cpu help: remove reference to specific CPUID leaves/registers

[Don Slutz]

On 09/06/12 16:05, Eduardo Habkost wrote:
> The -cpu configuration interface is based on a list of feature names or
> properties, on a single namespace, so there's no need to mention on
> which CPUID leaf/register each flag is located.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   target-i386/cpu.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4b65b33..ac12139 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1312,13 +1312,13 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>       }
>       (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
>       listflags(buf, sizeof(buf), (uint32_t)~0, feature_name, 1);
> -    (*cpu_fprintf)(f, "  f_edx: %s\n", buf);
> +    (*cpu_fprintf)(f, "  %s\n", buf);
>       listflags(buf, sizeof(buf), (uint32_t)~0, ext_feature_name, 1);
> -    (*cpu_fprintf)(f, "  f_ecx: %s\n", buf);
> +    (*cpu_fprintf)(f, "  %s\n", buf);
>       listflags(buf, sizeof(buf), (uint32_t)~0, ext2_feature_name, 1);
> -    (*cpu_fprintf)(f, "  extf_edx: %s\n", buf);
> +    (*cpu_fprintf)(f, "  %s\n", buf);
>       listflags(buf, sizeof(buf), (uint32_t)~0, ext3_feature_name, 1);
> -    (*cpu_fprintf)(f, "  extf_ecx: %s\n", buf);
> +    (*cpu_fprintf)(f, "  %s\n", buf);
>   }
>   
>   CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
Reviewed-by: Don Slutz <Don@CloudSwitch.com>

Re: [Qemu-devel] [PATCH 0/5] i386: cpu: remove duplicate feature names

[Igor Mammedov]

On Thu,  6 Sep 2012 17:05:34 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The problem:
> 
>  - Some features are report at the same time on both CPUID[1].EDX and
>    CPUID[8000_0001].EDX on AMD CPUs (e.g. fpu, tsc, msr, pae, mmx).
>  - "-cpu <model>,+feature" should enable the bit only on CPUID[1] if
>    it's not an AMD CPU, but it should enable the bit on both CPUID[1] and
>    CPUID[8000_0001] if it's an AMD CPU.
>  - The same should happen when implementing CPU properties: setting the
>    property that enables a feature should set the duplicate
> CPUID[8000_0001].EDX bit only if CPU vendor is AMD.
> 
> Reference: http://article.gmane.org/gmane.comp.emulators.qemu/166024
> 
> The solution implemented by this series is:
>  - On the CPU model table and while parsing CPU options/properties, set the
> bit only on CPUID[1] (the x86_def_t.features field).
>  - When finishing initialization of the CPU cpuid fields, duplicate those
>    feature bits on cpuid_ext2_features if and only if the CPU vendor is AMD.
> 
> This series depends on the "x86 CPU patches that didn't get into 1.2"
> series: http://article.gmane.org/gmane.comp.emulators.qemu/168633
>   Message-Id: <1346877673-9136-1-git-send-email-ehabkost@redhat.com>
> 
> 
> Eduardo Habkost (5):
>   i386: kvm: bit 10 of CPUID[8000_0001].EDX is reserved
>   i386: kvm: use a #define for the set of alias feature bits
>   i386: cpu: replace EXT2_FEATURE_MASK with CPUID_EXT2_AMD_ALIASES
>   i386: cpu: eliminate duplicate feature names
>   i386: -cpu help: remove reference to specific CPUID leaves/registers
> 
>  target-i386/cpu.c | 59
> +++++++++++++++++++++++++++++++++++-------------------- target-i386/cpu.h |
> 12 +++++++++++ target-i386/kvm.c |  2 +-
>  3 files changed, 51 insertions(+), 22 deletions(-)
> 

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [PATCH 0/5] i386: cpu: remove duplicate feature names

[Blue Swirl]

On Thu, Sep 6, 2012 at 8:05 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The problem:
>
>  - Some features are report at the same time on both CPUID[1].EDX and
>    CPUID[8000_0001].EDX on AMD CPUs (e.g. fpu, tsc, msr, pae, mmx).
>  - "-cpu <model>,+feature" should enable the bit only on CPUID[1] if
>    it's not an AMD CPU, but it should enable the bit on both CPUID[1] and
>    CPUID[8000_0001] if it's an AMD CPU.
>  - The same should happen when implementing CPU properties: setting the
>    property that enables a feature should set the duplicate CPUID[8000_0001].EDX
>    bit only if CPU vendor is AMD.
>
> Reference: http://article.gmane.org/gmane.comp.emulators.qemu/166024
>
> The solution implemented by this series is:
>  - On the CPU model table and while parsing CPU options/properties, set the bit
>    only on CPUID[1] (the x86_def_t.features field).
>  - When finishing initialization of the CPU cpuid fields, duplicate those
>    feature bits on cpuid_ext2_features if and only if the CPU vendor is AMD.
>
> This series depends on the "x86 CPU patches that didn't get into 1.2" series:
>   http://article.gmane.org/gmane.comp.emulators.qemu/168633
>   Message-Id: <1346877673-9136-1-git-send-email-ehabkost@redhat.com>

Thanks, applied all.

>
>
> Eduardo Habkost (5):
>   i386: kvm: bit 10 of CPUID[8000_0001].EDX is reserved
>   i386: kvm: use a #define for the set of alias feature bits
>   i386: cpu: replace EXT2_FEATURE_MASK with CPUID_EXT2_AMD_ALIASES
>   i386: cpu: eliminate duplicate feature names
>   i386: -cpu help: remove reference to specific CPUID leaves/registers
>
>  target-i386/cpu.c | 59 +++++++++++++++++++++++++++++++++++--------------------
>  target-i386/cpu.h | 12 +++++++++++
>  target-i386/kvm.c |  2 +-
>  3 files changed, 51 insertions(+), 22 deletions(-)
>
> --
> 1.7.11.4
>
>