[Qemu-devel] [PATCH qom-cpu 0/5] x86 CPU cleanup, part 4

[Qemu-devel] [PATCH qom-cpu 0/5] x86 CPU cleanup, part 4

[Igor Mammedov]

It's rebase of remnants of "[PATCH qom-cpu 00/17] x86 CPU cleanup, part 3" [1]
taking in account comments of reviewers.

This series is several cleanups, moved out from CPU properties series,
since they do not really depend on CPU properties re-factoring and could
simplify CPU subclasses work as well.

Series doesn't depend on cpu as device or any other series, and applies
to current master.

git tree for testing:
    https://github.com/imammedo/qemu/tree/x86_cpu_cleanup.part4

vendor related changes are tested with:
  https://github.com/imammedo/virt-test/tree/cpuid_features
  following command was used to run test:
   ./run -t kvm --qemu-bin=$QEMU --tests="qemu_cpu.qemu13 cpuid.custom_vendor"

*1 - http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01732.html

v6:
 - patches 1-3 from previous series already in master, dropped
 - patches 4-5 from previous series are sqashed in one
 - patches 7-8,10-17 from previous series are sqashed in one and
   cpu_x86_parse_featurestr() rewritten to set properties directly on CPU

v5:
 - dropped patch
   "[PATCH 06/20] target-i386: move out CPU features initialization in separate func"
   due to Andreas objection
 - fixed x86cpu to x86_cpu new function prefixes
 - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)" due conflicts
 - patches 1-4 from "[PATCH 00/20 v4] x86 CPU cleanup (wave 2)", already
   in master. So dropped from here
 - added patches 16-17 to deal with tsc_freq parsing without introducing
   new new visitor

v4:
 - rename [01/20] from:
     target-i386: filter out not TCG features if running without kvm at
       realize time
   to:
     target-i386: filter out unsupported features at realize time
 - make commit lines shorter for:
     target-i386: move kvm_check_features_against_host() check to realize
       time
 - restore removed by mistake host_cpuid() call in:
     target-i386: replace uint32_t vendor fields by vendor string in x86_def_t
 - fix spelling in:
     target-i386: print depricated warning if xlevel < 0x80000000
 - use qstring_append_int() for converting xlevel to string in:
     target-i386: set custom 'xlevel' without intermediate x86_def_t

v3:
 - [07/20] sets error if cpu name is empty, restore return -1 on error
 - get rid of *vendor_override field in CPUX86State & co
 - mark xlevel < 0x80000000 as depricated
 - squash idef-ing kvm specific functions in [08/20]
 - expand comment of [12/20] and reorder it right before "set custom" patches

v2:
 - cleanup commit message  and style fixes in
     [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time
 - extracted more patches [07-20] from cpu properties series, that were
   more cleanups and code reorganizing than conversion to static properties.

Igor Mammedov (5):
  target-i386: print deprecated warning if xlevel < 0x80000000
  target-i386: replace uint32_t vendor fields by vendor string in
    x86_def_t
  target-i386: remove vendor_override field from CPUX86State
  target-i386: set custom features/properties without intermediate
    x86_def_t
  target-i386: remove setting tsc-frequency from x86_def_t

 target-i386/cpu.c |  322 ++++++++++++++++++++++-------------------------------
 target-i386/cpu.h |    7 +-
 2 files changed, 138 insertions(+), 191 deletions(-)

[Qemu-devel] [PATCH 1/5] target-i386: print deprecated warning if xlevel < 0x80000000

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 333745b..ce914da 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1399,6 +1399,8 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
                     goto error;
                 }
                 if (numvalue < 0x80000000) {
+                    fprintf(stderr, "xlevel value shall always be >= 0x80000000"
+                            ", fixup will be deprecated in future versions\n");
                     numvalue += 0x80000000;
                 }
                 x86_cpu_def->xlevel = numvalue;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/5] target-i386: print deprecated warning if xlevel < 0x80000000

[Andreas Färber]

Am 17.01.2013 16:16, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 333745b..ce914da 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1399,6 +1399,8 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>                      goto error;
>                  }
>                  if (numvalue < 0x80000000) {
> +                    fprintf(stderr, "xlevel value shall always be >= 0x80000000"
> +                            ", fixup will be deprecated in future versions\n");
>                      numvalue += 0x80000000;
>                  }
>                  x86_cpu_def->xlevel = numvalue;

This has been reviewed without objections so far, so I would apply it
for 1.4. Either way you should document this intent for users already:
http://wiki.qemu.org/ChangeLog/1.4

We had such discussions before, around removing cpudef support.

When do you plan to remove this, and being deprecated, shouldn't it
rather read "fixup will be removed in future versions"? ;)
If it fits within 80 chars I can edit it myself.

Cheers,
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] [PATCH 1/5] target-i386: print deprecated warning if xlevel < 0x80000000

[Igor Mammedov]

On Mon, 21 Jan 2013 09:39:07 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 17.01.2013 16:16, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 333745b..ce914da 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c  
> > @@ -1399,6 +1399,8 @@ static int cpu_x86_parse_featurestr(x86_def_t
> > *x86_cpu_def, char *features) goto error;
> >                  }
> >                  if (numvalue < 0x800000  00) {
> > +                    fprintf(stderr, "xlevel value shall always be >=
> > 0x80000000"
> > +                            ", fixup will be deprecated in future
> > versions\n"); numvalue += 0x80000000;
> >                  }
> >                  x86_cpu_def->xlevel = numvalue;
> 
> This has been reviewed without objections so far, so I would apply it
> for 1.4. Either way you should document this intent for users already:
> http://wiki.qemu.org/ChangeLog/1.4
Would be something like this suitable:
xlevel argument for -cpu option, currently fix-ups it's value if it's less
than 0x80000000. Fix-up will be removed in QEMU 1.6 release and users are
expected to provide valid xlevel value or qemu will fail to start.

> 
> We had such discussions before, around removing cpudef support.
> 
> When do you plan to remove this, and being deprecated, shouldn't it
> rather read "fixup will be removed in future versions"? ;)
> If it fits within 80 chars I can edit it myself.
No need for it, I'll fix it and respin series.

> 
> Cheers,
> Andreas
> 

Thanks,
Igor

[Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

Vendor property setter takes string as vendor value but cpudefs
use uint32_t vendor[123] fields to define vendor value. It makes it
difficult to unify and use property setter for values from cpudefs.

Simplify code by using vendor property setter, vendor[123] fields
are converted into vendor[13] array to keep its value. And vendor
property setter is used to access/set value on CPU.

 - Make for() cycle reusable for the next patch by adding
   x86_cpu_vendor_words2str()

Intel's CPUID spec[1] says:
"
5.1.1 ...
These registers contain the ASCII string: GenuineIntel
...
"

List[2] of known vendor values shows that they all are 12 ASCII
characters long, padded where necessary with space

Current supported values are all ASCII characters packed in
ebx, edx, ecx. So lets state that qemu supports 12 ASCII characters
packed in ebx, edx, ecx registers for cpuid(0) instruction.

*1 - http://www.intel.com/Assets/PDF/appnote/241618.pdf
*2 - http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
 - add to commit msg that supported vendor should be 12 ASCII
   characters long string
 - squash in "add x86_cpu_vendor_words2str()" patch
 - use strncmp() instead of strcmp()
    Suggested-By: Andreas Färber <afaerber@suse.de>
 - style fix: align args on next line properly
v3:
 - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
   due to renaming of the last in previous patch
 - rebased with "target-i386: move out CPU features initialization
   in separate func" patch dropped
v2:
  - restore deleted host_cpuid() call in kvm_cpu_fill_host()
     Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |  148 ++++++++++++++++-------------------------------------
 target-i386/cpu.h |    6 +-
 2 files changed, 48 insertions(+), 106 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ce914da..ab80dbe 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -45,6 +45,18 @@
 #include "hw/apic_internal.h"
 #endif
 
+static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+                                     uint32_t vendor2, uint32_t vendor3)
+{
+    int i;
+    for (i = 0; i < 4; i++) {
+        dst[i] = vendor1 >> (8 * i);
+        dst[i + 4] = vendor2 >> (8 * i);
+        dst[i + 8] = vendor3 >> (8 * i);
+    }
+    dst[CPUID_VENDOR_SZ] = '\0';
+}
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -341,7 +353,7 @@ typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
     uint32_t level;
-    uint32_t vendor1, vendor2, vendor3;
+    char vendor[CPUID_VENDOR_SZ + 1];
     int family;
     int model;
     int stepping;
@@ -406,9 +418,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu64",
         .level = 4,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -425,9 +435,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "phenom",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 16,
         .model = 2,
         .stepping = 3,
@@ -453,9 +461,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "core2duo",
         .level = 10,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 15,
         .stepping = 11,
@@ -474,9 +480,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm64",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -500,9 +504,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu32",
         .level = 4,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 3,
         .stepping = 3,
@@ -513,9 +515,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm32",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -530,9 +530,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "coreduo",
         .level = 10,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 14,
         .stepping = 8,
@@ -548,9 +546,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "486",
         .level = 1,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 4,
         .model = 0,
         .stepping = 0,
@@ -560,9 +556,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium",
         .level = 1,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 5,
         .model = 4,
         .stepping = 3,
@@ -572,9 +566,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium2",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 5,
         .stepping = 2,
@@ -584,9 +576,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium3",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 7,
         .stepping = 3,
@@ -596,9 +586,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "athlon",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -612,9 +600,7 @@ static x86_def_t builtin_x86_defs[] = {
         .name = "n270",
         /* original is on level 10 */
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 28,
         .stepping = 2,
@@ -633,9 +619,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Conroe",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -653,9 +637,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Penryn",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -674,9 +656,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Nehalem",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -695,9 +675,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Westmere",
         .level = 11,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 44,
         .stepping = 1,
@@ -717,9 +695,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "SandyBridge",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 42,
         .stepping = 1,
@@ -742,9 +718,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Haswell",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 60,
         .stepping = 1,
@@ -772,9 +746,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G1",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -796,9 +768,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G2",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -822,9 +792,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G3",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -850,9 +818,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G4",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 1,
         .stepping = 2,
@@ -882,9 +848,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G5",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 2,
         .stepping = 0,
@@ -945,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     x86_cpu_def->name = "host";
     host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_def->vendor1 = ebx;
-    x86_cpu_def->vendor2 = edx;
-    x86_cpu_def->vendor3 = ecx;
+    x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
 
     host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
@@ -975,9 +937,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
-    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
-        x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
-        x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
+    if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA,
+        sizeof(x86_cpu_def->vendor))) {
         host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
         eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
         if (eax >= 0xC0000001) {
@@ -1213,15 +1174,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     char *value;
-    int i;
 
     value = (char *)g_malloc(CPUID_VENDOR_SZ + 1);
-    for (i = 0; i < 4; i++) {
-        value[i    ] = env->cpuid_vendor1 >> (8 * i);
-        value[i + 4] = env->cpuid_vendor2 >> (8 * i);
-        value[i + 8] = env->cpuid_vendor3 >> (8 * i);
-    }
-    value[CPUID_VENDOR_SZ] = '\0';
+    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
+                             env->cpuid_vendor3);
     return value;
 }
 
@@ -1341,7 +1297,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
  */
 static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
 {
-    unsigned int i;
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
     FeatureWordArray plus_features = { 0 };
@@ -1405,18 +1360,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
                 }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
-                if (strlen(val) != 12) {
-                    fprintf(stderr, "vendor string must be 12 chars long\n");
-                    goto error;
-                }
-                x86_cpu_def->vendor1 = 0;
-                x86_cpu_def->vendor2 = 0;
-                x86_cpu_def->vendor3 = 0;
-                for(i = 0; i < 4; i++) {
-                    x86_cpu_def->vendor1 |= ((uint8_t)val[i    ]) << (8 * i);
-                    x86_cpu_def->vendor2 |= ((uint8_t)val[i + 4]) << (8 * i);
-                    x86_cpu_def->vendor3 |= ((uint8_t)val[i + 8]) << (8 * i);
-                }
+                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
                 x86_cpu_def->vendor_override = 1;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
@@ -1611,10 +1555,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
         goto out;
     }
-    assert(def->vendor1);
-    env->cpuid_vendor1 = def->vendor1;
-    env->cpuid_vendor2 = def->vendor2;
-    env->cpuid_vendor3 = def->vendor3;
+    assert(def->vendor[0]);
+    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
     env->cpuid_vendor_override = def->vendor_override;
     object_property_set_int(OBJECT(cpu), def->level, "level", &error);
     object_property_set_int(OBJECT(cpu), def->family, "family", &error);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 4e091cd..554bd4a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -537,14 +537,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+#define CPUID_VENDOR_INTEL "GenuineIntel"
 
 #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
 #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
 #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
+#define CPUID_VENDOR_AMD   "AuthenticAMD"
 
-#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
-#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
-#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
+#define CPUID_VENDOR_VIA   "CentaurHauls"
 
 #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Eduardo Habkost]

