On Mon, Jan 14, 2013 at 10:52:52PM +0100, Igor Mammedov wrote:
[...]
> > > @@ -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?
I believe Andreas is worrying in case there are NUL characters in the
middle of the vendor info. e.g., this wouldn't work:
#define CPUID_VENDOR_FOOBAR "foo\0bar\0baz\0".
[...]
if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_FOOBAR,
sizeof(x86_cpu_def->vendor)))
[...]
It's true that CPU vendors can do whatever they want and return anything
on the CPUID vendor leaf, but I believe this is never going to happen
because vendors know it would be a bad idea to do that. And if it
happens one day, we can easily add additional vendor1/vendor2/vendor3
properties to allow low-level setting of the vendor ID bytes.
[...]
> > > @@ -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?
Well, every assert is _supposed_ to be redundant, right? I mean: we only
add assert()s to the code when we are already confident that other parts
of the code make sure the assert is never going to fail. I would like to
keep it.
--
Eduardo
