Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 07/10] pseries: DEFINE_SPAPR_MACHINE

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <david@gibson.dropbear.id.au>
>>> ---
>>>   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 <aik@ozlabs.ru>	


>
>
>>
>> Reviewed-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
>>
>
>


-- 
Alexey