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

[Marc-André Lureau]

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.


-- 
Marc-André Lureau