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

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

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