On Thu, Jan 17, 2013 at 04:16:31PM +0100, Igor Mammedov wrote:
> Vendor property setter takes string as vendor value but cpudefs
> use uint32_t vendor[123] fields to define vendor value. It makes it
> difficult to unify and use property setter for values from cpudefs.
> 
> Simplify code by using vendor property setter, vendor[123] fields
> are converted into vendor[13] array to keep its value. And vendor
> property setter is used to access/set value on CPU.
> 
>  - Make for() cycle reusable for the next patch by adding
>    x86_cpu_vendor_words2str()
> 
> Intel's CPUID spec[1] says:
> "
> 5.1.1 ...
> These registers contain the ASCII string: GenuineIntel
> ...
> "
> 
> List[2] of known vendor values shows that they all are 12 ASCII
> characters long, padded where necessary with space
> 
> Current supported values are all ASCII characters packed in
> ebx, edx, ecx. So lets state that qemu supports 12 ASCII characters
> packed in ebx, edx, ecx registers for cpuid(0) instruction.

Nit: I guess you mean ASCII _printable_ characters. NUL is an ASCII
character as well, but it won't be supported.

> 
> *1 - http://www.intel.com/Assets/PDF/appnote/241618.pdf
> *2 - http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Minor comment below, feel free to ignore.

> ---
> v4:
>  - add to commit msg that supported vendor should be 12 ASCII
>    characters long string
>  - squash in "add x86_cpu_vendor_words2str()" patch
>  - use strncmp() instead of strcmp()
>     Suggested-By: Andreas Färber <afaerber@suse.de>
>  - style fix: align args on next line properly
> v3:
>  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>    due to renaming of the last in previous patch
>  - rebased with "target-i386: move out CPU features initialization
>    in separate func" patch dropped
> v2:
>   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
>      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |  148 ++++++++++++++++-------------------------------------
>  target-i386/cpu.h |    6 +-
>  2 files changed, 48 insertions(+), 106 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ce914da..ab80dbe 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
[...]
> @@ -975,9 +937,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>      x86_cpu_def->vendor_override = 0;
>  
>      /* Call Centaur's CPUID instruction. */
> -    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
> -        x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
> -        x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
> +    if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA,
> +        sizeof(x86_cpu_def->vendor))) {

We don't need strncmp() if we are sure the vendor field is always
NUL-terminated.

>          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
>          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>          if (eax >= 0xC0000001) {
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[li guang]

在 2013-01-17四的 16:16 +0100，Igor Mammedov写道：

> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ce914da..ab80dbe 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -45,6 +45,18 @@
>  #include "hw/apic_internal.h"
>  #endif
>  
> +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> +                                     uint32_t vendor2, uint32_t vendor3)

sorry, but I should say "_vendor_words2str" seems not so suitable,
it's mostly not a convertor, but a compactor, so I suggest to use
"_vendor_str" directly.

> +{
> +    int i;
> +    for (i = 0; i < 4; i++) {
> +        dst[i] = vendor1 >> (8 * i);
> +        dst[i + 4] = vendor2 >> (8 * i);
> +        dst[i + 8] = vendor3 >> (8 * i);
> +    }
> +    dst[CPUID_VENDOR_SZ] = '\0';
> +}
> +

> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -537,14 +537,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
>  #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
>  #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
> +#define CPUID_VENDOR_INTEL "GenuineIntel"
>  

you said the reason you did not remove _VENDOR_INTEL_{1,2,3}
is they're used somewhere, did you mean "target-i386/translate.c"
for sysenter instruction?
if it is, why can't we also remove them there?

>  #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
>  #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
>  #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
> +#define CPUID_VENDOR_AMD   "AuthenticAMD"
>  
> -#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
> -#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
> -#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
> +#define CPUID_VENDOR_VIA   "CentaurHauls"
>  
>  #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
>  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */

-- 
regards!
li guang

Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

On Fri, 18 Jan 2013 15:12:36 +0800
li guang <lig.fnst@cn.fujitsu.com> wrote:

> 在 2013-01-17四的 16:16 +0100，Igor Mammedov写道：
> 
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ce914da..ab80dbe 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -45,6 +45,18 @@
> >  #include "hw/apic_internal.h"
> >  #endif
> >  
> > +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > +                                     uint32_t vendor2, uint32_t vendor3)
> 
> sorry, but I should say "_vendor_words2str" seems not so suitable,
> it's mostly not a convertor, but a compactor, so I suggest to use
> "_vendor_str" directly.
I think that "_vendor_words2str" describes more clearly what function does,
regardless whether it is conversion or compaction. "_vendor_str" seems more
ambiguous though. But if you insist, I can change to it.

BTW: it's not just copying, it copies from little endinan words to string.

> 
> > +{
> > +    int i;
> > +    for (i = 0; i < 4; i++) {
> > +        dst[i] = vendor1 >> (8 * i);
> > +        dst[i + 4] = vendor2 >> (8 * i);
> > +        dst[i + 8] = vendor3 >> (8 * i);
> > +    }
> > +    dst[CPUID_VENDOR_SZ] = '\0';
> > +}
> > +
> 
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -537,14 +537,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> >  #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
> >  #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
> >  #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
> > +#define CPUID_VENDOR_INTEL "GenuineIntel"
> >  
> 
> you said the reason you did not remove _VENDOR_INTEL_{1,2,3}
> is they're used somewhere, did you mean "target-i386/translate.c"
> for sysenter instruction?
> if it is, why can't we also remove them there?
That would imply conversion of CPUX86State to using string for cpuid_vendor
instead of currents words which would mean to do conversion every time cpuid
instruction is called in guest. I'd rather keep current cpuid_vendor{1,2,3}
in CPUX86State.

Purpose of this patch is to switch from direct field copying when initializing
CPU to using property setter.
If we ever decide to convert CPUX86State.cpuid_vendor{1,2,3} into string, it
could be done by a separate patch.

In addition, wouldn't strcmp() there be less effective performance wise,
versus just number comparison if we would convert
CPUX86State.cpuid_vendor{1,2,3} to string?

> 
> >  #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
> >  #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
> >  #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
> > +#define CPUID_VENDOR_AMD   "AuthenticAMD"
> >  
> > -#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
> > -#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
> > -#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
> > +#define CPUID_VENDOR_VIA   "CentaurHauls"
> >  
> >  #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
> >  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
> 

Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[li guang]

