[Qemu-devel] [RFC 0/6] i386: CPU: remove duplicate feature names

[Qemu-devel] [RFC 0/6] 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 also removes the "cpudef" config support, to make this work easier
(because the cpudef interface is based on low-level CPUID leaf+register
specification, instead of a set of higher-level per-feature object properties).

Eduardo Habkost (6):
  x86_cpudef_setup: coding style change
  i386: kill cpudef config section support
  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: eliminate duplicate feature names
  i386: -cpu help: remove reference to specific CPUID leaves/registers

 target-i386/cpu.c | 153 +++++++++++++-----------------------------------------
 target-i386/cpu.h |  12 +++++
 target-i386/kvm.c |   2 +-
 3 files changed, 50 insertions(+), 117 deletions(-)

-- 
1.7.11.2

[Qemu-devel] [RFC 1/6] x86_cpudef_setup: coding style change

[Eduardo Habkost]

Make source code lines shorter.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 96ec9e6..519a104 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1500,20 +1500,23 @@ void x86_cpudef_setup(void)
     static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
 
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
-        builtin_x86_defs[i].next = x86_defs;
-        builtin_x86_defs[i].flags = 1;
+        x86_def_t *def = &builtin_x86_defs[i];
+        def->next = x86_defs;
+        def->flags = 1;
 
         /* Look for specific "cpudef" models that */
         /* have the QEMU version in .model_id */
         for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) {
-            if (strcmp(model_with_versions[j], builtin_x86_defs[i].name) == 0) {
-                pstrcpy(builtin_x86_defs[i].model_id, sizeof(builtin_x86_defs[i].model_id), "QEMU Virtual CPU version ");
-                pstrcat(builtin_x86_defs[i].model_id, sizeof(builtin_x86_defs[i].model_id), qemu_get_version());
+            if (strcmp(model_with_versions[j], def->name) == 0) {
+                pstrcpy(def->model_id, sizeof(def->model_id),
+                        "QEMU Virtual CPU version ");
+                pstrcat(def->model_id, sizeof(def->model_id),
+                        qemu_get_version());
                 break;
             }
         }
 
-        x86_defs = &builtin_x86_defs[i];
+        x86_defs = def;
     }
 #if !defined(CONFIG_USER_ONLY)
     qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);
-- 
1.7.11.2

[Qemu-devel] [RFC 2/6] i386: kill cpudef config section support

[Eduardo Habkost]

It's nice to have a flexible system to maintain CPU models as data, but
this is holding us from making improvements in the CPU code because it's
not using the common infra-structure, and because the machine-type data
is still inside C code.

Users who want to configure CPU features directly may simply use the
"-cpu" command-line option (and maybe an equivalent -device option in
the future) to set CPU features.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 519a104..71c2ee7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -237,7 +237,6 @@ typedef struct x86_def_t {
     uint32_t xlevel;
     char model_id[48];
     int vendor_override;
-    uint32_t flags;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     char buf[256];
 
     for (def = x86_defs; def; def = def->next) {
-        snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name);
+        snprintf(buf, sizeof(buf), "%s", def->name);
         (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
     }
     if (kvm_enabled()) {
@@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 }
 
 #if !defined(CONFIG_USER_ONLY)
-/* copy vendor id string to 32 bit register, nul pad as needed
- */
-static void cpyid(const char *s, uint32_t *id)
-{
-    char *d = (char *)id;
-    char i;
-
-    for (i = sizeof (*id); i--; )
-        *d++ = *s ? *s++ : '\0';
-}
 
 /* interpret radix and convert from string to arbitrary scalar,
  * otherwise flag failure
@@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id)
     *str && !*pend ? (*pval = ul) : (*perr = 1);        \
 }
 
-/* map cpuid options to feature bits, otherwise return failure
- * (option tags in *str are delimited by whitespace)
- */
-static void setfeatures(uint32_t *pval, const char *str,
-    const char **featureset, int *perr)
-{
-    const char *p, *q;
-
-    for (q = p = str; *p || *q; q = p) {
-        while (iswhite(*p))
-            q = ++p;
-        while (*p && !iswhite(*p))
-            ++p;
-        if (!*q && !*p)
-            return;
-        if (!lookup_feature(pval, q, p, featureset)) {
-            fprintf(stderr, "error: feature \"%.*s\" not available in set\n",
-                (int)(p - q), q);
-            *perr = 1;
-            return;
-        }
-    }
-}
-
-/* map config file options to x86_def_t form
- */
-static int cpudef_setfield(const char *name, const char *str, void *opaque)
-{
-    x86_def_t *def = opaque;
-    int err = 0;
-
-    if (!strcmp(name, "name")) {
-        g_free((void *)def->name);
-        def->name = g_strdup(str);
-    } else if (!strcmp(name, "model_id")) {
-        strncpy(def->model_id, str, sizeof (def->model_id));
-    } else if (!strcmp(name, "level")) {
-        setscalar(&def->level, str, &err)
-    } else if (!strcmp(name, "vendor")) {
-        cpyid(&str[0], &def->vendor1);
-        cpyid(&str[4], &def->vendor2);
-        cpyid(&str[8], &def->vendor3);
-    } else if (!strcmp(name, "family")) {
-        setscalar(&def->family, str, &err)
-    } else if (!strcmp(name, "model")) {
-        setscalar(&def->model, str, &err)
-    } else if (!strcmp(name, "stepping")) {
-        setscalar(&def->stepping, str, &err)
-    } else if (!strcmp(name, "feature_edx")) {
-        setfeatures(&def->features, str, feature_name, &err);
-    } else if (!strcmp(name, "feature_ecx")) {
-        setfeatures(&def->ext_features, str, ext_feature_name, &err);
-    } else if (!strcmp(name, "extfeature_edx")) {
-        setfeatures(&def->ext2_features, str, ext2_feature_name, &err);
-    } else if (!strcmp(name, "extfeature_ecx")) {
-        setfeatures(&def->ext3_features, str, ext3_feature_name, &err);
-    } else if (!strcmp(name, "xlevel")) {
-        setscalar(&def->xlevel, str, &err)
-    } else {
-        fprintf(stderr, "error: unknown option [%s = %s]\n", name, str);
-        return (1);
-    }
-    if (err) {
-        fprintf(stderr, "error: bad option value [%s = %s]\n", name, str);
-        return (1);
-    }
-    return (0);
-}
-
-/* register config file entry as x86_def_t
- */
-static int cpudef_register(QemuOpts *opts, void *opaque)
-{
-    x86_def_t *def = g_malloc0(sizeof (x86_def_t));
-
-    qemu_opt_foreach(opts, cpudef_setfield, def, 1);
-    def->next = x86_defs;
-    x86_defs = def;
-    return (0);
-}
-
 void cpu_clear_apic_feature(CPUX86State *env)
 {
     env->cpuid_features &= ~CPUID_APIC;
@@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
 
 #endif /* !CONFIG_USER_ONLY */
 
-/* register "cpudef" models defined in configuration file.  Here we first
- * preload any built-in definitions
+/* Initialize list of CPU models, filling some non-static fields if necessary
  */
 void x86_cpudef_setup(void)
 {
@@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void)
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
         x86_def_t *def = &builtin_x86_defs[i];
         def->next = x86_defs;
-        def->flags = 1;
 
         /* Look for specific "cpudef" models that */
         /* have the QEMU version in .model_id */
@@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void)
 
         x86_defs = def;
     }
