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

[Eduardo Habkost]

On Wed, Dec 19, 2012 at 11:47:00PM +0100, Igor Mammedov wrote:
> On Wed, 19 Dec 2012 15:38:09 -0200
> Eduardo Habkost <address@hidden> 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.

The point is that the new behavior wouldn't make much sense: what's the
point of setting the vendor property and not getting the vendor ID
actually exposed to the guest?


>  
> > 
> > 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().

It wouldn't be "hardcoded initialization", it would be just code inside
instance_init(), that's supposed to have code inside it, too (but,
anyway, we probably can put that inside class_init).

> > 
> > (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.

This exactly what I suggested above, if you remove the
def->vendor_override check (that won't be necessary if all predefined
CPU models have vendor_override=false).


> It will keep default behavior the same as before and provide a real picture
> of what guest will see by default on class introspection.

Exactly.

> 
> I'll post patch in several minutes.
> 
> > 
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > >  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

-- 
Eduardo