[v7,03/11] hw/core: create Resettable QOM interface

This commit defines an interface allowing multi-phase reset. This aims
to solve a problem of the actual single-phase reset (built in
DeviceClass and BusClass): reset behavior is dependent on the order
in which reset handlers are called. In particular doing external
side-effect (like setting an qemu_irq) is problematic because receiving
object may not be reset yet.

The Resettable interface divides the reset in 3 well defined phases.
To reset an object tree, all 1st phases are executed then all 2nd then
all 3rd. See the comments in include/hw/resettable.h for a more complete
description. The interface defines 3 phases to let the future
possibility of holding an object into reset for some time.

The qdev/qbus reset in DeviceClass and BusClass will be modified in
following commits to use this interface. A mechanism is provided
to allow executing a transitional reset handler in place of the 2nd
phase which is executed in children-then-parent order inside a tree.
This will allow to transition devices and buses smoothly while
keeping the exact current qdev/qbus reset behavior for now.

Documentation will be added in a following commit.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---

v7 update: un-nest struct ResettablePhases
---
 Makefile.objs           |   1 +
 include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
 hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
 hw/core/Makefile.objs   |   1 +
 hw/core/trace-events    |  17 +++
 5 files changed, 468 insertions(+)
 create mode 100644 include/hw/resettable.h
 create mode 100644 hw/core/resettable.c

On 1/15/20 1:36 PM, Damien Hedde wrote:
> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
> 
> The Resettable interface divides the reset in 3 well defined phases.
> To reset an object tree, all 1st phases are executed then all 2nd then
> all 3rd. See the comments in include/hw/resettable.h for a more complete
> description. The interface defines 3 phases to let the future
> possibility of holding an object into reset for some time.
> 
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface. A mechanism is provided
> to allow executing a transitional reset handler in place of the 2nd
> phase which is executed in children-then-parent order inside a tree.
> This will allow to transition devices and buses smoothly while
> keeping the exact current qdev/qbus reset behavior for now.
> 
> Documentation will be added in a following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> v7 update: un-nest struct ResettablePhases
> ---
>   Makefile.objs           |   1 +
>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>   hw/core/Makefile.objs   |   1 +
>   hw/core/trace-events    |  17 +++
>   5 files changed, 468 insertions(+)
>   create mode 100644 include/hw/resettable.h
>   create mode 100644 hw/core/resettable.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 7c1e50f9d6..9752d549b4 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>   trace-events-subdirs += net
>   trace-events-subdirs += ui
>   endif
> +trace-events-subdirs += hw/core

TL;DR Duplicating this line breaks using the LTTng Userspace Tracer 
library (UST backend).

Probably because you started this series before commit 26b8e6dc42b got 
merged, Jun 13 2019!

Indeed Oct 02 2018...
https://www.mail-archive.com/qemu-devel@nongnu.org/msg564153.html

The problem is you (correctly) sorted alphabetically while Alexey 
appended at the end.

>   trace-events-subdirs += hw/display
>   trace-events-subdirs += qapi
>   trace-events-subdirs += qom
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> new file mode 100644
> index 0000000000..58b3df4c22
> --- /dev/null
> +++ b/include/hw/resettable.h
> @@ -0,0 +1,211 @@
> +/*
> + * Resettable interface header.
> + *
> + * Copyright (c) 2019 GreenSocs SAS
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_RESETTABLE_H
> +#define HW_RESETTABLE_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_RESETTABLE_INTERFACE "resettable"
> +
> +#define RESETTABLE_CLASS(class) \
> +    OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE_INTERFACE)
> +
> +#define RESETTABLE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
> +
> +typedef struct ResettableState ResettableState;
> +
> +/**
> + * ResetType:
> + * Types of reset.
> + *
> + * + Cold: reset resulting from a power cycle of the object.
> + *
> + * TODO: Support has to be added to handle more types. In particular,
> + * ResettableState structure needs to be expanded.
> + */
> +typedef enum ResetType {
> +    RESET_TYPE_COLD,
> +} ResetType;
> +
> +/*
> + * ResettableClass:
> + * Interface for resettable objects.
> + *
> + * See docs/devel/reset.rst for more detailed information about how QEMU models
> + * reset. This whole API must only be used when holding the iothread mutex.
> + *
> + * All objects which can be reset must implement this interface;
> + * it is usually provided by a base class such as DeviceClass or BusClass.
> + * Every Resettable object must maintain some state tracking the
> + * progress of a reset operation by providing a ResettableState structure.
> + * The functions defined in this module take care of updating the
> + * state of the reset.
> + * The base class implementation of the interface provides this
> + * state and implements the associated method: get_state.
> + *
> + * Concrete object implementations (typically specific devices
> + * such as a UART model) should provide the functions
> + * for the phases.enter, phases.hold and phases.exit methods, which
> + * they can set in their class init function, either directly or
> + * by calling resettable_class_set_parent_phases().
> + * The phase methods are guaranteed to only only ever be called once
> + * for any reset event, in the order 'enter', 'hold', 'exit'.
> + * An object will always move quickly from 'enter' to 'hold'
> + * but might remain in 'hold' for an arbitrary period of time
> + * before eventually reset is deasserted and the 'exit' phase is called.
> + * Object implementations should be prepared for functions handling
> + * inbound connections from other devices (such as qemu_irq handler
> + * functions) to be called at any point during reset after their
> + * 'enter' method has been called.
> + *
> + * Users of a resettable object should not call these methods
> + * directly, but instead use the function resettable_reset().
> + *
> + * @phases.enter: This phase is called when the object enters reset. It
> + * should reset local state of the object, but it must not do anything that
> + * has a side-effect on other objects, such as raising or lowering a qemu_irq
> + * line or reading or writing guest memory. It takes the reset's type as
> + * argument.
> + *
> + * @phases.hold: This phase is called for entry into reset, once every object
> + * in the system which is being reset has had its @phases.enter method called.
> + * At this point devices can do actions that affect other objects.
> + *
> + * @phases.exit: This phase is called when the object leaves the reset state.
> + * Actions affecting other objects are permitted.
> + *
> + * @get_state: Mandatory method which must return a pointer to a
> + * ResettableState.
> + *
> + * @get_transitional_function: transitional method to handle Resettable objects
> + * not yet fully moved to this interface. It will be removed as soon as it is
> + * not needed anymore. This method is optional and may return a pointer to a
> + * function to be used instead of the phases. If the method exists and returns
> + * a non-NULL function pointer then that function is executed as a replacement
> + * of the 'hold' phase method taking the object as argument. The two other phase
> + * methods are not executed.
> + *
> + * @child_foreach: Executes a given callback on every Resettable child. Child
> + * in this context means a child in the qbus tree, so the children of a qbus
> + * are the devices on it, and the children of a device are all the buses it
> + * owns. This is not the same as the QOM object hierarchy. The function takes
> + * additional opaque and ResetType arguments which must be passed unmodified to
> + * the callback.
> + */
> +typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
> +typedef void (*ResettableHoldPhase)(Object *obj);
> +typedef void (*ResettableExitPhase)(Object *obj);
> +typedef ResettableState * (*ResettableGetState)(Object *obj);
> +typedef void (*ResettableTrFunction)(Object *obj);
> +typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
> +typedef void (*ResettableChildCallback)(Object *, void *opaque,
> +                                        ResetType type);
> +typedef void (*ResettableChildForeach)(Object *obj,
> +                                       ResettableChildCallback cb,
> +                                       void *opaque, ResetType type);
> +typedef struct ResettablePhases {
> +    ResettableEnterPhase enter;
> +    ResettableHoldPhase hold;
> +    ResettableExitPhase exit;
> +} ResettablePhases;
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    /* Phase methods */
> +    ResettablePhases phases;
> +
> +    /* State access method */
> +    ResettableGetState get_state;
> +
> +    /* Transitional method for legacy reset compatibility */
> +    ResettableGetTrFunction get_transitional_function;
> +
> +    /* Hierarchy handling method */
> +    ResettableChildForeach child_foreach;
> +} ResettableClass;
> +
> +/**
> + * ResettableState:
> + * Structure holding reset related state. The fields should not be accessed
> + * directly; the definition is here to allow further inclusion into other
> + * objects.
> + *
> + * @count: Number of reset level the object is into. It is incremented when
> + * the reset operation starts and decremented when it finishes.
> + * @hold_phase_pending: flag which indicates that we need to invoke the 'hold'
> + * phase handler for this object.
> + * @exit_phase_in_progress: true if we are currently in the exit phase
> + */
> +struct ResettableState {
> +    uint32_t count;
> +    bool hold_phase_pending;
> +    bool exit_phase_in_progress;
> +};
> +
> +/**
> + * resettable_reset:
> + * Trigger a reset on an object @obj of type @type. @obj must implement
> + * Resettable interface.
> + *
> + * Calling this function is equivalent to calling @resettable_assert_reset()
> + * then @resettable_release_reset().
> + */
> +void resettable_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_assert_reset:
> + * Put an object @obj into reset. @obj must implement Resettable interface.
> + *
> + * @resettable_release_reset() must eventually be called after this call.
> + * There must be one call to @resettable_release_reset() per call of
> + * @resettable_assert_reset(), with the same type argument.
> + *
> + * NOTE: Until support for migration is added, the @resettable_release_reset()
> + * must not be delayed. It must occur just after @resettable_assert_reset() so
> + * that migration cannot be triggered in between. Prefer using
> + * @resettable_reset() for now.
> + */
> +void resettable_assert_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_release_reset:
> + * Release the object @obj from reset. @obj must implement Resettable interface.
> + *
> + * See @resettable_assert_reset() description for details.
> + */
> +void resettable_release_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_is_in_reset:
> + * Return true if @obj is under reset.
> + *
> + * @obj must implement Resettable interface.
> + */
> +bool resettable_is_in_reset(Object *obj);
> +
> +/**
> + * resettable_class_set_parent_phases:
> + *
> + * Save @rc current reset phases into @parent_phases and override @rc phases
> + * by the given new methods (@enter, @hold and @exit).
> + * Each phase is overridden only if the new one is not NULL allowing to
> + * override a subset of phases.
> + */
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableEnterPhase enter,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases);
> +
> +#endif
> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
> new file mode 100644
> index 0000000000..9133208487
> --- /dev/null
> +++ b/hw/core/resettable.c
> @@ -0,0 +1,238 @@
> +/*
> + * Resettable interface.
> + *
> + * Copyright (c) 2019 GreenSocs SAS
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "hw/resettable.h"
> +#include "trace.h"
> +
> +/**
> + * resettable_phase_enter/hold/exit:
> + * Function executing a phase recursively in a resettable object and its
> + * children.
> + */
> +static void resettable_phase_enter(Object *obj, void *opaque, ResetType type);
> +static void resettable_phase_hold(Object *obj, void *opaque, ResetType type);
> +static void resettable_phase_exit(Object *obj, void *opaque, ResetType type);
> +
> +/**
> + * enter_phase_in_progress:
> + * True if we are currently in reset enter phase.
> + *
> + * Note: This flag is only used to guarantee (using asserts) that the reset
> + * API is used correctly. We can use a global variable because we rely on the
> + * iothread mutex to ensure only one reset operation is in a progress at a
> + * given time.
> + */
> +static bool enter_phase_in_progress;
> +
> +void resettable_reset(Object *obj, ResetType type)
> +{
> +    trace_resettable_reset(obj, type);
> +    resettable_assert_reset(obj, type);
> +    resettable_release_reset(obj, type);
> +}
> +
> +void resettable_assert_reset(Object *obj, ResetType type)
> +{
> +    /* TODO: change this assert when adding support for other reset types */
> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_assert_begin(obj, type);
> +    assert(!enter_phase_in_progress);
> +
> +    enter_phase_in_progress = true;
> +    resettable_phase_enter(obj, NULL, type);
> +    enter_phase_in_progress = false;
> +
> +    resettable_phase_hold(obj, NULL, type);
> +
> +    trace_resettable_reset_assert_end(obj);
> +}
> +
> +void resettable_release_reset(Object *obj, ResetType type)
> +{
> +    /* TODO: change this assert when adding support for other reset types */
> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_release_begin(obj, type);
> +    assert(!enter_phase_in_progress);
> +
> +    resettable_phase_exit(obj, NULL, type);
> +
> +    trace_resettable_reset_release_end(obj);
> +}
> +
> +bool resettable_is_in_reset(Object *obj)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +
> +    return s->count > 0;
> +}
> +
> +/**
> + * resettable_child_foreach:
> + * helper to avoid checking the existence of the method.
> + */
> +static void resettable_child_foreach(ResettableClass *rc, Object *obj,
> +                                     ResettableChildCallback cb,
> +                                     void *opaque, ResetType type)
> +{
> +    if (rc->child_foreach) {
> +        rc->child_foreach(obj, cb, opaque, type);
> +    }
> +}
> +
> +/**
> + * resettable_get_tr_func:
> + * helper to fetch transitional reset callback if any.
> + */
> +static ResettableTrFunction resettable_get_tr_func(ResettableClass *rc,
> +                                                   Object *obj)
> +{
> +    ResettableTrFunction tr_func = NULL;
> +    if (rc->get_transitional_function) {
> +        tr_func = rc->get_transitional_function(obj);
> +    }
> +    return tr_func;
> +}
> +
> +static void resettable_phase_enter(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +    bool action_needed = false;
> +
> +    /* exit phase has to finish properly before entering back in reset */
> +    assert(!s->exit_phase_in_progress);
> +
> +    trace_resettable_phase_enter_begin(obj, obj_typename, s->count, type);
> +
> +    /* Only take action if we really enter reset for the 1st time. */
> +    /*
> +     * TODO: if adding more ResetType support, some additional checks
> +     * are probably needed here.
> +     */
> +    if (s->count++ == 0) {
> +        action_needed = true;
> +    }
> +    /*
> +     * We limit the count to an arbitrary "big" value. The value is big
> +     * enough not to be triggered normally.
> +     * The assert will stop an infinite loop if there is a cycle in the
> +     * reset tree. The loop goes through resettable_foreach_child below
> +     * which at some point will call us again.
> +     */
> +    assert(s->count <= 50);
> +
> +    /*
> +     * handle the children even if action_needed is at false so that
> +     * child counts are incremented too
> +     */
> +    resettable_child_foreach(rc, obj, resettable_phase_enter, NULL, type);
> +
> +    /* execute enter phase for the object if needed */
> +    if (action_needed) {
> +        trace_resettable_phase_enter_exec(obj, obj_typename, type,
> +                                          !!rc->phases.enter);
> +        if (rc->phases.enter && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.enter(obj, type);
> +        }
> +        s->hold_phase_pending = true;
> +    }
> +    trace_resettable_phase_enter_end(obj, obj_typename, s->count);
> +}
> +
> +static void resettable_phase_hold(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +
> +    /* exit phase has to finish properly before entering back in reset */
> +    assert(!s->exit_phase_in_progress);
> +
> +    trace_resettable_phase_hold_begin(obj, obj_typename, s->count, type);
> +
> +    /* handle children first */
> +    resettable_child_foreach(rc, obj, resettable_phase_hold, NULL, type);
> +
> +    /* exec hold phase */
> +    if (s->hold_phase_pending) {
> +        s->hold_phase_pending = false;
> +        ResettableTrFunction tr_func = resettable_get_tr_func(rc, obj);
> +        trace_resettable_phase_hold_exec(obj, obj_typename, !!rc->phases.hold);
> +        if (tr_func) {
> +            trace_resettable_transitional_function(obj, obj_typename);
> +            tr_func(obj);
> +        } else if (rc->phases.hold) {
> +            rc->phases.hold(obj);
> +        }
> +    }
> +    trace_resettable_phase_hold_end(obj, obj_typename, s->count);
> +}
> +
> +static void resettable_phase_exit(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +
> +    assert(!s->exit_phase_in_progress);
> +    trace_resettable_phase_exit_begin(obj, obj_typename, s->count, type);
> +
> +    /* exit_phase_in_progress ensures this phase is 'atomic' */
> +    s->exit_phase_in_progress = true;
> +    resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type);
> +
> +    assert(s->count > 0);
> +    if (s->count == 1) {
> +        trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit);
> +        if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.exit(obj);
> +        }
> +        s->count = 0;
> +    }
> +    s->exit_phase_in_progress = false;
> +    trace_resettable_phase_exit_end(obj, obj_typename, s->count);
> +}
> +
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableEnterPhase enter,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases)
> +{
> +    *parent_phases = rc->phases;
> +    if (enter) {
> +        rc->phases.enter = enter;
> +    }
> +    if (hold) {
> +        rc->phases.hold = hold;
> +    }
> +    if (exit) {
> +        rc->phases.exit = exit;
> +    }
> +}
> +
> +static const TypeInfo resettable_interface_info = {
> +    .name       = TYPE_RESETTABLE_INTERFACE,
> +    .parent     = TYPE_INTERFACE,
> +    .class_size = sizeof(ResettableClass),
> +};
> +
> +static void reset_register_types(void)
> +{
> +    type_register_static(&resettable_interface_info);
> +}
> +
> +type_init(reset_register_types)
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 0edd9e635d..1709a122d4 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,6 +1,7 @@
>   # core qdev-related obj files, also used by *-user:
>   common-obj-y += qdev.o qdev-properties.o
>   common-obj-y += bus.o reset.o
> +common-obj-y += resettable.o
>   common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>   common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>   # irq.o needed for qdev GPIO handling:
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index a375aa88a4..a2e43f1120 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -9,3 +9,20 @@ qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>   qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>   qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>   qdev_update_parent_bus(void *obj, const char *objtype, void *oldp, const char *oldptype, void *newp, const char *newptype) "obj=%p(%s) old_parent=%p(%s) new_parent=%p(%s)"
> +
> +# resettable.c
> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_assert_end(void *obj) "obj=%p"
> +resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_release_end(void *obj) "obj=%p"
> +resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_enter_exec(void *obj, const char *objtype, int type, int has_method) "obj=%p(%s) type=%d method=%d"
> +resettable_phase_enter_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_phase_hold_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_hold_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
> +resettable_phase_hold_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_phase_exit_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_exit_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
> +resettable_phase_exit_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"