-#if !defined(CONFIG_USER_ONLY)
-    qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);
-#endif
 }
 
 static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
-- 
1.7.11.2

[Qemu-devel] [RFC 3/6] 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>
---
 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 696b14a..bab1ef8 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -163,7 +163,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.2

[Qemu-devel] [RFC 4/6] 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>
---
 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 7b6340c..ba147ac 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 bab1ef8..63dac56 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -163,7 +163,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.2

[Qemu-devel] [RFC 5/6] 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 71c2ee7..a71c2fc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -58,15 +58,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 */,
@@ -1359,6 +1363,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.2

[Qemu-devel] [RFC 6/6] 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 a71c2fc..0a03c80 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1297,13 +1297,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 *qmp_query_cpu_definitions(Error **errp)
-- 
1.7.11.2

Re: [Qemu-devel] [RFC 2/6] i386: kill cpudef config section support

[Andreas Färber]

Am 17.08.2012 19:53, schrieb Eduardo Habkost:
> It's nice to have a flexible system to maintain CPU models as data, but
> this is holding us from making improvements in the CPU code because it's
> not using the common infra-structure, and because the machine-type data
> is still inside C code.
> 
> Users who want to configure CPU features directly may simply use the
> "-cpu" command-line option (and maybe an equivalent -device option in
> the future) to set CPU features.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 101 ++----------------------------------------------------
>  1 file changed, 2 insertions(+), 99 deletions(-)

Thanks for looking into this! I didn't get around to it myself yet.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 2/6] i386: kill cpudef config section support

[Igor Mammedov]

On Fri, 17 Aug 2012 14:53:38 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> It's nice to have a flexible system to maintain CPU models as data, but
> this is holding us from making improvements in the CPU code because it's
> not using the common infra-structure, and because the machine-type data
> is still inside C code.
> 
> Users who want to configure CPU features directly may simply use the
> "-cpu" command-line option (and maybe an equivalent -device option in
> the future) to set CPU features.
Haven't we agreed on a kvm call that config cpu section would be marked
obsolete in next release and only after that to be removed?

