| Message ID | 1360600511-25133-5-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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.
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 > > > > >
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 > > > > > > > > >
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 >
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
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 {
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 35 ++++++++++++++++++++++++++++++++++- 1 files changed, 34 insertions(+), 1 deletions(-)