Patchwork [4/9] target-i386: convert 'hv_relaxed' to static property

login
register
mail settings
Submitter Igor Mammedov
Date Feb. 11, 2013, 4:35 p.m.
Message ID <1360600511-25133-5-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/219654/
State New
Headers show

Comments

Igor Mammedov - Feb. 11, 2013, 4:35 p.m.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)
Eduardo Habkost - Feb. 19, 2013, 6:45 p.m.
On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |   35 ++++++++++++++++++++++++++++++++++-
>  1 files changed, 34 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1f14b65..b804031 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -528,6 +528,38 @@ PropertyInfo qdev_prop_spinlocks = {
>      .defval = _defval                                                          \
>  }
>  
> +static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    bool value = hyperv_relaxed_timing_enabled();
> +
> +    visit_type_bool(v, &value, name, errp);
> +}
> +
> +static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    bool value;
> +
> +    visit_type_bool(v, &value, name, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +    hyperv_enable_relaxed_timing(value);
> +}
> +
> +PropertyInfo qdev_prop_hv_relaxed = {
> +    .name  = "boolean",
> +    .get   = x86_get_hv_relaxed,
> +    .set   = x86_set_hv_relaxed,
> +};
> +#define DEFINE_PROP_HV_RELAXED(_n, _defval) {                                  \
> +    .name  = _n,                                                               \
> +    .info  = &qdev_prop_hv_relaxed,                                            \
> +    .qtype = QTYPE_QBOOL,                                                      \
> +    .defval = _defval                                                          \
> +}
> +
>  static Property cpu_x86_properties[] = {
>      DEFINE_PROP_FAMILY("family"),
>      DEFINE_PROP_MODEL("model"),
> @@ -538,6 +570,7 @@ static Property cpu_x86_properties[] = {
>      DEFINE_PROP_MODEL_ID("model-id"),
>      DEFINE_PROP_TSC_FREQ("tsc-frequency"),
>      DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks", HYPERV_SPINLOCK_NEVER_RETRY),
> +    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),

Why not simply make it a X86CPU struct field, so we don't need a special
PropertyInfo?

The whole contents of target-i386/hyperv.c are getters/setters for three
static variables that should have been X86CPU fields in the first place.


>      DEFINE_PROP_END_OF_LIST(),
>   };
>  
> @@ -1468,7 +1501,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
>          } else if (!strcmp(featurestr, "enforce")) {
>              check_cpuid = enforce_cpuid = 1;
>          } else if (!strcmp(featurestr, "hv_relaxed")) {
> -            hyperv_enable_relaxed_timing(true);
> +            object_property_parse(OBJECT(cpu), "on", "hv-relaxed", errp);
>          } else if (!strcmp(featurestr, "hv_vapic")) {
>              hyperv_enable_vapic_recommended(true);
>          } else {
> -- 
> 1.7.1
> 
>
Andreas Färber - Feb. 19, 2013, 8:19 p.m.
Am 19.02.2013 19:45, schrieb Eduardo Habkost:
> On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>  target-i386/cpu.c |   35 ++++++++++++++++++++++++++++++++++-
>>  1 files changed, 34 insertions(+), 1 deletions(-)
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 1f14b65..b804031 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -528,6 +528,38 @@ PropertyInfo qdev_prop_spinlocks = {
>>      .defval = _defval                                                          \
>>  }
>>  
>> +static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
>> +                                 const char *name, Error **errp)
>> +{
>> +    bool value = hyperv_relaxed_timing_enabled();
>> +
>> +    visit_type_bool(v, &value, name, errp);
>> +}
>> +
>> +static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
>> +                                 const char *name, Error **errp)
>> +{
>> +    bool value;
>> +
>> +    visit_type_bool(v, &value, name, errp);
>> +    if (error_is_set(errp)) {
>> +        return;
>> +    }
>> +    hyperv_enable_relaxed_timing(value);
>> +}
>> +
>> +PropertyInfo qdev_prop_hv_relaxed = {
>> +    .name  = "boolean",
>> +    .get   = x86_get_hv_relaxed,
>> +    .set   = x86_set_hv_relaxed,
>> +};
>> +#define DEFINE_PROP_HV_RELAXED(_n, _defval) {                                  \
>> +    .name  = _n,                                                               \
>> +    .info  = &qdev_prop_hv_relaxed,                                            \
>> +    .qtype = QTYPE_QBOOL,                                                      \
>> +    .defval = _defval                                                          \
>> +}
>> +
>>  static Property cpu_x86_properties[] = {
>>      DEFINE_PROP_FAMILY("family"),
>>      DEFINE_PROP_MODEL("model"),
>> @@ -538,6 +570,7 @@ static Property cpu_x86_properties[] = {
>>      DEFINE_PROP_MODEL_ID("model-id"),
>>      DEFINE_PROP_TSC_FREQ("tsc-frequency"),
>>      DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks", HYPERV_SPINLOCK_NEVER_RETRY),
>> +    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),
> 
> Why not simply make it a X86CPU struct field, so we don't need a special
> PropertyInfo?
> 
> The whole contents of target-i386/hyperv.c are getters/setters for three
> static variables that should have been X86CPU fields in the first place.

Note that I intentionally did not make them properties because of that.
If the setting is not per CPU, it should not be set through X86CPU.
I don't know whether it would make sense to make these tunable per CPU?

Andreas
Eduardo Habkost - Feb. 19, 2013, 8:44 p.m.
On Tue, Feb 19, 2013 at 09:19:18PM +0100, Andreas Färber wrote:
> Am 19.02.2013 19:45, schrieb Eduardo Habkost:
> > On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
[...]
> >> +static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> >> +                                 const char *name, Error **errp)
> >> +{
> >> +    bool value = hyperv_relaxed_timing_enabled();
> >> +
> >> +    visit_type_bool(v, &value, name, errp);
> >> +}
> >> +
> >> +static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> >> +                                 const char *name, Error **errp)
> >> +{
> >> +    bool value;
> >> +
> >> +    visit_type_bool(v, &value, name, errp);
> >> +    if (error_is_set(errp)) {
> >> +        return;
> >> +    }
> >> +    hyperv_enable_relaxed_timing(value);
> >> +}
> >> +
> >> +PropertyInfo qdev_prop_hv_relaxed = {
> >> +    .name  = "boolean",
> >> +    .get   = x86_get_hv_relaxed,
> >> +    .set   = x86_set_hv_relaxed,
> >> +};
> >> +#define DEFINE_PROP_HV_RELAXED(_n, _defval) {                                  \
> >> +    .name  = _n,                                                               \
> >> +    .info  = &qdev_prop_hv_relaxed,                                            \
> >> +    .qtype = QTYPE_QBOOL,                                                      \
> >> +    .defval = _defval                                                          \
> >> +}
> >> +
> >>  static Property cpu_x86_properties[] = {
> >>      DEFINE_PROP_FAMILY("family"),
> >>      DEFINE_PROP_MODEL("model"),
> >> @@ -538,6 +570,7 @@ static Property cpu_x86_properties[] = {
> >>      DEFINE_PROP_MODEL_ID("model-id"),
> >>      DEFINE_PROP_TSC_FREQ("tsc-frequency"),
> >>      DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks", HYPERV_SPINLOCK_NEVER_RETRY),
> >> +    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),
> > 
> > Why not simply make it a X86CPU struct field, so we don't need a special
> > PropertyInfo?
> > 
> > The whole contents of target-i386/hyperv.c are getters/setters for three
> > static variables that should have been X86CPU fields in the first place.
> 
> Note that I intentionally did not make them properties because of that.
> If the setting is not per CPU, it should not be set through X86CPU.
> I don't know whether it would make sense to make these tunable per CPU?

All the getters from hyperv.c are only used when building the CPUID
tables for a specific CPU, so they can (and should) be handled like
every other CPUID-related field from X86CPU.
Igor Mammedov - Feb. 20, 2013, 9:03 a.m.
On Tue, 19 Feb 2013 15:45:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c |   35 ++++++++++++++++++++++++++++++++++-
> >  1 files changed, 34 insertions(+), 1 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 1f14b65..b804031 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -528,6 +528,38 @@ PropertyInfo qdev_prop_spinlocks = {
> >      .defval =
> > _defval                                                          \ }
> >  
> > +static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > +                                 const char *name, Error **errp)
> > +{
> > +    bool value = hyperv_relaxed_timing_enabled();
> > +
> > +    visit_type_bool(v, &value, name, errp);
> > +}
> > +
> > +static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > +                                 const char *name, Error **errp)
> > +{
> > +    bool value;
> > +
> > +    visit_type_bool(v, &value, name, errp);
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +    hyperv_enable_relaxed_timing(value);
> > +}
> > +
> > +PropertyInfo qdev_prop_hv_relaxed = {
> > +    .name  = "boolean",
> > +    .get   = x86_get_hv_relaxed,
> > +    .set   = x86_set_hv_relaxed,
> > +};
> > +#define DEFINE_PROP_HV_RELAXED(_n, _defval)
> > {                                  \
> > +    .name  =
> > _n,                                                               \
> > +    .info  =
> > &qdev_prop_hv_relaxed,                                            \
> > +    .qtype =
> > QTYPE_QBOOL,                                                      \
> > +    .defval =
> > _defval                                                          \ +}
> > +
> >  static Property cpu_x86_properties[] = {
> >      DEFINE_PROP_FAMILY("family"),
> >      DEFINE_PROP_MODEL("model"),
> > @@ -538,6 +570,7 @@ static Property cpu_x86_properties[] = {
> >      DEFINE_PROP_MODEL_ID("model-id"),
> >      DEFINE_PROP_TSC_FREQ("tsc-frequency"),
> >      DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks",
> > HYPERV_SPINLOCK_NEVER_RETRY),
> > +    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),
> 
> Why not simply make it a X86CPU struct field, so we don't need a special
> PropertyInfo?
> 
> The whole contents of target-i386/hyperv.c are getters/setters for three
> static variables that should have been X86CPU fields in the first place.
I went via less intrusive approach to avoid breaking anything during
conversion. Can we proceed with conversion first and than decide whether to
move hv_* into CPU or not?