Something here breaks ./configure --enable-trace-backends=ust:

   CC      trace-ust-all.o
In file included from trace-ust-all.h:13,
                  from trace-ust-all.c:13:
trace-ust-all.h:35151:1: error: redefinition of 
‘__tracepoint_cb_qemu___loader_write_rom’
35151 | TRACEPOINT_EVENT(
       | ^~~~~~~~~~~~~~~~
trace-ust-all.h:31791:1: note: previous definition of 
‘__tracepoint_cb_qemu___loader_write_rom’ was here
31791 | TRACEPOINT_EVENT(
       | ^~~~~~~~~~~~~~~~
...
./trace-ust-all.h:35388:1: error: redefinition of 
‘__tp_event_signature___qemu___resettable_transitional_function’
35388 | TRACEPOINT_EVENT(
       | ^~~~~~~~~~~~~~~~
./trace-ust-all.h:32028:1: note: previous definition of 
‘__tp_event_signature___qemu___resettable_transitional_function’ was here
32028 | TRACEPOINT_EVENT(
       | ^~~~~~~~~~~~~~~~
In file included from /usr/include/lttng/tracepoint-event.h:58,
                  from trace-ust-all.h:35401,
                  from trace-ust-all.c:13:

Indeed:

32028 TRACEPOINT_EVENT(
32029    qemu,
32030    resettable_transitional_function,
32031    TP_ARGS(void *, obj, const char *, objtype),
32032    TP_FIELDS(
32033        ctf_integer_hex(void *, obj, obj)
32034        ctf_string(objtype, objtype)
32035    )
32036 )
32037
...
35388 TRACEPOINT_EVENT(
35389    qemu,
35390    resettable_transitional_function,
35391    TP_ARGS(void *, obj, const char *, objtype),
35392    TP_FIELDS(
35393        ctf_integer_hex(void *, obj, obj)
35394        ctf_string(objtype, objtype)
35395    )
35396 )
35397
35398 #endif /* TRACE_ALL_GENERATED_UST_H */

Ah! I was going to say "no clue what could be wrong, so Cc'ing Stefan" 
but got it:

$ git grep hw/core Makefile.objs
Makefile.objs:194:trace-events-subdirs += hw/core
Makefile.objs:207:trace-events-subdirs += hw/core

We might already have a 'uniq' makefile function to do:

trace-events-subdirs = $(call uniq $(trace-events-subdirs))

or maybe was it with $filter? I can't find it/remember, too tired.

So the fix is:

-- >8 --
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -191,7 +191,6 @@ trace-events-subdirs += migration
  trace-events-subdirs += net
  trace-events-subdirs += ui
  endif
-trace-events-subdirs += hw/core
  trace-events-subdirs += hw/display
  trace-events-subdirs += qapi
  trace-events-subdirs += qom
---

On 1/16/20 2:59 AM, Philippe Mathieu-Daudé wrote:
> On 1/15/20 1:36 PM, Damien Hedde wrote:
>> This commit defines an interface allowing multi-phase reset. This aims
>> to solve a problem of the actual single-phase reset (built in
>> DeviceClass and BusClass): reset behavior is dependent on the order
>> in which reset handlers are called. In particular doing external
>> side-effect (like setting an qemu_irq) is problematic because receiving
>> object may not be reset yet.
>>
>> The Resettable interface divides the reset in 3 well defined phases.
>> To reset an object tree, all 1st phases are executed then all 2nd then
>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>> description. The interface defines 3 phases to let the future
>> possibility of holding an object into reset for some time.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface. A mechanism is provided
>> to allow executing a transitional reset handler in place of the 2nd
>> phase which is executed in children-then-parent order inside a tree.
>> This will allow to transition devices and buses smoothly while
>> keeping the exact current qdev/qbus reset behavior for now.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> v7 update: un-nest struct ResettablePhases
>> ---
>>   Makefile.objs           |   1 +
>>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>   hw/core/Makefile.objs   |   1 +
>>   hw/core/trace-events    |  17 +++
>>   5 files changed, 468 insertions(+)
>>   create mode 100644 include/hw/resettable.h
>>   create mode 100644 hw/core/resettable.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 7c1e50f9d6..9752d549b4 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>>   trace-events-subdirs += net
>>   trace-events-subdirs += ui
>>   endif
>> +trace-events-subdirs += hw/core
> 
> TL;DR Duplicating this line breaks using the LTTng Userspace Tracer 
> library (UST backend).
> 
> Probably because you started this series before commit 26b8e6dc42b got 
> merged, Jun 13 2019!
> 
> Indeed Oct 02 2018...
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg564153.html
> 
> The problem is you (correctly) sorted alphabetically while Alexey 
> appended at the end.
> 
>>   trace-events-subdirs += hw/display
>>   trace-events-subdirs += qapi
>>   trace-events-subdirs += qom
[...]
>> diff --git a/hw/core/trace-events b/hw/core/trace-events
>> index a375aa88a4..a2e43f1120 100644
>> --- a/hw/core/trace-events
>> +++ b/hw/core/trace-events
>> @@ -9,3 +9,20 @@ qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>>   qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>>   qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>>   qdev_update_parent_bus(void *obj, const char *objtype, void *oldp, 
>> const char *oldptype, void *newp, const char *newptype) "obj=%p(%s) 
>> old_parent=%p(%s) new_parent=%p(%s)"
>> +
>> +# resettable.c
>> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_assert_end(void *obj) "obj=%p"
>> +resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_release_end(void *obj) "obj=%p"
>> +resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t 
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_enter_exec(void *obj, const char *objtype, int type, 
>> int has_method) "obj=%p(%s) type=%d method=%d"
>> +resettable_phase_enter_end(void *obj, const char *objtype, uint32_t 
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_phase_hold_begin(void *obj, const char *objtype, uint32_t 
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_hold_exec(void *obj, const char *objtype, int 
>> has_method) "obj=%p(%s) method=%d"
>> +resettable_phase_hold_end(void *obj, const char *objtype, uint32_t 
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_phase_exit_begin(void *obj, const char *objtype, uint32_t 
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_exit_exec(void *obj, const char *objtype, int 
>> has_method) "obj=%p(%s) method=%d"
>> +resettable_phase_exit_end(void *obj, const char *objtype, uint32_t 
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_transitional_function(void *obj, const char *objtype) 
>> "obj=%p(%s)"
> 
> Something here breaks ./configure --enable-trace-backends=ust:
> 
>    CC      trace-ust-all.o
> In file included from trace-ust-all.h:13,
>                   from trace-ust-all.c:13:
> trace-ust-all.h:35151:1: error: redefinition of 
> ‘__tracepoint_cb_qemu___loader_write_rom’
> 35151 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> trace-ust-all.h:31791:1: note: previous definition of 
> ‘__tracepoint_cb_qemu___loader_write_rom’ was here
> 31791 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> ...
> ./trace-ust-all.h:35388:1: error: redefinition of 
> ‘__tp_event_signature___qemu___resettable_transitional_function’
> 35388 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> ./trace-ust-all.h:32028:1: note: previous definition of 
> ‘__tp_event_signature___qemu___resettable_transitional_function’ was here
> 32028 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> In file included from /usr/include/lttng/tracepoint-event.h:58,
>                   from trace-ust-all.h:35401,
>                   from trace-ust-all.c:13:
> 
> Indeed:
> 
> 32028 TRACEPOINT_EVENT(
> 32029    qemu,
> 32030    resettable_transitional_function,
> 32031    TP_ARGS(void *, obj, const char *, objtype),
> 32032    TP_FIELDS(
> 32033        ctf_integer_hex(void *, obj, obj)
> 32034        ctf_string(objtype, objtype)
> 32035    )
> 32036 )
> 32037
> ...
> 35388 TRACEPOINT_EVENT(
> 35389    qemu,
> 35390    resettable_transitional_function,
> 35391    TP_ARGS(void *, obj, const char *, objtype),
> 35392    TP_FIELDS(
> 35393        ctf_integer_hex(void *, obj, obj)
> 35394        ctf_string(objtype, objtype)
> 35395    )
> 35396 )
> 35397
> 35398 #endif /* TRACE_ALL_GENERATED_UST_H */
> 
> Ah! I was going to say "no clue what could be wrong, so Cc'ing Stefan" 
> but got it:
> 
> $ git grep hw/core Makefile.objs
> Makefile.objs:194:trace-events-subdirs += hw/core
> Makefile.objs:207:trace-events-subdirs += hw/core
> 
> We might already have a 'uniq' makefile function to do:
> 
> trace-events-subdirs = $(call uniq $(trace-events-subdirs))
> 
> or maybe was it with $filter? I can't find it/remember, too tired.
> 
> So the fix is:
> 
> -- >8 --
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -191,7 +191,6 @@ trace-events-subdirs += migration
>   trace-events-subdirs += net
>   trace-events-subdirs += ui
>   endif
> -trace-events-subdirs += hw/core
>   trace-events-subdirs += hw/display
>   trace-events-subdirs += qapi
>   trace-events-subdirs += qom
> ---

Forgot to add, with trace-events-subdirs fixed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

On 1/16/20 2:59 AM, Philippe Mathieu-Daudé wrote:
> On 1/15/20 1:36 PM, Damien Hedde wrote:
>> This commit defines an interface allowing multi-phase reset. This aims
>> to solve a problem of the actual single-phase reset (built in
>> DeviceClass and BusClass): reset behavior is dependent on the order
>> in which reset handlers are called. In particular doing external
>> side-effect (like setting an qemu_irq) is problematic because receiving
>> object may not be reset yet.
>>
>> The Resettable interface divides the reset in 3 well defined phases.
>> To reset an object tree, all 1st phases are executed then all 2nd then
>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>> description. The interface defines 3 phases to let the future
>> possibility of holding an object into reset for some time.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface. A mechanism is provided
>> to allow executing a transitional reset handler in place of the 2nd
>> phase which is executed in children-then-parent order inside a tree.
>> This will allow to transition devices and buses smoothly while
>> keeping the exact current qdev/qbus reset behavior for now.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> v7 update: un-nest struct ResettablePhases
>> ---
>>   Makefile.objs           |   1 +
>>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>   hw/core/Makefile.objs   |   1 +
>>   hw/core/trace-events    |  17 +++
>>   5 files changed, 468 insertions(+)
>>   create mode 100644 include/hw/resettable.h
>>   create mode 100644 hw/core/resettable.c
>>

> 
> Something here breaks ./configure --enable-trace-backends=ust:
> 
>   CC      trace-ust-all.o
> In file included from trace-ust-all.h:13,
>                  from trace-ust-all.c:13:
> trace-ust-all.h:35151:1: error: redefinition of
> ‘__tracepoint_cb_qemu___loader_write_rom’
> 35151 | TRACEPOINT_EVENT(
>       | ^~~~~~~~~~~~~~~~
> trace-ust-all.h:31791:1: note: previous definition of
> ‘__tracepoint_cb_qemu___loader_write_rom’ was here
> 31791 | TRACEPOINT_EVENT(
>       | ^~~~~~~~~~~~~~~~
> ...
> ./trace-ust-all.h:35388:1: error: redefinition of
> ‘__tp_event_signature___qemu___resettable_transitional_function’
> 35388 | TRACEPOINT_EVENT(
>       | ^~~~~~~~~~~~~~~~
> ./trace-ust-all.h:32028:1: note: previous definition of
> ‘__tp_event_signature___qemu___resettable_transitional_function’ was here
> 32028 | TRACEPOINT_EVENT(
>       | ^~~~~~~~~~~~~~~~
> In file included from /usr/include/lttng/tracepoint-event.h:58,
>                  from trace-ust-all.h:35401,
>                  from trace-ust-all.c:13:
> 
> Indeed:
> 
> 32028 TRACEPOINT_EVENT(
> 32029    qemu,
> 32030    resettable_transitional_function,
> 32031    TP_ARGS(void *, obj, const char *, objtype),
> 32032    TP_FIELDS(
> 32033        ctf_integer_hex(void *, obj, obj)
> 32034        ctf_string(objtype, objtype)
> 32035    )
> 32036 )
> 32037
> ...
> 35388 TRACEPOINT_EVENT(
> 35389    qemu,
> 35390    resettable_transitional_function,
> 35391    TP_ARGS(void *, obj, const char *, objtype),
> 35392    TP_FIELDS(
> 35393        ctf_integer_hex(void *, obj, obj)
> 35394        ctf_string(objtype, objtype)
> 35395    )
> 35396 )
> 35397
> 35398 #endif /* TRACE_ALL_GENERATED_UST_H */
> 
> Ah! I was going to say "no clue what could be wrong, so Cc'ing Stefan"
> but got it:
> 
> $ git grep hw/core Makefile.objs
> Makefile.objs:194:trace-events-subdirs += hw/core
> Makefile.objs:207:trace-events-subdirs += hw/core
> 
> We might already have a 'uniq' makefile function to do:
> 
> trace-events-subdirs = $(call uniq $(trace-events-subdirs))
> 
> or maybe was it with $filter? I can't find it/remember, too tired.

You can use $sort to remove duplicates in make.

> 
> So the fix is:
> 
> -- >8 --
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -191,7 +191,6 @@ trace-events-subdirs += migration
>  trace-events-subdirs += net
>  trace-events-subdirs += ui
>  endif
> -trace-events-subdirs += hw/core
>  trace-events-subdirs += hw/display
>  trace-events-subdirs += qapi
>  trace-events-subdirs += qom
> ---
> 

I'll remove the duplicate entry.
Thanks,

Damien

On 1/16/20 9:53 AM, Damien Hedde wrote:
> On 1/16/20 2:59 AM, Philippe Mathieu-Daudé wrote:
>> On 1/15/20 1:36 PM, Damien Hedde wrote:
>>> This commit defines an interface allowing multi-phase reset. This aims
>>> to solve a problem of the actual single-phase reset (built in
>>> DeviceClass and BusClass): reset behavior is dependent on the order
>>> in which reset handlers are called. In particular doing external
>>> side-effect (like setting an qemu_irq) is problematic because receiving
>>> object may not be reset yet.
>>>
>>> The Resettable interface divides the reset in 3 well defined phases.
>>> To reset an object tree, all 1st phases are executed then all 2nd then
>>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>>> description. The interface defines 3 phases to let the future
>>> possibility of holding an object into reset for some time.
>>>
>>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>>> following commits to use this interface. A mechanism is provided
>>> to allow executing a transitional reset handler in place of the 2nd
>>> phase which is executed in children-then-parent order inside a tree.
>>> This will allow to transition devices and buses smoothly while
>>> keeping the exact current qdev/qbus reset behavior for now.
>>>
>>> Documentation will be added in a following commit.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>
>>> v7 update: un-nest struct ResettablePhases
>>> ---
>>>    Makefile.objs           |   1 +
>>>    include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>>    hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>>    hw/core/Makefile.objs   |   1 +
>>>    hw/core/trace-events    |  17 +++
>>>    5 files changed, 468 insertions(+)
>>>    create mode 100644 include/hw/resettable.h
>>>    create mode 100644 hw/core/resettable.c
>>>
> 
>>
>> Something here breaks ./configure --enable-trace-backends=ust:
>>
>>    CC      trace-ust-all.o
>> In file included from trace-ust-all.h:13,
>>                   from trace-ust-all.c:13:
>> trace-ust-all.h:35151:1: error: redefinition of
>> ‘__tracepoint_cb_qemu___loader_write_rom’
>> 35151 | TRACEPOINT_EVENT(
>>        | ^~~~~~~~~~~~~~~~
>> trace-ust-all.h:31791:1: note: previous definition of
>> ‘__tracepoint_cb_qemu___loader_write_rom’ was here
>> 31791 | TRACEPOINT_EVENT(
>>        | ^~~~~~~~~~~~~~~~
>> ...
>> ./trace-ust-all.h:35388:1: error: redefinition of
>> ‘__tp_event_signature___qemu___resettable_transitional_function’
>> 35388 | TRACEPOINT_EVENT(
>>        | ^~~~~~~~~~~~~~~~
>> ./trace-ust-all.h:32028:1: note: previous definition of
>> ‘__tp_event_signature___qemu___resettable_transitional_function’ was here
>> 32028 | TRACEPOINT_EVENT(
>>        | ^~~~~~~~~~~~~~~~
>> In file included from /usr/include/lttng/tracepoint-event.h:58,
>>                   from trace-ust-all.h:35401,
>>                   from trace-ust-all.c:13:
>>
>> Indeed:
>>
>> 32028 TRACEPOINT_EVENT(
>> 32029    qemu,
>> 32030    resettable_transitional_function,
>> 32031    TP_ARGS(void *, obj, const char *, objtype),
>> 32032    TP_FIELDS(
>> 32033        ctf_integer_hex(void *, obj, obj)
>> 32034        ctf_string(objtype, objtype)
>> 32035    )
>> 32036 )
>> 32037
>> ...
>> 35388 TRACEPOINT_EVENT(
>> 35389    qemu,
>> 35390    resettable_transitional_function,
>> 35391    TP_ARGS(void *, obj, const char *, objtype),
>> 35392    TP_FIELDS(
>> 35393        ctf_integer_hex(void *, obj, obj)
>> 35394        ctf_string(objtype, objtype)
>> 35395    )
>> 35396 )
>> 35397
>> 35398 #endif /* TRACE_ALL_GENERATED_UST_H */
>>
>> Ah! I was going to say "no clue what could be wrong, so Cc'ing Stefan"
>> but got it:
>>
>> $ git grep hw/core Makefile.objs
>> Makefile.objs:194:trace-events-subdirs += hw/core
>> Makefile.objs:207:trace-events-subdirs += hw/core
>>
>> We might already have a 'uniq' makefile function to do:
>>
>> trace-events-subdirs = $(call uniq $(trace-events-subdirs))
>>
>> or maybe was it with $filter? I can't find it/remember, too tired.
> 
> You can use $sort to remove duplicates in make.

Ah $(sort ...) thanks, I was too tired to remember it =)

>>
>> So the fix is:
>>
>> -- >8 --
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -191,7 +191,6 @@ trace-events-subdirs += migration
>>   trace-events-subdirs += net
>>   trace-events-subdirs += ui
>>   endif
>> -trace-events-subdirs += hw/core
>>   trace-events-subdirs += hw/display
>>   trace-events-subdirs += qapi
>>   trace-events-subdirs += qom
>> ---
>>
> 
> I'll remove the duplicate entry.

I prepared a patch to move hw/core, if qemu-trivial merges it first, you 
shouldn't need to respin your series.

On 1/15/20 1:36 PM, Damien Hedde wrote:
> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
> 
> The Resettable interface divides the reset in 3 well defined phases.
> To reset an object tree, all 1st phases are executed then all 2nd then
> all 3rd. See the comments in include/hw/resettable.h for a more complete
> description. The interface defines 3 phases to let the future
> possibility of holding an object into reset for some time.
> 
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface. A mechanism is provided
> to allow executing a transitional reset handler in place of the 2nd
> phase which is executed in children-then-parent order inside a tree.
> This will allow to transition devices and buses smoothly while
> keeping the exact current qdev/qbus reset behavior for now.
> 
> Documentation will be added in a following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> v7 update: un-nest struct ResettablePhases
> ---
>   Makefile.objs           |   1 +
>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>   hw/core/Makefile.objs   |   1 +
>   hw/core/trace-events    |  17 +++
>   5 files changed, 468 insertions(+)
>   create mode 100644 include/hw/resettable.h
>   create mode 100644 hw/core/resettable.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 7c1e50f9d6..9752d549b4 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>   trace-events-subdirs += net
>   trace-events-subdirs += ui
>   endif
> +trace-events-subdirs += hw/core
>   trace-events-subdirs += hw/display
>   trace-events-subdirs += qapi
>   trace-events-subdirs += qom
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> new file mode 100644
> index 0000000000..58b3df4c22
> --- /dev/null
> +++ b/include/hw/resettable.h
> @@ -0,0 +1,211 @@
> +/*
> + * Resettable interface header.
> + *
> + * Copyright (c) 2019 GreenSocs SAS
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_RESETTABLE_H
> +#define HW_RESETTABLE_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_RESETTABLE_INTERFACE "resettable"
> +
> +#define RESETTABLE_CLASS(class) \
> +    OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE_INTERFACE)
> +
> +#define RESETTABLE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
> +
> +typedef struct ResettableState ResettableState;
> +
> +/**
> + * ResetType:
> + * Types of reset.
> + *
> + * + Cold: reset resulting from a power cycle of the object.
> + *
> + * TODO: Support has to be added to handle more types. In particular,
> + * ResettableState structure needs to be expanded.
> + */
> +typedef enum ResetType {
> +    RESET_TYPE_COLD,
> +} ResetType;
> +
> +/*
> + * ResettableClass:
> + * Interface for resettable objects.
> + *
> + * See docs/devel/reset.rst for more detailed information about how QEMU models
> + * reset. This whole API must only be used when holding the iothread mutex.
> + *
> + * All objects which can be reset must implement this interface;
> + * it is usually provided by a base class such as DeviceClass or BusClass.
> + * Every Resettable object must maintain some state tracking the
> + * progress of a reset operation by providing a ResettableState structure.
> + * The functions defined in this module take care of updating the
> + * state of the reset.
> + * The base class implementation of the interface provides this
> + * state and implements the associated method: get_state.
> + *
> + * Concrete object implementations (typically specific devices
> + * such as a UART model) should provide the functions
> + * for the phases.enter, phases.hold and phases.exit methods, which
> + * they can set in their class init function, either directly or
> + * by calling resettable_class_set_parent_phases().
> + * The phase methods are guaranteed to only only ever be called once
> + * for any reset event, in the order 'enter', 'hold', 'exit'.
> + * An object will always move quickly from 'enter' to 'hold'
> + * but might remain in 'hold' for an arbitrary period of time
> + * before eventually reset is deasserted and the 'exit' phase is called.
> + * Object implementations should be prepared for functions handling
> + * inbound connections from other devices (such as qemu_irq handler
> + * functions) to be called at any point during reset after their
> + * 'enter' method has been called.
> + *
> + * Users of a resettable object should not call these methods
> + * directly, but instead use the function resettable_reset().
> + *
> + * @phases.enter: This phase is called when the object enters reset. It
> + * should reset local state of the object, but it must not do anything that
> + * has a side-effect on other objects, such as raising or lowering a qemu_irq
> + * line or reading or writing guest memory. It takes the reset's type as
> + * argument.
> + *
> + * @phases.hold: This phase is called for entry into reset, once every object
> + * in the system which is being reset has had its @phases.enter method called.
> + * At this point devices can do actions that affect other objects.
> + *
> + * @phases.exit: This phase is called when the object leaves the reset state.
> + * Actions affecting other objects are permitted.
> + *
> + * @get_state: Mandatory method which must return a pointer to a
> + * ResettableState.
> + *
> + * @get_transitional_function: transitional method to handle Resettable objects
> + * not yet fully moved to this interface. It will be removed as soon as it is
> + * not needed anymore. This method is optional and may return a pointer to a
> + * function to be used instead of the phases. If the method exists and returns
> + * a non-NULL function pointer then that function is executed as a replacement
> + * of the 'hold' phase method taking the object as argument. The two other phase
> + * methods are not executed.
> + *
> + * @child_foreach: Executes a given callback on every Resettable child. Child
> + * in this context means a child in the qbus tree, so the children of a qbus
> + * are the devices on it, and the children of a device are all the buses it
> + * owns. This is not the same as the QOM object hierarchy. The function takes
> + * additional opaque and ResetType arguments which must be passed unmodified to
> + * the callback.
> + */
> +typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
> +typedef void (*ResettableHoldPhase)(Object *obj);
> +typedef void (*ResettableExitPhase)(Object *obj);
> +typedef ResettableState * (*ResettableGetState)(Object *obj);
> +typedef void (*ResettableTrFunction)(Object *obj);
> +typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
> +typedef void (*ResettableChildCallback)(Object *, void *opaque,
> +                                        ResetType type);
> +typedef void (*ResettableChildForeach)(Object *obj,
> +                                       ResettableChildCallback cb,
> +                                       void *opaque, ResetType type);
> +typedef struct ResettablePhases {
> +    ResettableEnterPhase enter;
> +    ResettableHoldPhase hold;
> +    ResettableExitPhase exit;
> +} ResettablePhases;
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    /* Phase methods */
> +    ResettablePhases phases;
> +
> +    /* State access method */
> +    ResettableGetState get_state;
> +
> +    /* Transitional method for legacy reset compatibility */
> +    ResettableGetTrFunction get_transitional_function;
> +
> +    /* Hierarchy handling method */
> +    ResettableChildForeach child_foreach;
> +} ResettableClass;
> +
> +/**
> + * ResettableState:
> + * Structure holding reset related state. The fields should not be accessed
> + * directly; the definition is here to allow further inclusion into other
> + * objects.
> + *
> + * @count: Number of reset level the object is into. It is incremented when
> + * the reset operation starts and decremented when it finishes.
> + * @hold_phase_pending: flag which indicates that we need to invoke the 'hold'
> + * phase handler for this object.
> + * @exit_phase_in_progress: true if we are currently in the exit phase
> + */
> +struct ResettableState {
> +    uint32_t count;
> +    bool hold_phase_pending;
> +    bool exit_phase_in_progress;
> +};
> +
> +/**
> + * resettable_reset:
> + * Trigger a reset on an object @obj of type @type. @obj must implement
> + * Resettable interface.
> + *
> + * Calling this function is equivalent to calling @resettable_assert_reset()
> + * then @resettable_release_reset().
> + */
> +void resettable_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_assert_reset:
> + * Put an object @obj into reset. @obj must implement Resettable interface.
> + *
> + * @resettable_release_reset() must eventually be called after this call.
> + * There must be one call to @resettable_release_reset() per call of
> + * @resettable_assert_reset(), with the same type argument.
> + *
> + * NOTE: Until support for migration is added, the @resettable_release_reset()
> + * must not be delayed. It must occur just after @resettable_assert_reset() so
> + * that migration cannot be triggered in between. Prefer using
> + * @resettable_reset() for now.
> + */
> +void resettable_assert_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_release_reset:
> + * Release the object @obj from reset. @obj must implement Resettable interface.
> + *
> + * See @resettable_assert_reset() description for details.
> + */
> +void resettable_release_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_is_in_reset:
> + * Return true if @obj is under reset.
> + *
> + * @obj must implement Resettable interface.
> + */
> +bool resettable_is_in_reset(Object *obj);
> +
> +/**
> + * resettable_class_set_parent_phases:
> + *
> + * Save @rc current reset phases into @parent_phases and override @rc phases
> + * by the given new methods (@enter, @hold and @exit).
> + * Each phase is overridden only if the new one is not NULL allowing to
> + * override a subset of phases.
> + */
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableEnterPhase enter,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases);
> +
> +#endif
> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
> new file mode 100644
> index 0000000000..9133208487
> --- /dev/null
> +++ b/hw/core/resettable.c
> @@ -0,0 +1,238 @@
> +/*
> + * Resettable interface.
> + *
> + * Copyright (c) 2019 GreenSocs SAS
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "hw/resettable.h"
> +#include "trace.h"
> +
> +/**
> + * resettable_phase_enter/hold/exit:
> + * Function executing a phase recursively in a resettable object and its
> + * children.
> + */
> +static void resettable_phase_enter(Object *obj, void *opaque, ResetType type);
> +static void resettable_phase_hold(Object *obj, void *opaque, ResetType type);
> +static void resettable_phase_exit(Object *obj, void *opaque, ResetType type);
> +
> +/**
> + * enter_phase_in_progress:
> + * True if we are currently in reset enter phase.
> + *
> + * Note: This flag is only used to guarantee (using asserts) that the reset
> + * API is used correctly. We can use a global variable because we rely on the
> + * iothread mutex to ensure only one reset operation is in a progress at a
> + * given time.
> + */
> +static bool enter_phase_in_progress;
> +
> +void resettable_reset(Object *obj, ResetType type)
> +{
> +    trace_resettable_reset(obj, type);
> +    resettable_assert_reset(obj, type);
> +    resettable_release_reset(obj, type);
> +}
> +
> +void resettable_assert_reset(Object *obj, ResetType type)
> +{
> +    /* TODO: change this assert when adding support for other reset types */
> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_assert_begin(obj, type);
> +    assert(!enter_phase_in_progress);
> +
> +    enter_phase_in_progress = true;
> +    resettable_phase_enter(obj, NULL, type);
> +    enter_phase_in_progress = false;
> +
> +    resettable_phase_hold(obj, NULL, type);
> +
> +    trace_resettable_reset_assert_end(obj);
> +}
> +
> +void resettable_release_reset(Object *obj, ResetType type)
> +{
> +    /* TODO: change this assert when adding support for other reset types */
> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_release_begin(obj, type);
> +    assert(!enter_phase_in_progress);
> +
> +    resettable_phase_exit(obj, NULL, type);
> +
> +    trace_resettable_reset_release_end(obj);
> +}
> +
> +bool resettable_is_in_reset(Object *obj)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +
> +    return s->count > 0;
> +}
> +
> +/**
> + * resettable_child_foreach:
> + * helper to avoid checking the existence of the method.
> + */
> +static void resettable_child_foreach(ResettableClass *rc, Object *obj,
> +                                     ResettableChildCallback cb,
> +                                     void *opaque, ResetType type)
> +{
> +    if (rc->child_foreach) {
> +        rc->child_foreach(obj, cb, opaque, type);
> +    }
> +}
> +
> +/**
> + * resettable_get_tr_func:
> + * helper to fetch transitional reset callback if any.
> + */
> +static ResettableTrFunction resettable_get_tr_func(ResettableClass *rc,
> +                                                   Object *obj)
> +{
> +    ResettableTrFunction tr_func = NULL;
> +    if (rc->get_transitional_function) {
> +        tr_func = rc->get_transitional_function(obj);
> +    }
> +    return tr_func;
> +}
> +
> +static void resettable_phase_enter(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +    bool action_needed = false;
> +
> +    /* exit phase has to finish properly before entering back in reset */
> +    assert(!s->exit_phase_in_progress);
> +
> +    trace_resettable_phase_enter_begin(obj, obj_typename, s->count, type);
> +
> +    /* Only take action if we really enter reset for the 1st time. */
> +    /*
> +     * TODO: if adding more ResetType support, some additional checks
> +     * are probably needed here.
> +     */
> +    if (s->count++ == 0) {
> +        action_needed = true;
> +    }
> +    /*
> +     * We limit the count to an arbitrary "big" value. The value is big
> +     * enough not to be triggered normally.
> +     * The assert will stop an infinite loop if there is a cycle in the
> +     * reset tree. The loop goes through resettable_foreach_child below
> +     * which at some point will call us again.
> +     */
> +    assert(s->count <= 50);
> +
> +    /*
> +     * handle the children even if action_needed is at false so that
> +     * child counts are incremented too
> +     */
> +    resettable_child_foreach(rc, obj, resettable_phase_enter, NULL, type);
> +
> +    /* execute enter phase for the object if needed */
> +    if (action_needed) {
> +        trace_resettable_phase_enter_exec(obj, obj_typename, type,
> +                                          !!rc->phases.enter);
> +        if (rc->phases.enter && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.enter(obj, type);
> +        }
> +        s->hold_phase_pending = true;
> +    }
> +    trace_resettable_phase_enter_end(obj, obj_typename, s->count);
> +}
> +
> +static void resettable_phase_hold(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +
> +    /* exit phase has to finish properly before entering back in reset */
> +    assert(!s->exit_phase_in_progress);
> +
> +    trace_resettable_phase_hold_begin(obj, obj_typename, s->count, type);
> +
> +    /* handle children first */
> +    resettable_child_foreach(rc, obj, resettable_phase_hold, NULL, type);
> +
> +    /* exec hold phase */
> +    if (s->hold_phase_pending) {
> +        s->hold_phase_pending = false;
> +        ResettableTrFunction tr_func = resettable_get_tr_func(rc, obj);
> +        trace_resettable_phase_hold_exec(obj, obj_typename, !!rc->phases.hold);
> +        if (tr_func) {
> +            trace_resettable_transitional_function(obj, obj_typename);
> +            tr_func(obj);
> +        } else if (rc->phases.hold) {
> +            rc->phases.hold(obj);
> +        }
> +    }
> +    trace_resettable_phase_hold_end(obj, obj_typename, s->count);
> +}
> +
> +static void resettable_phase_exit(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +
> +    assert(!s->exit_phase_in_progress);
> +    trace_resettable_phase_exit_begin(obj, obj_typename, s->count, type);
> +
> +    /* exit_phase_in_progress ensures this phase is 'atomic' */
> +    s->exit_phase_in_progress = true;
> +    resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type);
> +
> +    assert(s->count > 0);
> +    if (s->count == 1) {
> +        trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit);
> +        if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.exit(obj);
> +        }
> +        s->count = 0;
> +    }
> +    s->exit_phase_in_progress = false;
> +    trace_resettable_phase_exit_end(obj, obj_typename, s->count);
> +}
> +
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableEnterPhase enter,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases)
> +{
> +    *parent_phases = rc->phases;
> +    if (enter) {

Why care about checking if handlers are not NULL? It is not like you are 
overwriting a pointer previously set.

Anyway:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        rc->phases.enter = enter;
> +    }
> +    if (hold) {
> +        rc->phases.hold = hold;
> +    }
> +    if (exit) {
> +        rc->phases.exit = exit;
> +    }
> +}
> +
> +static const TypeInfo resettable_interface_info = {
> +    .name       = TYPE_RESETTABLE_INTERFACE,
> +    .parent     = TYPE_INTERFACE,
> +    .class_size = sizeof(ResettableClass),
> +};
> +
> +static void reset_register_types(void)
> +{
> +    type_register_static(&resettable_interface_info);
> +}
> +
> +type_init(reset_register_types)
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 0edd9e635d..1709a122d4 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,6 +1,7 @@
>   # core qdev-related obj files, also used by *-user:
>   common-obj-y += qdev.o qdev-properties.o
>   common-obj-y += bus.o reset.o
> +common-obj-y += resettable.o
>   common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>   common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>   # irq.o needed for qdev GPIO handling:
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index a375aa88a4..a2e43f1120 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -9,3 +9,20 @@ qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>   qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>   qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>   qdev_update_parent_bus(void *obj, const char *objtype, void *oldp, const char *oldptype, void *newp, const char *newptype) "obj=%p(%s) old_parent=%p(%s) new_parent=%p(%s)"
> +
> +# resettable.c
> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_assert_end(void *obj) "obj=%p"
> +resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_release_end(void *obj) "obj=%p"
> +resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_enter_exec(void *obj, const char *objtype, int type, int has_method) "obj=%p(%s) type=%d method=%d"
> +resettable_phase_enter_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_phase_hold_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_hold_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
> +resettable_phase_hold_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_phase_exit_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_exit_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
> +resettable_phase_exit_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"
>

On 1/15/20 1:36 PM, Damien Hedde wrote:
> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
> 
> The Resettable interface divides the reset in 3 well defined phases.
> To reset an object tree, all 1st phases are executed then all 2nd then
> all 3rd. See the comments in include/hw/resettable.h for a more complete
> description. The interface defines 3 phases to let the future
> possibility of holding an object into reset for some time.
> 
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface. A mechanism is provided
> to allow executing a transitional reset handler in place of the 2nd
> phase which is executed in children-then-parent order inside a tree.
> This will allow to transition devices and buses smoothly while
> keeping the exact current qdev/qbus reset behavior for now.
> 
> Documentation will be added in a following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> v7 update: un-nest struct ResettablePhases
> ---
>   Makefile.objs           |   1 +
>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>   hw/core/Makefile.objs   |   1 +
>   hw/core/trace-events    |  17 +++
>   5 files changed, 468 insertions(+)
>   create mode 100644 include/hw/resettable.h
>   create mode 100644 hw/core/resettable.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 7c1e50f9d6..9752d549b4 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>   trace-events-subdirs += net
>   trace-events-subdirs += ui
>   endif
> +trace-events-subdirs += hw/core
>   trace-events-subdirs += hw/display
>   trace-events-subdirs += qapi
>   trace-events-subdirs += qom
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> new file mode 100644
> index 0000000000..58b3df4c22
> --- /dev/null
> +++ b/include/hw/resettable.h
> @@ -0,0 +1,211 @@
> +/*
> + * Resettable interface header.
> + *
> + * Copyright (c) 2019 GreenSocs SAS
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_RESETTABLE_H
> +#define HW_RESETTABLE_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_RESETTABLE_INTERFACE "resettable"
> +
> +#define RESETTABLE_CLASS(class) \
> +    OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE_INTERFACE)
> +
> +#define RESETTABLE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
> +
> +typedef struct ResettableState ResettableState;
> +
> +/**
> + * ResetType:
> + * Types of reset.
> + *
> + * + Cold: reset resulting from a power cycle of the object.
> + *
> + * TODO: Support has to be added to handle more types. In particular,
> + * ResettableState structure needs to be expanded.
> + */
> +typedef enum ResetType {
> +    RESET_TYPE_COLD,
> +} ResetType;
> +
> +/*
> + * ResettableClass:
> + * Interface for resettable objects.
> + *
> + * See docs/devel/reset.rst for more detailed information about how QEMU models
> + * reset. This whole API must only be used when holding the iothread mutex.
> + *
> + * All objects which can be reset must implement this interface;
> + * it is usually provided by a base class such as DeviceClass or BusClass.
> + * Every Resettable object must maintain some state tracking the
> + * progress of a reset operation by providing a ResettableState structure.
> + * The functions defined in this module take care of updating the
> + * state of the reset.
> + * The base class implementation of the interface provides this
> + * state and implements the associated method: get_state.
> + *
> + * Concrete object implementations (typically specific devices
> + * such as a UART model) should provide the functions
> + * for the phases.enter, phases.hold and phases.exit methods, which
> + * they can set in their class init function, either directly or
> + * by calling resettable_class_set_parent_phases().
> + * The phase methods are guaranteed to only only ever be called once
> + * for any reset event, in the order 'enter', 'hold', 'exit'.
> + * An object will always move quickly from 'enter' to 'hold'
> + * but might remain in 'hold' for an arbitrary period of time
> + * before eventually reset is deasserted and the 'exit' phase is called.
> + * Object implementations should be prepared for functions handling
> + * inbound connections from other devices (such as qemu_irq handler
> + * functions) to be called at any point during reset after their
> + * 'enter' method has been called.
> + *
> + * Users of a resettable object should not call these methods
> + * directly, but instead use the function resettable_reset().
> + *
> + * @phases.enter: This phase is called when the object enters reset. It
> + * should reset local state of the object, but it must not do anything that
> + * has a side-effect on other objects, such as raising or lowering a qemu_irq
> + * line or reading or writing guest memory. It takes the reset's type as
> + * argument.
> + *
> + * @phases.hold: This phase is called for entry into reset, once every object
> + * in the system which is being reset has had its @phases.enter method called.
> + * At this point devices can do actions that affect other objects.
> + *
> + * @phases.exit: This phase is called when the object leaves the reset state.
> + * Actions affecting other objects are permitted.
> + *
> + * @get_state: Mandatory method which must return a pointer to a
> + * ResettableState.
> + *
> + * @get_transitional_function: transitional method to handle Resettable objects
> + * not yet fully moved to this interface. It will be removed as soon as it is
> + * not needed anymore. This method is optional and may return a pointer to a
> + * function to be used instead of the phases. If the method exists and returns
> + * a non-NULL function pointer then that function is executed as a replacement
> + * of the 'hold' phase method taking the object as argument. The two other phase
> + * methods are not executed.
> + *
> + * @child_foreach: Executes a given callback on every Resettable child. Child
> + * in this context means a child in the qbus tree, so the children of a qbus
> + * are the devices on it, and the children of a device are all the buses it
> + * owns. This is not the same as the QOM object hierarchy. The function takes
> + * additional opaque and ResetType arguments which must be passed unmodified to
> + * the callback.
> + */
> +typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
> +typedef void (*ResettableHoldPhase)(Object *obj);
> +typedef void (*ResettableExitPhase)(Object *obj);
> +typedef ResettableState * (*ResettableGetState)(Object *obj);
> +typedef void (*ResettableTrFunction)(Object *obj);
> +typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
> +typedef void (*ResettableChildCallback)(Object *, void *opaque,
> +                                        ResetType type);
> +typedef void (*ResettableChildForeach)(Object *obj,
> +                                       ResettableChildCallback cb,
> +                                       void *opaque, ResetType type);
> +typedef struct ResettablePhases {
> +    ResettableEnterPhase enter;
> +    ResettableHoldPhase hold;
> +    ResettableExitPhase exit;
> +} ResettablePhases;
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    /* Phase methods */
> +    ResettablePhases phases;
> +
> +    /* State access method */
> +    ResettableGetState get_state;
> +
> +    /* Transitional method for legacy reset compatibility */
> +    ResettableGetTrFunction get_transitional_function;
> +
> +    /* Hierarchy handling method */
> +    ResettableChildForeach child_foreach;
> +} ResettableClass;
> +
> +/**
> + * ResettableState:
> + * Structure holding reset related state. The fields should not be accessed
> + * directly; the definition is here to allow further inclusion into other
> + * objects.
> + *
> + * @count: Number of reset level the object is into. It is incremented when
> + * the reset operation starts and decremented when it finishes.

Maybe you can add this comment and the variable in patch 5/11, that 
would make patch 5 easier to review.

> + * @hold_phase_pending: flag which indicates that we need to invoke the 'hold'
> + * phase handler for this object.
> + * @exit_phase_in_progress: true if we are currently in the exit phase
> + */
> +struct ResettableState {
> +    uint32_t count;

Maybe simply 'unsigned'?

> +    bool hold_phase_pending;
> +    bool exit_phase_in_progress;
> +};
> +
> +/**
> + * resettable_reset:
> + * Trigger a reset on an object @obj of type @type. @obj must implement
> + * Resettable interface.
> + *
> + * Calling this function is equivalent to calling @resettable_assert_reset()
> + * then @resettable_release_reset().
> + */
> +void resettable_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_assert_reset:
> + * Put an object @obj into reset. @obj must implement Resettable interface.
> + *
> + * @resettable_release_reset() must eventually be called after this call.
> + * There must be one call to @resettable_release_reset() per call of
> + * @resettable_assert_reset(), with the same type argument.
> + *
> + * NOTE: Until support for migration is added, the @resettable_release_reset()
> + * must not be delayed. It must occur just after @resettable_assert_reset() so
> + * that migration cannot be triggered in between. Prefer using
> + * @resettable_reset() for now.
> + */
> +void resettable_assert_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_release_reset:
> + * Release the object @obj from reset. @obj must implement Resettable interface.
> + *
> + * See @resettable_assert_reset() description for details.
> + */
> +void resettable_release_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_is_in_reset:
> + * Return true if @obj is under reset.
> + *
> + * @obj must implement Resettable interface.
> + */
> +bool resettable_is_in_reset(Object *obj);
> +
> +/**
> + * resettable_class_set_parent_phases:
> + *
> + * Save @rc current reset phases into @parent_phases and override @rc phases
> + * by the given new methods (@enter, @hold and @exit).
> + * Each phase is overridden only if the new one is not NULL allowing to
> + * override a subset of phases.
> + */
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableEnterPhase enter,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases);
> +
> +#endif
> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
> new file mode 100644
> index 0000000000..9133208487
> --- /dev/null
> +++ b/hw/core/resettable.c
> @@ -0,0 +1,238 @@
> +/*
> + * Resettable interface.
> + *
> + * Copyright (c) 2019 GreenSocs SAS
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "hw/resettable.h"
> +#include "trace.h"
> +
> +/**
> + * resettable_phase_enter/hold/exit:
> + * Function executing a phase recursively in a resettable object and its
> + * children.
> + */
> +static void resettable_phase_enter(Object *obj, void *opaque, ResetType type);
> +static void resettable_phase_hold(Object *obj, void *opaque, ResetType type);
> +static void resettable_phase_exit(Object *obj, void *opaque, ResetType type);
> +
> +/**
> + * enter_phase_in_progress:
> + * True if we are currently in reset enter phase.
> + *
> + * Note: This flag is only used to guarantee (using asserts) that the reset
> + * API is used correctly. We can use a global variable because we rely on the
> + * iothread mutex to ensure only one reset operation is in a progress at a
> + * given time.
> + */
> +static bool enter_phase_in_progress;
> +
> +void resettable_reset(Object *obj, ResetType type)
> +{
> +    trace_resettable_reset(obj, type);
> +    resettable_assert_reset(obj, type);
> +    resettable_release_reset(obj, type);
> +}
> +
> +void resettable_assert_reset(Object *obj, ResetType type)
> +{
> +    /* TODO: change this assert when adding support for other reset types */
> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_assert_begin(obj, type);
> +    assert(!enter_phase_in_progress);
> +
> +    enter_phase_in_progress = true;
> +    resettable_phase_enter(obj, NULL, type);
> +    enter_phase_in_progress = false;
> +
> +    resettable_phase_hold(obj, NULL, type);
> +
> +    trace_resettable_reset_assert_end(obj);
> +}
> +
> +void resettable_release_reset(Object *obj, ResetType type)
> +{
> +    /* TODO: change this assert when adding support for other reset types */
> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_release_begin(obj, type);
> +    assert(!enter_phase_in_progress);
> +
> +    resettable_phase_exit(obj, NULL, type);
> +
> +    trace_resettable_reset_release_end(obj);
> +}
> +
> +bool resettable_is_in_reset(Object *obj)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +
> +    return s->count > 0;
> +}
> +
> +/**
> + * resettable_child_foreach:
> + * helper to avoid checking the existence of the method.
> + */
> +static void resettable_child_foreach(ResettableClass *rc, Object *obj,
> +                                     ResettableChildCallback cb,
> +                                     void *opaque, ResetType type)
> +{
> +    if (rc->child_foreach) {
> +        rc->child_foreach(obj, cb, opaque, type);
> +    }
> +}
> +
> +/**
> + * resettable_get_tr_func:
> + * helper to fetch transitional reset callback if any.
> + */
> +static ResettableTrFunction resettable_get_tr_func(ResettableClass *rc,
> +                                                   Object *obj)
> +{
> +    ResettableTrFunction tr_func = NULL;
> +    if (rc->get_transitional_function) {
> +        tr_func = rc->get_transitional_function(obj);
> +    }
> +    return tr_func;
> +}
> +
> +static void resettable_phase_enter(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +    bool action_needed = false;
> +
> +    /* exit phase has to finish properly before entering back in reset */
> +    assert(!s->exit_phase_in_progress);
> +
> +    trace_resettable_phase_enter_begin(obj, obj_typename, s->count, type);
> +
> +    /* Only take action if we really enter reset for the 1st time. */
> +    /*
> +     * TODO: if adding more ResetType support, some additional checks
> +     * are probably needed here.
> +     */
> +    if (s->count++ == 0) {
> +        action_needed = true;
> +    }
> +    /*
> +     * We limit the count to an arbitrary "big" value. The value is big
> +     * enough not to be triggered normally.
> +     * The assert will stop an infinite loop if there is a cycle in the
> +     * reset tree. The loop goes through resettable_foreach_child below
> +     * which at some point will call us again.
> +     */
> +    assert(s->count <= 50);
> +
> +    /*
> +     * handle the children even if action_needed is at false so that
> +     * child counts are incremented too
> +     */
> +    resettable_child_foreach(rc, obj, resettable_phase_enter, NULL, type);
> +
> +    /* execute enter phase for the object if needed */
> +    if (action_needed) {
> +        trace_resettable_phase_enter_exec(obj, obj_typename, type,
> +                                          !!rc->phases.enter);
> +        if (rc->phases.enter && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.enter(obj, type);
> +        }
> +        s->hold_phase_pending = true;
> +    }
> +    trace_resettable_phase_enter_end(obj, obj_typename, s->count);
> +}
> +
> +static void resettable_phase_hold(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +
> +    /* exit phase has to finish properly before entering back in reset */
> +    assert(!s->exit_phase_in_progress);
> +
> +    trace_resettable_phase_hold_begin(obj, obj_typename, s->count, type);
> +
> +    /* handle children first */
> +    resettable_child_foreach(rc, obj, resettable_phase_hold, NULL, type);
> +
> +    /* exec hold phase */
> +    if (s->hold_phase_pending) {
> +        s->hold_phase_pending = false;
> +        ResettableTrFunction tr_func = resettable_get_tr_func(rc, obj);
> +        trace_resettable_phase_hold_exec(obj, obj_typename, !!rc->phases.hold);
> +        if (tr_func) {
> +            trace_resettable_transitional_function(obj, obj_typename);
> +            tr_func(obj);
> +        } else if (rc->phases.hold) {
> +            rc->phases.hold(obj);
> +        }
> +    }
> +    trace_resettable_phase_hold_end(obj, obj_typename, s->count);
> +}
> +
> +static void resettable_phase_exit(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +
> +    assert(!s->exit_phase_in_progress);
> +    trace_resettable_phase_exit_begin(obj, obj_typename, s->count, type);
> +
> +    /* exit_phase_in_progress ensures this phase is 'atomic' */
> +    s->exit_phase_in_progress = true;
> +    resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type);
> +
> +    assert(s->count > 0);
> +    if (s->count == 1) {
> +        trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit);
> +        if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.exit(obj);
> +        }
> +        s->count = 0;
> +    }
> +    s->exit_phase_in_progress = false;
> +    trace_resettable_phase_exit_end(obj, obj_typename, s->count);
> +}
> +
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableEnterPhase enter,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases)
> +{
> +    *parent_phases = rc->phases;
> +    if (enter) {
> +        rc->phases.enter = enter;
> +    }
> +    if (hold) {
> +        rc->phases.hold = hold;
> +    }
> +    if (exit) {
> +        rc->phases.exit = exit;
> +    }
> +}
> +
> +static const TypeInfo resettable_interface_info = {
> +    .name       = TYPE_RESETTABLE_INTERFACE,
> +    .parent     = TYPE_INTERFACE,
> +    .class_size = sizeof(ResettableClass),
> +};
> +
> +static void reset_register_types(void)
> +{
> +    type_register_static(&resettable_interface_info);
> +}
> +
> +type_init(reset_register_types)
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 0edd9e635d..1709a122d4 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,6 +1,7 @@
>   # core qdev-related obj files, also used by *-user:
>   common-obj-y += qdev.o qdev-properties.o
>   common-obj-y += bus.o reset.o
> +common-obj-y += resettable.o
>   common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>   common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>   # irq.o needed for qdev GPIO handling:
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index a375aa88a4..a2e43f1120 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -9,3 +9,20 @@ qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>   qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>   qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>   qdev_update_parent_bus(void *obj, const char *objtype, void *oldp, const char *oldptype, void *newp, const char *newptype) "obj=%p(%s) old_parent=%p(%s) new_parent=%p(%s)"
> +
> +# resettable.c
> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_assert_end(void *obj) "obj=%p"
> +resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
> +resettable_reset_release_end(void *obj) "obj=%p"
> +resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_enter_exec(void *obj, const char *objtype, int type, int has_method) "obj=%p(%s) type=%d method=%d"
> +resettable_phase_enter_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_phase_hold_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_hold_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
> +resettable_phase_hold_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_phase_exit_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
> +resettable_phase_exit_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
> +resettable_phase_exit_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> +resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"
>

On 1/18/20 7:35 AM, Philippe Mathieu-Daudé wrote:
> On 1/15/20 1:36 PM, Damien Hedde wrote:
>> This commit defines an interface allowing multi-phase reset. This aims
>> to solve a problem of the actual single-phase reset (built in
>> DeviceClass and BusClass): reset behavior is dependent on the order
>> in which reset handlers are called. In particular doing external
>> side-effect (like setting an qemu_irq) is problematic because receiving
>> object may not be reset yet.
>>
>> The Resettable interface divides the reset in 3 well defined phases.
>> To reset an object tree, all 1st phases are executed then all 2nd then
>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>> description. The interface defines 3 phases to let the future
>> possibility of holding an object into reset for some time.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface. A mechanism is provided
>> to allow executing a transitional reset handler in place of the 2nd
>> phase which is executed in children-then-parent order inside a tree.
>> This will allow to transition devices and buses smoothly while
>> keeping the exact current qdev/qbus reset behavior for now.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> v7 update: un-nest struct ResettablePhases
>> ---
>>   Makefile.objs           |   1 +
>>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>   hw/core/Makefile.objs   |   1 +
>>   hw/core/trace-events    |  17 +++
>>   5 files changed, 468 insertions(+)
>>   create mode 100644 include/hw/resettable.h
>>   create mode 100644 hw/core/resettable.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 7c1e50f9d6..9752d549b4 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>>   trace-events-subdirs += net
>>   trace-events-subdirs += ui
>>   endif
>> +trace-events-subdirs += hw/core
>>   trace-events-subdirs += hw/display
>>   trace-events-subdirs += qapi
>>   trace-events-subdirs += qom
>> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
>> new file mode 100644
>> index 0000000000..58b3df4c22
>> --- /dev/null
>> +++ b/include/hw/resettable.h
>> @@ -0,0 +1,211 @@
>> +/*
>> + * Resettable interface header.
>> + *
>> + * Copyright (c) 2019 GreenSocs SAS
>> + *
>> + * Authors:
>> + *   Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef HW_RESETTABLE_H
>> +#define HW_RESETTABLE_H
>> +
>> +#include "qom/object.h"
>> +
>> +#define TYPE_RESETTABLE_INTERFACE "resettable"
>> +
>> +#define RESETTABLE_CLASS(class) \
>> +    OBJECT_CLASS_CHECK(ResettableClass, (class),
>> TYPE_RESETTABLE_INTERFACE)
>> +
>> +#define RESETTABLE_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
>> +
>> +typedef struct ResettableState ResettableState;
>> +
>> +/**
>> + * ResetType:
>> + * Types of reset.
>> + *
>> + * + Cold: reset resulting from a power cycle of the object.
>> + *
>> + * TODO: Support has to be added to handle more types. In particular,
>> + * ResettableState structure needs to be expanded.
>> + */
>> +typedef enum ResetType {
>> +    RESET_TYPE_COLD,
>> +} ResetType;
>> +
>> +/*
>> + * ResettableClass:
>> + * Interface for resettable objects.
>> + *
>> + * See docs/devel/reset.rst for more detailed information about how
>> QEMU models
>> + * reset. This whole API must only be used when holding the iothread
>> mutex.
>> + *
>> + * All objects which can be reset must implement this interface;
>> + * it is usually provided by a base class such as DeviceClass or
>> BusClass.
>> + * Every Resettable object must maintain some state tracking the
>> + * progress of a reset operation by providing a ResettableState
>> structure.
>> + * The functions defined in this module take care of updating the
>> + * state of the reset.
>> + * The base class implementation of the interface provides this
>> + * state and implements the associated method: get_state.
>> + *
>> + * Concrete object implementations (typically specific devices
>> + * such as a UART model) should provide the functions
>> + * for the phases.enter, phases.hold and phases.exit methods, which
>> + * they can set in their class init function, either directly or
>> + * by calling resettable_class_set_parent_phases().
>> + * The phase methods are guaranteed to only only ever be called once
>> + * for any reset event, in the order 'enter', 'hold', 'exit'.
>> + * An object will always move quickly from 'enter' to 'hold'
>> + * but might remain in 'hold' for an arbitrary period of time
>> + * before eventually reset is deasserted and the 'exit' phase is called.
>> + * Object implementations should be prepared for functions handling
>> + * inbound connections from other devices (such as qemu_irq handler
>> + * functions) to be called at any point during reset after their
>> + * 'enter' method has been called.
>> + *
>> + * Users of a resettable object should not call these methods
>> + * directly, but instead use the function resettable_reset().
>> + *
>> + * @phases.enter: This phase is called when the object enters reset. It
>> + * should reset local state of the object, but it must not do
>> anything that
>> + * has a side-effect on other objects, such as raising or lowering a
>> qemu_irq
>> + * line or reading or writing guest memory. It takes the reset's type as
>> + * argument.
>> + *
>> + * @phases.hold: This phase is called for entry into reset, once
>> every object
>> + * in the system which is being reset has had its @phases.enter
>> method called.
>> + * At this point devices can do actions that affect other objects.
>> + *
>> + * @phases.exit: This phase is called when the object leaves the
>> reset state.
>> + * Actions affecting other objects are permitted.
>> + *
>> + * @get_state: Mandatory method which must return a pointer to a
>> + * ResettableState.
>> + *
>> + * @get_transitional_function: transitional method to handle
>> Resettable objects
>> + * not yet fully moved to this interface. It will be removed as soon
>> as it is
>> + * not needed anymore. This method is optional and may return a
>> pointer to a
>> + * function to be used instead of the phases. If the method exists
>> and returns
>> + * a non-NULL function pointer then that function is executed as a
>> replacement
>> + * of the 'hold' phase method taking the object as argument. The two
>> other phase
>> + * methods are not executed.
>> + *
>> + * @child_foreach: Executes a given callback on every Resettable
>> child. Child
>> + * in this context means a child in the qbus tree, so the children of
>> a qbus
>> + * are the devices on it, and the children of a device are all the
>> buses it
>> + * owns. This is not the same as the QOM object hierarchy. The
>> function takes
>> + * additional opaque and ResetType arguments which must be passed
>> unmodified to
>> + * the callback.
>> + */
>> +typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
>> +typedef void (*ResettableHoldPhase)(Object *obj);
>> +typedef void (*ResettableExitPhase)(Object *obj);
>> +typedef ResettableState * (*ResettableGetState)(Object *obj);
>> +typedef void (*ResettableTrFunction)(Object *obj);
>> +typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
>> +typedef void (*ResettableChildCallback)(Object *, void *opaque,
>> +                                        ResetType type);
>> +typedef void (*ResettableChildForeach)(Object *obj,
>> +                                       ResettableChildCallback cb,
>> +                                       void *opaque, ResetType type);
>> +typedef struct ResettablePhases {
>> +    ResettableEnterPhase enter;
>> +    ResettableHoldPhase hold;
>> +    ResettableExitPhase exit;
>> +} ResettablePhases;
>> +typedef struct ResettableClass {
>> +    InterfaceClass parent_class;
>> +
>> +    /* Phase methods */
>> +    ResettablePhases phases;
>> +
>> +    /* State access method */
>> +    ResettableGetState get_state;
>> +
>> +    /* Transitional method for legacy reset compatibility */
>> +    ResettableGetTrFunction get_transitional_function;
>> +
>> +    /* Hierarchy handling method */
>> +    ResettableChildForeach child_foreach;
>> +} ResettableClass;
>> +
>> +/**
>> + * ResettableState:
>> + * Structure holding reset related state. The fields should not be
>> accessed
>> + * directly; the definition is here to allow further inclusion into
>> other
>> + * objects.
>> + *
>> + * @count: Number of reset level the object is into. It is
>> incremented when
>> + * the reset operation starts and decremented when it finishes.
>> + * @hold_phase_pending: flag which indicates that we need to invoke
>> the 'hold'
>> + * phase handler for this object.
>> + * @exit_phase_in_progress: true if we are currently in the exit phase
>> + */
>> +struct ResettableState {
>> +    uint32_t count;
>> +    bool hold_phase_pending;
>> +    bool exit_phase_in_progress;
>> +};
>> +
>> +/**
>> + * resettable_reset:
>> + * Trigger a reset on an object @obj of type @type. @obj must implement
>> + * Resettable interface.
>> + *
>> + * Calling this function is equivalent to calling
>> @resettable_assert_reset()
>> + * then @resettable_release_reset().
>> + */
>> +void resettable_reset(Object *obj, ResetType type);
>> +
>> +/**
>> + * resettable_assert_reset:
>> + * Put an object @obj into reset. @obj must implement Resettable
>> interface.
>> + *
>> + * @resettable_release_reset() must eventually be called after this
>> call.
>> + * There must be one call to @resettable_release_reset() per call of
>> + * @resettable_assert_reset(), with the same type argument.
>> + *
>> + * NOTE: Until support for migration is added, the
>> @resettable_release_reset()
>> + * must not be delayed. It must occur just after
>> @resettable_assert_reset() so
>> + * that migration cannot be triggered in between. Prefer using
>> + * @resettable_reset() for now.
>> + */
>> +void resettable_assert_reset(Object *obj, ResetType type);
>> +
>> +/**
>> + * resettable_release_reset:
>> + * Release the object @obj from reset. @obj must implement Resettable
>> interface.
>> + *
>> + * See @resettable_assert_reset() description for details.
>> + */
>> +void resettable_release_reset(Object *obj, ResetType type);
>> +
>> +/**
>> + * resettable_is_in_reset:
>> + * Return true if @obj is under reset.
>> + *
>> + * @obj must implement Resettable interface.
>> + */
>> +bool resettable_is_in_reset(Object *obj);
>> +
>> +/**
>> + * resettable_class_set_parent_phases:
>> + *
>> + * Save @rc current reset phases into @parent_phases and override @rc
>> phases
>> + * by the given new methods (@enter, @hold and @exit).
>> + * Each phase is overridden only if the new one is not NULL allowing to
>> + * override a subset of phases.
>> + */
>> +void resettable_class_set_parent_phases(ResettableClass *rc,
>> +                                        ResettableEnterPhase enter,
>> +                                        ResettableHoldPhase hold,
>> +                                        ResettableExitPhase exit,
>> +                                        ResettablePhases
>> *parent_phases);
>> +
>> +#endif
>> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
>> new file mode 100644
>> index 0000000000..9133208487
>> --- /dev/null
>> +++ b/hw/core/resettable.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Resettable interface.
>> + *
>> + * Copyright (c) 2019 GreenSocs SAS
>> + *
>> + * Authors:
>> + *   Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/module.h"
>> +#include "hw/resettable.h"
>> +#include "trace.h"
>> +
>> +/**
>> + * resettable_phase_enter/hold/exit:
>> + * Function executing a phase recursively in a resettable object and its
>> + * children.
>> + */
>> +static void resettable_phase_enter(Object *obj, void *opaque,
>> ResetType type);
>> +static void resettable_phase_hold(Object *obj, void *opaque,
>> ResetType type);
>> +static void resettable_phase_exit(Object *obj, void *opaque,
>> ResetType type);
>> +
>> +/**
>> + * enter_phase_in_progress:
>> + * True if we are currently in reset enter phase.
>> + *
>> + * Note: This flag is only used to guarantee (using asserts) that the
>> reset
>> + * API is used correctly. We can use a global variable because we
>> rely on the
>> + * iothread mutex to ensure only one reset operation is in a progress
>> at a
>> + * given time.
>> + */
>> +static bool enter_phase_in_progress;
>> +
>> +void resettable_reset(Object *obj, ResetType type)
>> +{
>> +    trace_resettable_reset(obj, type);
>> +    resettable_assert_reset(obj, type);
>> +    resettable_release_reset(obj, type);
>> +}
>> +
>> +void resettable_assert_reset(Object *obj, ResetType type)
>> +{
>> +    /* TODO: change this assert when adding support for other reset
>> types */
>> +    assert(type == RESET_TYPE_COLD);
>> +    trace_resettable_reset_assert_begin(obj, type);
>> +    assert(!enter_phase_in_progress);
>> +
>> +    enter_phase_in_progress = true;
>> +    resettable_phase_enter(obj, NULL, type);
>> +    enter_phase_in_progress = false;
>> +
>> +    resettable_phase_hold(obj, NULL, type);
>> +
>> +    trace_resettable_reset_assert_end(obj);
>> +}
>> +
>> +void resettable_release_reset(Object *obj, ResetType type)
>> +{
>> +    /* TODO: change this assert when adding support for other reset
>> types */
>> +    assert(type == RESET_TYPE_COLD);
>> +    trace_resettable_reset_release_begin(obj, type);
>> +    assert(!enter_phase_in_progress);
>> +
>> +    resettable_phase_exit(obj, NULL, type);
>> +
>> +    trace_resettable_reset_release_end(obj);
>> +}
>> +
>> +bool resettable_is_in_reset(Object *obj)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResettableState *s = rc->get_state(obj);
>> +
>> +    return s->count > 0;
>> +}
>> +
>> +/**
>> + * resettable_child_foreach:
>> + * helper to avoid checking the existence of the method.
>> + */
>> +static void resettable_child_foreach(ResettableClass *rc, Object *obj,
>> +                                     ResettableChildCallback cb,
>> +                                     void *opaque, ResetType type)
>> +{
>> +    if (rc->child_foreach) {
>> +        rc->child_foreach(obj, cb, opaque, type);
>> +    }
>> +}
>> +
>> +/**
>> + * resettable_get_tr_func:
>> + * helper to fetch transitional reset callback if any.
>> + */
>> +static ResettableTrFunction resettable_get_tr_func(ResettableClass *rc,
>> +                                                   Object *obj)
>> +{
>> +    ResettableTrFunction tr_func = NULL;
>> +    if (rc->get_transitional_function) {
>> +        tr_func = rc->get_transitional_function(obj);
>> +    }
>> +    return tr_func;
>> +}
>> +
>> +static void resettable_phase_enter(Object *obj, void *opaque,
>> ResetType type)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResettableState *s = rc->get_state(obj);
>> +    const char *obj_typename = object_get_typename(obj);
>> +    bool action_needed = false;
>> +
>> +    /* exit phase has to finish properly before entering back in
>> reset */
>> +    assert(!s->exit_phase_in_progress);
>> +
>> +    trace_resettable_phase_enter_begin(obj, obj_typename, s->count,
>> type);
>> +
>> +    /* Only take action if we really enter reset for the 1st time. */
>> +    /*
>> +     * TODO: if adding more ResetType support, some additional checks
>> +     * are probably needed here.
>> +     */
>> +    if (s->count++ == 0) {
>> +        action_needed = true;
>> +    }
>> +    /*
>> +     * We limit the count to an arbitrary "big" value. The value is big
>> +     * enough not to be triggered normally.
>> +     * The assert will stop an infinite loop if there is a cycle in the
>> +     * reset tree. The loop goes through resettable_foreach_child below
>> +     * which at some point will call us again.
>> +     */
>> +    assert(s->count <= 50);
>> +
>> +    /*
>> +     * handle the children even if action_needed is at false so that
>> +     * child counts are incremented too
>> +     */
>> +    resettable_child_foreach(rc, obj, resettable_phase_enter, NULL,
>> type);
>> +
>> +    /* execute enter phase for the object if needed */
>> +    if (action_needed) {
>> +        trace_resettable_phase_enter_exec(obj, obj_typename, type,
>> +                                          !!rc->phases.enter);
>> +        if (rc->phases.enter && !resettable_get_tr_func(rc, obj)) {
>> +            rc->phases.enter(obj, type);
>> +        }
>> +        s->hold_phase_pending = true;
>> +    }
>> +    trace_resettable_phase_enter_end(obj, obj_typename, s->count);
>> +}
>> +
>> +static void resettable_phase_hold(Object *obj, void *opaque,
>> ResetType type)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResettableState *s = rc->get_state(obj);
>> +    const char *obj_typename = object_get_typename(obj);
>> +
>> +    /* exit phase has to finish properly before entering back in
>> reset */
>> +    assert(!s->exit_phase_in_progress);
>> +
>> +    trace_resettable_phase_hold_begin(obj, obj_typename, s->count,
>> type);
>> +
>> +    /* handle children first */
>> +    resettable_child_foreach(rc, obj, resettable_phase_hold, NULL,
>> type);
>> +
>> +    /* exec hold phase */
>> +    if (s->hold_phase_pending) {
>> +        s->hold_phase_pending = false;
>> +        ResettableTrFunction tr_func = resettable_get_tr_func(rc, obj);
>> +        trace_resettable_phase_hold_exec(obj, obj_typename,
>> !!rc->phases.hold);
>> +        if (tr_func) {
>> +            trace_resettable_transitional_function(obj, obj_typename);
>> +            tr_func(obj);
>> +        } else if (rc->phases.hold) {
>> +            rc->phases.hold(obj);
>> +        }
>> +    }
>> +    trace_resettable_phase_hold_end(obj, obj_typename, s->count);
>> +}
>> +
>> +static void resettable_phase_exit(Object *obj, void *opaque,
>> ResetType type)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResettableState *s = rc->get_state(obj);
>> +    const char *obj_typename = object_get_typename(obj);
>> +
>> +    assert(!s->exit_phase_in_progress);
>> +    trace_resettable_phase_exit_begin(obj, obj_typename, s->count,
>> type);
>> +
>> +    /* exit_phase_in_progress ensures this phase is 'atomic' */
>> +    s->exit_phase_in_progress = true;
>> +    resettable_child_foreach(rc, obj, resettable_phase_exit, NULL,
>> type);
>> +
>> +    assert(s->count > 0);
>> +    if (s->count == 1) {
>> +        trace_resettable_phase_exit_exec(obj, obj_typename,
>> !!rc->phases.exit);
>> +        if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) {
>> +            rc->phases.exit(obj);
>> +        }
>> +        s->count = 0;
>> +    }
>> +    s->exit_phase_in_progress = false;
>> +    trace_resettable_phase_exit_end(obj, obj_typename, s->count);
>> +}
>> +
>> +void resettable_class_set_parent_phases(ResettableClass *rc,
>> +                                        ResettableEnterPhase enter,
>> +                                        ResettableHoldPhase hold,
>> +                                        ResettableExitPhase exit,
>> +                                        ResettablePhases *parent_phases)
>> +{
>> +    *parent_phases = rc->phases;
>> +    if (enter) {
> 
> Why care about checking if handlers are not NULL? It is not like you are
> overwriting a pointer previously set.

It may be previously set. You may just want to override one of the 3
phases and inherit the 2 others from the parent class version.

> 
> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> +        rc->phases.enter = enter;
>> +    }
>> +    if (hold) {
>> +        rc->phases.hold = hold;
>> +    }
>> +    if (exit) {
>> +        rc->phases.exit = exit;
>> +    }
>> +}
>> +
>> +static const TypeInfo resettable_interface_info = {
>> +    .name       = TYPE_RESETTABLE_INTERFACE,
>> +    .parent     = TYPE_INTERFACE,
>> +    .class_size = sizeof(ResettableClass),
>> +};
>> +
>> +static void reset_register_types(void)
>> +{
>> +    type_register_static(&resettable_interface_info);
>> +}
>> +
>> +type_init(reset_register_types)
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index 0edd9e635d..1709a122d4 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -1,6 +1,7 @@
>>   # core qdev-related obj files, also used by *-user:
>>   common-obj-y += qdev.o qdev-properties.o
>>   common-obj-y += bus.o reset.o
>> +common-obj-y += resettable.o
>>   common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>>   common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>>   # irq.o needed for qdev GPIO handling:
>> diff --git a/hw/core/trace-events b/hw/core/trace-events
>> index a375aa88a4..a2e43f1120 100644
>> --- a/hw/core/trace-events
>> +++ b/hw/core/trace-events
>> @@ -9,3 +9,20 @@ qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>>   qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>>   qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>>   qdev_update_parent_bus(void *obj, const char *objtype, void *oldp,
>> const char *oldptype, void *newp, const char *newptype) "obj=%p(%s)
>> old_parent=%p(%s) new_parent=%p(%s)"
>> +
>> +# resettable.c
>> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_assert_end(void *obj) "obj=%p"
>> +resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_release_end(void *obj) "obj=%p"
>> +resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_enter_exec(void *obj, const char *objtype, int type,
>> int has_method) "obj=%p(%s) type=%d method=%d"
>> +resettable_phase_enter_end(void *obj, const char *objtype, uint32_t
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_phase_hold_begin(void *obj, const char *objtype, uint32_t
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_hold_exec(void *obj, const char *objtype, int
>> has_method) "obj=%p(%s) method=%d"
>> +resettable_phase_hold_end(void *obj, const char *objtype, uint32_t
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_phase_exit_begin(void *obj, const char *objtype, uint32_t
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_exit_exec(void *obj, const char *objtype, int
>> has_method) "obj=%p(%s) method=%d"
>> +resettable_phase_exit_end(void *obj, const char *objtype, uint32_t
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_transitional_function(void *obj, const char *objtype)
>> "obj=%p(%s)"
>>
>

On 1/18/20 7:42 AM, Philippe Mathieu-Daudé wrote:
> On 1/15/20 1:36 PM, Damien Hedde wrote:
>> This commit defines an interface allowing multi-phase reset. This aims
>> to solve a problem of the actual single-phase reset (built in
>> DeviceClass and BusClass): reset behavior is dependent on the order
>> in which reset handlers are called. In particular doing external
>> side-effect (like setting an qemu_irq) is problematic because receiving
>> object may not be reset yet.
>>
>> The Resettable interface divides the reset in 3 well defined phases.
>> To reset an object tree, all 1st phases are executed then all 2nd then
>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>> description. The interface defines 3 phases to let the future
>> possibility of holding an object into reset for some time.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface. A mechanism is provided
>> to allow executing a transitional reset handler in place of the 2nd
>> phase which is executed in children-then-parent order inside a tree.
>> This will allow to transition devices and buses smoothly while
>> keeping the exact current qdev/qbus reset behavior for now.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> v7 update: un-nest struct ResettablePhases
>> ---
>>   Makefile.objs           |   1 +
>>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>   hw/core/Makefile.objs   |   1 +
>>   hw/core/trace-events    |  17 +++
>>   5 files changed, 468 insertions(+)
>>   create mode 100644 include/hw/resettable.h
>>   create mode 100644 hw/core/resettable.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 7c1e50f9d6..9752d549b4 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>>   trace-events-subdirs += net
>>   trace-events-subdirs += ui
>>   endif
>> +trace-events-subdirs += hw/core
>>   trace-events-subdirs += hw/display
>>   trace-events-subdirs += qapi
>>   trace-events-subdirs += qom
>> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
>> new file mode 100644
>> index 0000000000..58b3df4c22
>> --- /dev/null
>> +++ b/include/hw/resettable.h
>> @@ -0,0 +1,211 @@
>> +/*
>> + * Resettable interface header.
>> + *
>> + * Copyright (c) 2019 GreenSocs SAS
>> + *
>> + * Authors:
>> + *   Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef HW_RESETTABLE_H
>> +#define HW_RESETTABLE_H
>> +
>> +#include "qom/object.h"
>> +
>> +#define TYPE_RESETTABLE_INTERFACE "resettable"
>> +
>> +#define RESETTABLE_CLASS(class) \
>> +    OBJECT_CLASS_CHECK(ResettableClass, (class),
>> TYPE_RESETTABLE_INTERFACE)
>> +
>> +#define RESETTABLE_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
>> +
>> +typedef struct ResettableState ResettableState;
>> +
>> +/**
>> + * ResetType:
>> + * Types of reset.
>> + *
>> + * + Cold: reset resulting from a power cycle of the object.
>> + *
>> + * TODO: Support has to be added to handle more types. In particular,
>> + * ResettableState structure needs to be expanded.
>> + */
>> +typedef enum ResetType {
>> +    RESET_TYPE_COLD,
>> +} ResetType;
>> +
>> +/*
>> + * ResettableClass:
>> + * Interface for resettable objects.
>> + *
>> + * See docs/devel/reset.rst for more detailed information about how
>> QEMU models
>> + * reset. This whole API must only be used when holding the iothread
>> mutex.
>> + *
>> + * All objects which can be reset must implement this interface;
>> + * it is usually provided by a base class such as DeviceClass or
>> BusClass.
>> + * Every Resettable object must maintain some state tracking the
>> + * progress of a reset operation by providing a ResettableState
>> structure.
>> + * The functions defined in this module take care of updating the
>> + * state of the reset.
>> + * The base class implementation of the interface provides this
>> + * state and implements the associated method: get_state.
>> + *
>> + * Concrete object implementations (typically specific devices
>> + * such as a UART model) should provide the functions
>> + * for the phases.enter, phases.hold and phases.exit methods, which
>> + * they can set in their class init function, either directly or
>> + * by calling resettable_class_set_parent_phases().
>> + * The phase methods are guaranteed to only only ever be called once
>> + * for any reset event, in the order 'enter', 'hold', 'exit'.
>> + * An object will always move quickly from 'enter' to 'hold'
>> + * but might remain in 'hold' for an arbitrary period of time
>> + * before eventually reset is deasserted and the 'exit' phase is called.
>> + * Object implementations should be prepared for functions handling
>> + * inbound connections from other devices (such as qemu_irq handler
>> + * functions) to be called at any point during reset after their
>> + * 'enter' method has been called.
>> + *
>> + * Users of a resettable object should not call these methods
>> + * directly, but instead use the function resettable_reset().
>> + *
>> + * @phases.enter: This phase is called when the object enters reset. It
>> + * should reset local state of the object, but it must not do
>> anything that
>> + * has a side-effect on other objects, such as raising or lowering a
>> qemu_irq
>> + * line or reading or writing guest memory. It takes the reset's type as
>> + * argument.
>> + *
>> + * @phases.hold: This phase is called for entry into reset, once
>> every object
>> + * in the system which is being reset has had its @phases.enter
>> method called.
>> + * At this point devices can do actions that affect other objects.
>> + *
>> + * @phases.exit: This phase is called when the object leaves the
>> reset state.
>> + * Actions affecting other objects are permitted.
>> + *
>> + * @get_state: Mandatory method which must return a pointer to a
>> + * ResettableState.
>> + *
>> + * @get_transitional_function: transitional method to handle
>> Resettable objects
>> + * not yet fully moved to this interface. It will be removed as soon
>> as it is
>> + * not needed anymore. This method is optional and may return a
>> pointer to a
>> + * function to be used instead of the phases. If the method exists
>> and returns
>> + * a non-NULL function pointer then that function is executed as a
>> replacement
>> + * of the 'hold' phase method taking the object as argument. The two
>> other phase
>> + * methods are not executed.
>> + *
>> + * @child_foreach: Executes a given callback on every Resettable
>> child. Child
>> + * in this context means a child in the qbus tree, so the children of
>> a qbus
>> + * are the devices on it, and the children of a device are all the
>> buses it
>> + * owns. This is not the same as the QOM object hierarchy. The
>> function takes
>> + * additional opaque and ResetType arguments which must be passed
>> unmodified to
>> + * the callback.
>> + */
>> +typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
>> +typedef void (*ResettableHoldPhase)(Object *obj);
>> +typedef void (*ResettableExitPhase)(Object *obj);
>> +typedef ResettableState * (*ResettableGetState)(Object *obj);
>> +typedef void (*ResettableTrFunction)(Object *obj);
>> +typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
>> +typedef void (*ResettableChildCallback)(Object *, void *opaque,
>> +                                        ResetType type);
>> +typedef void (*ResettableChildForeach)(Object *obj,
>> +                                       ResettableChildCallback cb,
>> +                                       void *opaque, ResetType type);
>> +typedef struct ResettablePhases {
>> +    ResettableEnterPhase enter;
>> +    ResettableHoldPhase hold;
>> +    ResettableExitPhase exit;
>> +} ResettablePhases;
>> +typedef struct ResettableClass {
>> +    InterfaceClass parent_class;
>> +
>> +    /* Phase methods */
>> +    ResettablePhases phases;
>> +
>> +    /* State access method */
>> +    ResettableGetState get_state;
>> +
>> +    /* Transitional method for legacy reset compatibility */
>> +    ResettableGetTrFunction get_transitional_function;
>> +
>> +    /* Hierarchy handling method */
>> +    ResettableChildForeach child_foreach;
>> +} ResettableClass;
>> +
>> +/**
>> + * ResettableState:
>> + * Structure holding reset related state. The fields should not be
>> accessed
>> + * directly; the definition is here to allow further inclusion into
>> other
>> + * objects.
>> + *
>> + * @count: Number of reset level the object is into. It is
>> incremented when
>> + * the reset operation starts and decremented when it finishes.
> 
> Maybe you can add this comment and the variable in patch 5/11, that
> would make patch 5 easier to review.

The variable is used in this patch so I cannot do that.

> 
>> + * @hold_phase_pending: flag which indicates that we need to invoke
>> the 'hold'
>> + * phase handler for this object.
>> + * @exit_phase_in_progress: true if we are currently in the exit phase
>> + */
>> +struct ResettableState {
>> +    uint32_t count;
> 
> Maybe simply 'unsigned'?
> 

Ok.

--
Damien

On 1/20/20 10:08 AM, Damien Hedde wrote:
> On 1/18/20 7:42 AM, Philippe Mathieu-Daudé wrote:
>> On 1/15/20 1:36 PM, Damien Hedde wrote:
>>> This commit defines an interface allowing multi-phase reset. This aims
>>> to solve a problem of the actual single-phase reset (built in
>>> DeviceClass and BusClass): reset behavior is dependent on the order
>>> in which reset handlers are called. In particular doing external
>>> side-effect (like setting an qemu_irq) is problematic because receiving
>>> object may not be reset yet.
>>>
>>> The Resettable interface divides the reset in 3 well defined phases.
>>> To reset an object tree, all 1st phases are executed then all 2nd then
>>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>>> description. The interface defines 3 phases to let the future
>>> possibility of holding an object into reset for some time.
>>>
>>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>>> following commits to use this interface. A mechanism is provided
>>> to allow executing a transitional reset handler in place of the 2nd
>>> phase which is executed in children-then-parent order inside a tree.
>>> This will allow to transition devices and buses smoothly while
>>> keeping the exact current qdev/qbus reset behavior for now.
>>>
>>> Documentation will be added in a following commit.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>
>>> v7 update: un-nest struct ResettablePhases
>>> ---
>>>    Makefile.objs           |   1 +
>>>    include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>>    hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>>    hw/core/Makefile.objs   |   1 +
>>>    hw/core/trace-events    |  17 +++
>>>    5 files changed, 468 insertions(+)
>>>    create mode 100644 include/hw/resettable.h
>>>    create mode 100644 hw/core/resettable.c
>>>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 7c1e50f9d6..9752d549b4 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>>>    trace-events-subdirs += net
>>>    trace-events-subdirs += ui
>>>    endif
>>> +trace-events-subdirs += hw/core
>>>    trace-events-subdirs += hw/display
>>>    trace-events-subdirs += qapi
>>>    trace-events-subdirs += qom
>>> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
>>> new file mode 100644
>>> index 0000000000..58b3df4c22
>>> --- /dev/null
>>> +++ b/include/hw/resettable.h
>>> @@ -0,0 +1,211 @@
>>> +/*
>>> + * Resettable interface header.
>>> + *
>>> + * Copyright (c) 2019 GreenSocs SAS
>>> + *
>>> + * Authors:
>>> + *   Damien Hedde
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef HW_RESETTABLE_H
>>> +#define HW_RESETTABLE_H
>>> +
>>> +#include "qom/object.h"
>>> +
>>> +#define TYPE_RESETTABLE_INTERFACE "resettable"
>>> +
>>> +#define RESETTABLE_CLASS(class) \
>>> +    OBJECT_CLASS_CHECK(ResettableClass, (class),
>>> TYPE_RESETTABLE_INTERFACE)
>>> +
>>> +#define RESETTABLE_GET_CLASS(obj) \
>>> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
>>> +
>>> +typedef struct ResettableState ResettableState;
>>> +
>>> +/**
>>> + * ResetType:
>>> + * Types of reset.
>>> + *
>>> + * + Cold: reset resulting from a power cycle of the object.
>>> + *
>>> + * TODO: Support has to be added to handle more types. In particular,
>>> + * ResettableState structure needs to be expanded.
>>> + */
>>> +typedef enum ResetType {
>>> +    RESET_TYPE_COLD,
>>> +} ResetType;
>>> +
>>> +/*
>>> + * ResettableClass:
>>> + * Interface for resettable objects.
>>> + *
>>> + * See docs/devel/reset.rst for more detailed information about how
>>> QEMU models
>>> + * reset. This whole API must only be used when holding the iothread
>>> mutex.
>>> + *
>>> + * All objects which can be reset must implement this interface;
>>> + * it is usually provided by a base class such as DeviceClass or
>>> BusClass.
>>> + * Every Resettable object must maintain some state tracking the
>>> + * progress of a reset operation by providing a ResettableState
>>> structure.
>>> + * The functions defined in this module take care of updating the
>>> + * state of the reset.
>>> + * The base class implementation of the interface provides this
>>> + * state and implements the associated method: get_state.
>>> + *
>>> + * Concrete object implementations (typically specific devices
>>> + * such as a UART model) should provide the functions
>>> + * for the phases.enter, phases.hold and phases.exit methods, which
>>> + * they can set in their class init function, either directly or
>>> + * by calling resettable_class_set_parent_phases().
>>> + * The phase methods are guaranteed to only only ever be called once
>>> + * for any reset event, in the order 'enter', 'hold', 'exit'.
>>> + * An object will always move quickly from 'enter' to 'hold'
>>> + * but might remain in 'hold' for an arbitrary period of time
>>> + * before eventually reset is deasserted and the 'exit' phase is called.
>>> + * Object implementations should be prepared for functions handling
>>> + * inbound connections from other devices (such as qemu_irq handler
>>> + * functions) to be called at any point during reset after their
>>> + * 'enter' method has been called.
>>> + *
>>> + * Users of a resettable object should not call these methods
>>> + * directly, but instead use the function resettable_reset().
>>> + *
>>> + * @phases.enter: This phase is called when the object enters reset. It
>>> + * should reset local state of the object, but it must not do
>>> anything that
>>> + * has a side-effect on other objects, such as raising or lowering a
>>> qemu_irq
>>> + * line or reading or writing guest memory. It takes the reset's type as
>>> + * argument.
>>> + *
>>> + * @phases.hold: This phase is called for entry into reset, once
>>> every object
>>> + * in the system which is being reset has had its @phases.enter
>>> method called.
>>> + * At this point devices can do actions that affect other objects.
>>> + *
>>> + * @phases.exit: This phase is called when the object leaves the
>>> reset state.
>>> + * Actions affecting other objects are permitted.
>>> + *
>>> + * @get_state: Mandatory method which must return a pointer to a
>>> + * ResettableState.
>>> + *
>>> + * @get_transitional_function: transitional method to handle
>>> Resettable objects
>>> + * not yet fully moved to this interface. It will be removed as soon
>>> as it is
>>> + * not needed anymore. This method is optional and may return a
>>> pointer to a
>>> + * function to be used instead of the phases. If the method exists
>>> and returns
>>> + * a non-NULL function pointer then that function is executed as a
>>> replacement
>>> + * of the 'hold' phase method taking the object as argument. The two
>>> other phase
>>> + * methods are not executed.
>>> + *
>>> + * @child_foreach: Executes a given callback on every Resettable
>>> child. Child
>>> + * in this context means a child in the qbus tree, so the children of
>>> a qbus
>>> + * are the devices on it, and the children of a device are all the
>>> buses it
>>> + * owns. This is not the same as the QOM object hierarchy. The
>>> function takes
>>> + * additional opaque and ResetType arguments which must be passed
>>> unmodified to
>>> + * the callback.
>>> + */
>>> +typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
>>> +typedef void (*ResettableHoldPhase)(Object *obj);
>>> +typedef void (*ResettableExitPhase)(Object *obj);
>>> +typedef ResettableState * (*ResettableGetState)(Object *obj);
>>> +typedef void (*ResettableTrFunction)(Object *obj);
>>> +typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
>>> +typedef void (*ResettableChildCallback)(Object *, void *opaque,
>>> +                                        ResetType type);
>>> +typedef void (*ResettableChildForeach)(Object *obj,
>>> +                                       ResettableChildCallback cb,
>>> +                                       void *opaque, ResetType type);
>>> +typedef struct ResettablePhases {
>>> +    ResettableEnterPhase enter;
>>> +    ResettableHoldPhase hold;
>>> +    ResettableExitPhase exit;
>>> +} ResettablePhases;
>>> +typedef struct ResettableClass {
>>> +    InterfaceClass parent_class;
>>> +
>>> +    /* Phase methods */
>>> +    ResettablePhases phases;
>>> +
>>> +    /* State access method */
>>> +    ResettableGetState get_state;
>>> +
>>> +    /* Transitional method for legacy reset compatibility */
>>> +    ResettableGetTrFunction get_transitional_function;
>>> +
>>> +    /* Hierarchy handling method */
>>> +    ResettableChildForeach child_foreach;
>>> +} ResettableClass;
>>> +
>>> +/**
>>> + * ResettableState:
>>> + * Structure holding reset related state. The fields should not be
>>> accessed
>>> + * directly; the definition is here to allow further inclusion into
>>> other
>>> + * objects.
>>> + *
>>> + * @count: Number of reset level the object is into. It is
>>> incremented when
>>> + * the reset operation starts and decremented when it finishes.
>>
>> Maybe you can add this comment and the variable in patch 5/11, that
>> would make patch 5 easier to review.
> 
> The variable is used in this patch so I cannot do that.

Oops you are right o_O ...

>>
>>> + * @hold_phase_pending: flag which indicates that we need to invoke
>>> the 'hold'
>>> + * phase handler for this object.
>>> + * @exit_phase_in_progress: true if we are currently in the exit phase
>>> + */
>>> +struct ResettableState {
>>> +    uint32_t count;
>>
>> Maybe simply 'unsigned'?
>>
> 
> Ok.
> 
> --
> Damien