Re: [Qemu-ppc] [RFC 6/9] spapr: Restructure class_compat functions

[Thomas Huth]

On 30/11/15 09:51, David Gibson wrote:
> Currently each of the *_class_compat() functions for the pseries-2.1 ..
> pseries-2.5 machine types are standalone.  This will become harder to
> maintain as new versions are added.
> 
> This patch restructures them similarly to x86 where each function calls
> the one from the next version, then overrides anything necessary for
> compatibility with the specific version and older.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/spapr.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fae62ce..5a130ff 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2336,10 +2336,10 @@ static void 
> spapr_machine_2_5_class_compat(MachineClass *mc)
>  {
>      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>  
> +    smc->dr_lmb_enabled = true;
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
>      mc->alias = "pseries";
>      mc->is_default = 1;
> -    smc->dr_lmb_enabled = true;
>  }

That change above looks a little bit unnecessary?

> @@ -2352,6 +2352,12 @@ DEFINE_SPAPR_MACHINE(2_5, "2.5", NULL);
>  
>  static void spapr_machine_2_4_class_compat(MachineClass *mc)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
> +    spapr_machine_2_5_class_compat(mc);
> +    mc->alias = NULL;
> +    mc->is_default = 0;
> +    smc->dr_lmb_enabled = false;
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_4);

Wouldn't it be nicer to keep the "mc->" lines together, i.e. move the
"smc->" either before or after them?

> @@ -2378,6 +2384,7 @@ static void 
> spapr_machine_2_3_instance_compat(MachineState *machine)
>  
>  static void spapr_machine_2_3_class_compat(MachineClass *mc)
>  {
> +    spapr_machine_2_4_class_compat(mc);
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_3);
>  }
> @@ -2403,6 +2410,7 @@ static void 
> spapr_machine_2_2_instance_compat(MachineState *machine)
>  
>  static void spapr_machine_2_2_class_compat(MachineClass *mc)
>  {
> +    spapr_machine_2_3_class_compat(mc);
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.2";
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_2);
>  }
> @@ -2422,6 +2430,7 @@ static void 
> spapr_machine_2_1_instance_compat(MachineState *machine)
>  
>  static void spapr_machine_2_1_class_compat(MachineClass *mc)
>  {
> +    spapr_machine_2_2_class_compat(mc);
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.1";
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_1);
>  }

Anyway, patch looks also fine to me like it currently is already, so:

Reviewed-by: Thomas Huth <address@hidden>