if we'll target this patch for 1.3 then I'll rebase CPU propeties on top of
this series.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 101 ++----------------------------------------------------
>  1 file changed, 2 insertions(+), 99 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 519a104..71c2ee7 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -237,7 +237,6 @@ typedef struct x86_def_t {
>      uint32_t xlevel;
>      char model_id[48];
>      int vendor_override;
> -    uint32_t flags;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t ext4_features;
>      uint32_t xlevel2;
> @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>      char buf[256];
>  
>      for (def = x86_defs; def; def = def->next) {
> -        snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name);
> +        snprintf(buf, sizeof(buf), "%s", def->name);
>          (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
>      }
>      if (kvm_enabled()) {
> @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -/* copy vendor id string to 32 bit register, nul pad as needed
> - */
> -static void cpyid(const char *s, uint32_t *id)
> -{
> -    char *d = (char *)id;
> -    char i;
> -
> -    for (i = sizeof (*id); i--; )
> -        *d++ = *s ? *s++ : '\0';
> -}
>  
>  /* interpret radix and convert from string to arbitrary scalar,
>   * otherwise flag failure
> @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id)
>      *str && !*pend ? (*pval = ul) : (*perr = 1);        \
>  }
>  
> -/* map cpuid options to feature bits, otherwise return failure
> - * (option tags in *str are delimited by whitespace)
> - */
> -static void setfeatures(uint32_t *pval, const char *str,
> -    const char **featureset, int *perr)
> -{
> -    const char *p, *q;
> -
> -    for (q = p = str; *p || *q; q = p) {
> -        while (iswhite(*p))
> -            q = ++p;
> -        while (*p && !iswhite(*p))
> -            ++p;
> -        if (!*q && !*p)
> -            return;
> -        if (!lookup_feature(pval, q, p, featureset)) {
> -            fprintf(stderr, "error: feature \"%.*s\" not available in set\n",
> -                (int)(p - q), q);
> -            *perr = 1;
> -            return;
> -        }
> -    }
> -}
> -
> -/* map config file options to x86_def_t form
> - */
> -static int cpudef_setfield(const char *name, const char *str, void *opaque)
> -{
> -    x86_def_t *def = opaque;
> -    int err = 0;
> -
> -    if (!strcmp(name, "name")) {
> -        g_free((void *)def->name);
> -        def->name = g_strdup(str);
> -    } else if (!strcmp(name, "model_id")) {
> -        strncpy(def->model_id, str, sizeof (def->model_id));
> -    } else if (!strcmp(name, "level")) {
> -        setscalar(&def->level, str, &err)
> -    } else if (!strcmp(name, "vendor")) {
> -        cpyid(&str[0], &def->vendor1);
> -        cpyid(&str[4], &def->vendor2);
> -        cpyid(&str[8], &def->vendor3);
> -    } else if (!strcmp(name, "family")) {
> -        setscalar(&def->family, str, &err)
> -    } else if (!strcmp(name, "model")) {
> -        setscalar(&def->model, str, &err)
> -    } else if (!strcmp(name, "stepping")) {
> -        setscalar(&def->stepping, str, &err)
> -    } else if (!strcmp(name, "feature_edx")) {
> -        setfeatures(&def->features, str, feature_name, &err);
> -    } else if (!strcmp(name, "feature_ecx")) {
> -        setfeatures(&def->ext_features, str, ext_feature_name, &err);
> -    } else if (!strcmp(name, "extfeature_edx")) {
> -        setfeatures(&def->ext2_features, str, ext2_feature_name, &err);
> -    } else if (!strcmp(name, "extfeature_ecx")) {
> -        setfeatures(&def->ext3_features, str, ext3_feature_name, &err);
> -    } else if (!strcmp(name, "xlevel")) {
> -        setscalar(&def->xlevel, str, &err)
> -    } else {
> -        fprintf(stderr, "error: unknown option [%s = %s]\n", name, str);
> -        return (1);
> -    }
> -    if (err) {
> -        fprintf(stderr, "error: bad option value [%s = %s]\n", name, str);
> -        return (1);
> -    }
> -    return (0);
> -}
> -
> -/* register config file entry as x86_def_t
> - */
> -static int cpudef_register(QemuOpts *opts, void *opaque)
> -{
> -    x86_def_t *def = g_malloc0(sizeof (x86_def_t));
> -
> -    qemu_opt_foreach(opts, cpudef_setfield, def, 1);
> -    def->next = x86_defs;
> -    x86_defs = def;
> -    return (0);
> -}
> -
>  void cpu_clear_apic_feature(CPUX86State *env)
>  {
>      env->cpuid_features &= ~CPUID_APIC;
> @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
>  
>  #endif /* !CONFIG_USER_ONLY */
>  
> -/* register "cpudef" models defined in configuration file.  Here we first
> - * preload any built-in definitions
> +/* Initialize list of CPU models, filling some non-static fields if necessary
>   */
>  void x86_cpudef_setup(void)
>  {
> @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void)
>      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
>          x86_def_t *def = &builtin_x86_defs[i];
>          def->next = x86_defs;
> -        def->flags = 1;
>  
>          /* Look for specific "cpudef" models that */
>          /* have the QEMU version in .model_id */
> @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void)
>  
>          x86_defs = def;
>      }
> -#if !defined(CONFIG_USER_ONLY)
> -    qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);
> -#endif
>  }
>  
>  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
> -- 
> 1.7.11.2
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 0/6] i386: CPU: remove duplicate feature names

[Igor Mammedov]

On Fri, 17 Aug 2012 14:53:36 -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 also removes the "cpudef" config support, to make this work easier
> (because the cpudef interface is based on low-level CPUID leaf+register
> specification, instead of a set of higher-level per-feature object properties).
> 
> Eduardo Habkost (6):
>   x86_cpudef_setup: coding style change
>   i386: kill cpudef config section support

>   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: eliminate duplicate feature names
above 3 patches should go before cpu properties

>   i386: -cpu help: remove reference to specific CPUID leaves/registers
> 
>  target-i386/cpu.c | 153 +++++++++++++-----------------------------------------
>  target-i386/cpu.h |  12 +++++
>  target-i386/kvm.c |   2 +-
>  3 files changed, 50 insertions(+), 117 deletions(-)
> 
> -- 
> 1.7.11.2
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 2/6] i386: kill cpudef config section support

[Eduardo Habkost]

On Mon, Aug 20, 2012 at 02:00:14PM +0200, Igor Mammedov wrote:
> On Fri, 17 Aug 2012 14:53:38 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > It's nice to have a flexible system to maintain CPU models as data, but
> > this is holding us from making improvements in the CPU code because it's
> > not using the common infra-structure, and because the machine-type data
> > is still inside C code.
> > 
> > Users who want to configure CPU features directly may simply use the
> > "-cpu" command-line option (and maybe an equivalent -device option in
> > the future) to set CPU features.
> Haven't we agreed on a kvm call that config cpu section would be marked
> obsolete in next release and only after that to be removed?

Yes. If I recall correctly, it will be deprecated on 1.2 and removed on
1.3.


> 
> if we'll target this patch for 1.3 then I'll rebase CPU propeties on top of
> this series.

That was my plan, yes.


> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 101 ++----------------------------------------------------
> >  1 file changed, 2 insertions(+), 99 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 519a104..71c2ee7 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -237,7 +237,6 @@ typedef struct x86_def_t {
> >      uint32_t xlevel;
> >      char model_id[48];
> >      int vendor_override;
> > -    uint32_t flags;
> >      /* Store the results of Centaur's CPUID instructions */
> >      uint32_t ext4_features;
> >      uint32_t xlevel2;
> > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> >      char buf[256];
> >  
> >      for (def = x86_defs; def; def = def->next) {
> > -        snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name);
> > +        snprintf(buf, sizeof(buf), "%s", def->name);
> >          (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
> >      }
> >      if (kvm_enabled()) {
> > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -/* copy vendor id string to 32 bit register, nul pad as needed
> > - */
> > -static void cpyid(const char *s, uint32_t *id)
> > -{
> > -    char *d = (char *)id;
> > -    char i;
> > -
> > -    for (i = sizeof (*id); i--; )
> > -        *d++ = *s ? *s++ : '\0';
> > -}
> >  
> >  /* interpret radix and convert from string to arbitrary scalar,
> >   * otherwise flag failure
> > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id)
> >      *str && !*pend ? (*pval = ul) : (*perr = 1);        \
> >  }
> >  
> > -/* map cpuid options to feature bits, otherwise return failure
> > - * (option tags in *str are delimited by whitespace)
> > - */
> > -static void setfeatures(uint32_t *pval, const char *str,
> > -    const char **featureset, int *perr)
> > -{
> > -    const char *p, *q;
> > -
> > -    for (q = p = str; *p || *q; q = p) {
> > -        while (iswhite(*p))
> > -            q = ++p;
> > -        while (*p && !iswhite(*p))
> > -            ++p;
> > -        if (!*q && !*p)
> > -            return;
> > -        if (!lookup_feature(pval, q, p, featureset)) {
> > -            fprintf(stderr, "error: feature \"%.*s\" not available in set\n",
> > -                (int)(p - q), q);
> > -            *perr = 1;
> > -            return;
> > -        }
> > -    }
> > -}
> > -
> > -/* map config file options to x86_def_t form
> > - */
> > -static int cpudef_setfield(const char *name, const char *str, void *opaque)
> > -{
> > -    x86_def_t *def = opaque;
> > -    int err = 0;
> > -
> > -    if (!strcmp(name, "name")) {
> > -        g_free((void *)def->name);
> > -        def->name = g_strdup(str);
> > -    } else if (!strcmp(name, "model_id")) {
> > -        strncpy(def->model_id, str, sizeof (def->model_id));
> > -    } else if (!strcmp(name, "level")) {
> > -        setscalar(&def->level, str, &err)
> > -    } else if (!strcmp(name, "vendor")) {
> > -        cpyid(&str[0], &def->vendor1);
> > -        cpyid(&str[4], &def->vendor2);
> > -        cpyid(&str[8], &def->vendor3);
> > -    } else if (!strcmp(name, "family")) {
> > -        setscalar(&def->family, str, &err)
> > -    } else if (!strcmp(name, "model")) {
> > -        setscalar(&def->model, str, &err)
> > -    } else if (!strcmp(name, "stepping")) {
> > -        setscalar(&def->stepping, str, &err)
> > -    } else if (!strcmp(name, "feature_edx")) {
> > -        setfeatures(&def->features, str, feature_name, &err);
> > -    } else if (!strcmp(name, "feature_ecx")) {
> > -        setfeatures(&def->ext_features, str, ext_feature_name, &err);
> > -    } else if (!strcmp(name, "extfeature_edx")) {
> > -        setfeatures(&def->ext2_features, str, ext2_feature_name, &err);
> > -    } else if (!strcmp(name, "extfeature_ecx")) {
> > -        setfeatures(&def->ext3_features, str, ext3_feature_name, &err);
> > -    } else if (!strcmp(name, "xlevel")) {
> > -        setscalar(&def->xlevel, str, &err)
> > -    } else {
> > -        fprintf(stderr, "error: unknown option [%s = %s]\n", name, str);
> > -        return (1);
> > -    }
> > -    if (err) {
> > -        fprintf(stderr, "error: bad option value [%s = %s]\n", name, str);
> > -        return (1);
> > -    }
> > -    return (0);
> > -}
> > -
> > -/* register config file entry as x86_def_t
> > - */
> > -static int cpudef_register(QemuOpts *opts, void *opaque)
> > -{
> > -    x86_def_t *def = g_malloc0(sizeof (x86_def_t));
> > -
> > -    qemu_opt_foreach(opts, cpudef_setfield, def, 1);
> > -    def->next = x86_defs;
> > -    x86_defs = def;
> > -    return (0);
> > -}
> > -
> >  void cpu_clear_apic_feature(CPUX86State *env)
> >  {
> >      env->cpuid_features &= ~CPUID_APIC;
> > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
> >  
> >  #endif /* !CONFIG_USER_ONLY */
> >  
> > -/* register "cpudef" models defined in configuration file.  Here we first
> > - * preload any built-in definitions
> > +/* Initialize list of CPU models, filling some non-static fields if necessary
> >   */
> >  void x86_cpudef_setup(void)
> >  {
> > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void)
> >      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
> >          x86_def_t *def = &builtin_x86_defs[i];
> >          def->next = x86_defs;
> > -        def->flags = 1;
> >  
> >          /* Look for specific "cpudef" models that */
> >          /* have the QEMU version in .model_id */
> > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void)
> >  
> >          x86_defs = def;
> >      }
> > -#if !defined(CONFIG_USER_ONLY)
> > -    qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);
> > -#endif
> >  }
> >  
> >  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
> > -- 
> > 1.7.11.2
> > 
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [RFC 1/6] x86_cpudef_setup: coding style change

[Igor Mammedov]

