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

[Damien Hedde]

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 <address@hidden>
>> Reviewed-by: Richard Henderson <address@hidden>
>> ---
>>
>> 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é <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
> 
>> +        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)"
>>
>