diff mbox

[05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

Message ID 1357870231-26762-6-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Jan. 11, 2013, 2:10 a.m. UTC 
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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
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 |  126 +++++++++++++----------------------------------------
 target-i386/cpu.h |    6 +-
 2 files changed, 33 insertions(+), 99 deletions(-)

Comments

Eduardo Habkost Jan. 11, 2013, 2:30 a.m. UTC | #1 
On Fri, Jan 11, 2013 at 03:10:19AM +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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 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 |  126 +++++++++++++----------------------------------------
>  target-i386/cpu.h |    6 +-
>  2 files changed, 33 insertions(+), 99 deletions(-)
> 

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Andreas Färber Jan. 14, 2013, 7:32 p.m. UTC | #2 
Am 11.01.2013 03:10, 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I have doubts about this patch as-is, maybe unfounded. It happens to
work for the three vendors we have in-tree. But three numeric words
would be well capable of respesenting a NUL char whereas the QOM
property API has no concept of a string length != strlen(value) IIUC (no
way to specify on setting and using g_strdup() internally).

Is there any validity guarantee documented in some Intel spec?

Another disadvantage I see is that each X86CPU subclass will have to use
pstrcpy() to initialize the value - or we would have to go with three
vendor base classes to hide this annoyance from each model. It still
"happens" to look nice here due to C99 x86_def_t initialization.

> ---
> 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 |  126 +++++++++++++----------------------------------------
>  target-i386/cpu.h |    6 +-
>  2 files changed, 33 insertions(+), 99 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 64cc88f..0b4fa57 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -353,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;
[...]
> @@ -957,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);
> @@ -987,9 +937,7 @@ 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 (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {

I'd be more comfortable doing such comparisons with a length or, even
better, using a static inline helper doing so: In this case
("CentaurHauls") it happens to work but I'm worried about setting a bad
example that gets copied-and-pasted.

>          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
>          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>          if (eax >= 0xC0000001) {
> @@ -1348,7 +1296,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 };
> @@ -1410,18 +1357,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),

This one is fine by comparison.

> @@ -1616,10 +1552,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]);

Instead of asserting on an empty string here, we should set the Error*
inside the property setter.

> +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);

The QOM API does not allow to specify the length here.

That property is not added in this patch, so it was probably added by
myself for the command line handling, where we could expect a sane
input. Inside x86_def_t or X86CPUClass anything could be coded though,
in theory. Admittedly the property output would already be broken then.

Regards,
Andreas

>      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 e4a7c50..983aab1 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -531,14 +531,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 */
Eduardo Habkost Jan. 14, 2013, 9:44 p.m. UTC | #3 
On Mon, Jan 14, 2013 at 08:32:12PM +0100, Andreas Färber wrote:
> Am 11.01.2013 03:10, 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.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> I have doubts about this patch as-is, maybe unfounded. It happens to
> work for the three vendors we have in-tree. But three numeric words
> would be well capable of respesenting a NUL char whereas the QOM
> property API has no concept of a string length != strlen(value) IIUC (no
> way to specify on setting and using g_strdup() internally).

Internally, we can easily represent the values with embeded NULs using
the char array, and I believe the char array is clearer easier to
manipulate than the three integers.

Externally, a vendor with NUL characters would be a problem for the
"vendor=foo" interface. But this is an existing problem with the
existing command-line interface, this patch doesn't change that
interface at all.

We could add numeric vendor{1,2,3} properties for people who really want
to set the vendor IDs bit-by-bit. But I suspect nobody would ever user
that interface at all.


> 
> Is there any validity guarantee documented in some Intel spec?

Yes: the Intel spec says all their CPUs have vendor=GenuineIntel. If you
see any other value, their documentation don't apply at all[1]. :-)

We can't even guarantee that QEMU is emulating the CPU correctly if an
unknown string is used, because the CPU won't be covered by any known
documentation. We allow arbitrary vendor strings just because it would
be extra code to care about, but I don't think we should document
anything except GenuineIntel/AuthenticAMD/CentaurHauls as supported.


> 
> Another disadvantage I see is that each X86CPU subclass will have to use
> pstrcpy() to initialize the value - or we would have to go with three
> vendor base classes to hide this annoyance from each model. It still
> "happens" to look nice here due to C99 x86_def_t initialization.

Actually, each subclass would just set a default value to the "vendor"
property, eventually. The pstrcpy()-based initialization would be just
temporary. (On either case, maybe having vendor-specific base classes
would be a good thing).

----
References:

[1] Intel Application Note 485: Intel® Processor Identification and the
CPUID Instruction, section 5.1.1 - "Vendor-ID and Largest Standard
Function (Function 0)":

   If the “GenuineIntel” string is not returned after execution of the
   CPUID instruction, do not rely upon the information described in this
   document to interpret the information returned by the CPUID
   instruction.