在 2013-01-18五的 14:40 +0100，Igor Mammedov写道：
> On Fri, 18 Jan 2013 15:12:36 +0800
> li guang <lig.fnst@cn.fujitsu.com> wrote:
> 
> > 在 2013-01-17四的 16:16 +0100，Igor Mammedov写道：
> > 
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index ce914da..ab80dbe 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -45,6 +45,18 @@
> > >  #include "hw/apic_internal.h"
> > >  #endif
> > >  
> > > +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > > +                                     uint32_t vendor2, uint32_t vendor3)
> > 
> > sorry, but I should say "_vendor_words2str" seems not so suitable,
> > it's mostly not a convertor, but a compactor, so I suggest to use
> > "_vendor_str" directly.
> I think that "_vendor_words2str" describes more clearly what function does,
> regardless whether it is conversion or compaction. "_vendor_str" seems more
> ambiguous though. But if you insist, I can change to it.
> 

No, "_vendor_words2str" is OK, though I still prefer "_vendor_str"
stubbornly :)

> BTW: it's not just copying, it copies from little endinan words to string.
> 
> > 
> > > +{
> > > +    int i;
> > > +    for (i = 0; i < 4; i++) {
> > > +        dst[i] = vendor1 >> (8 * i);
> > > +        dst[i + 4] = vendor2 >> (8 * i);
> > > +        dst[i + 8] = vendor3 >> (8 * i);
> > > +    }
> > > +    dst[CPUID_VENDOR_SZ] = '\0';
> > > +}
> > > +
> > 
> > > --- a/target-i386/cpu.h
> > > +++ b/target-i386/cpu.h
> > > @@ -537,14 +537,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> > >  #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
> > >  #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
> > >  #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
> > > +#define CPUID_VENDOR_INTEL "GenuineIntel"
> > >  
> > 
> > you said the reason you did not remove _VENDOR_INTEL_{1,2,3}
> > is they're used somewhere, did you mean "target-i386/translate.c"
> > for sysenter instruction?
> > if it is, why can't we also remove them there?
> That would imply conversion of CPUX86State to using string for cpuid_vendor
> instead of currents words which would mean to do conversion every time cpuid
> instruction is called in guest. I'd rather keep current cpuid_vendor{1,2,3}
> in CPUX86State.
> 
> Purpose of this patch is to switch from direct field copying when initializing
> CPU to using property setter.
> If we ever decide to convert CPUX86State.cpuid_vendor{1,2,3} into string, it
> could be done by a separate patch.
> 
> In addition, wouldn't strcmp() there be less effective performance wise,
> versus just number comparison if we would convert
> CPUX86State.cpuid_vendor{1,2,3} to string?
> 

that's true.

Thanks!

> > 
> > >  #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
> > >  #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
> > >  #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
> > > +#define CPUID_VENDOR_AMD   "AuthenticAMD"
> > >  
> > > -#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
> > > -#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
> > > -#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
> > > +#define CPUID_VENDOR_VIA   "CentaurHauls"
> > >  
> > >  #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
> > >  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
> > 
> 

-- 
regards!
li guang

Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Andreas Färber]

Am 17.01.2013 16:16, schrieb Igor Mammedov:
> Vendor property setter takes string as vendor value but cpudefs
> use uint32_t vendor[123] fields to define vendor value. It makes it
> difficult to unify and use property setter for values from cpudefs.
> 
> Simplify code by using vendor property setter, vendor[123] fields
> are converted into vendor[13] array to keep its value. And vendor
> property setter is used to access/set value on CPU.
> 
>  - Make for() cycle reusable for the next patch by adding
>    x86_cpu_vendor_words2str()
> 
> Intel's CPUID spec[1] says:
> "
> 5.1.1 ...
> These registers contain the ASCII string: GenuineIntel
> ...
> "
> 
> List[2] of known vendor values shows that they all are 12 ASCII
> characters long, padded where necessary with space
> 
> Current supported values are all ASCII characters packed in
> ebx, edx, ecx. So lets state that qemu supports 12 ASCII characters
> packed in ebx, edx, ecx registers for cpuid(0) instruction.
> 
> *1 - http://www.intel.com/Assets/PDF/appnote/241618.pdf
> *2 - http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

So, hearing that my suggestion of a union to give us the best of both
worlds did not work out well due to endianness conversions, I would
still like to drop the vendor[0] assertion. And I spot no documentation
for char vendor[...] in this patch, only in the commit message; we could
spare that if we change char vendor[...] array to char *vendor, what do
you think? Erroring out (or padding) could be done when setting it via
"vendor" property onto X86CPU (maybe I'll try to cook up something for
demonstration).

Regards,
Andreas

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

[Qemu-devel] [PATCH 3/5] target-i386: remove vendor_override field from CPUX86State

[Igor Mammedov]

commit 8935499831312 makes cpuid return to guest host's vendor value
instead of built-in one by default if kvm_enabled() == true and allows
to override this behavior if 'vendor' is specified on -cpu command line.

But every time guest calls cpuid to get 'vendor' value, host's value is
read again and again in default case.

It complicates semantic of vendor property and makes it harder to use.

Instead of reading 'vendor' value from host every time cpuid[vendor] is
called, override 'vendor' value only once in cpu_x86_find_by_name(), when
built-in CPU model is found and if(kvm_enabled() == true).

It provides the same default semantic
 if (kvm_enabled() == true)  vendor = host's vendor
 else vendor = built-in vendor

and then later:
 if (custom vendor) vendor = custom vendor

'vendor' value is overridden when user provides it on -cpu command line,
and there isn't need in vendor_override field anymore, remove it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v4:
   - rebased with "target-i386: move out CPU features initialization
     in separate func" dropped. So remove vendor_override in
     cpu_x86_register() instead.
   - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
     due to renaming of the last in previous patch
---
 target-i386/cpu.c |   27 ++++++++++++---------------
 target-i386/cpu.h |    1 -
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ab80dbe..5cd7917 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -362,7 +362,6 @@ typedef struct x86_def_t {
     uint32_t kvm_features, svm_features;
     uint32_t xlevel;
     char model_id[48];
-    int vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -934,7 +933,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
                 kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
-    x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
     if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA,
@@ -1202,7 +1200,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
         env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
         env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
     }
-    env->cpuid_vendor_override = 1;
 }
 
 static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
@@ -1288,6 +1285,18 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
         return -1;
     } else {
         memcpy(x86_cpu_def, def, sizeof(*def));
+        /* sysenter isn't supported on compatibility mode on AMD, syscall
+         * isn't supported in compatibility mode on Intel.
+         * Normally we advertise the actual cpu vendor, but you can override
+         * this using the 'vendor' property if you want to use KVM's
+         * sysenter/syscall emulation in compatibility mode and when doing
+         * cross vendor migration
+         */
+        if (kvm_enabled()) {
+            uint32_t  ebx = 0, ecx = 0, edx = 0;
+            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+            x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
+        }
     }
 
     return 0;
@@ -1361,7 +1370,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
                 pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
