diff mbox

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

Message ID 1350918203-25198-16-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Oct. 22, 2012, 3:03 p.m. UTC 
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Don Slutz Oct. 23, 2012, 9:39 p.m. UTC | #1 
On 10/22/12 11:03, Igor Mammedov 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;
>       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) {
Reviewed-by: Don Slutz <Don@CloudSwitch.com>
Eduardo Habkost Nov. 9, 2012, 3:48 p.m. UTC | #2 
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?

>     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
> 
>
Igor Mammedov Nov. 29, 2012, 2:56 p.m. UTC | #3 
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.

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
> > 
> > 
>
Eduardo Habkost Nov. 29, 2012, 3:47 p.m. UTC | #4 
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);
 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
> > > 
> > > 
> > 
>
Igor Mammedov Nov. 29, 2012, 6:30 p.m. UTC | #5 
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
> > > > 
> > > > 
> > > 
> > 
>
Eduardo Habkost Nov. 29, 2012, 7:14 p.m. UTC | #6 
On Thu, Nov 29, 2012 at 07:30:29PM +0100, Igor Mammedov wrote:
> 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).

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

Patch

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;
     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) {