AMD docs say something similar:

AMD64 Architecture Programmer’s Manual Volume 3
CPUID Instruction

   Standard function 0 and extended function 8000_0000h both load a
   12-character string into the EBX, EDX, and ECX registers identifying the
   processor vendor. For AMD processors, the string is AuthenticAMD. This
   string informs software that it should follow the AMD CPUID definition
   for subsequent CPUID function calls. If the function returns another
   vendor’s string, software must use that vendor’s CPUID definition when
   interpreting the results of subsequent CPUID function calls.


> 
> > ---
> > 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 |  126 +++++++++++++----------------------------------------
> >  target-i386/cpu.h |    6 +-
> >  2 files changed, 33 insertions(+), 99 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 64cc88f..0b4fa57 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -353,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;
> [...]
> > @@ -957,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);
> > @@ -987,9 +937,7 @@ 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 (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> 
> I'd be more comfortable doing such comparisons with a length or, even
> better, using a static inline helper doing so: In this case
> ("CentaurHauls") it happens to work but I'm worried about setting a bad
> example that gets copied-and-pasted.
> 
> >          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> >          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> >          if (eax >= 0xC0000001) {
> > @@ -1348,7 +1296,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 };
> > @@ -1410,18 +1357,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),
> 
> This one is fine by comparison.
> 
> > @@ -1616,10 +1552,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]);
> 
> Instead of asserting on an empty string here, we should set the Error*
> inside the property setter.
> 
> > +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> 
> The QOM API does not allow to specify the length here.
> 
> That property is not added in this patch, so it was probably added by
> myself for the command line handling, where we could expect a sane
> input. Inside x86_def_t or X86CPUClass anything could be coded though,
> in theory. Admittedly the property output would already be broken then.
> 
> Regards,
> Andreas
> 
> >      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 e4a7c50..983aab1 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -531,14 +531,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 */
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Igor Mammedov Jan. 14, 2013, 9:52 p.m. UTC | #4 
On Mon, 14 Jan 2013 20:32:12 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 11.01.2013 03:10, 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.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> I have doubts about this patch as-is, maybe unfounded. It happens to
> work for the three vendors we have in-tree. But three numeric words
> would be well capable of respesenting a NUL char whereas the QOM
> property API has no concept of a string length != strlen(value) IIUC (no
> way to specify on setting and using g_strdup() internally).
> 
> Is there any validity guarantee documented in some Intel spec?

AMD's spec http://support.amd.com/us/Embedded_TechDocs/25481.pdf
just states that it's "AuthenticAMD" packed in ebx,edx,exc registers.

Intel's CPUID spec http://www.intel.com/Assets/PDF/appnote/241618.pdf
says:
"
5.1.1 ...
These registers contain the ASCII string: GenuineIntel
...
"

It's not spec but a list of known vendor values here
http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID
suggests that they all are 12 ASCII characters long, padded where necessary
with space.

I guess we can safely assume that it's 12 characters long ASCII string.

> 
> Another disadvantage I see is that each X86CPU subclass will have to use
> pstrcpy() to initialize the value - or we would have to go with three
> vendor base classes to hide this annoyance from each model. It still
> "happens" to look nice here due to C99 x86_def_t initialization.
If we have static properties before sub-classes and add field defval_str to
struct Property with associated handling [1] in qdev_property_add_static(),
then when vendor is converted to static property, we could keep C99
initialization in class_init's [2].

For example look at hack of cpu sub-classes that converts only qemu64 cpu
model using static properties for setting defaults:
Branch https://github.com/imammedo/qemu/commits/x86-cpu-classes.Jan142013

*1) qdev: extend DEFINE_GENERIC_PROP() to support default values
        https://github.com/imammedo/qemu/commit/e9fd18f308baffe0bde70a6710afa4ec41533a66

*2) target-i386: add helpers to change default values of static properties before object is created
        https://github.com/imammedo/qemu/commit/8b3080e53843a5fe63ef3e9733c8e98cb68758f2
    target-i386: declare subclass for qemu64 cpu model
        https://github.com/imammedo/qemu/commit/a48e252a2800bf8dd56320e68e4f9517d0a25e5c

if sub-classes would go before static properties then we could use pstrcpy() in
every sub-class temporally and then replace it with setting default property
value. Not sure if three vendor base classes would are worth effort to
eliminate one-liner from class_init()-s.

