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 07:30:29PM +0100, Igor Mammedov wrote:
> On Thu, 29 Nov 2012 13:47:37 -0200
> Eduardo Habkost <address@hidden> 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 <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);
> 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).

If the features are set by default on all models, we can hardcode or set
them by default either in the fill_cpudef_for_model_somehow() step
(meaning that the models have it implicitly set), or inside
cpudef_2_x86_cpu() (meaning that the feature can't be controlled by the
CPU feature string in any way).

In think both the KVM defaults and the hypervisor flag fit the former
case (because the defaults can be changed by the feature string later).
In other words: even if the feature is not mentioned in
builtin_x86_defs, it can be simply be  considered part of the defaults
for all CPU models.

Currently the defaults are set inside the feature string parsing code
(in the pseudo-code I wrote, that means setting them by default on
cpu_parse_feature_string()). That works, too, but it's confusing IMO.

One thing I forgot to mention about the example code, is that eventually
the two steps:

  fill_cpudef_for_model_somehow(model, &cpudef);
  cpudef_2_x86_cpu(cpu, &cpudef);

will become just a single step:

  initialize_default_flags_for_model(cpu, model);

or, even better, it will become:

  cpu = create_cpu_object_with_default_flags_already_set(model);

When we change the code to do that, we will be able to easily introduce
the subclasses, and to easily eliminate the X86CPUDefinnition struct
(aka x86_def_t).

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

The feature string can be used to disable features too, not just to
enable them, so we can't just merge two bitmaps later.

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

We don't need subclasses and class_init() to do that, we just need the
code to do that to run at the right point in cpu_x86_init(). And then
that code can be easily converted to subclasses.

I mean: the initialization will be done by class_init when we implement
subclasses, of course, but we don't need (and I don't want to) reorder
the initialization _and_ add the subclasses all in one big patch.


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

I really prefer the approach of cleaning up (or just isolating) the code
first, and then simply replace the isolated code with the new CPU
properties code. I think it makes it easier to review the code and make
sure that we are not introducing subtle unexpected behavior changes.
That's why I sent the CPU initialization refactor series.

Even if the CPU init cleanup series is not included, I think I will
rebase your series on top of the CPU init refactor locally, as part of
the review process--this way I can compare cpu_x86_parse_featurestr()
with the new CPU property setting code, and make sure nothing was left
out.

But if Andreas think the CPU properties series is reasonable as-is,
without applying the CPU init cleanup first, I won't mind redoing (parts
of) the CPU init changes later.


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

I will take a look, thanks.

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