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

[Igor Mammedov]

On Mon, 14 Jan 2013 20:32:12 +0100
Andreas Färber <address@hidden> 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).
> 
> 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 <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.
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


-- 
Regards,
  Igor