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

["Andreas Färber" <afaerber@suse.de>]

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 */

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