Re: [qemu-s390x] [Qemu-devel] [PATCH for-3.2 v5 08/19] hw: apply machine compat properties without touching globals

[Eduardo Habkost]

I have specific questions about the approach we are using to
eliminate the macros.

My main goal when asking this to be moved to a separate patch is
to not make this discussion block the register_global_properties() &
device_post_init() changes (which look good to me).


On Tue, Dec 04, 2018 at 06:20:12PM +0400, Marc-André Lureau wrote:
[...]
> -#define VIRT_COMPAT_3_0 \
> +static GlobalProperty virt_compat_3_0[] = {
>      HW_COMPAT_3_0
> +};

What about moving the array inside virt_machine_3_0_options()?

>  
>  static void virt_3_0_instance_init(Object *obj)
>  {
> @@ -1883,12 +1884,14 @@ static void virt_3_0_instance_init(Object *obj)
>  static void virt_machine_3_0_options(MachineClass *mc)
>  {
>      virt_machine_3_1_options(mc);
> -    SET_MACHINE_COMPAT(mc, VIRT_COMPAT_3_0);
> +    compat_props_add(mc->compat_props,
> +                     virt_compat_3_0, G_N_ELEMENTS(virt_compat_3_0));
>  }
>  DEFINE_VIRT_MACHINE(3, 0)

This is nice, because it's basically the same amount of
boilerplate code, but I would find a NULL-terminated array much
easier to use than having to use G_N_ELEMENTS().

This would be nice in cases like virt, because we wouldn't even
need to declare a separate array.  We could do something like:

    compat_props_add(mc->compat_props, hw_compat_3_0);

and that's all.  No need for HW_COMPAT_* macros, no need for
extra arrays to be declared.


[...]
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7092d6d13f..575566e466 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -438,74 +438,112 @@ static void pc_i440fx_3_1_machine_options(MachineClass 
> *m)
>  DEFINE_I440FX_MACHINE(v3_1, "pc-i440fx-3.1", NULL,
>                        pc_i440fx_3_1_machine_options);
>  
> +static GlobalProperty pc_compat_3_0[] = {
> +    PC_COMPAT_3_0
> +};
> +
>  static void pc_i440fx_3_0_machine_options(MachineClass *m)
>  {
>      pc_i440fx_3_1_machine_options(m);
>      m->is_default = 0;
>      m->alias = NULL;
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
> +
> +    compat_props_add(m->compat_props,
> +                     pc_compat_3_0, G_N_ELEMENTS(pc_compat_3_0));
>  }
>  
>  DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
>                        pc_i440fx_3_0_machine_options);

Now, this is requiring _more_ boilerplate code than before.  I'd
like us to find a way to eliminate the macro without increasing
the amount of boilerplate code.


