[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: |
Igor Mammedov |
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 22:52:52 +0100 |
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
- Re: [Qemu-devel] [PATCH 06/17] target-i386: remove vendor_override field from CPUX86State, (continued)
- [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t, Igor Mammedov, 2013/01/10
- Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t, Eduardo Habkost, 2013/01/10
- Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t, Andreas Färber, 2013/01/14
- Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t, Eduardo Habkost, 2013/01/14
- [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State, Igor Mammedov, 2013/01/15
- Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State, Eduardo Habkost, 2013/01/15
- Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State, Igor Mammedov, 2013/01/15
- Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State, li guang, 2013/01/16
- Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State, Igor Mammedov, 2013/01/16
- Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t,
Igor Mammedov <=
- Re: [Qemu-devel] [PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t, Eduardo Habkost, 2013/01/15
[Qemu-devel] [PATCH 07/17] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Igor Mammedov, 2013/01/10
[Qemu-devel] [PATCH 08/17] target-i386: set custom 'vendor' without intermediate x86_def_t, Igor Mammedov, 2013/01/10
[Qemu-devel] [PATCH 10/17] target-i386: set custom 'xlevel' without intermediate x86_def_t, Igor Mammedov, 2013/01/10
[Qemu-devel] [PATCH 11/17] target-i386: set custom 'level' without intermediate x86_def_t, Igor Mammedov, 2013/01/10
[Qemu-devel] [PATCH 12/17] target-i386: set custom 'model-id' without intermediate x86_def_t, Igor Mammedov, 2013/01/10
[Qemu-devel] [PATCH 09/17] target-i386: print deprecated warning if xlevel < 0x80000000, Igor Mammedov, 2013/01/10
[Qemu-devel] [PATCH 13/17] target-i386: set custom 'stepping' without intermediate x86_def_t, Igor Mammedov, 2013/01/10