Re: [Qemu-block] [RFC PATCH 1/6] qdev: Fix latent bug with compat_props and onboard devices

[Marc-André Lureau]

Hi

On Mon, Feb 25, 2019 at 7:38 PM Markus Armbruster <address@hidden> wrote:
>
> Compatibility properties started life as a qdev property thing: we
> supported them only for qdev properties, and implemented them with the
> machinery backing command line option -global.
>
> Recent commit fa0cb34d221 put them to use (tacitly) with memory
> backend objects (subtypes of TYPE_MEMORY_BACKEND).  To make that
> possible, we first moved the work of applying them from the -global
> machinery into TYPE_DEVICE's .instance_post_init() method
> device_post_init(), in commits ea9ce8934c5 and b66bbee39f6, then made
> it available to TYPE_MEMORY_BACKEND's .instance_post_init() method
> host_memory_backend_post_init() as object_apply_compat_props(), in
> commit 1c3994f6d2a.
>
> Note the code smell: we now have function name starting with object_
> in hw/core/qdev.c.  It has to be there rather than in qom/, because it
> calls qdev_get_machine() to find the current accelerator's and
> machine's compat_props.
>
> Turns out calling qdev_get_machine() there is problematic.  If we
> qdev_create() from a machine's .instance_init() method, we call
> device_post_init() and thus qdev_get_machine() before main() can
> create "/machine" in QOM.  qdev_get_machine() tries to get it with
> container_get(), which "helpfully" creates it as "container" object,
> and returns that.  object_apply_compat_props() tries to paper over the
> problem by doing nothing when the value of qdev_get_machine() isn't a
> TYPE_MACHINE.  But the damage is done already: when main() later
> attempts to create the real "/machine", it fails with "attempt to add
> duplicate property 'machine' to object (type 'container')", and
> aborts.
>
> Since no machine .instance_init() calls qdev_create() so far, the bug
> is latent.  But since I want to do that, I get to fix the bug first.
>
> Observe that object_apply_compat_props() doesn't actually need the
> MachineState, only its the compat_props member of its MachineClass and
> AccelClass.  This permits a simple fix: register MachineClass and
> AccelClass compat_props with the object_apply_compat_props() machinery
> right after these classes get selected.
>
> This is actually similar to how things worked before commits
> ea9ce8934c5 and b66bbee39f6, except we now register much earlier.  The
> old code registered them only after the machine's .instance_init()
> ran, which would've broken compatibility properties for any devices
> created there.
>
> Cc: Marc-André Lureau <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>


Reviewed-by: Marc-André Lureau <address@hidden>

> ---
>  accel/accel.c          |  1 +
>  hw/core/qdev.c         | 48 ++++++++++++++++++++++++++++++++----------
>  include/hw/qdev-core.h |  2 ++
>  vl.c                   |  1 +
>  4 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/accel/accel.c b/accel/accel.c
> index 68b6d56323..4a1670e404 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -66,6 +66,7 @@ static int accel_init_machine(AccelClass *acc, MachineState 
> *ms)
>          *(acc->allowed) = false;
>          object_unref(OBJECT(accel));
>      }
> +    object_set_accelerator_compat_props(acc->compat_props);
>      return ret;
>  }
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index d59071b8ed..1a86c7990a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -970,25 +970,51 @@ static void device_initfn(Object *obj)
>      QLIST_INIT(&dev->gpios);
>  }
>
> +/*
> + * Global property defaults
> + * Slot 0: accelerator's global property defaults
> + * Slot 1: machine's global property defaults
> + * Each is a GPtrArray of of GlobalProperty.
> + * Applied in order, later entries override earlier ones.
> + */
> +static GPtrArray *object_compat_props[2];
> +
> +/*
> + * Set machine's global property defaults to @compat_props.
> + * May be called at most once.
> + */
> +void object_set_machine_compat_props(GPtrArray *compat_props)
> +{
> +    assert(!object_compat_props[1]);
> +    object_compat_props[1] = compat_props;
> +}
> +
> +/*
> + * Set accelerator's global property defaults to @compat_props.
> + * May be called at most once.
> + */
> +void object_set_accelerator_compat_props(GPtrArray *compat_props)
> +{
> +    assert(!object_compat_props[0]);
> +    object_compat_props[0] = compat_props;
> +}
> +
>  void object_apply_compat_props(Object *obj)
>  {
> -    if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> -        MachineState *m = MACHINE(qdev_get_machine());
> -        MachineClass *mc = MACHINE_GET_CLASS(m);
> +    int i;
>
> -        if (m->accelerator) {
> -            AccelClass *ac = ACCEL_GET_CLASS(m->accelerator);
> -
> -            if (ac->compat_props) {
> -                object_apply_global_props(obj, ac->compat_props, 
> &error_abort);
> -            }
> -        }
> -        object_apply_global_props(obj, mc->compat_props, &error_abort);
> +    for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
> +        object_apply_global_props(obj, object_compat_props[i],
> +                                  &error_abort);
>      }
>  }
>
>  static void device_post_init(Object *obj)
>  {
> +    /*
> +     * Note: ordered so that the user's global properties take
> +     * precedence.
> +     */
>      object_apply_compat_props(obj);
>      qdev_prop_set_globals(DEVICE(obj));
>  }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 0a84c42756..bced1f2666 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -418,6 +418,8 @@ const char *qdev_fw_name(DeviceState *dev);
>
>  Object *qdev_get_machine(void);
>
> +void object_set_machine_compat_props(GPtrArray *compat_props);
> +void object_set_accelerator_compat_props(GPtrArray *compat_props);
>  void object_apply_compat_props(Object *obj);
>
>  /* FIXME: make this a link<> */
> diff --git a/vl.c b/vl.c
> index 502857a176..c50c2d6178 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3954,6 +3954,7 @@ int main(int argc, char **argv, char **envp)
>      configure_rtc(qemu_find_opts_singleton("rtc"));
>
>      machine_class = select_machine();
> +    object_set_machine_compat_props(machine_class->compat_props);
>
>      set_memory_options(&ram_slots, &maxram_size, machine_class);
>
> --
> 2.17.2
>