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

[Eduardo Habkost]

On Tue, Dec 11, 2018 at 04:07:19PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 10, 2018 at 9:31 PM Eduardo Habkost <address@hidden> wrote:
> >
> > On Tue, Dec 04, 2018 at 06:20:12PM +0400, Marc-André Lureau wrote:
> > > Similarly to accel properties, move compat properties out of globals
> > > registration, and apply the machine compat properties during
> > > device_post_init().
> > >
> > > As suggested during review, populating the arrays can be done directly
> > > without resorting to using macros.
> > >
> > > Signed-off-by: Marc-André Lureau <address@hidden>
> > > ---
> > >  include/hw/boards.h        |   3 +-
> > >  hw/arm/virt.c              |  48 ++-
> > >  hw/core/machine.c          |  19 +-
> > >  hw/core/qdev.c             |   2 +
> > >  hw/i386/pc_piix.c          | 588 +++++++++++++++++++++----------------
> > >  hw/i386/pc_q35.c           |  70 ++++-
> > >  hw/ppc/spapr.c             | 209 +++++++------
> > >  hw/s390x/s390-virtio-ccw.c | 220 +++++++-------
> > >  vl.c                       |   1 -
> > >  9 files changed, 673 insertions(+), 487 deletions(-)
> > >
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index f82f28468b..28c2b0af41 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -69,7 +69,6 @@ int machine_kvm_shadow_mem(MachineState *machine);
> > >  int machine_phandle_start(MachineState *machine);
> > >  bool machine_dump_guest_core(MachineState *machine);
> > >  bool machine_mem_merge(MachineState *machine);
> > > -void machine_register_compat_props(MachineState *machine);
> > >  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState 
> > > *machine);
> > >  void machine_set_cpu_numa_node(MachineState *machine,
> > >                                 const CpuInstanceProperties *props,
> > > @@ -191,7 +190,7 @@ struct MachineClass {
> > >      const char *default_machine_opts;
> > >      const char *default_boot_order;
> > >      const char *default_display;
> > > -    GArray *compat_props;
> > > +    GPtrArray *compat_props;
> > >      const char *hw_version;
> > >      ram_addr_t default_ram_size;
> > >      const char *default_cpu_type;
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index f69e7eb399..530c8ca89d 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -1872,8 +1872,9 @@ static void virt_machine_3_1_options(MachineClass 
> > > *mc)
> > >  }
> > >  DEFINE_VIRT_MACHINE_AS_LATEST(3, 1)
> > >
> > > -#define VIRT_COMPAT_3_0 \
> > > +static GlobalProperty virt_compat_3_0[] = {
> > >      HW_COMPAT_3_0
> > > +};
> > [...]
> >
> > All the changes to compat macros are independent from the change
> > you describe in the patch subject, and makes review more
> > difficult.  What about doing that in a separate patch?
> >
> > We can replace all the virt.c, pc_piix.c, pc_q35.c, and spapr.c
> > hunks in this patch with the following:
> >
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index f82f28468b..622bbaf939 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -290,18 +289,10 @@ struct MachineState {
> >
> >  #define SET_MACHINE_COMPAT(m, COMPAT) \
> >      do {                              \
> > -        int i;                        \
> >          static GlobalProperty props[] = {       \
> >              COMPAT                              \
> > -            { /* end of list */ }               \
> >          };                                      \
> > -        if (!m->compat_props) { \
> > -            m->compat_props = g_array_new(false, false, sizeof(void *)); \
> > -        } \
> > -        for (i = 0; props[i].driver != NULL; i++) {    \
> > -            GlobalProperty *prop = &props[i];          \
> > -            g_array_append_val(m->compat_props, prop); \
> > -        }                                              \
> > +        compat_props_add(m->compat_props, props, G_N_ELEMENTS(props)); \
> >      } while (0)
> >
> >  #endif
> >
> 
> Didn't you ask in a previous iteration to remove the macros?
> "I like us to be able to
> register compat properties without macro magic.  The existence of
> SET_MACHINE_COMPAT is a bug and not a feature.

I did ask you to not introduce a new macro in accel.c, and
removing the existing macros is really welcome.  I just don't
think we should do everything in a single patch.

-- 
Eduardo