-                x86_cpu_def->vendor_override = 1;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
                         val);
@@ -1557,7 +1565,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     }
     assert(def->vendor[0]);
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
-    env->cpuid_vendor_override = def->vendor_override;
     object_property_set_int(OBJECT(cpu), def->level, "level", &error);
     object_property_set_int(OBJECT(cpu), def->family, "family", &error);
     object_property_set_int(OBJECT(cpu), def->model, "model", &error);
@@ -1629,16 +1636,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
     *ebx = env->cpuid_vendor1;
     *edx = env->cpuid_vendor2;
     *ecx = env->cpuid_vendor3;
-
-    /* sysenter isn't supported on compatibility mode on AMD, syscall
-     * isn't supported in compatibility mode on Intel.
-     * Normally we advertise the actual cpu vendor, but you can override
-     * this if you want to use KVM's sysenter/syscall emulation
-     * in compatibility mode and when doing cross vendor migration
-     */
-    if (kvm_enabled() && ! env->cpuid_vendor_override) {
-        host_cpuid(0, 0, NULL, ebx, ecx, edx);
-    }
 }
 
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 554bd4a..53e1ea0 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -835,7 +835,6 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int cpuid_vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t cpuid_xlevel2;
     uint32_t cpuid_ext4_features;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/5] target-i386: remove vendor_override field from CPUX86State

[Eduardo Habkost]

On Thu, Jan 17, 2013 at 04:16:32PM +0100, Igor Mammedov wrote:
> commit 8935499831312 makes cpuid return to guest host's vendor value
> instead of built-in one by default if kvm_enabled() == true and allows
> to override this behavior if 'vendor' is specified on -cpu command line.
> 
> But every time guest calls cpuid to get 'vendor' value, host's value is
> read again and again in default case.
> 
> It complicates semantic of vendor property and makes it harder to use.
> 
> Instead of reading 'vendor' value from host every time cpuid[vendor] is
> called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> built-in CPU model is found and if(kvm_enabled() == true).
> 
> It provides the same default semantic
>  if (kvm_enabled() == true)  vendor = host's vendor
>  else vendor = built-in vendor
> 
> and then later:
>  if (custom vendor) vendor = custom vendor
> 
> 'vendor' value is overridden when user provides it on -cpu command line,
> and there isn't need in vendor_override field anymore, remove it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  v4:
>    - rebased with "target-i386: move out CPU features initialization
>      in separate func" dropped. So remove vendor_override in
>      cpu_x86_register() instead.
>    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>      due to renaming of the last in previous patch
> ---
>  target-i386/cpu.c |   27 ++++++++++++---------------
>  target-i386/cpu.h |    1 -
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ab80dbe..5cd7917 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -362,7 +362,6 @@ typedef struct x86_def_t {
>      uint32_t kvm_features, svm_features;
>      uint32_t xlevel;
>      char model_id[48];
> -    int vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t ext4_features;
>      uint32_t xlevel2;
> @@ -934,7 +933,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>                  kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
>  
>      cpu_x86_fill_model_id(x86_cpu_def->model_id);
> -    x86_cpu_def->vendor_override = 0;
>  
>      /* Call Centaur's CPUID instruction. */
>      if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA,
> @@ -1202,7 +1200,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>          env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
>          env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
>      }
> -    env->cpuid_vendor_override = 1;
>  }
>  
>  static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> @@ -1288,6 +1285,18 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>          return -1;
>      } else {
>          memcpy(x86_cpu_def, def, sizeof(*def));
> +        /* sysenter isn't supported on compatibility mode on AMD, syscall
> +         * isn't supported in compatibility mode on Intel.
> +         * Normally we advertise the actual cpu vendor, but you can override
> +         * this using the 'vendor' property if you want to use KVM's
> +         * sysenter/syscall emulation in compatibility mode and when doing
> +         * cross vendor migration
> +         */
> +        if (kvm_enabled()) {
> +            uint32_t  ebx = 0, ecx = 0, edx = 0;
> +            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> +            x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> +        }
>      }
>  
>      return 0;
> @@ -1361,7 +1370,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>                  x86_cpu_def->xlevel = numvalue;
>              } else if (!strcmp(featurestr, "vendor")) {
>                  pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> -                x86_cpu_def->vendor_override = 1;
>              } else if (!strcmp(featurestr, "model_id")) {
>                  pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
>                          val);
> @@ -1557,7 +1565,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      }
>      assert(def->vendor[0]);
>      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> -    env->cpuid_vendor_override = def->vendor_override;
>      object_property_set_int(OBJECT(cpu), def->level, "level", &error);
>      object_property_set_int(OBJECT(cpu), def->family, "family", &error);
>      object_property_set_int(OBJECT(cpu), def->model, "model", &error);
> @@ -1629,16 +1636,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>      *ebx = env->cpuid_vendor1;
>      *edx = env->cpuid_vendor2;
>      *ecx = env->cpuid_vendor3;
> -
> -    /* sysenter isn't supported on compatibility mode on AMD, syscall
> -     * isn't supported in compatibility mode on Intel.
> -     * Normally we advertise the actual cpu vendor, but you can override
> -     * this if you want to use KVM's sysenter/syscall emulation
> -     * in compatibility mode and when doing cross vendor migration
> -     */
> -    if (kvm_enabled() && ! env->cpuid_vendor_override) {
> -        host_cpuid(0, 0, NULL, ebx, ecx, edx);
> -    }
>  }
>  
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 554bd4a..53e1ea0 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -835,7 +835,6 @@ typedef struct CPUX86State {
>      uint32_t cpuid_ext2_features;
>      uint32_t cpuid_ext3_features;
>      uint32_t cpuid_apic_id;
> -    int cpuid_vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t cpuid_xlevel2;
>      uint32_t cpuid_ext4_features;
> -- 
> 1.7.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 4/5] target-i386: set custom features/properties without intermediate x86_def_t

[Igor Mammedov]

Move custom features parsing after built-in cpu_model defaults are set
and set custom features directly on CPU instance. That allows to make
clear distinction between built-in cpu model defaults that eventually
should go into clas_init() and extra property setting which is done
after defaults are set on CPU instance.

Impl. details:
 - features that are already properties, are converted to normalized
   (name, values) list. And after featurestr has been parsed,
   properties from the list are applied directly to CPU instance.
   * For now it provides uniform handling of properties with single
     object_property_parse() property setter.
   * And after current features/properties are converted into static
     properties, it will take a trivial patch to switch to global properties.
     Which will allow to:
     * get CPU instance initialized with all parameters passed on -cpu ...
       cmd. line from object_new() call.
     * call cpu_model/featurestr parsing only once before CPUs are created
     * open a road for removing CPUxxxState.cpu_model_str field, when other
       CPUs are similarly converted to subclasses and static properties.
 - re-factor error handling, to use Error instead of fprintf()s, since
   it is anyway passed in for property setter.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |  144 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 77 insertions(+), 67 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5cd7917..50e10b1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
     return 0;
 }
 
