From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41367) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6VaJ-00033C-Ei for qemu-devel@nongnu.org; Tue, 08 Dec 2015 22:38:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6VaI-0006JE-4T for qemu-devel@nongnu.org; Tue, 08 Dec 2015 22:38:31 -0500 Received: from mail-pa0-x22e.google.com ([2607:f8b0:400e:c03::22e]:35132) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6VaH-0006J6-TI for qemu-devel@nongnu.org; Tue, 08 Dec 2015 22:38:30 -0500 Received: by pacej9 with SMTP id ej9so22514277pac.2 for ; Tue, 08 Dec 2015 19:38:29 -0800 (PST) References: <1449459280-14983-1-git-send-email-david@gibson.dropbear.id.au> <1449459280-14983-8-git-send-email-david@gibson.dropbear.id.au> <20151208023842.GA8706@tungsten.ozlabs.ibm.com> <5667A072.5010904@ozlabs.ru> From: Alexey Kardashevskiy Message-ID: <5667A22E.4060809@ozlabs.ru> Date: Wed, 9 Dec 2015 14:38:22 +1100 MIME-Version: 1.0 In-Reply-To: <5667A072.5010904@ozlabs.ru> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit 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: Sam Bobroff , David Gibson Cc: lvivier@redhat.com, thuth@redhat.com, ehabkost@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, agraf@suse.de, qemu-ppc@nongnu.org On 12/09/2015 02:30 PM, Alexey Kardashevskiy wrote: > On 12/08/2015 01:38 PM, Sam Bobroff wrote: >> 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); > > > I suggested this earlier but for unknown reason this was rejected :) ah, also: Reviewed-by: Alexey Kardashevskiy > > >> >> Reviewed-by: Sam Bobroff >> > > -- Alexey