On Mon, 14 Jan 2013 20:32:12 +0100 Andreas Färber <afaer...@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 <imamm...@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 <ehabk...@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 -- Regards, Igor