+typedef struct NameValuePair {
+    char *name;
+    char *value;
+    QTAILQ_ENTRY(NameValuePair) next;
+} NameValuePair;
+typedef QTAILQ_HEAD(NVList, NameValuePair) NVList;
+
+static void x86_cpu_add_nv_pair(NVList *list, const char *name,
+                                const char *value) {
+    NameValuePair *p = g_malloc0(sizeof(*p));
+    p->name = g_strdup(name);
+    p->value = g_strdup(value);
+    QTAILQ_INSERT_TAIL(list, p, next);
+}
+
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
-static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
+static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
 {
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
@@ -1312,6 +1327,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     /* Features to be removed */
     FeatureWordArray minus_features = { 0 };
     uint32_t numvalue;
+    CPUX86State *env = &cpu->env;
+    NVList props = QTAILQ_HEAD_INITIALIZER(props);
+    NameValuePair *p, *tmp;
 
     featurestr = features ? strtok(features, ",") : NULL;
 
@@ -1324,77 +1342,57 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             if (!strcmp(featurestr, "family")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff + 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->family = numvalue;
+                x86_cpu_add_nv_pair(&props, featurestr, val);
             } else if (!strcmp(featurestr, "model")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->model = numvalue;
+                x86_cpu_add_nv_pair(&props, featurestr, val);
             } else if (!strcmp(featurestr, "stepping")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->stepping = numvalue ;
+                x86_cpu_add_nv_pair(&props, featurestr, val);
             } else if (!strcmp(featurestr, "level")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->level = numvalue;
+                x86_cpu_add_nv_pair(&props, featurestr, val);
             } else if (!strcmp(featurestr, "xlevel")) {
                 char *err;
+                char num[32];
+
                 numvalue = strtoul(val, &err, 0);
                 if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s\n", val);
+                    goto out;
                 }
                 if (numvalue < 0x80000000) {
                     fprintf(stderr, "xlevel value shall always be >= 0x80000000"
                             ", fixup will be deprecated in future versions\n");
                     numvalue += 0x80000000;
                 }
-                x86_cpu_def->xlevel = numvalue;
+                snprintf(num, sizeof(num), "%" PRIu32, numvalue);
+                x86_cpu_add_nv_pair(&props, featurestr, num);
             } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
+                x86_cpu_add_nv_pair(&props, featurestr, val);
             } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
+                x86_cpu_add_nv_pair(&props, "model-id", val);
             } else if (!strcmp(featurestr, "tsc_freq")) {
                 int64_t tsc_freq;
                 char *err;
+                char num[32];
 
                 tsc_freq = strtosz_suffix_unit(val, &err,
                                                STRTOSZ_DEFSUFFIX_B, 1000);
                 if (tsc_freq < 0 || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s\n", val);
+                    goto out;
                 }
-                x86_cpu_def->tsc_khz = tsc_freq / 1000;
+                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
+                x86_cpu_add_nv_pair(&props, "tsc-frequency", num);
             } else if (!strcmp(featurestr, "hv_spinlocks")) {
                 char *err;
                 numvalue = strtoul(val, &err, 0);
                 if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s\n", val);
+                    goto out;
                 }
                 hyperv_set_spinlock_retries(numvalue);
             } else {
-                fprintf(stderr, "unrecognized feature %s\n", featurestr);
-                goto error;
+                error_setg(errp, "unrecognized feature %s\n", featurestr);
+                goto out;
             }
         } else if (!strcmp(featurestr, "check")) {
             check_cpuid = 1;
@@ -1405,31 +1403,46 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
         } else if (!strcmp(featurestr, "hv_vapic")) {
             hyperv_enable_vapic_recommended(true);
         } else {
-            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
-            goto error;
+            error_setg(errp, "feature string `%s' not in format (+feature|"
+                       "-feature|feature=xyz)\n", featurestr);
+            goto out;
         }
         featurestr = strtok(NULL, ",");
     }
-    x86_cpu_def->features |= plus_features[FEAT_1_EDX];
-    x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
-    x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
-    x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
-    x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX];
-    x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
-    x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
-    x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
-    x86_cpu_def->features &= ~minus_features[FEAT_1_EDX];
-    x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
-    x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
-    x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
-    x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
-    x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
-    x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
-    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
-    return 0;
+    env->cpuid_features |= plus_features[FEAT_1_EDX];
+    env->cpuid_ext_features |= plus_features[FEAT_1_ECX];
+    env->cpuid_ext2_features |= plus_features[FEAT_8000_0001_EDX];
+    env->cpuid_ext3_features |= plus_features[FEAT_8000_0001_ECX];
+    env->cpuid_ext4_features |= plus_features[FEAT_C000_0001_EDX];
+    env->cpuid_kvm_features |= plus_features[FEAT_KVM];
+    env->cpuid_svm_features |= plus_features[FEAT_SVM];
+    env->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
+    env->cpuid_features &= ~minus_features[FEAT_1_EDX];
+    env->cpuid_ext_features &= ~minus_features[FEAT_1_ECX];
+    env->cpuid_ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
+    env->cpuid_ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
+    env->cpuid_ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
+    env->cpuid_kvm_features &= ~minus_features[FEAT_KVM];
+    env->cpuid_svm_features &= ~minus_features[FEAT_SVM];
+    env->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
+
+    /* Set features on X86CPU object based on a provided key,value list */
+    QTAILQ_FOREACH_SAFE(p, &props, next, tmp) {
+        /* TODO: switch to using global properties after subclasses and
+         * static properties are done */
+        object_property_parse(OBJECT(cpu), p->value, p->name, errp);
+        if (error_is_set(errp)) {
+            goto out;
+        }
+    }
 