> 
> 
> >      DEFINE_PROP_END_OF_LIST(),
> >   };
> >  
> > @@ -1468,7 +1501,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu,
> > char *features, Error **errp) } else if (!strcmp(featurestr, "enforce")) {
> >              check_cpuid = enforce_cpuid = 1;
> >          } else if (!strcmp(featurestr, "hv_relaxed")) {
> > -            hyperv_enable_relaxed_timing(true);
> > +            object_property_parse(OBJECT(cpu), "on", "hv-relaxed", errp);
> >          } else if (!strcmp(featurestr, "hv_vapic")) {
> >              hyperv_enable_vapic_recommended(true);
> >          } else {
> > -- 
> > 1.7.1
> > 
> > 
>
Eduardo Habkost - Feb. 20, 2013, 11:55 a.m.
On Wed, Feb 20, 2013 at 10:03:37AM +0100, Igor Mammedov wrote:
> On Tue, 19 Feb 2013 15:45:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  target-i386/cpu.c |   35 ++++++++++++++++++++++++++++++++++-
> > >  1 files changed, 34 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 1f14b65..b804031 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -528,6 +528,38 @@ PropertyInfo qdev_prop_spinlocks = {
> > >      .defval =
> > > _defval                                                          \ }
> > >  
> > > +static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > > +                                 const char *name, Error **errp)
> > > +{
> > > +    bool value = hyperv_relaxed_timing_enabled();
> > > +
> > > +    visit_type_bool(v, &value, name, errp);
> > > +}
> > > +
> > > +static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > > +                                 const char *name, Error **errp)
> > > +{
> > > +    bool value;
> > > +
> > > +    visit_type_bool(v, &value, name, errp);
> > > +    if (error_is_set(errp)) {
> > > +        return;
> > > +    }
> > > +    hyperv_enable_relaxed_timing(value);
> > > +}
> > > +
> > > +PropertyInfo qdev_prop_hv_relaxed = {
> > > +    .name  = "boolean",
> > > +    .get   = x86_get_hv_relaxed,
> > > +    .set   = x86_set_hv_relaxed,
> > > +};
> > > +#define DEFINE_PROP_HV_RELAXED(_n, _defval)
> > > {                                  \
> > > +    .name  =
> > > _n,                                                               \
> > > +    .info  =
> > > &qdev_prop_hv_relaxed,                                            \
> > > +    .qtype =
> > > QTYPE_QBOOL,                                                      \
> > > +    .defval =
> > > _defval                                                          \ +}
> > > +
> > >  static Property cpu_x86_properties[] = {
> > >      DEFINE_PROP_FAMILY("family"),
> > >      DEFINE_PROP_MODEL("model"),
> > > @@ -538,6 +570,7 @@ static Property cpu_x86_properties[] = {
> > >      DEFINE_PROP_MODEL_ID("model-id"),
> > >      DEFINE_PROP_TSC_FREQ("tsc-frequency"),
> > >      DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks",
> > > HYPERV_SPINLOCK_NEVER_RETRY),
> > > +    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),
> > 
> > Why not simply make it a X86CPU struct field, so we don't need a special
> > PropertyInfo?
> > 
> > The whole contents of target-i386/hyperv.c are getters/setters for three
> > static variables that should have been X86CPU fields in the first place.
> I went via less intrusive approach to avoid breaking anything during
> conversion. Can we proceed with conversion first and than decide whether to
> move hv_* into CPU or not?

