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

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

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"
behave differently from setting "vendor" using all other interfaces
(e.g. -device, -global, QMP commands).

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" */

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

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

Am 19.12.2012 18:38, schrieb Eduardo Habkost:
> 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"
> behave differently from setting "vendor" using all other interfaces
> (e.g. -device, -global, QMP commands).
> 
> 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" */
> 
> (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.

+1

I was thinking it might be possible to just reset vendor_override to
false when set internally via property - I didn't get to trying out
whether there is a second place affected though.

Andreas

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
>

Makes code cleaner and avoids headache with 'vendor' property.

replaces previous patches:
  [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()
  [PATCH 13/20] target-i386: convert [cpuid_]vendor_override to bool
with:
  [PATCH 11/20] target-i386: add x86cpu_vendor_words2str()
  [PATCH 13/20] target-i386: remove vendor_override field from CPUX86State
ammends:
  [PATCH 12/20] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

On Wed, Dec 19, 2012 at 11:47:00PM +0100, Igor Mammedov wrote:
> 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.

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