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

[Eduardo Habkost]

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