On Wed, 19 Dec 2012 15:38:09 -0200
Eduardo Habkost <ehabk...@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 <imamm...@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
