Re: [Qemu-devel] [PATCH 06/17] target-i386: remove vendor_override field from CPUX86State

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 04:09:45AM +0100, Igor Mammedov wrote:
> On Fri, 11 Jan 2013 00:42:13 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden>

> 
> > 
> > [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 <address@hidden>
> > > ---
> > >  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(-)
> > > 
[...]

-- 
Eduardo