On Fri, 17 Aug 2012 14:53:37 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Make source code lines shorter.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 96ec9e6..519a104 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1500,20 +1500,23 @@ void x86_cpudef_setup(void)
>      static const char *model_with_versions[] = { "qemu32", "qemu64",
> "athlon" }; 
>      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
> -        builtin_x86_defs[i].next = x86_defs;
> -        builtin_x86_defs[i].flags = 1;
> +        x86_def_t *def = &builtin_x86_defs[i];
> +        def->next = x86_defs;
> +        def->flags = 1;
>  
>          /* Look for specific "cpudef" models that */
>          /* have the QEMU version in .model_id */
>          for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) {
> -            if (strcmp(model_with_versions[j], builtin_x86_defs[i].name)
> == 0) {
> -                pstrcpy(builtin_x86_defs[i].model_id,
> sizeof(builtin_x86_defs[i].model_id), "QEMU Virtual CPU version ");
> -                pstrcat(builtin_x86_defs[i].model_id,
> sizeof(builtin_x86_defs[i].model_id), qemu_get_version());
> +            if (strcmp(model_with_versions[j], def->name) == 0) {
> +                pstrcpy(def->model_id, sizeof(def->model_id),
> +                        "QEMU Virtual CPU version ");
> +                pstrcat(def->model_id, sizeof(def->model_id),
> +                        qemu_get_version());
>                  break;
>              }
>          }
>  
> -        x86_defs = &builtin_x86_defs[i];
> +        x86_defs = def;
>      }
>  #if !defined(CONFIG_USER_ONLY)
>      qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);

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

Re: [Qemu-devel] [RFC 3/6] i386: kvm: bit 10 of CPUID[8000_0001].EDX is reserved

[Igor Mammedov]

On Fri, 17 Aug 2012 14:53:39 -0300
Eduardo Habkost <ehabkost@redhat.com> 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>
> ---
>  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 696b14a..bab1ef8 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -163,7 +163,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: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [RFC 4/6] i386: kvm: use a #define for the set of alias feature bits

[Igor Mammedov]

On Fri, 17 Aug 2012 14:53:40 -0300
Eduardo Habkost <ehabkost@redhat.com> 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>
> ---
>  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 7b6340c..ba147ac 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 bab1ef8..63dac56 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -163,7 +163,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: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [RFC 2/6] i386: kill cpudef config section support

[Igor Mammedov]

On Fri, 17 Aug 2012 14:53:38 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> It's nice to have a flexible system to maintain CPU models as data, but
> this is holding us from making improvements in the CPU code because it's
> not using the common infra-structure, and because the machine-type data
> is still inside C code.
> 
> Users who want to configure CPU features directly may simply use the
> "-cpu" command-line option (and maybe an equivalent -device option in
> the future) to set CPU features.
>
Have I missed a patch that moves CPU definitions from cpus-x86_64.conf into
the code?
Perhaps they should be added to builtin_x86_defs in this patch.

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 101
> ++---------------------------------------------------- 1 file changed, 2
> insertions(+), 99 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 519a104..71c2ee7 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -237,7 +237,6 @@ typedef struct x86_def_t {
>      uint32_t xlevel;
>      char model_id[48];
>      int vendor_override;
> -    uint32_t flags;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t ext4_features;
>      uint32_t xlevel2;
> @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function
> cpu_fprintf) char buf[256];
>  
>      for (def = x86_defs; def; def = def->next) {
> -        snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name);
> +        snprintf(buf, sizeof(buf), "%s", def->name);
>          (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
>      }
>      if (kvm_enabled()) {
> @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> *cpu_model) }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -/* copy vendor id string to 32 bit register, nul pad as needed
> - */
> -static void cpyid(const char *s, uint32_t *id)
> -{
> -    char *d = (char *)id;
> -    char i;
> -
> -    for (i = sizeof (*id); i--; )
> -        *d++ = *s ? *s++ : '\0';
> -}
>  
>  /* interpret radix and convert from string to arbitrary scalar,
>   * otherwise flag failure
> @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id)
>      *str && !*pend ? (*pval = ul) : (*perr = 1);        \
>  }
>  
> -/* map cpuid options to feature bits, otherwise return failure
> - * (option tags in *str are delimited by whitespace)
> - */
> -static void setfeatures(uint32_t *pval, const char *str,
> -    const char **featureset, int *perr)
> -{
> -    const char *p, *q;
> -
> -    for (q = p = str; *p || *q; q = p) {
> -        while (iswhite(*p))
> -            q = ++p;
> -        while (*p && !iswhite(*p))
> -            ++p;
> -        if (!*q && !*p)
> -            return;
> -        if (!lookup_feature(pval, q, p, featureset)) {
> -            fprintf(stderr, "error: feature \"%.*s\" not available in
> set\n",
> -                (int)(p - q), q);
> -            *perr = 1;
> -            return;
> -        }
> -    }
> -}
> -
> -/* map config file options to x86_def_t form
> - */
> -static int cpudef_setfield(const char *name, const char *str, void *opaque)
> -{
> -    x86_def_t *def = opaque;
> -    int err = 0;
> -
> -    if (!strcmp(name, "name")) {
> -        g_free((void *)def->name);
> -        def->name = g_strdup(str);
> -    } else if (!strcmp(name, "model_id")) {
> -        strncpy(def->model_id, str, sizeof (def->model_id));
> -    } else if (!strcmp(name, "level")) {
> -        setscalar(&def->level, str, &err)
> -    } else if (!strcmp(name, "vendor")) {
> -        cpyid(&str[0], &def->vendor1);
> -        cpyid(&str[4], &def->vendor2);
> -        cpyid(&str[8], &def->vendor3);
> -    } else if (!strcmp(name, "family")) {
> -        setscalar(&def->family, str, &err)
> -    } else if (!strcmp(name, "model")) {
> -        setscalar(&def->model, str, &err)
> -    } else if (!strcmp(name, "stepping")) {
> -        setscalar(&def->stepping, str, &err)
> -    } else if (!strcmp(name, "feature_edx")) {
> -        setfeatures(&def->features, str, feature_name, &err);
> -    } else if (!strcmp(name, "feature_ecx")) {
> -        setfeatures(&def->ext_features, str, ext_feature_name, &err);
> -    } else if (!strcmp(name, "extfeature_edx")) {
> -        setfeatures(&def->ext2_features, str, ext2_feature_name, &err);
> -    } else if (!strcmp(name, "extfeature_ecx")) {
> -        setfeatures(&def->ext3_features, str, ext3_feature_name, &err);
> -    } else if (!strcmp(name, "xlevel")) {
> -        setscalar(&def->xlevel, str, &err)
> -    } else {
> -        fprintf(stderr, "error: unknown option [%s = %s]\n", name, str);
> -        return (1);
> -    }
> -    if (err) {
> -        fprintf(stderr, "error: bad option value [%s = %s]\n", name, str);
> -        return (1);
> -    }
> -    return (0);
> -}
> -
> -/* register config file entry as x86_def_t
> - */
> -static int cpudef_register(QemuOpts *opts, void *opaque)
> -{
> -    x86_def_t *def = g_malloc0(sizeof (x86_def_t));
> -
> -    qemu_opt_foreach(opts, cpudef_setfield, def, 1);
> -    def->next = x86_defs;
> -    x86_defs = def;
> -    return (0);
> -}
> -
>  void cpu_clear_apic_feature(CPUX86State *env)
>  {
>      env->cpuid_features &= ~CPUID_APIC;
> @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
>  
>  #endif /* !CONFIG_USER_ONLY */
>  
> -/* register "cpudef" models defined in configuration file.  Here we first
> - * preload any built-in definitions
> +/* Initialize list of CPU models, filling some non-static fields if
> necessary */
>  void x86_cpudef_setup(void)
>  {
> @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void)
>      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
>          x86_def_t *def = &builtin_x86_defs[i];
>          def->next = x86_defs;
> -        def->flags = 1;
>  
>          /* Look for specific "cpudef" models that */
>          /* have the QEMU version in .model_id */
> @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void)
>  
>          x86_defs = def;
>      }
> -#if !defined(CONFIG_USER_ONLY)
> -    qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);
> -#endif
>  }
>  
>  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,

