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

[David Gibson]

On Wed, Dec 02, 2015 at 01:03:58PM +1100, Alexey Kardashevskiy wrote:
> On 12/01/2015 07:59 PM, David Gibson wrote:
> >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.
> 
> 
> PC machines have different names, like isapc, xenfv, pc-i440fx-X.Y, pc-X.Y -
> for this zoo it probably makes it easier to have separate prefix and verstr
> (as making a c-language identifier from "pc-i440fx-1.4" is ugly).
> 
> All pseries machines have "pseries-X.Y" names, always. If we did the
> DEFINE_SPAPR_MACHINE macro my way (with major and minor), just this would
> tell a random person reading the code about this "pseries-X.Y" convention
> without any additional comment.

Hmm.

> >>>>>+    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.
> 
> 
> spapr_machine_##suffix##_class_compat exactly is
> spapr_machine_##suffix##_class_init, does not add a thing (later it only
> adds "dflt", which btw is better to be "default").

It can't be 'default' because that's a reserved word in C.

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