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
