| Message ID | 1503050939-227939-4-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 18, 2017 at 12:08:35PM +0200, Igor Mammedov wrote: > SPARC is the last target that uses legacy way of parsing > and initializing cpu features, drop legacy approach and > convert features to properties so that SPARC could as minimum > benefit from generic cpu_generic_init(), common with > x86 +-feat parser > > PS: > the main purpose is to remove legacy way of cpu creation as > a blocker for unifying cpu creation code across targets. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> [...] > --- > CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > CC: Artyom Tarasenko <atar4qemu@gmail.com> > CC: Eduardo Habkost <ehabkost@redhat.com> > --- > target/sparc/cpu.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c > index f4e7343..e735d73 100644 > --- a/target/sparc/cpu.c > +++ b/target/sparc/cpu.c > @@ -22,6 +22,8 @@ > #include "cpu.h" > #include "qemu/error-report.h" > #include "exec/exec-all.h" > +#include "hw/qdev-properties.h" > +#include "qapi/visitor.h" > > //#define DEBUG_FEATURES > > @@ -853,6 +855,69 @@ static void sparc_cpu_initfn(Object *obj) > } > } > > +static void sparc_get_nwindows(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + SPARCCPU *cpu = SPARC_CPU(obj); > + int64_t value = cpu->env.def.nwindows; > + > + visit_type_int(v, name, &value, errp); > +} > + > +static void sparc_set_nwindows(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + const int64_t min = MIN_NWINDOWS; > + const int64_t max = MAX_NWINDOWS; > + SPARCCPU *cpu = SPARC_CPU(obj); > + Error *err = NULL; > + int64_t value; > + > + visit_type_int(v, name, &value, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + if (value < min || value > max) { > + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 > + " (minimum: %" PRId64 ", maximum: %" PRId64 ")", > + object_get_typename(obj), name ? name : "null", > + value, min, max); > + return; > + } > + cpu->env.def.nwindows = value; I think it would be much simpler to just validate nwindows at realize time and use DEFINE_PROP_UINT32 below, but I won't mind if you really prefer the custom setter. But I have another question: > +} > + > +static PropertyInfo qdev_prop_nwindows = { > + .name = "int", > + .get = sparc_get_nwindows, > + .set = sparc_set_nwindows, > +}; > + > +static Property sparc_cpu_properties[] = { > + DEFINE_PROP_BIT("float", SPARCCPU, env.def.features, 0, false), > + DEFINE_PROP_BIT("float128", SPARCCPU, env.def.features, 1, false), > + DEFINE_PROP_BIT("swap", SPARCCPU, env.def.features, 2, false), > + DEFINE_PROP_BIT("mul", SPARCCPU, env.def.features, 3, false), > + DEFINE_PROP_BIT("div", SPARCCPU, env.def.features, 4, false), > + DEFINE_PROP_BIT("flush", SPARCCPU, env.def.features, 5, false), > + DEFINE_PROP_BIT("fsqrt", SPARCCPU, env.def.features, 6, false), > + DEFINE_PROP_BIT("fmul", SPARCCPU, env.def.features, 7, false), > + DEFINE_PROP_BIT("vis1", SPARCCPU, env.def.features, 8, false), > + DEFINE_PROP_BIT("vis2", SPARCCPU, env.def.features, 9, false), > + DEFINE_PROP_BIT("fsmuld", SPARCCPU, env.def.features, 10, false), > + DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), > + DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), > + DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), > + DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, > + qdev_prop_uint64, target_ulong), > + DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), > + DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), > + { .name = "nwindows", .info = &qdev_prop_nwindows }, What's the advantage of using a custom PropertyInfo struct if you can just call object_class_property_add() at sparc_cpu_class_init()? > + DEFINE_PROP_END_OF_LIST() > +}; > + > static void sparc_cpu_class_init(ObjectClass *oc, void *data) > { > SPARCCPUClass *scc = SPARC_CPU_CLASS(oc); > @@ -861,6 +926,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > scc->parent_realize = dc->realize; > dc->realize = sparc_cpu_realizefn; > + dc->props = sparc_cpu_properties; > > scc->parent_reset = cc->reset; > cc->reset = sparc_cpu_reset; > -- > 2.7.4 > >
On Fri, 18 Aug 2017 15:24:04 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Aug 18, 2017 at 12:08:35PM +0200, Igor Mammedov wrote: > > SPARC is the last target that uses legacy way of parsing > > and initializing cpu features, drop legacy approach and > > convert features to properties so that SPARC could as minimum > > benefit from generic cpu_generic_init(), common with > > x86 +-feat parser > > > > PS: > > the main purpose is to remove legacy way of cpu creation as > > a blocker for unifying cpu creation code across targets. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > [...] > > --- > > CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > CC: Artyom Tarasenko <atar4qemu@gmail.com> > > CC: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target/sparc/cpu.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > > > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c > > index f4e7343..e735d73 100644 > > --- a/target/sparc/cpu.c > > +++ b/target/sparc/cpu.c > > @@ -22,6 +22,8 @@ > > #include "cpu.h" > > #include "qemu/error-report.h" > > #include "exec/exec-all.h" > > +#include "hw/qdev-properties.h" > > +#include "qapi/visitor.h" > > > > //#define DEBUG_FEATURES > > > > @@ -853,6 +855,69 @@ static void sparc_cpu_initfn(Object *obj) > > } > > } > > > > +static void sparc_get_nwindows(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + SPARCCPU *cpu = SPARC_CPU(obj); > > + int64_t value = cpu->env.def.nwindows; > > + > > + visit_type_int(v, name, &value, errp); > > +} > > + > > +static void sparc_set_nwindows(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + const int64_t min = MIN_NWINDOWS; > > + const int64_t max = MAX_NWINDOWS; > > + SPARCCPU *cpu = SPARC_CPU(obj); > > + Error *err = NULL; > > + int64_t value; > > + > > + visit_type_int(v, name, &value, &err); > > + if (err) { > > + error_propagate(errp, err); > > + return; > > + } > > + > > + if (value < min || value > max) { > > + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 > > + " (minimum: %" PRId64 ", maximum: %" PRId64 ")", > > + object_get_typename(obj), name ? name : "null", > > + value, min, max); > > + return; > > + } > > + cpu->env.def.nwindows = value; > > I think it would be much simpler to just validate nwindows at > realize time and use DEFINE_PROP_UINT32 below, but I won't mind > if you really prefer the custom setter. Indeed it would shave off ~12LOC if check is done at realize time, but I prefer to throw error at the place where it's set. I'd go for realize approach only if check couldn't be done at property setting time. > But I have another question: > > > +} > > + > > +static PropertyInfo qdev_prop_nwindows = { > > + .name = "int", > > + .get = sparc_get_nwindows, > > + .set = sparc_set_nwindows, > > +}; > > + > > +static Property sparc_cpu_properties[] = { > > + DEFINE_PROP_BIT("float", SPARCCPU, env.def.features, 0, false), > > + DEFINE_PROP_BIT("float128", SPARCCPU, env.def.features, 1, false), > > + DEFINE_PROP_BIT("swap", SPARCCPU, env.def.features, 2, false), > > + DEFINE_PROP_BIT("mul", SPARCCPU, env.def.features, 3, false), > > + DEFINE_PROP_BIT("div", SPARCCPU, env.def.features, 4, false), > > + DEFINE_PROP_BIT("flush", SPARCCPU, env.def.features, 5, false), > > + DEFINE_PROP_BIT("fsqrt", SPARCCPU, env.def.features, 6, false), > > + DEFINE_PROP_BIT("fmul", SPARCCPU, env.def.features, 7, false), > > + DEFINE_PROP_BIT("vis1", SPARCCPU, env.def.features, 8, false), > > + DEFINE_PROP_BIT("vis2", SPARCCPU, env.def.features, 9, false), > > + DEFINE_PROP_BIT("fsmuld", SPARCCPU, env.def.features, 10, false), > > + DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), > > + DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), > > + DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), > > + DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, > > + qdev_prop_uint64, target_ulong), > > + DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), > > + DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), > > + { .name = "nwindows", .info = &qdev_prop_nwindows }, > > What's the advantage of using a custom PropertyInfo struct if you > can just call object_class_property_add() at > sparc_cpu_class_init()? consistentcy with the rest of properties added above and instead of a zoo of ways to add property within the patch/featureset > > > + DEFINE_PROP_END_OF_LIST() > > +}; > > + > > static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > { > > SPARCCPUClass *scc = SPARC_CPU_CLASS(oc); > > @@ -861,6 +926,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > > > scc->parent_realize = dc->realize; > > dc->realize = sparc_cpu_realizefn; > > + dc->props = sparc_cpu_properties; > > > > scc->parent_reset = cc->reset; > > cc->reset = sparc_cpu_reset; > > -- > > 2.7.4 > > > > >
On Mon, Aug 21, 2017 at 01:03:07PM +0200, Igor Mammedov wrote: > On Fri, 18 Aug 2017 15:24:04 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Aug 18, 2017 at 12:08:35PM +0200, Igor Mammedov wrote: > > > SPARC is the last target that uses legacy way of parsing > > > and initializing cpu features, drop legacy approach and > > > convert features to properties so that SPARC could as minimum > > > benefit from generic cpu_generic_init(), common with > > > x86 +-feat parser > > > > > > PS: > > > the main purpose is to remove legacy way of cpu creation as > > > a blocker for unifying cpu creation code across targets. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > [...] > > > --- > > > CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > CC: Artyom Tarasenko <atar4qemu@gmail.com> > > > CC: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > target/sparc/cpu.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 66 insertions(+) > > > > > > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c > > > index f4e7343..e735d73 100644 > > > --- a/target/sparc/cpu.c > > > +++ b/target/sparc/cpu.c > > > @@ -22,6 +22,8 @@ > > > #include "cpu.h" > > > #include "qemu/error-report.h" > > > #include "exec/exec-all.h" > > > +#include "hw/qdev-properties.h" > > > +#include "qapi/visitor.h" > > > > > > //#define DEBUG_FEATURES > > > > > > @@ -853,6 +855,69 @@ static void sparc_cpu_initfn(Object *obj) > > > } > > > } > > > > > > +static void sparc_get_nwindows(Object *obj, Visitor *v, const char *name, > > > + void *opaque, Error **errp) > > > +{ > > > + SPARCCPU *cpu = SPARC_CPU(obj); > > > + int64_t value = cpu->env.def.nwindows; > > > + > > > + visit_type_int(v, name, &value, errp); > > > +} > > > + > > > +static void sparc_set_nwindows(Object *obj, Visitor *v, const char *name, > > > + void *opaque, Error **errp) > > > +{ > > > + const int64_t min = MIN_NWINDOWS; > > > + const int64_t max = MAX_NWINDOWS; > > > + SPARCCPU *cpu = SPARC_CPU(obj); > > > + Error *err = NULL; > > > + int64_t value; > > > + > > > + visit_type_int(v, name, &value, &err); > > > + if (err) { > > > + error_propagate(errp, err); > > > + return; > > > + } > > > + > > > + if (value < min || value > max) { > > > + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 > > > + " (minimum: %" PRId64 ", maximum: %" PRId64 ")", > > > + object_get_typename(obj), name ? name : "null", > > > + value, min, max); > > > + return; > > > + } > > > + cpu->env.def.nwindows = value; > > > > I think it would be much simpler to just validate nwindows at > > realize time and use DEFINE_PROP_UINT32 below, but I won't mind > > if you really prefer the custom setter. > Indeed it would shave off ~12LOC if check is done at realize time, > but I prefer to throw error at the place where it's set. > I'd go for realize approach only if check couldn't be done > at property setting time. I would prefer to avoid custom getters/setters when possible, but I agree it is better to validate the value as soon as possible (on the setter instead of realize). If we want to avoid custom setters, we need to improve the DEFINE_PROP_* interface. > > > > But I have another question: > > > > > +} > > > + > > > +static PropertyInfo qdev_prop_nwindows = { > > > + .name = "int", > > > + .get = sparc_get_nwindows, > > > + .set = sparc_set_nwindows, > > > +}; > > > + > > > +static Property sparc_cpu_properties[] = { > > > + DEFINE_PROP_BIT("float", SPARCCPU, env.def.features, 0, false), > > > + DEFINE_PROP_BIT("float128", SPARCCPU, env.def.features, 1, false), > > > + DEFINE_PROP_BIT("swap", SPARCCPU, env.def.features, 2, false), > > > + DEFINE_PROP_BIT("mul", SPARCCPU, env.def.features, 3, false), > > > + DEFINE_PROP_BIT("div", SPARCCPU, env.def.features, 4, false), > > > + DEFINE_PROP_BIT("flush", SPARCCPU, env.def.features, 5, false), > > > + DEFINE_PROP_BIT("fsqrt", SPARCCPU, env.def.features, 6, false), > > > + DEFINE_PROP_BIT("fmul", SPARCCPU, env.def.features, 7, false), > > > + DEFINE_PROP_BIT("vis1", SPARCCPU, env.def.features, 8, false), > > > + DEFINE_PROP_BIT("vis2", SPARCCPU, env.def.features, 9, false), > > > + DEFINE_PROP_BIT("fsmuld", SPARCCPU, env.def.features, 10, false), > > > + DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), > > > + DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), > > > + DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), > > > + DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, > > > + qdev_prop_uint64, target_ulong), > > > + DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), > > > + DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), > > > + { .name = "nwindows", .info = &qdev_prop_nwindows }, > > > > What's the advantage of using a custom PropertyInfo struct if you > > can just call object_class_property_add() at > > sparc_cpu_class_init()? > consistentcy with the rest of properties added above > and instead of a zoo of ways to add property within the patch/featureset Well, both options (custom PropertyInfo and manual object_class_property_add() call) look bad to me. But improving the property API to not require a custom setter is out of context of this series, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > > > + DEFINE_PROP_END_OF_LIST() > > > +}; > > > + > > > static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > > { > > > SPARCCPUClass *scc = SPARC_CPU_CLASS(oc); > > > @@ -861,6 +926,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > > > > > scc->parent_realize = dc->realize; > > > dc->realize = sparc_cpu_realizefn; > > > + dc->props = sparc_cpu_properties; > > > > > > scc->parent_reset = cc->reset; > > > cc->reset = sparc_cpu_reset; > > > -- > > > 2.7.4 > > > > > > > > >
On Wed, 23 Aug 2017 10:07:14 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Aug 21, 2017 at 01:03:07PM +0200, Igor Mammedov wrote: > > On Fri, 18 Aug 2017 15:24:04 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Fri, Aug 18, 2017 at 12:08:35PM +0200, Igor Mammedov wrote: > > > > SPARC is the last target that uses legacy way of parsing > > > > and initializing cpu features, drop legacy approach and > > > > convert features to properties so that SPARC could as minimum > > > > benefit from generic cpu_generic_init(), common with > > > > x86 +-feat parser > > > > > > > > PS: > > > > the main purpose is to remove legacy way of cpu creation as > > > > a blocker for unifying cpu creation code across targets. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > [...] > > > > --- > > > > CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > > CC: Artyom Tarasenko <atar4qemu@gmail.com> > > > > CC: Eduardo Habkost <ehabkost@redhat.com> > > > > --- > > > > target/sparc/cpu.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 66 insertions(+) > > > > > > > > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c > > > > index f4e7343..e735d73 100644 > > > > --- a/target/sparc/cpu.c > > > > +++ b/target/sparc/cpu.c > > > > @@ -22,6 +22,8 @@ > > > > #include "cpu.h" > > > > #include "qemu/error-report.h" > > > > #include "exec/exec-all.h" > > > > +#include "hw/qdev-properties.h" > > > > +#include "qapi/visitor.h" > > > > > > > > //#define DEBUG_FEATURES > > > > > > > > @@ -853,6 +855,69 @@ static void sparc_cpu_initfn(Object *obj) > > > > } > > > > } > > > > > > > > +static void sparc_get_nwindows(Object *obj, Visitor *v, const char *name, > > > > + void *opaque, Error **errp) > > > > +{ > > > > + SPARCCPU *cpu = SPARC_CPU(obj); > > > > + int64_t value = cpu->env.def.nwindows; > > > > + > > > > + visit_type_int(v, name, &value, errp); > > > > +} > > > > + > > > > +static void sparc_set_nwindows(Object *obj, Visitor *v, const char *name, > > > > + void *opaque, Error **errp) > > > > +{ > > > > + const int64_t min = MIN_NWINDOWS; > > > > + const int64_t max = MAX_NWINDOWS; > > > > + SPARCCPU *cpu = SPARC_CPU(obj); > > > > + Error *err = NULL; > > > > + int64_t value; > > > > + > > > > + visit_type_int(v, name, &value, &err); > > > > + if (err) { > > > > + error_propagate(errp, err); > > > > + return; > > > > + } > > > > + > > > > + if (value < min || value > max) { > > > > + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 > > > > + " (minimum: %" PRId64 ", maximum: %" PRId64 ")", > > > > + object_get_typename(obj), name ? name : "null", > > > > + value, min, max); > > > > + return; > > > > + } > > > > + cpu->env.def.nwindows = value; > > > > > > I think it would be much simpler to just validate nwindows at > > > realize time and use DEFINE_PROP_UINT32 below, but I won't mind > > > if you really prefer the custom setter. > > Indeed it would shave off ~12LOC if check is done at realize time, > > but I prefer to throw error at the place where it's set. > > I'd go for realize approach only if check couldn't be done > > at property setting time. > > I would prefer to avoid custom getters/setters when possible, but > I agree it is better to validate the value as soon as possible > (on the setter instead of realize). If we want to avoid custom > setters, we need to improve the DEFINE_PROP_* interface. Agreed, we need better properties handling API, I just never got to actually improving it. > > > > > > > > But I have another question: > > > > > > > +} > > > > + > > > > +static PropertyInfo qdev_prop_nwindows = { > > > > + .name = "int", > > > > + .get = sparc_get_nwindows, > > > > + .set = sparc_set_nwindows, > > > > +}; > > > > + > > > > +static Property sparc_cpu_properties[] = { > > > > + DEFINE_PROP_BIT("float", SPARCCPU, env.def.features, 0, false), > > > > + DEFINE_PROP_BIT("float128", SPARCCPU, env.def.features, 1, false), > > > > + DEFINE_PROP_BIT("swap", SPARCCPU, env.def.features, 2, false), > > > > + DEFINE_PROP_BIT("mul", SPARCCPU, env.def.features, 3, false), > > > > + DEFINE_PROP_BIT("div", SPARCCPU, env.def.features, 4, false), > > > > + DEFINE_PROP_BIT("flush", SPARCCPU, env.def.features, 5, false), > > > > + DEFINE_PROP_BIT("fsqrt", SPARCCPU, env.def.features, 6, false), > > > > + DEFINE_PROP_BIT("fmul", SPARCCPU, env.def.features, 7, false), > > > > + DEFINE_PROP_BIT("vis1", SPARCCPU, env.def.features, 8, false), > > > > + DEFINE_PROP_BIT("vis2", SPARCCPU, env.def.features, 9, false), > > > > + DEFINE_PROP_BIT("fsmuld", SPARCCPU, env.def.features, 10, false), > > > > + DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), > > > > + DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), > > > > + DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), > > > > + DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, > > > > + qdev_prop_uint64, target_ulong), > > > > + DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), > > > > + DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), > > > > + { .name = "nwindows", .info = &qdev_prop_nwindows }, > > > > > > What's the advantage of using a custom PropertyInfo struct if you > > > can just call object_class_property_add() at > > > sparc_cpu_class_init()? > > consistentcy with the rest of properties added above > > and instead of a zoo of ways to add property within the patch/featureset > > Well, both options (custom PropertyInfo and manual > object_class_property_add() call) look bad to me. But improving > the property API to not require a custom setter is out of context > of this series, so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Thanks! > > > > > > > > > > + DEFINE_PROP_END_OF_LIST() > > > > +}; > > > > + > > > > static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > > > { > > > > SPARCCPUClass *scc = SPARC_CPU_CLASS(oc); > > > > @@ -861,6 +926,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > > > > > > > scc->parent_realize = dc->realize; > > > > dc->realize = sparc_cpu_realizefn; > > > > + dc->props = sparc_cpu_properties; > > > > > > > > scc->parent_reset = cc->reset; > > > > cc->reset = sparc_cpu_reset; > > > > -- > > > > 2.7.4 > > > > > > > > > > > > > >
On 08/18/2017 07:08 AM, Igor Mammedov wrote: > SPARC is the last target that uses legacy way of parsing > and initializing cpu features, drop legacy approach and > convert features to properties so that SPARC could as minimum > benefit from generic cpu_generic_init(), common with > x86 +-feat parser > > PS: > the main purpose is to remove legacy way of cpu creation as > a blocker for unifying cpu creation code across targets. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > CC: Artyom Tarasenko <atar4qemu@gmail.com> > CC: Eduardo Habkost <ehabkost@redhat.com> > --- > target/sparc/cpu.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c > index f4e7343..e735d73 100644 > --- a/target/sparc/cpu.c > +++ b/target/sparc/cpu.c > @@ -22,6 +22,8 @@ > #include "cpu.h" > #include "qemu/error-report.h" > #include "exec/exec-all.h" > +#include "hw/qdev-properties.h" > +#include "qapi/visitor.h" > > //#define DEBUG_FEATURES > > @@ -853,6 +855,69 @@ static void sparc_cpu_initfn(Object *obj) > } > } > > +static void sparc_get_nwindows(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + SPARCCPU *cpu = SPARC_CPU(obj); > + int64_t value = cpu->env.def.nwindows; > + > + visit_type_int(v, name, &value, errp); > +} > + > +static void sparc_set_nwindows(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + const int64_t min = MIN_NWINDOWS; > + const int64_t max = MAX_NWINDOWS; > + SPARCCPU *cpu = SPARC_CPU(obj); > + Error *err = NULL; > + int64_t value; > + > + visit_type_int(v, name, &value, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + if (value < min || value > max) { > + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 > + " (minimum: %" PRId64 ", maximum: %" PRId64 ")", > + object_get_typename(obj), name ? name : "null", > + value, min, max); > + return; > + } > + cpu->env.def.nwindows = value; > +} > + > +static PropertyInfo qdev_prop_nwindows = { > + .name = "int", > + .get = sparc_get_nwindows, > + .set = sparc_set_nwindows, > +}; > + > +static Property sparc_cpu_properties[] = { > + DEFINE_PROP_BIT("float", SPARCCPU, env.def.features, 0, false), > + DEFINE_PROP_BIT("float128", SPARCCPU, env.def.features, 1, false), > + DEFINE_PROP_BIT("swap", SPARCCPU, env.def.features, 2, false), > + DEFINE_PROP_BIT("mul", SPARCCPU, env.def.features, 3, false), > + DEFINE_PROP_BIT("div", SPARCCPU, env.def.features, 4, false), > + DEFINE_PROP_BIT("flush", SPARCCPU, env.def.features, 5, false), > + DEFINE_PROP_BIT("fsqrt", SPARCCPU, env.def.features, 6, false), > + DEFINE_PROP_BIT("fmul", SPARCCPU, env.def.features, 7, false), > + DEFINE_PROP_BIT("vis1", SPARCCPU, env.def.features, 8, false), > + DEFINE_PROP_BIT("vis2", SPARCCPU, env.def.features, 9, false), > + DEFINE_PROP_BIT("fsmuld", SPARCCPU, env.def.features, 10, false), > + DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), > + DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), > + DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), > + DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, > + qdev_prop_uint64, target_ulong), > + DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), > + DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), > + { .name = "nwindows", .info = &qdev_prop_nwindows }, > + DEFINE_PROP_END_OF_LIST() > +}; > + > static void sparc_cpu_class_init(ObjectClass *oc, void *data) > { > SPARCCPUClass *scc = SPARC_CPU_CLASS(oc); > @@ -861,6 +926,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) > > scc->parent_realize = dc->realize; > dc->realize = sparc_cpu_realizefn; > + dc->props = sparc_cpu_properties; > > scc->parent_reset = cc->reset; > cc->reset = sparc_cpu_reset; >
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c index f4e7343..e735d73 100644 --- a/target/sparc/cpu.c +++ b/target/sparc/cpu.c @@ -22,6 +22,8 @@ #include "cpu.h" #include "qemu/error-report.h" #include "exec/exec-all.h" +#include "hw/qdev-properties.h" +#include "qapi/visitor.h" //#define DEBUG_FEATURES @@ -853,6 +855,69 @@ static void sparc_cpu_initfn(Object *obj) } } +static void sparc_get_nwindows(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + SPARCCPU *cpu = SPARC_CPU(obj); + int64_t value = cpu->env.def.nwindows; + + visit_type_int(v, name, &value, errp); +} + +static void sparc_set_nwindows(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + const int64_t min = MIN_NWINDOWS; + const int64_t max = MAX_NWINDOWS; + SPARCCPU *cpu = SPARC_CPU(obj); + Error *err = NULL; + int64_t value; + + visit_type_int(v, name, &value, &err); + if (err) { + error_propagate(errp, err); + return; + } + + if (value < min || value > max) { + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 + " (minimum: %" PRId64 ", maximum: %" PRId64 ")", + object_get_typename(obj), name ? name : "null", + value, min, max); + return; + } + cpu->env.def.nwindows = value; +} + +static PropertyInfo qdev_prop_nwindows = { + .name = "int", + .get = sparc_get_nwindows, + .set = sparc_set_nwindows, +}; + +static Property sparc_cpu_properties[] = { + DEFINE_PROP_BIT("float", SPARCCPU, env.def.features, 0, false), + DEFINE_PROP_BIT("float128", SPARCCPU, env.def.features, 1, false), + DEFINE_PROP_BIT("swap", SPARCCPU, env.def.features, 2, false), + DEFINE_PROP_BIT("mul", SPARCCPU, env.def.features, 3, false), + DEFINE_PROP_BIT("div", SPARCCPU, env.def.features, 4, false), + DEFINE_PROP_BIT("flush", SPARCCPU, env.def.features, 5, false), + DEFINE_PROP_BIT("fsqrt", SPARCCPU, env.def.features, 6, false), + DEFINE_PROP_BIT("fmul", SPARCCPU, env.def.features, 7, false), + DEFINE_PROP_BIT("vis1", SPARCCPU, env.def.features, 8, false), + DEFINE_PROP_BIT("vis2", SPARCCPU, env.def.features, 9, false), + DEFINE_PROP_BIT("fsmuld", SPARCCPU, env.def.features, 10, false), + DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), + DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), + DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), + DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, + qdev_prop_uint64, target_ulong), + DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), + DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), + { .name = "nwindows", .info = &qdev_prop_nwindows }, + DEFINE_PROP_END_OF_LIST() +}; + static void sparc_cpu_class_init(ObjectClass *oc, void *data) { SPARCCPUClass *scc = SPARC_CPU_CLASS(oc); @@ -861,6 +926,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) scc->parent_realize = dc->realize; dc->realize = sparc_cpu_realizefn; + dc->props = sparc_cpu_properties; scc->parent_reset = cc->reset; cc->reset = sparc_cpu_reset;
SPARC is the last target that uses legacy way of parsing and initializing cpu features, drop legacy approach and convert features to properties so that SPARC could as minimum benefit from generic cpu_generic_init(), common with x86 +-feat parser PS: the main purpose is to remove legacy way of cpu creation as a blocker for unifying cpu creation code across targets. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> CC: Artyom Tarasenko <atar4qemu@gmail.com> CC: Eduardo Habkost <ehabkost@redhat.com> --- target/sparc/cpu.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)