Re: [Qemu-devel] [PATCH for-2.11 03/27] sparc: convert cpu features to qdev properties

[Eduardo Habkost <ehabkost@redhat.com>]

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
> > > 
> > >   
> > 
> 

-- 
Eduardo