Re: [Qemu-devel] [PATCH 15/37] target-i386: set default value of "hypervisor" feature using static property

[Igor Mammedov]

On Fri, 9 Nov 2012 16:48:07 +0100
Eduardo Habkost <address@hidden> wrote:

> 
> On 22/10/2012, at 17:03, Igor Mammedov <address@hidden> wrote:
> 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > target-i386/cpu.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3131945..dc4fcdf 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -174,7 +174,7 @@ static Property cpu_x86_properties[] = {
> >     DEFINE_PROP_BIT("f-xsave", X86CPU, env.cpuid_ext_features, 26, false),
> >     DEFINE_PROP_BIT("f-osxsave", X86CPU, env.cpuid_ext_features, 27,
> > false), DEFINE_PROP_BIT("f-avx", X86CPU, env.cpuid_ext_features, 28,
> > false),
> > -    DEFINE_PROP_BIT("f-hypervisor", X86CPU, env.cpuid_ext_features, 31,
> > false),
> > +    DEFINE_PROP_BIT("f-hypervisor", X86CPU, env.cpuid_ext_features, 31,
> > true), DEFINE_PROP_BIT("f-syscall", X86CPU, env.cpuid_ext2_features, 11,
> > false), DEFINE_PROP_BIT("f-nx", X86CPU, env.cpuid_ext2_features, 20,
> > false), DEFINE_PROP_BIT("f-xd", X86CPU, env.cpuid_ext2_features, 20,
> > false), @@ -1307,11 +1307,12 @@ static int cpu_x86_find_by_name(X86CPU
> > *cpu, x86_def_t *x86_cpu_def, {
> >     unsigned int i;
> >     x86_def_t *def;
> > +    CPUX86State *env = &cpu->env;
> > 
> >     char *s = g_strdup(cpu_model);
> >     char *featurestr, *name = strtok(s, ",");
> >     /* Features to be added*/
> > -    uint32_t plus_features = 0, plus_ext_features = 0;
> > +    uint32_t plus_features = 0, plus_ext_features =
> > env->cpuid_ext_features;
> 
> Moving data back and forth between CPUX86State and x86_def_t makes the
> initialization ordering confusing (today data is moved from x86_def_t to
> X86CPU, and never the other way around).
> 
> As this code is removed in the next patches, I don't mind too much, but
> maybe it's simpler to implement this change only after the "use static
> properties for setting cpuid features" patch?
It won't eliminate moving data back and forth between CPUX86State and
x86_def_t until CPU sub-classes (i.e. when x86_def_t isn't used anymore),
if defaults from static properties are used.

But I've rewritten patches in a way to make this easier to review and less
confusing.

> 
> >     uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> >     uint32_t plus_kvm_features = 0, plus_svm_features = 0;
> >     uint32_t plus_7_0_ebx_features = 0;
> > @@ -1345,10 +1346,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu,
> > x86_def_t *x86_cpu_def, plus_kvm_features = 0;
> > #endif
> > 
> > -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> > -            &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> > -            &plus_kvm_features, &plus_svm_features,
> > &plus_7_0_ebx_features); -
> >     featurestr = strtok(NULL, ",");
> > 
> >     while (featurestr) {
> > -- 
> > 1.7.11.7
> > 
> > 
>