Re: [Qemu-ppc] [RFC 3/9] spapr: DEFINE_SPAPR_MACHINE

[Alexey Kardashevskiy]

On 12/01/2015 02:10 PM, David Gibson wrote:
> On Tue, Dec 01, 2015 at 01:38:20PM +1100, Alexey Kardashevskiy wrote:
> > On 11/30/2015 07:51 PM, David Gibson wrote:
> > > At the moment all the class_init functions and TypeInfo structures for the
> > > various versioned pseries machine types are open-coded.  As more versions
> > > are created this is getting increasingly clumsy.
> > >
> > > This patch borrows the approach used in PC, using a DEFINE_SPAPR_MACHINE()
> > > macro to construct most of the boilerplate from simpler 'class_compat' and
> > > 'instance_compat' functions.
> > >
> > > This patch makes a small semantic change - the versioned machine types are
> > > now registered through machine_init() instead of type_init().  Since the
> > > new way is how PC already did it, I'm assuming that's correct.
> > >
> > > Signed-off-by: David Gibson <address@hidden>
> > > ---
> > >   hw/ppc/spapr.c | 114 ++++++++++++++++++++++-----------------------------------
> > >   1 file changed, 44 insertions(+), 70 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index c126e10..ca62343 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2301,13 +2301,40 @@ static const TypeInfo spapr_machine_info = {
> > >       },
> > >   };
> > >
> > > +#define DEFINE_SPAPR_MACHINE(suffix, verstr, instance_compat)        \
> >
> >
> > instance_compat is passed directly (I like it) but
> > spapr_machine_##suffix##_class_compat is not (I do not like it - cannot grep
> > for it), would be nice to have these similar.
>
> Hrm.  I was trying to avoid having lots of macro arguments that all
> have the same form.


PC passes 2 functions though. Both or neither will do.


> > Also, this could be:
> > #define DEFINE_SPAPR_MACHINE(major, minor, instance_compat)
> >
> > and then use
> > spapr_machine__##major##_##minor##_class_init()
> > "pseries-"#major"."#minor
> >
> > ?
>
> I suppose, but I'd prefer to keep it similar to PC.

If it was DEFINE_PPC_MACHINE(), then I would not say a word against but in 
this case it would be just self-documenting that pseries machines are per 
QEMU versions.



> > > +    static void spapr_machine_##suffix##_class_init(ObjectClass *oc, \
> > > +                                                    void *data)      \
> > > +    {                                                                \
> > > +        MachineClass *mc = MACHINE_CLASS(oc);                        \
> > > +        spapr_machine_##suffix##_class_compat(mc);                   \
> > > +    }                                                                \
> > > +    static void spapr_machine_##suffix##_instance_init(Object *obj)  \
> > > +    {                                                                \
> > > +        void (*compat)(MachineState *m) = (instance_compat);         \
> > > +        MachineState *machine = MACHINE(obj);                        \
> > > +        if (compat) {                                                \
> > > +            compat(machine);                                         \
> > > +        }                                                            \
> > > +        spapr_machine_initfn(obj);                                   \
> >
> >
> > I'd swap compat() and spapr_machine_initfn() and let a new machine overwrite
> > some of default stuff.
>
> I was thinking about that, but I don't want to do it in this patch - I
> want to separate rearrangements from semantic changes to make review
> easier.
>
> > > +    }                                                                \
> > > +    static const TypeInfo spapr_machine_##suffix##_info = {          \
> > > +        .name = MACHINE_TYPE_NAME("pseries-" verstr),                \
> > > +        .parent = TYPE_SPAPR_MACHINE,                                \
> > > +        .class_init = spapr_machine_##suffix##_class_init,           \
> > > +        .instance_init = spapr_machine_##suffix##_instance_init,     \
> > > +    };                                                               \
> > > +    static void spapr_machine_register_##suffix(void)                \
> > > +    {                                                                \
> > > +        type_register(&spapr_machine_##suffix##_info);               \
> > > +    }                                                                \
> > > +    machine_init(spapr_machine_register_##suffix)
> > > +
> > >   /*
> > >    * pseries-2.5
> > >    */
> > > -static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data)
> > > +static void spapr_machine_2_5_class_compat(MachineClass *mc)
> > >   {
> > > -    MachineClass *mc = MACHINE_CLASS(oc);
> > > -    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > >
> > >       mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
> > >       mc->alias = "pseries";
> > > @@ -2315,11 +2342,7 @@ static void spapr_machine_2_5_class_init(ObjectClass 
> > > *oc, void *data)
> > >       smc->dr_lmb_enabled = true;
> > >   }
> > >
> > > -static const TypeInfo spapr_machine_2_5_info = {
> > > -    .name          = MACHINE_TYPE_NAME("pseries-2.5"),
> > > -    .parent        = TYPE_SPAPR_MACHINE,
> > > -    .class_init    = spapr_machine_2_5_class_init,
> > > -};
> > > +DEFINE_SPAPR_MACHINE(2_5, "2.5", NULL);
> > >
> > >   /*
> > >    * pseries-2.4
> > > @@ -2327,23 +2350,18 @@ static const TypeInfo spapr_machine_2_5_info = {
> > >   #define SPAPR_COMPAT_2_4 \
> > >           HW_COMPAT_2_4
> > >
> > > -static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
> > > +static void spapr_machine_2_4_class_compat(MachineClass *mc)
> > >   {
> > >       static GlobalProperty compat_props[] = {
> > >           SPAPR_COMPAT_2_4
> > >           { /* end of list */ }
> > >       };
> > > -    MachineClass *mc = MACHINE_CLASS(oc);
> > >
> > >       mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
> > >       mc->compat_props = compat_props;
> >
> >
> > xxx_init() is still a better name than xxx_compat().
> > Same about xxx_instance_compat().
>
> It's really not.  I deliberately avoided calling things 'init' or
> 'initfn', because that's used in so many places and it's not clear
> whether I'm talking about class_init, instance_init, mc->init or
> something else.


These were "class_init" and "instance_init", not "something_init" so it was 
clear what is what. Now with "compat" I read it as they only do 
compatibility stuff (compatibilize? is there such a word?) and the rest is 
hidden somewhere else but the whole idea of machine versions is to support 
versions compatibility so using "compat" in the names is redundant. Or I am 
missing the point here...


--
Alexey