Personally, I find the complex getter+setter+PropertyInfo+DEFINE_PROP_*
code above more complex and harder to review (thus harder to make me
confident it won't break anything) than simply moving a static variable
to a X86CPU field.

Also, it doesn't even make sense to have a X86CPU property available for
a static variable that is not per-CPU. What do we gain by making it look
like a per-X86CPU property if it is not?


> 
> > 
> > 
> > >      DEFINE_PROP_END_OF_LIST(),
> > >   };
> > >  
> > > @@ -1468,7 +1501,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu,
> > > char *features, Error **errp) } else if (!strcmp(featurestr, "enforce")) {
> > >              check_cpuid = enforce_cpuid = 1;
> > >          } else if (!strcmp(featurestr, "hv_relaxed")) {
> > > -            hyperv_enable_relaxed_timing(true);
> > > +            object_property_parse(OBJECT(cpu), "on", "hv-relaxed", errp);
> > >          } else if (!strcmp(featurestr, "hv_vapic")) {
> > >              hyperv_enable_vapic_recommended(true);
> > >          } else {
> > > -- 
> > > 1.7.1
> > > 
> > > 
> > 
>
Igor Mammedov - Feb. 22, 2013, 6:31 a.m.
On Wed, 20 Feb 2013 08:55:42 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Feb 20, 2013 at 10:03:37AM +0100, Igor Mammedov wrote:
> > On Tue, 19 Feb 2013 15:45:16 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  target-i386/cpu.c |   35 ++++++++++++++++++++++++++++++++++-
> > > >  1 files changed, 34 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 1f14b65..b804031 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -528,6 +528,38 @@ PropertyInfo qdev_prop_spinlocks = {
> > > >      .defval =
> > > > _defval                                                          \ }
> > > >  
> > > > +static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > > > +                                 const char *name, Error **errp)
> > > > +{
> > > > +    bool value = hyperv_relaxed_timing_enabled();
> > > > +
> > > > +    visit_type_bool(v, &value, name, errp);
> > > > +}
> > > > +
> > > > +static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > > > +                                 const char *name, Error **errp)
> > > > +{
> > > > +    bool value;
> > > > +
> > > > +    visit_type_bool(v, &value, name, errp);
> > > > +    if (error_is_set(errp)) {
> > > > +        return;
> > > > +    }
> > > > +    hyperv_enable_relaxed_timing(value);
> > > > +}
> > > > +
> > > > +PropertyInfo qdev_prop_hv_relaxed = {
> > > > +    .name  = "boolean",
> > > > +    .get   = x86_get_hv_relaxed,
> > > > +    .set   = x86_set_hv_relaxed,
> > > > +};
> > > > +#define DEFINE_PROP_HV_RELAXED(_n, _defval)
> > > > {                                  \
> > > > +    .name  =
> > > > _n,                                                               \
> > > > +    .info  =
> > > > &qdev_prop_hv_relaxed,                                            \
> > > > +    .qtype =
> > > > QTYPE_QBOOL,                                                      \
> > > > +    .defval =
> > > > _defval                                                          \ +}
> > > > +
> > > >  static Property cpu_x86_properties[] = {
> > > >      DEFINE_PROP_FAMILY("family"),
> > > >      DEFINE_PROP_MODEL("model"),
> > > > @@ -538,6 +570,7 @@ static Property cpu_x86_properties[] = {
> > > >      DEFINE_PROP_MODEL_ID("model-id"),
> > > >      DEFINE_PROP_TSC_FREQ("tsc-frequency"),
> > > >      DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks",
> > > > HYPERV_SPINLOCK_NEVER_RETRY),
> > > > +    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),
> > > 
> > > Why not simply make it a X86CPU struct field, so we don't need a special
> > > PropertyInfo?
> > > 
> > > The whole contents of target-i386/hyperv.c are getters/setters for three
> > > static variables that should have been X86CPU fields in the first place.
> > I went via less intrusive approach to avoid breaking anything during
> > conversion. Can we proceed with conversion first and than decide whether to
> > move hv_* into CPU or not?
> 
> Personally, I find the complex getter+setter+PropertyInfo+DEFINE_PROP_*
> code above more complex and harder to review (thus harder to make me
> confident it won't break anything) than simply moving a static variable
> to a X86CPU field.
That isn't as easy as you say. That would be more intrusive since it requires
to hack all the code that access this static variables, and I do not have
anything to test that kind of change. While using the same hv_* functions from
cpu_x86_parse_featurestr() in setters is easy to test with much less chance to
regress.

> 
> Also, it doesn't even make sense to have a X86CPU property available for
> a static variable that is not per-CPU. What do we gain by making it look
> like a per-X86CPU property if it is not?
getting/setting it. Aside of academic interest, It's not likely you'll have
CPUs with different hv_* values set ever and expect guest to work.

You can make this movement later if you like.

> 
> 
> > 
> > > 
> > > 
> > > >      DEFINE_PROP_END_OF_LIST(),
> > > >   };
> > > >  
> > > > @@ -1468,7 +1501,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu,
> > > > char *features, Error **errp) } else if (!strcmp(featurestr, "enforce")) {
> > > >              check_cpuid = enforce_cpuid = 1;
> > > >          } else if (!strcmp(featurestr, "hv_relaxed")) {
> > > > -            hyperv_enable_relaxed_timing(true);
> > > > +            object_property_parse(OBJECT(cpu), "on", "hv-relaxed", errp);
> > > >          } else if (!strcmp(featurestr, "hv_vapic")) {
> > > >              hyperv_enable_vapic_recommended(true);
> > > >          } else {
> > > > -- 
> > > > 1.7.1
> > > > 
> > > > 
> > > 
> > 
> 
> -- 
> Eduardo
>
Eduardo Habkost - Feb. 22, 2013, 12:39 p.m.
On Fri, Feb 22, 2013 at 07:31:17AM +0100, Igor Mammedov wrote:
> On Wed, 20 Feb 2013 08:55:42 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Feb 20, 2013 at 10:03:37AM +0100, Igor Mammedov wrote:
> > > On Tue, 19 Feb 2013 15:45:16 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > >  target-i386/cpu.c |   35 ++++++++++++++++++++++++++++++++++-
> > > > >  1 files changed, 34 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > index 1f14b65..b804031 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -528,6 +528,38 @@ PropertyInfo qdev_prop_spinlocks = {
> > > > >      .defval =
> > > > > _defval                                                          \ }
> > > > >  
> > > > > +static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > > > > +                                 const char *name, Error **errp)
> > > > > +{
> > > > > +    bool value = hyperv_relaxed_timing_enabled();
> > > > > +
> > > > > +    visit_type_bool(v, &value, name, errp);
> > > > > +}
> > > > > +
> > > > > +static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
> > > > > +                                 const char *name, Error **errp)
> > > > > +{
> > > > > +    bool value;
> > > > > +
> > > > > +    visit_type_bool(v, &value, name, errp);
> > > > > +    if (error_is_set(errp)) {
> > > > > +        return;
> > > > > +    }
> > > > > +    hyperv_enable_relaxed_timing(value);
> > > > > +}
> > > > > +
> > > > > +PropertyInfo qdev_prop_hv_relaxed = {
> > > > > +    .name  = "boolean",
> > > > > +    .get   = x86_get_hv_relaxed,
> > > > > +    .set   = x86_set_hv_relaxed,
> > > > > +};
> > > > > +#define DEFINE_PROP_HV_RELAXED(_n, _defval)
> > > > > {                                  \
> > > > > +    .name  =
> > > > > _n,                                                               \
> > > > > +    .info  =
> > > > > &qdev_prop_hv_relaxed,                                            \
> > > > > +    .qtype =
> > > > > QTYPE_QBOOL,                                                      \
> > > > > +    .defval =
> > > > > _defval                                                          \ +}
> > > > > +
> > > > >  static Property cpu_x86_properties[] = {
> > > > >      DEFINE_PROP_FAMILY("family"),
> > > > >      DEFINE_PROP_MODEL("model"),
> > > > > @@ -538,6 +570,7 @@ static Property cpu_x86_properties[] = {
> > > > >      DEFINE_PROP_MODEL_ID("model-id"),
> > > > >      DEFINE_PROP_TSC_FREQ("tsc-frequency"),
> > > > >      DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks",
> > > > > HYPERV_SPINLOCK_NEVER_RETRY),
> > > > > +    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),
> > > > 
> > > > Why not simply make it a X86CPU struct field, so we don't need a special
> > > > PropertyInfo?
> > > > 
> > > > The whole contents of target-i386/hyperv.c are getters/setters for three
> > > > static variables that should have been X86CPU fields in the first place.
> > > I went via less intrusive approach to avoid breaking anything during
> > > conversion. Can we proceed with conversion first and than decide whether to
> > > move hv_* into CPU or not?
> > 
> > Personally, I find the complex getter+setter+PropertyInfo+DEFINE_PROP_*
> > code above more complex and harder to review (thus harder to make me
> > confident it won't break anything) than simply moving a static variable
> > to a X86CPU field.
> That isn't as easy as you say. That would be more intrusive since it requires
> to hack all the code that access this static variables, and I do not have
> anything to test that kind of change. While using the same hv_* functions from
> cpu_x86_parse_featurestr() in setters is easy to test with much less chance to
> regress.
> 
> > 
> > Also, it doesn't even make sense to have a X86CPU property available for
> > a static variable that is not per-CPU. What do we gain by making it look
> > like a per-X86CPU property if it is not?
> getting/setting it.

Who exactly have the need to get/set it as if it were a property of the
CPU object, today?

I don't see any harm done if we don't introduce the property yet and
keep handling it as (yet another) special case inside
parse_featurestr().


> Aside of academic interest, It's not likely you'll have
> CPUs with different hv_* values set ever and expect guest to work.

This statement applies to most CPUID fields.

But the main problem here is not the existence of the static variables
themselves (they never bothered me too much), but that you are making a
externally-visible object model that doesn't match reality. To the
outside, the properties look like they are per-CPU, while they are not.
Then when we finally move the field, the behavior and semantics of the
properties will suddenly change.


> 
> You can make this movement later if you like.

No problem in moving it later, I don't see a need of moving the variable
as soon as possible. But I also don't see the point of adding properties
that look like CPU properties but are global variables. Can't we just
add the property only after we move the field, then?

> 
> > 
> > 
> > > 
> > > > 
> > > > 
> > > > >      DEFINE_PROP_END_OF_LIST(),
> > > > >   };
> > > > >  
> > > > > @@ -1468,7 +1501,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu,
> > > > > char *features, Error **errp) } else if (!strcmp(featurestr, "enforce")) {
> > > > >              check_cpuid = enforce_cpuid = 1;
> > > > >          } else if (!strcmp(featurestr, "hv_relaxed")) {
> > > > > -            hyperv_enable_relaxed_timing(true);
> > > > > +            object_property_parse(OBJECT(cpu), "on", "hv-relaxed", errp);
> > > > >          } else if (!strcmp(featurestr, "hv_vapic")) {
> > > > >              hyperv_enable_vapic_recommended(true);
> > > > >          } else {
> > > > > -- 
> > > > > 1.7.1
> > > > > 
> > > > > 
> > > > 
> > > 
> > 
> > -- 
> > Eduardo
> > 
> 
> 
> -- 
> Regards,
>   Igor

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1f14b65..b804031 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -528,6 +528,38 @@  PropertyInfo qdev_prop_spinlocks = {
     .defval = _defval                                                          \
 }
 
+static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value = hyperv_relaxed_timing_enabled();
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    hyperv_enable_relaxed_timing(value);
+}
+
+PropertyInfo qdev_prop_hv_relaxed = {
+    .name  = "boolean",
+    .get   = x86_get_hv_relaxed,
+    .set   = x86_set_hv_relaxed,
+};
+#define DEFINE_PROP_HV_RELAXED(_n, _defval) {                                  \
+    .name  = _n,                                                               \
+    .info  = &qdev_prop_hv_relaxed,                                            \
+    .qtype = QTYPE_QBOOL,                                                      \
+    .defval = _defval                                                          \
+}
+
 static Property cpu_x86_properties[] = {
     DEFINE_PROP_FAMILY("family"),
     DEFINE_PROP_MODEL("model"),
@@ -538,6 +570,7 @@  static Property cpu_x86_properties[] = {
     DEFINE_PROP_MODEL_ID("model-id"),
     DEFINE_PROP_TSC_FREQ("tsc-frequency"),
     DEFINE_PROP_HV_SPINLOCKS("hv-spinlocks", HYPERV_SPINLOCK_NEVER_RETRY),
+    DEFINE_PROP_HV_RELAXED("hv-relaxed", false),
     DEFINE_PROP_END_OF_LIST(),
  };
 
@@ -1468,7 +1501,7 @@  static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
         } else if (!strcmp(featurestr, "enforce")) {
             check_cpuid = enforce_cpuid = 1;
         } else if (!strcmp(featurestr, "hv_relaxed")) {
-            hyperv_enable_relaxed_timing(true);
+            object_property_parse(OBJECT(cpu), "on", "hv-relaxed", errp);
         } else if (!strcmp(featurestr, "hv_vapic")) {
             hyperv_enable_vapic_recommended(true);
         } else {