Hi
On Mon, Feb 25, 2019 at 7:38 PM Markus Armbruster <arm...@redhat.com> 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 <marcandre.lur...@redhat.com>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
> 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
>
