From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34605) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a68Cd-0001Zx-6E for qemu-devel@nongnu.org; Mon, 07 Dec 2015 21:40:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a68CZ-0004Yt-Sf for qemu-devel@nongnu.org; Mon, 07 Dec 2015 21:40:31 -0500 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:41306) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a68CZ-0004Y5-22 for qemu-devel@nongnu.org; Mon, 07 Dec 2015 21:40:27 -0500 Received: from localhost by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 8 Dec 2015 12:40:23 +1000 Date: Tue, 8 Dec 2015 13:38:42 +1100 From: Sam Bobroff Message-ID: <20151208023842.GA8706@tungsten.ozlabs.ibm.com> References: <1449459280-14983-1-git-send-email-david@gibson.dropbear.id.au> <1449459280-14983-8-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449459280-14983-8-git-send-email-david@gibson.dropbear.id.au> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 07/10] pseries: DEFINE_SPAPR_MACHINE List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: lvivier@redhat.com, thuth@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, agraf@suse.de, qemu-ppc@nongnu.org On Mon, Dec 07, 2015 at 02:34:37PM +1100, 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_options' and > 'instance_options' 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 > --- > hw/ppc/spapr.c | 119 ++++++++++++++++++++++++--------------------------------- > 1 file changed, 49 insertions(+), 70 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 3078e60..4f645f3 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2302,24 +2302,47 @@ static const TypeInfo spapr_machine_info = { > }, > }; > > +#define DEFINE_SPAPR_MACHINE(suffix, verstr) \ > + static void spapr_machine_##suffix##_class_init(ObjectClass *oc, \ > + void *data) \ > + { \ > + MachineClass *mc = MACHINE_CLASS(oc); \ > + spapr_machine_##suffix##_class_options(mc); \ > + } \ > + static void spapr_machine_##suffix##_instance_init(Object *obj) \ > + { \ > + MachineState *machine = MACHINE(obj); \ > + spapr_machine_##suffix##_instance_options(machine); \ > + } \ > + 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_instance_options(MachineState *machine) > { > - MachineClass *mc = MACHINE_CLASS(oc); > - sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); > +} > + > +static void spapr_machine_2_5_class_options(MachineClass *mc) > +{ > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc); > > mc->alias = "pseries"; > mc->is_default = 1; > 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"); > > /* > * pseries-2.4 > @@ -2327,18 +2350,17 @@ 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_instance_options(MachineState *machine) > { > - MachineClass *mc = MACHINE_CLASS(oc); > + spapr_machine_2_5_instance_options(machine); > +} > > +static void spapr_machine_2_4_class_options(MachineClass *mc) > +{ > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_4); > } > > -static const TypeInfo spapr_machine_2_4_info = { > - .name = MACHINE_TYPE_NAME("pseries-2.4"), > - .parent = TYPE_SPAPR_MACHINE, > - .class_init = spapr_machine_2_4_class_init, > -}; > +DEFINE_SPAPR_MACHINE(2_4, "2.4"); > > /* > * pseries-2.3 > @@ -2352,30 +2374,18 @@ static const TypeInfo spapr_machine_2_4_info = { > .value = "off",\ > }, > > -static void spapr_compat_2_3(Object *obj) > +static void spapr_machine_2_3_instance_options(MachineState *machine) > { > + spapr_machine_2_4_instance_options(machine); > savevm_skip_section_footers(); > global_state_set_optional(); > } > > -static void spapr_machine_2_3_instance_init(Object *obj) > -{ > - spapr_compat_2_3(obj); > -} > - > -static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data) > +static void spapr_machine_2_3_class_options(MachineClass *mc) > { > - MachineClass *mc = MACHINE_CLASS(oc); > - > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_3); > } > - > -static const TypeInfo spapr_machine_2_3_info = { > - .name = MACHINE_TYPE_NAME("pseries-2.3"), > - .parent = TYPE_SPAPR_MACHINE, > - .class_init = spapr_machine_2_3_class_init, > - .instance_init = spapr_machine_2_3_instance_init, > -}; > +DEFINE_SPAPR_MACHINE(2_3, "2.3"); > > /* > * pseries-2.2 > @@ -2390,29 +2400,16 @@ static const TypeInfo spapr_machine_2_3_info = { > .value = "0x20000000",\ > }, > > -static void spapr_compat_2_2(Object *obj) > -{ > - spapr_compat_2_3(obj); > -} > - > -static void spapr_machine_2_2_instance_init(Object *obj) > +static void spapr_machine_2_2_instance_options(MachineState *machine) > { > - spapr_compat_2_2(obj); > + spapr_machine_2_3_instance_options(machine); > } > > -static void spapr_machine_2_2_class_init(ObjectClass *oc, void *data) > +static void spapr_machine_2_2_class_options(MachineClass *mc) > { > - MachineClass *mc = MACHINE_CLASS(oc); > - > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_2); > } > - > -static const TypeInfo spapr_machine_2_2_info = { > - .name = MACHINE_TYPE_NAME("pseries-2.2"), > - .parent = TYPE_SPAPR_MACHINE, > - .class_init = spapr_machine_2_2_class_init, > - .instance_init = spapr_machine_2_2_instance_init, > -}; > +DEFINE_SPAPR_MACHINE(2_2, "2.2"); > > /* > * pseries-2.1 > @@ -2421,38 +2418,20 @@ static const TypeInfo spapr_machine_2_2_info = { > SPAPR_COMPAT_2_2 \ > HW_COMPAT_2_1 > > -static void spapr_compat_2_1(Object *obj) > -{ > - spapr_compat_2_2(obj); > -} > - > -static void spapr_machine_2_1_instance_init(Object *obj) > +static void spapr_machine_2_1_instance_options(MachineState *machine) > { > - spapr_compat_2_1(obj); > + spapr_machine_2_2_instance_options(machine); > } > > -static void spapr_machine_2_1_class_init(ObjectClass *oc, void *data) > +static void spapr_machine_2_1_class_options(MachineClass *mc) > { > - MachineClass *mc = MACHINE_CLASS(oc); > - > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_1); > } > - > -static const TypeInfo spapr_machine_2_1_info = { > - .name = MACHINE_TYPE_NAME("pseries-2.1"), > - .parent = TYPE_SPAPR_MACHINE, > - .class_init = spapr_machine_2_1_class_init, > - .instance_init = spapr_machine_2_1_instance_init, > -}; > +DEFINE_SPAPR_MACHINE(2_1, "2.1"); > > static void spapr_machine_register_types(void) > { > type_register_static(&spapr_machine_info); > - type_register_static(&spapr_machine_2_1_info); > - type_register_static(&spapr_machine_2_2_info); > - type_register_static(&spapr_machine_2_3_info); > - type_register_static(&spapr_machine_2_4_info); > - type_register_static(&spapr_machine_2_5_info); > } > > type_init(spapr_machine_register_types) > -- > 2.5.0 Looks good to me. Just one nit: I couldn't help noticing that the suffix and verstr parameters to the macro were both encoding the same information. Did you consider a macro with the major and minor version as the parameters? e.g. #define DEFINE_SPAPR_MACHINE(major, minor) So usage would be: DEFINE_SPAPR_MACHINE(2, 1); Reviewed-by: Sam Bobroff