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

[Eduardo Habkost]

On Thu, Nov 29, 2012 at 03:56:09PM +0100, Igor Mammedov wrote:
> 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.

You don't need CPU subclasses to reduce back-and-forth data movement.
We can just reorder code to look like:

 split_cpu_model_string(cpu_model, &model, &features);
 fill_cpudef_for_model_somehow(model, &cpudef);
 cpudef_2_x86_cpu(cpu, &cpudef);
 cpu_parse_feature_string(cpu, features);

Instead of what we have today, that is:

 split_cpu_model_string(cpu_model, &model, &features);
 fill_cpudef_for_model_somehow(model, &cpudef);
 cpu_parse_feature_string(cpu, features);
 /* (the 3 steps above are all done inside cpu_x86_find_by_name()) */
 cpudef_2_x86_cpu(cpu, &cpudef);

You did that on patch 32/37, but I would be interesting to do that
_before_ introducing the whole new CPU properties code, to make review
easier.

Reordering the steps to make the code less confusing before introducing
the CPU properties makes the code easier to review, that's why I sent
the CPU init cleanup series. And it makes us one step closer to
completely eliminate the X86CPUDefinition struct in the future.

I actually made the reordering change above using the CPU init series as
base, and it was very easy to do, after the CPU init was cleaned up. You
can see the result at:
https://github.com/ehabkost/qemu-hacks/commit/5256503135e787cb3550092c24dbeb3c5fc5a747
(it's completely untested. The rest of the changes are on the
work/cpu-model-classes branch)


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

I'm curious to see the result. Right now I am using the CPU init cleanup
series as base for my CPU subclass experiments.


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

-- 
Eduardo