Re: [Qemu-devel] [RFC 2/6] i386: kill cpudef config section support

[Igor Mammedov]

On Wed, 5 Sep 2012 11:09:16 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 17 Aug 2012 14:53:38 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > It's nice to have a flexible system to maintain CPU models as data, but
> > this is holding us from making improvements in the CPU code because it's
> > not using the common infra-structure, and because the machine-type data
> > is still inside C code.
> > 
> > Users who want to configure CPU features directly may simply use the
> > "-cpu" command-line option (and maybe an equivalent -device option in
> > the future) to set CPU features.
> >
> Have I missed a patch that moves CPU definitions from cpus-x86_64.conf into
> the code?
> Perhaps they should be added to builtin_x86_defs in this patch.

Ah, found it
http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg00223.html
It seems missed 1.2 and this series just don't mention that it depends on
above mentioned series.
  
> 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 101
> > ++---------------------------------------------------- 1 file changed, 2
> > insertions(+), 99 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 519a104..71c2ee7 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -237,7 +237,6 @@ typedef struct x86_def_t {
> >      uint32_t xlevel;
> >      char model_id[48];
> >      int vendor_override;
> > -    uint32_t flags;
> >      /* Store the results of Centaur's CPUID instructions */
> >      uint32_t ext4_features;
> >      uint32_t xlevel2;
> > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function
> > cpu_fprintf) char buf[256];
> >  
> >      for (def = x86_defs; def; def = def->next) {
> > -        snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s",
> > def->name);
> > +        snprintf(buf, sizeof(buf), "%s", def->name);
> >          (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
> >      }
> >      if (kvm_enabled()) {
> > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -/* copy vendor id string to 32 bit register, nul pad as needed
> > - */
> > -static void cpyid(const char *s, uint32_t *id)
> > -{
> > -    char *d = (char *)id;
> > -    char i;
> > -
> > -    for (i = sizeof (*id); i--; )
> > -        *d++ = *s ? *s++ : '\0';
> > -}
> >  
> >  /* interpret radix and convert from string to arbitrary scalar,
> >   * otherwise flag failure
> > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id)
> >      *str && !*pend ? (*pval = ul) : (*perr = 1);        \
> >  }
> >  
> > -/* map cpuid options to feature bits, otherwise return failure
> > - * (option tags in *str are delimited by whitespace)
> > - */
> > -static void setfeatures(uint32_t *pval, const char *str,
> > -    const char **featureset, int *perr)
> > -{
> > -    const char *p, *q;
> > -
> > -    for (q = p = str; *p || *q; q = p) {
> > -        while (iswhite(*p))
> > -            q = ++p;
> > -        while (*p && !iswhite(*p))
> > -            ++p;
> > -        if (!*q && !*p)
> > -            return;
> > -        if (!lookup_feature(pval, q, p, featureset)) {
> > -            fprintf(stderr, "error: feature \"%.*s\" not available in
> > set\n",
> > -                (int)(p - q), q);
> > -            *perr = 1;
> > -            return;
> > -        }
> > -    }
> > -}
> > -
> > -/* map config file options to x86_def_t form
> > - */
> > -static int cpudef_setfield(const char *name, const char *str, void
> > *opaque) -{
> > -    x86_def_t *def = opaque;
> > -    int err = 0;
> > -
> > -    if (!strcmp(name, "name")) {
> > -        g_free((void *)def->name);
> > -        def->name = g_strdup(str);
> > -    } else if (!strcmp(name, "model_id")) {
> > -        strncpy(def->model_id, str, sizeof (def->model_id));
> > -    } else if (!strcmp(name, "level")) {
> > -        setscalar(&def->level, str, &err)
> > -    } else if (!strcmp(name, "vendor")) {
> > -        cpyid(&str[0], &def->vendor1);
> > -        cpyid(&str[4], &def->vendor2);
> > -        cpyid(&str[8], &def->vendor3);
> > -    } else if (!strcmp(name, "family")) {
> > -        setscalar(&def->family, str, &err)
> > -    } else if (!strcmp(name, "model")) {
> > -        setscalar(&def->model, str, &err)
> > -    } else if (!strcmp(name, "stepping")) {
> > -        setscalar(&def->stepping, str, &err)
> > -    } else if (!strcmp(name, "feature_edx")) {
> > -        setfeatures(&def->features, str, feature_name, &err);
> > -    } else if (!strcmp(name, "feature_ecx")) {
> > -        setfeatures(&def->ext_features, str, ext_feature_name, &err);
> > -    } else if (!strcmp(name, "extfeature_edx")) {
> > -        setfeatures(&def->ext2_features, str, ext2_feature_name, &err);
> > -    } else if (!strcmp(name, "extfeature_ecx")) {
> > -        setfeatures(&def->ext3_features, str, ext3_feature_name, &err);
> > -    } else if (!strcmp(name, "xlevel")) {
> > -        setscalar(&def->xlevel, str, &err)
> > -    } else {
> > -        fprintf(stderr, "error: unknown option [%s = %s]\n", name, str);
> > -        return (1);
> > -    }
> > -    if (err) {
> > -        fprintf(stderr, "error: bad option value [%s = %s]\n", name,
> > str);
> > -        return (1);
> > -    }
> > -    return (0);
> > -}
> > -
> > -/* register config file entry as x86_def_t
> > - */
> > -static int cpudef_register(QemuOpts *opts, void *opaque)
> > -{
> > -    x86_def_t *def = g_malloc0(sizeof (x86_def_t));
> > -
> > -    qemu_opt_foreach(opts, cpudef_setfield, def, 1);
> > -    def->next = x86_defs;
> > -    x86_defs = def;
> > -    return (0);
> > -}
> > -
> >  void cpu_clear_apic_feature(CPUX86State *env)
> >  {
> >      env->cpuid_features &= ~CPUID_APIC;
> > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
> >  
> >  #endif /* !CONFIG_USER_ONLY */
> >  
> > -/* register "cpudef" models defined in configuration file.  Here we first
> > - * preload any built-in definitions
> > +/* Initialize list of CPU models, filling some non-static fields if
> > necessary */
> >  void x86_cpudef_setup(void)
> >  {
> > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void)
> >      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
> >          x86_def_t *def = &builtin_x86_defs[i];
> >          def->next = x86_defs;
> > -        def->flags = 1;
> >  
> >          /* Look for specific "cpudef" models that */
> >          /* have the QEMU version in .model_id */
> > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void)
> >  
> >          x86_defs = def;
> >      }
> > -#if !defined(CONFIG_USER_ONLY)
> > -    qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL,
> > 0); -#endif
> >  }
> >  
> >  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
> 
> 
> 

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

