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

[David Gibson]

On Tue, Dec 01, 2015 at 03:12:25PM +1100, Alexey Kardashevskiy wrote:
> 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.

Hmm.  I'll think about it.

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

Sorry, I can't parse that statement.

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

Well, they're the parts of the class and instance init functions which
deal with version compatibility - the macro adds some extra
boilerplate to the actual class_init and instance_init functions.

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