> 
> > ---
> > 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 |  126 +++++++++++++----------------------------------------
> >  target-i386/cpu.h |    6 +-
> >  2 files changed, 33 insertions(+), 99 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 64cc88f..0b4fa57 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -353,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;
> [...]
> > @@ -957,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);
> > @@ -987,9 +937,7 @@ 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 (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> 
> I'd be more comfortable doing such comparisons with a length or, even
> better, using a static inline helper doing so: In this case
> ("CentaurHauls") it happens to work but I'm worried about setting a bad
> example that gets copied-and-pasted.
x86_cpu_vendor_words2str() guaranties that x86_cpu_def->vendor is nill
terminated string, would you prefer:

 if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA, sizeof(x86_cpu_def->vendor))) 

instead?

> 
> >          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> >          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> >          if (eax >= 0xC0000001) {
> > @@ -1348,7 +1296,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 };
> > @@ -1410,18 +1357,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),
> 
> This one is fine by comparison.
> 
> > @@ -1616,10 +1552,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]);
> 
> Instead of asserting on an empty string here, we should set the Error*
> inside the property setter.
x86_cpuid_set_vendor() you've added some time ago, already checks for value
to be 12 characters exactly. If it is not, it sets *Error
Reason I've kept assert here is that after patch vendor in this place would
represent built-in vendor value, so it would be easier to detect invalid
default value by aborting here (it wouldn't be valid for cpu sub-classes
though).
But this check is really redundant, would you like it to be dropped? 

> > +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> 
> The QOM API does not allow to specify the length here.
> 
> That property is not added in this patch, so it was probably added by
> myself for the command line handling, where we could expect a sane
> input. Inside x86_def_t or X86CPUClass anything could be coded though,
> in theory. Admittedly the property output would already be broken then.
Perhaps we could check in x86_cpuid_set_vendor() that value is ASCII string,
but it is unrelated to this patch and I'd put that in separate patch anyway.
 
> 
> Regards,
> Andreas
> 
[...]
> >  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Eduardo Habkost Jan. 15, 2013, 10:53 a.m. UTC | #5 
On Mon, Jan 14, 2013 at 10:52:52PM +0100, Igor Mammedov wrote:
[...]
> > > @@ -987,9 +937,7 @@ 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 (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> > 
> > I'd be more comfortable doing such comparisons with a length or, even
> > better, using a static inline helper doing so: In this case
> > ("CentaurHauls") it happens to work but I'm worried about setting a bad
> > example that gets copied-and-pasted.
> x86_cpu_vendor_words2str() guaranties that x86_cpu_def->vendor is nill
> terminated string, would you prefer:
> 
>  if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA, sizeof(x86_cpu_def->vendor))) 
> 
> instead?

I believe Andreas is worrying in case there are NUL characters in the
middle of the vendor info. e.g., this wouldn't work:

  #define CPUID_VENDOR_FOOBAR "foo\0bar\0baz\0".
  [...]
  if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_FOOBAR, sizeof(x86_cpu_def->vendor))) 
      [...]

It's true that CPU vendors can do whatever they want and return anything
on the CPUID vendor leaf, but I believe this is never going to happen
because vendors know it would be a bad idea to do that. And if it
happens one day, we can easily add additional vendor1/vendor2/vendor3
properties to allow low-level setting of the vendor ID bytes.

[...]
> > > @@ -1616,10 +1552,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]);
> > 
> > Instead of asserting on an empty string here, we should set the Error*
> > inside the property setter.
> x86_cpuid_set_vendor() you've added some time ago, already checks for value
> to be 12 characters exactly. If it is not, it sets *Error
> Reason I've kept assert here is that after patch vendor in this place would
> represent built-in vendor value, so it would be easier to detect invalid
> default value by aborting here (it wouldn't be valid for cpu sub-classes
> though).
> But this check is really redundant, would you like it to be dropped? 

Well, every assert is _supposed_ to be redundant, right? I mean: we only
add assert()s to the code when we are already confident that other parts
of the code make sure the assert is never going to fail. I would like to
keep it.
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 64cc88f..0b4fa57 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -353,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;
@@ -418,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,
@@ -437,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,
@@ -465,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,
@@ -486,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,
@@ -512,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,
@@ -525,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,
@@ -542,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,
@@ -560,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,
@@ -572,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,
@@ -584,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,
@@ -596,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,
@@ -608,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,
@@ -624,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,
@@ -645,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,
@@ -665,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,
@@ -686,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,
@@ -707,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,
@@ -729,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,
@@ -754,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,
@@ -784,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,
@@ -808,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,
@@ -834,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,
@@ -862,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,
@@ -894,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,
@@ -957,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);
@@ -987,9 +937,7 @@  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 (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
         host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
         eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
         if (eax >= 0xC0000001) {
@@ -1348,7 +1296,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 };
@@ -1410,18 +1357,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),
@@ -1616,10 +1552,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 e4a7c50..983aab1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -531,14 +531,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 */