[06/17] target-i386: remove vendor_override field from CPUX86State

commit 8935499831312 makes cpuid return to guest host's vendor value
instead of built-in one by default if kvm_enabled() == true and allows
to override this behavior if 'vendor' is specified on -cpu command line.

But every time guest calls cpuid to get 'vendor' value, host's value is
read again and again in default case.

It complicates semantic of vendor property and makes it harder to use.

Instead of reading 'vendor' value from host every time cpuid[vendor] is
called, override 'vendor' value only once in cpu_x86_find_by_name(), when
built-in CPU model is found and if(kvm_enabled() == true).

It provides the same default semantic
 if (kvm_enabled() == true)  vendor = host's vendor
 else vendor = built-in vendor

and then later:
 if (custom vendor) vendor = custom vendor

'vendor' value is overridden when user provides it on -cpu command line,
and there isn't need in vendor_override field anymore, remove it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v4:
   - rebased with "target-i386: move out CPU features initialization
     in separate func" dropped. So remove vendor_override in
     cpu_x86_register() instead.
   - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
     due to renaming of the last in previous patch
---
 target-i386/cpu.c |   27 ++++++++++++---------------
 target-i386/cpu.h |    1 -
 2 files changed, 12 insertions(+), 16 deletions(-)

On Fri, Jan 11, 2013 at 03:10:20AM +0100, Igor Mammedov wrote:
> commit 8935499831312 makes cpuid return to guest host's vendor value
> instead of built-in one by default if kvm_enabled() == true and allows
> to override this behavior if 'vendor' is specified on -cpu command line.
> 
> But every time guest calls cpuid to get 'vendor' value, host's value is
> read again and again in default case.
> 
> It complicates semantic of vendor property and makes it harder to use.
> 
> Instead of reading 'vendor' value from host every time cpuid[vendor] is
> called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> built-in CPU model is found and if(kvm_enabled() == true).
> 
> It provides the same default semantic
>  if (kvm_enabled() == true)  vendor = host's vendor
>  else vendor = built-in vendor
> 
> and then later:
>  if (custom vendor) vendor = custom vendor
> 

How do you plan to make this work when the vendor string for each model
gets represented using a default defined at class_init-time[1]? When we do
that, we wouldn't be able to differentiate the built/predefined vendor
value (used for TCG only) and the user-defined vendor value (that would
be set in KVM mode too).

[1] AFAIU, class_init would never be able to check kvm_enabled(), by
design, because it may run before anything is configured (even the
choice to enable/disable KVM).

Maybe we can later add a "tcg-vendor" property, that would override the
vendor only if in TCG mode. Then the predefined CPU models could have a
"tcg-vendor" default set, and no "vendor" default set, to emulate the
current behavior.