[Igor Mammedov]

On Fri, 17 Aug 2012 14:53:42 -0300
Eduardo Habkost <ehabkost@redhat.com> 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 a71c2fc..0a03c80 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1297,13 +1297,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 *qmp_query_cpu_definitions(Error **errp)

If you are removing this ones, why do you leave:
if (dump) {
...
(*cpu_fprintf)(f, "  feature_edx %08x (%s)\n", def->features,
....

Re: [Qemu-devel] [RFC 2/6] i386: kill cpudef config section support

[Eduardo Habkost]

On Wed, Sep 05, 2012 at 11:39:21AM +0200, Igor Mammedov wrote:
> On Wed, 5 Sep 2012 11:09:16 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Fri, 17 Aug 2012 14:53:38 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > It's nice to have a flexible system to maintain CPU models as data, but
> > > this is holding us from making improvements in the CPU code because it's
> > > not using the common infra-structure, and because the machine-type data
> > > is still inside C code.
> > > 
> > > Users who want to configure CPU features directly may simply use the
> > > "-cpu" command-line option (and maybe an equivalent -device option in
> > > the future) to set CPU features.
> > >
> > Have I missed a patch that moves CPU definitions from cpus-x86_64.conf into
> > the code?
> > Perhaps they should be added to builtin_x86_defs in this patch.
> 
> Ah, found it
> http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg00223.html
> It seems missed 1.2 and this series just don't mention that it depends on
> above mentioned series.

Yes. Sorry if I didn't mention that. We have so many series in the queue
that sometimes I get confused by the dependencies myself.

>   
> > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  target-i386/cpu.c | 101
> > > ++---------------------------------------------------- 1 file changed, 2
> > > insertions(+), 99 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 519a104..71c2ee7 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -237,7 +237,6 @@ typedef struct x86_def_t {
> > >      uint32_t xlevel;
> > >      char model_id[48];
> > >      int vendor_override;
> > > -    uint32_t flags;
> > >      /* Store the results of Centaur's CPUID instructions */
> > >      uint32_t ext4_features;
> > >      uint32_t xlevel2;
> > > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function
> > > cpu_fprintf) char buf[256];
> > >  
> > >      for (def = x86_defs; def; def = def->next) {
> > > -        snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s",
> > > def->name);
> > > +        snprintf(buf, sizeof(buf), "%s", def->name);
> > >          (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
> > >      }
> > >      if (kvm_enabled()) {
> > > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> > > *cpu_model) }
> > >  
> > >  #if !defined(CONFIG_USER_ONLY)
> > > -/* copy vendor id string to 32 bit register, nul pad as needed
> > > - */
> > > -static void cpyid(const char *s, uint32_t *id)
> > > -{
> > > -    char *d = (char *)id;
> > > -    char i;
> > > -
> > > -    for (i = sizeof (*id); i--; )
> > > -        *d++ = *s ? *s++ : '\0';
> > > -}
> > >  
> > >  /* interpret radix and convert from string to arbitrary scalar,
> > >   * otherwise flag failure
> > > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id)
> > >      *str && !*pend ? (*pval = ul) : (*perr = 1);        \
> > >  }
> > >  
> > > -/* map cpuid options to feature bits, otherwise return failure
> > > - * (option tags in *str are delimited by whitespace)
> > > - */
> > > -static void setfeatures(uint32_t *pval, const char *str,
> > > -    const char **featureset, int *perr)
> > > -{
> > > -    const char *p, *q;
> > > -
> > > -    for (q = p = str; *p || *q; q = p) {
> > > -        while (iswhite(*p))
> > > -            q = ++p;
> > > -        while (*p && !iswhite(*p))
> > > -            ++p;
> > > -        if (!*q && !*p)
> > > -            return;
> > > -        if (!lookup_feature(pval, q, p, featureset)) {
> > > -            fprintf(stderr, "error: feature \"%.*s\" not available in
> > > set\n",
> > > -                (int)(p - q), q);
> > > -            *perr = 1;
> > > -            return;
> > > -        }
> > > -    }
> > > -}
> > > -
> > > -/* map config file options to x86_def_t form
> > > - */
> > > -static int cpudef_setfield(const char *name, const char *str, void
> > > *opaque) -{
> > > -    x86_def_t *def = opaque;
> > > -    int err = 0;
> > > -
> > > -    if (!strcmp(name, "name")) {
> > > -        g_free((void *)def->name);
> > > -        def->name = g_strdup(str);
> > > -    } else if (!strcmp(name, "model_id")) {
> > > -        strncpy(def->model_id, str, sizeof (def->model_id));
> > > -    } else if (!strcmp(name, "level")) {
> > > -        setscalar(&def->level, str, &err)
> > > -    } else if (!strcmp(name, "vendor")) {
> > > -        cpyid(&str[0], &def->vendor1);
> > > -        cpyid(&str[4], &def->vendor2);
> > > -        cpyid(&str[8], &def->vendor3);
> > > -    } else if (!strcmp(name, "family")) {
> > > -        setscalar(&def->family, str, &err)
> > > -    } else if (!strcmp(name, "model")) {
> > > -        setscalar(&def->model, str, &err)
> > > -    } else if (!strcmp(name, "stepping")) {
> > > -        setscalar(&def->stepping, str, &err)
> > > -    } else if (!strcmp(name, "feature_edx")) {
> > > -        setfeatures(&def->features, str, feature_name, &err);
> > > -    } else if (!strcmp(name, "feature_ecx")) {
> > > -        setfeatures(&def->ext_features, str, ext_feature_name, &err);
> > > -    } else if (!strcmp(name, "extfeature_edx")) {
> > > -        setfeatures(&def->ext2_features, str, ext2_feature_name, &err);
> > > -    } else if (!strcmp(name, "extfeature_ecx")) {
> > > -        setfeatures(&def->ext3_features, str, ext3_feature_name, &err);
> > > -    } else if (!strcmp(name, "xlevel")) {
> > > -        setscalar(&def->xlevel, str, &err)
> > > -    } else {
> > > -        fprintf(stderr, "error: unknown option [%s = %s]\n", name, str);
> > > -        return (1);
> > > -    }
> > > -    if (err) {
> > > -        fprintf(stderr, "error: bad option value [%s = %s]\n", name,
> > > str);
> > > -        return (1);
> > > -    }
> > > -    return (0);
> > > -}
> > > -
> > > -/* register config file entry as x86_def_t
> > > - */
> > > -static int cpudef_register(QemuOpts *opts, void *opaque)
> > > -{
> > > -    x86_def_t *def = g_malloc0(sizeof (x86_def_t));
> > > -
> > > -    qemu_opt_foreach(opts, cpudef_setfield, def, 1);
> > > -    def->next = x86_defs;
> > > -    x86_defs = def;
> > > -    return (0);
> > > -}
> > > -
> > >  void cpu_clear_apic_feature(CPUX86State *env)
> > >  {
> > >      env->cpuid_features &= ~CPUID_APIC;
> > > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
> > >  
> > >  #endif /* !CONFIG_USER_ONLY */
> > >  
> > > -/* register "cpudef" models defined in configuration file.  Here we first
> > > - * preload any built-in definitions
> > > +/* Initialize list of CPU models, filling some non-static fields if
> > > necessary */
> > >  void x86_cpudef_setup(void)
> > >  {
> > > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void)
> > >      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
> > >          x86_def_t *def = &builtin_x86_defs[i];
> > >          def->next = x86_defs;
> > > -        def->flags = 1;
> > >  
> > >          /* Look for specific "cpudef" models that */
> > >          /* have the QEMU version in .model_id */
> > > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void)
> > >  
> > >          x86_defs = def;
> > >      }
> > > -#if !defined(CONFIG_USER_ONLY)
> > > -    qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL,
> > > 0); -#endif
> > >  }
> > >  
> > >  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
> > 
> > 
> > 
> 

-- 
Eduardo