From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39404) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Turxi-0007Sx-RE for qemu-devel@nongnu.org; Mon, 14 Jan 2013 16:53:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Turxg-0002MU-SZ for qemu-devel@nongnu.org; Mon, 14 Jan 2013 16:52:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34505) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Turxg-0002MO-KN for qemu-devel@nongnu.org; Mon, 14 Jan 2013 16:52:56 -0500 Date: Mon, 14 Jan 2013 22:52:52 +0100 From: Igor Mammedov Message-ID: <20130114225252.5e7d86ad@thinkpad.mammed.net> In-Reply-To: <50F45D3C.7050104@suse.de> References: <1357870231-26762-1-git-send-email-imammedo@redhat.com> <1357870231-26762-6-git-send-email-imammedo@redhat.com> <50F45D3C.7050104@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?UTF-8?B?RsOkcmJlcg==?= Cc: qemu-devel@nongnu.org, ehabkost@redhat.com On Mon, 14 Jan 2013 20:32:12 +0100 Andreas F=C3=A4rber 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. > >=20 > > 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. > >=20 > > Signed-off-by: Igor Mammedov >=20 > 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 !=3D strlen(value) IIUC (no > way to specify on setting and using g_strdup() internally). >=20 > 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. >=20 > 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/e9fd18f308baffe0bde70a6710a= fa4ec41533a66 *2) target-i386: add helpers to change default values of static properties = before object is created https://github.com/imammedo/qemu/commit/8b3080e53843a5fe63ef3e9733c= 8e98cb68758f2 target-i386: declare subclass for qemu64 cpu model https://github.com/imammedo/qemu/commit/a48e252a2800bf8dd56320e68e4= f9517d0a25e5c 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. >=20 > > --- > > 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 > > --- > > target-i386/cpu.c | 126 +++++++++++++--------------------------------= -------- > > target-i386/cpu.h | 6 +- > > 2 files changed, 33 insertions(+), 99 deletions(-) > >=20 > > 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_de= f) > > =20 > > x86_cpu_def->name =3D "host"; > > host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx); > > - x86_cpu_def->vendor1 =3D ebx; > > - x86_cpu_def->vendor2 =3D edx; > > - x86_cpu_def->vendor3 =3D ecx; > > + x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx); > > =20 > > host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx); > > x86_cpu_def->family =3D ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF); > > @@ -987,9 +937,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_de= f) > > x86_cpu_def->vendor_override =3D 0; > > =20 > > /* Call Centaur's CPUID instruction. */ > > - if (x86_cpu_def->vendor1 =3D=3D CPUID_VENDOR_VIA_1 && > > - x86_cpu_def->vendor2 =3D=3D CPUID_VENDOR_VIA_2 && > > - x86_cpu_def->vendor3 =3D=3D CPUID_VENDOR_VIA_3) { > > + if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) { >=20 > 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->ve= ndor)))=20 instead? >=20 > > host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx); > > eax =3D kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX); > > if (eax >=3D 0xC0000001) { > > @@ -1348,7 +1296,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cp= u_def, const char *name) > > */ > > static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *feat= ures) > > { > > - unsigned int i; > > char *featurestr; /* Single 'key=3Dvalue" string being parsed */ > > /* Features to be added */ > > FeatureWordArray plus_features =3D { 0 }; > > @@ -1410,18 +1357,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x= 86_cpu_def, char *features) > > } > > x86_cpu_def->xlevel =3D numvalue; > > } else if (!strcmp(featurestr, "vendor")) { > > - if (strlen(val) !=3D 12) { > > - fprintf(stderr, "vendor string must be 12 chars lo= ng\n"); > > - goto error; > > - } > > - x86_cpu_def->vendor1 =3D 0; > > - x86_cpu_def->vendor2 =3D 0; > > - x86_cpu_def->vendor3 =3D 0; > > - for(i =3D 0; i < 4; i++) { > > - x86_cpu_def->vendor1 |=3D ((uint8_t)val[i ]) <<= (8 * i); > > - x86_cpu_def->vendor2 |=3D ((uint8_t)val[i + 4]) <<= (8 * i); > > - x86_cpu_def->vendor3 |=3D ((uint8_t)val[i + 8]) <<= (8 * i); > > - } > > + pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendo= r), val); > > x86_cpu_def->vendor_override =3D 1; > > } else if (!strcmp(featurestr, "model_id")) { > > pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->mod= el_id), >=20 > This one is fine by comparison. >=20 > > @@ -1616,10 +1552,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cp= u_model) > > error_setg(&error, "Invalid cpu_model string format: %s", cpu_= model); > > goto out; > > } > > - assert(def->vendor1); > > - env->cpuid_vendor1 =3D def->vendor1; > > - env->cpuid_vendor2 =3D def->vendor2; > > - env->cpuid_vendor3 =3D def->vendor3; > > + assert(def->vendor[0]); >=20 > 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?=20 > > + object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error= ); >=20 > The QOM API does not allow to specify the length here. >=20 > 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. =20 >=20 > Regards, > Andreas >=20 [...] > > #define CPUID_MWAIT_EMX (1 << 0) /* enumeration supported */ >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3= =BCrnberg --=20 Regards, Igor