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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 29 Nov 2012 13:47:37 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Nov 29, 2012 at 03:56:09PM +0100, Igor Mammedov wrote:
> > On Fri, 9 Nov 2012 16:48:07 +0100
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > 
> > > On 22/10/2012, at 17:03, Igor Mammedov <imammedo@redhat.com> wrote:
> > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > 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);
somewhere between here and [2], One still needs to enable "hypervisor"
feature, and set kvm_features defaults. I guess "hypervisor" could be pushed
out into x86_def_t for each model and kvm_cpu_fill_host(). But it would be
hard to do so for KVM features (see how pv_eoi is set).

see 8d239fbb9b, ded9f32153, a7bb291e5b, 2623e2f876, bd539e14ef in
https://github.com/imammedo/qemu/commits/x86-cpu-properties.WIP

>  cpudef_2_x86_cpu(cpu, &cpudef);
As alternative it's possible to merge (OR) ext_features and kvm_features from
cpudef into env inside of cpudef_2_x86_cpu() to keep defaults SET by static
properties when cpu obj was created.

[2]

This merge won't disappear completely until cpudefs are converted into
classes and X_class_init() will set all defaults for a given cpu model.

and common defaults could/should be set by parent X86CPU class_init(),
and ideally rewrite should end up in a inheritance tree of subclasses, each
adding new features relative to previous model.

>  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
After some playing, I've abandoned approach to change
cpu_x86_find_by_name() and opted in favor of not touching it and feature
string parsing much, except of doing almost the same split as you. As
additional thing, I've moved setting defaults out of it as well.

> (it's completely untested. The rest of the changes are on the
> work/cpu-model-classes branch)

Some reordering has been done to series to make it clearer and easier to
review. Pls, checkout it out on my x86-cpu-properties.WIP

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