Hi
On Tue, Dec 11, 2018 at 6:24 PM Eduardo Habkost <ehabk...@redhat.com> wrote:
>
> 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()?
Sure
>
> >
> > 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().
But easier to get wrong too. I prefer the explicit N arguments. (it
also gives some flexibility, since you can point to inner pointer +
size, although we don't care at this point)
>
> 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.
I am open to ideas
>
>
> [...]
> > 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.
That could be done on top, I imagine. I can give it a try.
>
> 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.
>
>
thanks
> > }
> > DEFINE_CCW_MACHINE(3_0, "3.0", false);
> [...]
>
> --
> Eduardo
>
--
Marc-André Lureau
