On Wed, 23 Aug 2017 10:07:14 -0300
Eduardo Habkost <ehabk...@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 <ehabk...@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 <imamm...@redhat.com>
> > > [...]
> > > > ---
> > > > CC: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> > > > CC: Artyom Tarasenko <atar4q...@gmail.com>
> > > > CC: Eduardo Habkost <ehabk...@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 <ehabk...@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
> > > >
> > > >
> > >
> >
>
