Re: [Qemu-devel] [PATCH 4/9] target-i386: convert 'hv_relaxed' to static property

[Eduardo Habkost]

On Fri, Feb 22, 2013 at 07:31:17AM +0100, Igor Mammedov wrote:
> On Wed, 20 Feb 2013 08:55:42 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden> wrote:
> > > 
> > > > On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
> > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > ---
> > > > >  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

-- 
Eduardo