Re: [Qemu-devel] [PATCHv2 02/10] pseries: Rearrange versioned machine type code

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9795 bytes --]

On Mon, Dec 07, 2015 at 11:21:26AM +0100, Thomas Huth wrote:
> On 07/12/15 04:34, David Gibson wrote:
> > hw/ppc/spapr.c has a number of definitions related to the various versioned
> > machine types ("pseries-2.1" .. "pseries-2.5") it defines.  These are
> > mostly arranged by type of function first, then machine version second, and
> > it's not consistent about whether it goes in increasing or decreasing
> > version order.
> > 
> > This rearranges the code to keep all the definitions for a particular
> > machine version together, and arrange then consistently in order most
> > recent to least recent.
> > 
> > This brings us closer to matching the way PC does things, and makes later
> > cleanups easier to follow.
> > 
> > Apart from adding some comments marking each section, this is a pure
> > mechanical rearrangement with no semantic changes.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 180 +++++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 98 insertions(+), 82 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index a69856f..c126e10 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2301,9 +2301,53 @@ static const TypeInfo spapr_machine_info = {
> >      },
> >  };
> >  
> > +/*
> > + * pseries-2.5
> > + */
> > +static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data)
> > +{
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> > +
> > +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
> > +    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,
> > +};
> > +
> > +/*
> > + * pseries-2.4
> > + */
> >  #define SPAPR_COMPAT_2_4 \
> >          HW_COMPAT_2_4
> >  
> > +static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
> > +{
> > +    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;
> > +}
> > +
> > +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,
> > +};
> > +
> > +/*
> > + * pseries-2.3
> > + */
> >  #define SPAPR_COMPAT_2_3 \
> >          SPAPR_COMPAT_2_4 \
> >          HW_COMPAT_2_3 \
> > @@ -2313,72 +2357,61 @@ static const TypeInfo spapr_machine_info = {
> >              .value    = "off",\
> >          },
> >  
> > -#define SPAPR_COMPAT_2_2 \
> > -        SPAPR_COMPAT_2_3 \
> > -        HW_COMPAT_2_2 \
> > -        {\
> > -            .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> > -            .property = "mem_win_size",\
> > -            .value    = "0x20000000",\
> > -        },
> > -
> > -#define SPAPR_COMPAT_2_1 \
> > -        SPAPR_COMPAT_2_2 \
> > -        HW_COMPAT_2_1
> > -
> >  static void spapr_compat_2_3(Object *obj)
> >  {
> >      savevm_skip_section_footers();
> >      global_state_set_optional();
> >  }
> >  
> > -static void spapr_compat_2_2(Object *obj)
> > -{
> > -    spapr_compat_2_3(obj);
> > -}
> > -
> > -static void spapr_compat_2_1(Object *obj)
> > -{
> > -    spapr_compat_2_2(obj);
> > -}
> > -
> >  static void spapr_machine_2_3_instance_init(Object *obj)
> >  {
> >      spapr_compat_2_3(obj);
> >      spapr_machine_initfn(obj);
> >  }
> >  
> > -static void spapr_machine_2_2_instance_init(Object *obj)
> > -{
> > -    spapr_compat_2_2(obj);
> > -    spapr_machine_initfn(obj);
> > -}
> > -
> > -static void spapr_machine_2_1_instance_init(Object *obj)
> > -{
> > -    spapr_compat_2_1(obj);
> > -    spapr_machine_initfn(obj);
> > -}
> > -
> > -static void spapr_machine_2_1_class_init(ObjectClass *oc, void *data)
> > +static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
> >  {
> > -    MachineClass *mc = MACHINE_CLASS(oc);
> >      static GlobalProperty compat_props[] = {
> > -        SPAPR_COMPAT_2_1
> > +        SPAPR_COMPAT_2_3
> >          { /* end of list */ }
> >      };
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> >  
> > -    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.1";
> > +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
> >      mc->compat_props = compat_props;
> >  }
> >  
> > -static const TypeInfo spapr_machine_2_1_info = {
> > -    .name          = MACHINE_TYPE_NAME("pseries-2.1"),
> > +static const TypeInfo spapr_machine_2_3_info = {
> > +    .name          = MACHINE_TYPE_NAME("pseries-2.3"),
> >      .parent        = TYPE_SPAPR_MACHINE,
> > -    .class_init    = spapr_machine_2_1_class_init,
> > -    .instance_init = spapr_machine_2_1_instance_init,
> > +    .class_init    = spapr_machine_2_3_class_init,
> > +    .instance_init = spapr_machine_2_3_instance_init,
> >  };
> >  
> > +/*
> > + * pseries-2.2
> > + */
> > +
> > +#define SPAPR_COMPAT_2_2 \
> > +        SPAPR_COMPAT_2_3 \
> > +        HW_COMPAT_2_2 \
> > +        {\
> > +            .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> > +            .property = "mem_win_size",\
> > +            .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)
> > +{
> > +    spapr_compat_2_2(obj);
> > +    spapr_machine_initfn(obj);
> > +}
> > +
> >  static void spapr_machine_2_2_class_init(ObjectClass *oc, void *data)
> >  {
> >      static GlobalProperty compat_props[] = {
> > @@ -2398,58 +2431,41 @@ static const TypeInfo spapr_machine_2_2_info = {
> >      .instance_init = spapr_machine_2_2_instance_init,
> >  };
> >  
> > -static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
> > -{
> > -    static GlobalProperty compat_props[] = {
> > -        SPAPR_COMPAT_2_3
> > -        { /* end of list */ }
> > -    };
> > -    MachineClass *mc = MACHINE_CLASS(oc);
> > +/*
> > + * pseries-2.1
> > + */
> > +#define SPAPR_COMPAT_2_1 \
> > +        SPAPR_COMPAT_2_2 \
> > +        HW_COMPAT_2_1
> >  
> > -    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
> > -    mc->compat_props = compat_props;
> > +static void spapr_compat_2_1(Object *obj)
> > +{
> > +    spapr_compat_2_2(obj);
> >  }
> >  
> > -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,
> > -};
> > +static void spapr_machine_2_1_instance_init(Object *obj)
> > +{
> > +    spapr_compat_2_1(obj);
> > +    spapr_machine_initfn(obj);
> > +}
> >  
> > -static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
> > +static void spapr_machine_2_1_class_init(ObjectClass *oc, void *data)
> >  {
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> >      static GlobalProperty compat_props[] = {
> > -        SPAPR_COMPAT_2_4
> > +        SPAPR_COMPAT_2_1
> >          { /* end of list */ }
> >      };
> > -    MachineClass *mc = MACHINE_CLASS(oc);
> >  
> > -    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
> > +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.1";
> >      mc->compat_props = compat_props;
> >  }
> >  
> > -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,
> > -};
> > -
> > -static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data)
> > -{
> > -    MachineClass *mc = MACHINE_CLASS(oc);
> > -    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> > -
> > -    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
> > -    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"),
> > +static const TypeInfo spapr_machine_2_1_info = {
> > +    .name          = MACHINE_TYPE_NAME("pseries-2.1"),
> >      .parent        = TYPE_SPAPR_MACHINE,
> > -    .class_init    = spapr_machine_2_5_class_init,
> > +    .class_init    = spapr_machine_2_1_class_init,
> > +    .instance_init = spapr_machine_2_1_instance_init,
> >  };
> >  
> >  static void spapr_machine_register_types(void)
> 
> The patch might have been a little bit easier to read if you'd moved the
> next two cleanup patches before this one in the series, but as far as I
> can see, it's all clean code movement, without further modification, and
> IMHO it also makes sense, so:

Yeah, maybe. But because this is a big code move, doing so would have
meant basically rewriting both patches from scratch, which was more
hassle than I wanted.

> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]