Re: [Qemu-devel] [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 19 Dec 2012 15:38:09 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:23PM +0100, Igor Mammedov wrote:
> > commit d480e1af which introduced vendor property was setting
> > env->cpuid_vendor_override = 1, which prevents using vendor property
> > on its own without triggering vendor override.
> > Fix it by removing setting cpuid_vendor_override in x86_cpuid_set_vendor()
> > to allow to use vendor property in other places that doesn't require
> > cpuid_vendor_override to be set to 1.
> 
> By making "vendor" not force override, you are making "-cpu vendor=xxx"
                                                         ^^^^^^^^^^^^^^^
old behavior is taken care in cpu_x86_parse_featurestr()
> behave differently from setting "vendor" using all other interfaces
> (e.g. -device, -global, QMP commands).
all other users do not exits for|use CPU yet, so we have a chance to new
behavior there.
 
> 
> What about taking the opposite approach? Setting "vendor" could always
> force vendor override, but the code that initialize the defaults would
> take care of not overriding the vendor ID if unsafe. e.g.: it could just
> do this:
> 
>  if (!kvm_enabled() || def->vendor_override) {
>    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
>  } /* else, leave the "vendor" property untouched" */
Unless it's placed in some class_init() I would strongly object, because
it introduces extra hardcoded initialization step between
object_new()..realize_fn().
> 
> (something equivalent could be done inside class_init() when we
> introduce subclasses)
> 
> On all I cases I can think of somebody setting the "vendor" property
> (e.g. using -cpu, QMP, -device, or -global), it means they want vendor
> override (otherwise, what's the point of setting the property?). Setting
> vendor in no-override mode is the special case, not the other way
> around.
Partly it's true,
currently vendor_override has meaning only for kvm guests and default vendor
value guest see changes as following:

1. tcg mode: guest always sees built-in or user provided vendor value,
             vendor_override has no effect here, we could assume it's true
        * and then vendor property setting it always to true is fine.
2. kvm mode: by default guest doesn't see built-in vendor value (it sees
             host's value instead), setting custom vendor value from command
             line currently makes guest to see vendor value that are kept env.
        * this is not OK with vendor property setting it always to true.

Perhaps we could in class_x86xxx_init() use host's vendor value as default
instead of built-in cpu_def's one if kvm_enabled()==true and remove
vendor_override field altogether.
It will keep default behavior the same as before and provide a real picture
of what guest will see by default on class introspection.

I'll post patch in several minutes.

> 
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index a74d74b..c6c074f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1163,7 +1163,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
> >          env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
> >          env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
> >      }
> > -    env->cpuid_vendor_override = 1;
> >  }
> >  
> >  static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> > -- 
> > 1.7.1
> > 
> > 
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor