qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fiel


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t
Date: Mon, 14 Jan 2013 19:44:00 -0200
User-agent: Mutt/1.5.21 (2010-09-15)

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 <address@hidden>
> 
> 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 <address@hidden>
> > ---
> >  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

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]