[...]
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7afd1a175b..d801ba71eb 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3973,8 +3973,9 @@ DEFINE_SPAPR_MACHINE(3_1, "3.1", true);
>  /*
>   * pseries-3.0
>   */
> -#define SPAPR_COMPAT_3_0                                              \
> +static GlobalProperty spapr_compat_3_0[] = {
>      HW_COMPAT_3_0
> +};
>  
>  static void spapr_machine_3_0_instance_options(MachineState *machine)
>  {
> @@ -3986,7 +3987,8 @@ static void 
> spapr_machine_3_0_class_options(MachineClass *mc)
>      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>  
>      spapr_machine_3_1_class_options(mc);
> -    SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_3_0);
> +    compat_props_add(mc->compat_props,
> +                     spapr_compat_3_0, G_N_ELEMENTS(spapr_compat_3_0));
>  
>      smc->legacy_irq_allocation = true;
>      smc->irq = &spapr_irq_xics_legacy;
> @@ -3997,18 +3999,19 @@ DEFINE_SPAPR_MACHINE(3_0, "3.0", false);
>  /*
>   * pseries-2.12
>   */
> -#define SPAPR_COMPAT_2_12                                              \
> -    HW_COMPAT_2_12                                                     \
> -    {                                                                  \
> -        .driver = TYPE_POWERPC_CPU,                                    \
> -        .property = "pre-3.0-migration",                               \
> -        .value    = "on",                                              \
> -    },                                                                 \
> -    {                                                                  \
> -        .driver = TYPE_SPAPR_CPU_CORE,                                 \
> -        .property = "pre-3.0-migration",                               \
> -        .value    = "on",                                              \
> +static GlobalProperty spapr_compat_2_12[] = {
> +    HW_COMPAT_2_12
> +    {
> +        .driver = TYPE_POWERPC_CPU,
> +        .property = "pre-3.0-migration",
> +        .value    = "on",
> +    },
> +    {
> +        .driver = TYPE_SPAPR_CPU_CORE,
> +        .property = "pre-3.0-migration",
> +        .value    = "on",
>      },
> +};
>  
>  static void spapr_machine_2_12_instance_options(MachineState *machine)
>  {
> @@ -4020,7 +4023,8 @@ static void 
> spapr_machine_2_12_class_options(MachineClass *mc)
>      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>  
>      spapr_machine_3_0_class_options(mc);
> -    SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
> +    compat_props_add(mc->compat_props,
> +                     spapr_compat_2_12, G_N_ELEMENTS(spapr_compat_2_12));

It would be nice to be able to write this as:

    static GlobalProperty spapr_compat_2_12[] = {
        {
            .driver = TYPE_POWERPC_CPU,
            .property = "pre-3.0-migration",
            .value    = "on",
        },
        {
            .driver = TYPE_SPAPR_CPU_CORE,
            .property = "pre-3.0-migration",
            .value    = "on",
        },
        { /* end of list */ },
    };

    compat_props_add(mc->compat_props, hw_compat_3_0);
    compat_props_add(mc->compat_props, spapr_compat_2_12);

This way we won't need the HW_COMPAT_* macros anymore.

Other than that, it's also basically the same amount of
boilerplate as before, so that's good.


>  
>      /* We depend on kvm_enabled() to choose a default value for the
>       * hpt-max-page-size capability. Of course we can't do it here
[...]
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index a0615a8b35..275cbe5da4 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -651,96 +651,106 @@ bool css_migration_enabled(void)
>      }                                                                        
>  \
>      type_init(ccw_machine_register_##suffix)
>  
> -#define CCW_COMPAT_3_0 \
> -        HW_COMPAT_3_0
> -
> -#define CCW_COMPAT_2_12 \
> -        HW_COMPAT_2_12
> -
> -#define CCW_COMPAT_2_11 \
> -        HW_COMPAT_2_11 \
> -        {\
> -            .driver   = TYPE_SCLP_EVENT_FACILITY,\
> -            .property = "allow_all_mask_sizes",\
> -            .value    = "off",\
> -        },
[...]
> +static GlobalProperty ccw_compat_3_0[] = {
> +    HW_COMPAT_3_0
> +};
> +
> +static GlobalProperty ccw_compat_2_12[] = {
> +    HW_COMPAT_2_12
> +};
> +
> +static GlobalProperty ccw_compat_2_11[] = {
> +    HW_COMPAT_2_11
> +    {
> +        .driver   = TYPE_SCLP_EVENT_FACILITY,
> +        .property = "allow_all_mask_sizes",
> +        .value    = "off",
> +    },
> +};
[...]
>  
>  static void ccw_machine_3_1_instance_options(MachineState *machine)
>  {
> @@ -762,7 +772,8 @@ static void ccw_machine_3_0_class_options(MachineClass 
> *mc)
>  
>      s390mc->hpage_1m_allowed = false;
>      ccw_machine_3_1_class_options(mc);
> -    SET_MACHINE_COMPAT(mc, CCW_COMPAT_3_0);
> +    compat_props_add(mc->compat_props,
> +                     ccw_compat_3_0, G_N_ELEMENTS(ccw_compat_3_0));

Same comments from spapr apply here: getting rid of the
HW_COMPAT_* macros would be nice, but this version is good enough
because it has the same amount of boilerplate code.


>  }
>  DEFINE_CCW_MACHINE(3_0, "3.0", false);
[...]

-- 
Eduardo