> 'vendor' value is overridden when user provides it on -cpu command line,
> and there isn't need in vendor_override field anymore, remove it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  v4:
>    - rebased with "target-i386: move out CPU features initialization
>      in separate func" dropped. So remove vendor_override in
>      cpu_x86_register() instead.
>    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>      due to renaming of the last in previous patch
> ---
>  target-i386/cpu.c |   27 ++++++++++++---------------
>  target-i386/cpu.h |    1 -
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0b4fa57..4250c77 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -362,7 +362,6 @@ typedef struct x86_def_t {
>      uint32_t kvm_features, svm_features;
>      uint32_t xlevel;
>      char model_id[48];
> -    int vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t ext4_features;
>      uint32_t xlevel2;
> @@ -934,7 +933,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>                  kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
>  
>      cpu_x86_fill_model_id(x86_cpu_def->model_id);
> -    x86_cpu_def->vendor_override = 0;
>  
>      /* Call Centaur's CPUID instruction. */
>      if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> @@ -1201,7 +1199,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)
> @@ -1287,6 +1284,18 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>          return -1;
>      } else {
>          memcpy(x86_cpu_def, def, sizeof(*def));
> +        /* sysenter isn't supported on compatibility mode on AMD, syscall
> +         * isn't supported in compatibility mode on Intel.
> +         * Normally we advertise the actual cpu vendor, but you can override
> +         * this using the 'vendor' property if you want to use KVM's
> +         * sysenter/syscall emulation in compatibility mode and when doing
> +         * cross vendor migration
> +         */
> +        if (kvm_enabled()) {
> +            uint32_t  ebx = 0, ecx = 0, edx = 0;
> +            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> +            x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> +        }
>      }
>  
>      return 0;
> @@ -1358,7 +1367,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>                  x86_cpu_def->xlevel = numvalue;
>              } else if (!strcmp(featurestr, "vendor")) {
>                  pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> -                x86_cpu_def->vendor_override = 1;
>              } else if (!strcmp(featurestr, "model_id")) {
>                  pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
>                          val);
> @@ -1554,7 +1562,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      }
>      assert(def->vendor[0]);
>      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> -    env->cpuid_vendor_override = def->vendor_override;
>      object_property_set_int(OBJECT(cpu), def->level, "level", &error);
>      object_property_set_int(OBJECT(cpu), def->family, "family", &error);
>      object_property_set_int(OBJECT(cpu), def->model, "model", &error);
> @@ -1626,16 +1633,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>      *ebx = env->cpuid_vendor1;
>      *edx = env->cpuid_vendor2;
>      *ecx = env->cpuid_vendor3;
> -
> -    /* sysenter isn't supported on compatibility mode on AMD, syscall
> -     * isn't supported in compatibility mode on Intel.
> -     * Normally we advertise the actual cpu vendor, but you can override
> -     * this if you want to use KVM's sysenter/syscall emulation
> -     * in compatibility mode and when doing cross vendor migration
> -     */
> -    if (kvm_enabled() && ! env->cpuid_vendor_override) {
> -        host_cpuid(0, 0, NULL, ebx, ecx, edx);
> -    }
>  }
>  
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 983aab1..8bb5a58 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -829,7 +829,6 @@ typedef struct CPUX86State {
>      uint32_t cpuid_ext2_features;
>      uint32_t cpuid_ext3_features;
>      uint32_t cpuid_apic_id;
> -    int cpuid_vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t cpuid_xlevel2;
>      uint32_t cpuid_ext4_features;
> -- 
> 1.7.1
>

On Fri, 11 Jan 2013 00:42:13 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jan 11, 2013 at 03:10:20AM +0100, Igor Mammedov wrote:
> > commit 8935499831312 makes cpuid return to guest host's vendor value
> > instead of built-in one by default if kvm_enabled() == true and allows
> > to override this behavior if 'vendor' is specified on -cpu command line.
> > 
> > But every time guest calls cpuid to get 'vendor' value, host's value is
> > read again and again in default case.
> > 
> > It complicates semantic of vendor property and makes it harder to use.
> > 
> > Instead of reading 'vendor' value from host every time cpuid[vendor] is
> > called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> > built-in CPU model is found and if(kvm_enabled() == true).
> > 
> > It provides the same default semantic
> >  if (kvm_enabled() == true)  vendor = host's vendor
> >  else vendor = built-in vendor
> > 
> > and then later:
> >  if (custom vendor) vendor = custom vendor
> > 
> 
> How do you plan to make this work when the vendor string for each model
> gets represented using a default defined at class_init-time[1]? When we do
it could be done by late update hook like you did for host cpu subclass or
forcing early kvm_init() and then do it at class_init-time. I still hope that
second option would work(but I haven't checked yet).

> that, we wouldn't be able to differentiate the built/predefined vendor
> value (used for TCG only) and the user-defined vendor value (that would
> be set in KVM mode too).
If we have subclasses with already correct vendor for a specific mode then we
needn't to know if it is built-in vendor or user-defined.

> 
> [1] AFAIU, class_init would never be able to check kvm_enabled(), by
> design, because it may run before anything is configured (even the
> choice to enable/disable KVM).
> 
> Maybe we can later add a "tcg-vendor" property, that would override the
> vendor only if in TCG mode. Then the predefined CPU models could have a
> "tcg-vendor" default set, and no "vendor" default set, to emulate the
> current behavior.
> 
> > 'vendor' value is overridden when user provides it on -cpu command line,
> > and there isn't need in vendor_override field anymore, remove it.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  v4:
> >    - rebased with "target-i386: move out CPU features initialization
> >      in separate func" dropped. So remove vendor_override in
> >      cpu_x86_register() instead.
> >    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> >      due to renaming of the last in previous patch
> > ---
> >  target-i386/cpu.c |   27 ++++++++++++---------------
> >  target-i386/cpu.h |    1 -
> >  2 files changed, 12 insertions(+), 16 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 0b4fa57..4250c77 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -362,7 +362,6 @@ typedef struct x86_def_t {
> >      uint32_t kvm_features, svm_features;
> >      uint32_t xlevel;
> >      char model_id[48];
> > -    int vendor_override;
> >      /* Store the results of Centaur's CPUID instructions */
> >      uint32_t ext4_features;
> >      uint32_t xlevel2;
> > @@ -934,7 +933,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
> >                  kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
> >  
> >      cpu_x86_fill_model_id(x86_cpu_def->model_id);
> > -    x86_cpu_def->vendor_override = 0;
> >  
> >      /* Call Centaur's CPUID instruction. */
> >      if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> > @@ -1201,7 +1199,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)
> > @@ -1287,6 +1284,18 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
> >          return -1;
> >      } else {
> >          memcpy(x86_cpu_def, def, sizeof(*def));
> > +        /* sysenter isn't supported on compatibility mode on AMD, syscall
> > +         * isn't supported in compatibility mode on Intel.
> > +         * Normally we advertise the actual cpu vendor, but you can override
> > +         * this using the 'vendor' property if you want to use KVM's
> > +         * sysenter/syscall emulation in compatibility mode and when doing
> > +         * cross vendor migration
> > +         */
> > +        if (kvm_enabled()) {
> > +            uint32_t  ebx = 0, ecx = 0, edx = 0;
> > +            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> > +            x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> > +        }
> >      }
> >  
> >      return 0;
> > @@ -1358,7 +1367,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >                  x86_cpu_def->xlevel = numvalue;
> >              } else if (!strcmp(featurestr, "vendor")) {
> >                  pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> > -                x86_cpu_def->vendor_override = 1;
> >              } else if (!strcmp(featurestr, "model_id")) {
> >                  pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> >                          val);
> > @@ -1554,7 +1562,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      }
> >      assert(def->vendor[0]);
> >      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> > -    env->cpuid_vendor_override = def->vendor_override;
> >      object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> >      object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> >      object_property_set_int(OBJECT(cpu), def->model, "model", &error);
> > @@ -1626,16 +1633,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
> >      *ebx = env->cpuid_vendor1;
> >      *edx = env->cpuid_vendor2;
> >      *ecx = env->cpuid_vendor3;
> > -
> > -    /* sysenter isn't supported on compatibility mode on AMD, syscall
> > -     * isn't supported in compatibility mode on Intel.
> > -     * Normally we advertise the actual cpu vendor, but you can override
> > -     * this if you want to use KVM's sysenter/syscall emulation
> > -     * in compatibility mode and when doing cross vendor migration
> > -     */
> > -    if (kvm_enabled() && ! env->cpuid_vendor_override) {
> > -        host_cpuid(0, 0, NULL, ebx, ecx, edx);
> > -    }
> >  }
> >  
> >  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 983aab1..8bb5a58 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -829,7 +829,6 @@ typedef struct CPUX86State {
> >      uint32_t cpuid_ext2_features;
> >      uint32_t cpuid_ext3_features;
> >      uint32_t cpuid_apic_id;
> > -    int cpuid_vendor_override;
> >      /* Store the results of Centaur's CPUID instructions */
> >      uint32_t cpuid_xlevel2;
> >      uint32_t cpuid_ext4_features;
> > -- 
> > 1.7.1
> > 
> 
> -- 
> Eduardo

On Fri, Jan 11, 2013 at 04:09:45AM +0100, Igor Mammedov wrote:
> On Fri, 11 Jan 2013 00:42:13 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jan 11, 2013 at 03:10:20AM +0100, Igor Mammedov wrote:
> > > commit 8935499831312 makes cpuid return to guest host's vendor value
> > > instead of built-in one by default if kvm_enabled() == true and allows
> > > to override this behavior if 'vendor' is specified on -cpu command line.
> > > 
> > > But every time guest calls cpuid to get 'vendor' value, host's value is
> > > read again and again in default case.
> > > 
> > > It complicates semantic of vendor property and makes it harder to use.
> > > 
> > > Instead of reading 'vendor' value from host every time cpuid[vendor] is
> > > called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> > > built-in CPU model is found and if(kvm_enabled() == true).
> > > 
> > > It provides the same default semantic
> > >  if (kvm_enabled() == true)  vendor = host's vendor
> > >  else vendor = built-in vendor
> > > 
> > > and then later:
> > >  if (custom vendor) vendor = custom vendor
> > > 
> > 
> > How do you plan to make this work when the vendor string for each model
> > gets represented using a default defined at class_init-time[1]? When we do
> it could be done by late update hook like you did for host cpu subclass or
> forcing early kvm_init() and then do it at class_init-time. I still hope that
> second option would work(but I haven't checked yet).

I believe that it is by design that class_init runs before kvm={on,off}
configuration is handled, and we won't be able to change it. We'll need
to clarify that.

And I don't believe we will be allowed to change the class defaults
after class_init already ran. That may be a problem for -cpu "host" as
well. Maybe in the case of -cpu "host" we will need to translate the
behavior to a "enable-all-features-supported-by-the-host=true" property
that would be handled by instance_init and override all f-* flags, or
making the feature flags tristate (on/off/host), and setting
"f-feature=host" for all features.

> 
> > that, we wouldn't be able to differentiate the built/predefined vendor
> > value (used for TCG only) and the user-defined vendor value (that would
> > be set in KVM mode too).
> If we have subclasses with already correct vendor for a specific mode then we
> needn't to know if it is built-in vendor or user-defined.

True, but I still don't see an obvious way to make sure the class has
the already correct vendor for a specific mode.

That's why having separate "vendor"/"tcg-vendor" (and maybe even
"kvm-vendor") properties sounded like a feasible solution.

Anyway, this shouldn't hold this specific patch, and we will be forced
to discuss that when we have a patch that adds a "vendor" static
property, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> 
> > 
> > [1] AFAIU, class_init would never be able to check kvm_enabled(), by
> > design, because it may run before anything is configured (even the
> > choice to enable/disable KVM).
> > 
> > Maybe we can later add a "tcg-vendor" property, that would override the
> > vendor only if in TCG mode. Then the predefined CPU models could have a
> > "tcg-vendor" default set, and no "vendor" default set, to emulate the
> > current behavior.
> > 
> > > 'vendor' value is overridden when user provides it on -cpu command line,
> > > and there isn't need in vendor_override field anymore, remove it.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  v4:
> > >    - rebased with "target-i386: move out CPU features initialization
> > >      in separate func" dropped. So remove vendor_override in
> > >      cpu_x86_register() instead.
> > >    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> > >      due to renaming of the last in previous patch
> > > ---
> > >  target-i386/cpu.c |   27 ++++++++++++---------------
> > >  target-i386/cpu.h |    1 -
> > >  2 files changed, 12 insertions(+), 16 deletions(-)
> > > 
[...]

On Fri, Jan 11, 2013 at 03:10:20AM +0100, Igor Mammedov wrote:
> commit 8935499831312 makes cpuid return to guest host's vendor value
> instead of built-in one by default if kvm_enabled() == true and allows
> to override this behavior if 'vendor' is specified on -cpu command line.
> 
> But every time guest calls cpuid to get 'vendor' value, host's value is
> read again and again in default case.
> 
> It complicates semantic of vendor property and makes it harder to use.
> 
> Instead of reading 'vendor' value from host every time cpuid[vendor] is
> called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> built-in CPU model is found and if(kvm_enabled() == true).
> 
> It provides the same default semantic
>  if (kvm_enabled() == true)  vendor = host's vendor
>  else vendor = built-in vendor
> 
> and then later:
>  if (custom vendor) vendor = custom vendor
> 
> 'vendor' value is overridden when user provides it on -cpu command line,
> and there isn't need in vendor_override field anymore, remove it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

In case my previous reviewed-by line (buried in the discussion about
property defaults) was missed:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  v4:
>    - rebased with "target-i386: move out CPU features initialization
>      in separate func" dropped. So remove vendor_override in
>      cpu_x86_register() instead.
>    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>      due to renaming of the last in previous patch
> ---
>  target-i386/cpu.c |   27 ++++++++++++---------------
>  target-i386/cpu.h |    1 -
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 
[...]