On Wed, 20 Feb 2013 08:55:42 -0300
Eduardo Habkost <ehabk...@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 <ehabk...@redhat.com> wrote:
> >
> > > On Mon, Feb 11, 2013 at 05:35:06PM +0100, Igor Mammedov wrote:
> > > > Signed-off-by: Igor Mammedov <imamm...@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
>
--
Regards,
Igor