-error:
-    return -1;
+out:
+    QTAILQ_FOREACH_SAFE(p, &props, next, tmp) {
+        QTAILQ_REMOVE(&props, p, next);
+        g_free(p->value);
+        g_free(p->name);
+        g_free(p);
+    }
 }
 
 /* generate a composite string into buf of all cpuid names in featureset
@@ -1559,10 +1572,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     def->kvm_features |= kvm_default_features;
     def->ext_features |= CPUID_EXT_HYPERVISOR;
 
-    if (cpu_x86_parse_featurestr(def, features) < 0) {
-        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
-        goto out;
-    }
     assert(def->vendor[0]);
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
     object_property_set_int(OBJECT(cpu), def->level, "level", &error);
@@ -1584,6 +1593,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
 
+    cpu_x86_parse_featurestr(cpu, features, &error);
 out:
     g_strfreev(model_pieces);
     if (error) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/5] target-i386: set custom features/properties without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Jan 17, 2013 at 04:16:33PM +0100, Igor Mammedov wrote:
> Move custom features parsing after built-in cpu_model defaults are set
> and set custom features directly on CPU instance. That allows to make
> clear distinction between built-in cpu model defaults that eventually
> should go into clas_init() and extra property setting which is done
> after defaults are set on CPU instance.
> 
> Impl. details:
>  - features that are already properties, are converted to normalized
>    (name, values) list. And after featurestr has been parsed,
>    properties from the list are applied directly to CPU instance.
>    * For now it provides uniform handling of properties with single
>      object_property_parse() property setter.
>    * And after current features/properties are converted into static
>      properties, it will take a trivial patch to switch to global properties.
>      Which will allow to:
>      * get CPU instance initialized with all parameters passed on -cpu ...
>        cmd. line from object_new() call.
>      * call cpu_model/featurestr parsing only once before CPUs are created
>      * open a road for removing CPUxxxState.cpu_model_str field, when other
>        CPUs are similarly converted to subclasses and static properties.
>  - re-factor error handling, to use Error instead of fprintf()s, since
>    it is anyway passed in for property setter.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |  144 ++++++++++++++++++++++++++++-------------------------
>  1 files changed, 77 insertions(+), 67 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 5cd7917..50e10b1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>      return 0;
>  }
>  
> +typedef struct NameValuePair {
> +    char *name;
> +    char *value;
> +    QTAILQ_ENTRY(NameValuePair) next;
> +} NameValuePair;
> +typedef QTAILQ_HEAD(NVList, NameValuePair) NVList;
> +
> +static void x86_cpu_add_nv_pair(NVList *list, const char *name,
> +                                const char *value) {
> +    NameValuePair *p = g_malloc0(sizeof(*p));
> +    p->name = g_strdup(name);
> +    p->value = g_strdup(value);
> +    QTAILQ_INSERT_TAIL(list, p, next);
> +}

I am not sure I like this extra complexity. We don't need it if we
simply set/register the properties directly.

I have a different proposal:

1) By now, use:
      object_property_parse(OBJECT(cpu), P, V, errp)
   in the places you are using:
      x86_cpu_add_nv_pair(&props, P, V)
   below.

2) The day we move to global properties, we just need to mechanically
   replace the occurrences of:
     object_property_parse(OBJECT(cpu), P, V, errp)
   with:
     qdev_prop_register_global("X86CPU", P, V)

What do you think?


> +        object_property_parse(OBJECT(cpu), p->value, p->name, errp);
> +
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
> -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> +static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
>  {
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
> @@ -1312,6 +1327,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      /* Features to be removed */
>      FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
> +    CPUX86State *env = &cpu->env;
> +    NVList props = QTAILQ_HEAD_INITIALIZER(props);
> +    NameValuePair *p, *tmp;
>  
>      featurestr = features ? strtok(features, ",") : NULL;
>  
> @@ -1324,77 +1342,57 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>          } else if ((val = strchr(featurestr, '='))) {
>              *val = 0; val++;
>              if (!strcmp(featurestr, "family")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff + 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->family = numvalue;
> +                x86_cpu_add_nv_pair(&props, featurestr, val);
>              } else if (!strcmp(featurestr, "model")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->model = numvalue;
> +                x86_cpu_add_nv_pair(&props, featurestr, val);
>              } else if (!strcmp(featurestr, "stepping")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->stepping = numvalue ;
> +                x86_cpu_add_nv_pair(&props, featurestr, val);
>              } else if (!strcmp(featurestr, "level")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->level = numvalue;
> +                x86_cpu_add_nv_pair(&props, featurestr, val);
>              } else if (!strcmp(featurestr, "xlevel")) {
>                  char *err;
> +                char num[32];
> +
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> +                    error_setg(errp, "bad numerical value %s\n", val);
> +                    goto out;
>                  }
>                  if (numvalue < 0x80000000) {
>                      fprintf(stderr, "xlevel value shall always be >= 0x80000000"
>                              ", fixup will be deprecated in future versions\n");
>                      numvalue += 0x80000000;
>                  }
> -                x86_cpu_def->xlevel = numvalue;
> +                snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> +                x86_cpu_add_nv_pair(&props, featurestr, num);
>              } else if (!strcmp(featurestr, "vendor")) {
> -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> +                x86_cpu_add_nv_pair(&props, featurestr, val);
>              } else if (!strcmp(featurestr, "model_id")) {
> -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> -                        val);
> +                x86_cpu_add_nv_pair(&props, "model-id", val);
>              } else if (!strcmp(featurestr, "tsc_freq")) {
>                  int64_t tsc_freq;
>                  char *err;
> +                char num[32];
>  
>                  tsc_freq = strtosz_suffix_unit(val, &err,
>                                                 STRTOSZ_DEFSUFFIX_B, 1000);
>                  if (tsc_freq < 0 || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> +                    error_setg(errp, "bad numerical value %s\n", val);
> +                    goto out;
>                  }
> -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> +                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> +                x86_cpu_add_nv_pair(&props, "tsc-frequency", num);
>              } else if (!strcmp(featurestr, "hv_spinlocks")) {
>                  char *err;
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> +                    error_setg(errp, "bad numerical value %s\n", val);
> +                    goto out;
>                  }
>                  hyperv_set_spinlock_retries(numvalue);
>              } else {
> -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> -                goto error;
> +                error_setg(errp, "unrecognized feature %s\n", featurestr);
> +                goto out;
>              }
>          } else if (!strcmp(featurestr, "check")) {
>              check_cpuid = 1;
> @@ -1405,31 +1403,46 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>          } else if (!strcmp(featurestr, "hv_vapic")) {
>              hyperv_enable_vapic_recommended(true);
>          } else {
> -            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> -            goto error;
> +            error_setg(errp, "feature string `%s' not in format (+feature|"
> +                       "-feature|feature=xyz)\n", featurestr);
> +            goto out;
>          }
>          featurestr = strtok(NULL, ",");
>      }
> -    x86_cpu_def->features |= plus_features[FEAT_1_EDX];
> -    x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
> -    x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
> -    x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
> -    x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX];
> -    x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
> -    x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
> -    x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
> -    x86_cpu_def->features &= ~minus_features[FEAT_1_EDX];
> -    x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
> -    x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
> -    x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
> -    x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
> -    x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
> -    x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
> -    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
> -    return 0;
> +    env->cpuid_features |= plus_features[FEAT_1_EDX];
> +    env->cpuid_ext_features |= plus_features[FEAT_1_ECX];
> +    env->cpuid_ext2_features |= plus_features[FEAT_8000_0001_EDX];
> +    env->cpuid_ext3_features |= plus_features[FEAT_8000_0001_ECX];
> +    env->cpuid_ext4_features |= plus_features[FEAT_C000_0001_EDX];
> +    env->cpuid_kvm_features |= plus_features[FEAT_KVM];
> +    env->cpuid_svm_features |= plus_features[FEAT_SVM];
> +    env->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
> +    env->cpuid_features &= ~minus_features[FEAT_1_EDX];
> +    env->cpuid_ext_features &= ~minus_features[FEAT_1_ECX];
> +    env->cpuid_ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
> +    env->cpuid_ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
> +    env->cpuid_ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
> +    env->cpuid_kvm_features &= ~minus_features[FEAT_KVM];
> +    env->cpuid_svm_features &= ~minus_features[FEAT_SVM];
> +    env->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
> +
> +    /* Set features on X86CPU object based on a provided key,value list */
> +    QTAILQ_FOREACH_SAFE(p, &props, next, tmp) {
> +        /* TODO: switch to using global properties after subclasses and
> +         * static properties are done */
> +        object_property_parse(OBJECT(cpu), p->value, p->name, errp);
> +        if (error_is_set(errp)) {
> +            goto out;
> +        }
> +    }
>  
> -error:
> -    return -1;
> +out:
> +    QTAILQ_FOREACH_SAFE(p, &props, next, tmp) {
> +        QTAILQ_REMOVE(&props, p, next);
> +        g_free(p->value);
> +        g_free(p->name);
> +        g_free(p);
> +    }
>  }
>  
>  /* generate a composite string into buf of all cpuid names in featureset
> @@ -1559,10 +1572,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      def->kvm_features |= kvm_default_features;
>      def->ext_features |= CPUID_EXT_HYPERVISOR;
>  
> -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> -        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> -        goto out;
> -    }
>      assert(def->vendor[0]);
>      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
>      object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> @@ -1584,6 +1593,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
>  
> +    cpu_x86_parse_featurestr(cpu, features, &error);
>  out:
>      g_strfreev(model_pieces);
>      if (error) {
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/5] target-i386: set custom features/properties without intermediate x86_def_t

[Igor Mammedov]

On Thu, 17 Jan 2013 15:44:45 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jan 17, 2013 at 04:16:33PM +0100, Igor Mammedov wrote:
> > Move custom features parsing after built-in cpu_model defaults are set
> > and set custom features directly on CPU instance. That allows to make
> > clear distinction between built-in cpu model defaults that eventually
> > should go into clas_init() and extra property setting which is done
> > after defaults are set on CPU instance.
> > 
> > Impl. details:
> >  - features that are already properties, are converted to normalized
> >    (name, values) list. And after featurestr has been parsed,
> >    properties from the list are applied directly to CPU instance.
> >    * For now it provides uniform handling of properties with single
> >      object_property_parse() property setter.
> >    * And after current features/properties are converted into static
> >      properties, it will take a trivial patch to switch to global
> > properties. Which will allow to:
> >      * get CPU instance initialized with all parameters passed on -cpu ...
> >        cmd. line from object_new() call.
> >      * call cpu_model/featurestr parsing only once before CPUs are created
> >      * open a road for removing CPUxxxState.cpu_model_str field, when
> > other CPUs are similarly converted to subclasses and static properties.
> >  - re-factor error handling, to use Error instead of fprintf()s, since
> >    it is anyway passed in for property setter.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c |  144
> > ++++++++++++++++++++++++++++------------------------- 1 files changed, 77
> > insertions(+), 67 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 5cd7917..50e10b1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t
> > *x86_cpu_def, const char *name) return 0;
> >  }
> >  
> > +typedef struct NameValuePair {
> > +    char *name;
> > +    char *value;
> > +    QTAILQ_ENTRY(NameValuePair) next;
> > +} NameValuePair;
> > +typedef QTAILQ_HEAD(NVList, NameValuePair) NVList;
> > +
> > +static void x86_cpu_add_nv_pair(NVList *list, const char *name,
> > +                                const char *value) {
> > +    NameValuePair *p = g_malloc0(sizeof(*p));
> > +    p->name = g_strdup(name);
> > +    p->value = g_strdup(value);
> > +    QTAILQ_INSERT_TAIL(list, p, next);
> > +}
> 
> I am not sure I like this extra complexity. We don't need it if we
> simply set/register the properties directly.
> 
> I have a different proposal:
> 
> 1) By now, use:
>       object_property_parse(OBJECT(cpu), P, V, errp)
>    in the places you are using:
>       x86_cpu_add_nv_pair(&props, P, V)
>    below.
> 
> 2) The day we move to global properties, we just need to mechanically
>    replace the occurrences of:
>      object_property_parse(OBJECT(cpu), P, V, errp)
>    with:
>      qdev_prop_register_global("X86CPU", P, V)
> 
> What do you think?
Agreed, changes, when switching to global properties, will be mechanical
and not in many places, so it makes sense to drop list approach.
I'll resubmit series.

> 
> 
> > +        object_property_parse(OBJECT(cpu), p->value, p->name, errp);
> > +
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
[...]

[Qemu-devel] [PATCH 5/5] target-i386: remove setting tsc-frequency from x86_def_t

[Igor Mammedov]

Setting tsc-frequency from x86_def_t is NOP because default tsc_khz
in x86_def_t is 0 and CPUX86State.tsc_khz is also initialized to 0
by default. So there is not need to set ovewrite tsc_khz with default
0 because field was already initialized to 0.

custom tsc-frequency setting is not affected due to it is being set
without using x86_def_t (previous patch)

Field tsc_khz in x86_def_t becomes unused with this patch, so drop it
as well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 v2:
  - rebased with "target-i386: move out CPU features initialization
    in separate func" patch dropped
---
 target-i386/cpu.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 50e10b1..cc20ada 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -357,7 +357,6 @@ typedef struct x86_def_t {
     int family;
     int model;
     int stepping;
-    int tsc_khz;
     uint32_t features, ext_features, ext2_features, ext3_features;
     uint32_t kvm_features, svm_features;
     uint32_t xlevel;
@@ -1588,8 +1587,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     env->cpuid_ext4_features = def->ext4_features;
     env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
     env->cpuid_xlevel2 = def->xlevel2;
-    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
-                            "tsc-frequency", &error);
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
 
-- 
1.7.1