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

[David Gibson]

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.

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

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature