On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.he...@greensocs.com> 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.he...@greensocs.com>
> ---
>
> In this patch only a single reset type is supported, but the interface
> allows for more to be defined.
>
> I had some thought about problems which may arise when having multiple
> reset types:
[snip]
Yeah, these all seem right. We clearly need to think a bit
more before we add multiple reset types. Let's get this basic
just-cold-reset in for now and come back to the rest later.
Almost all of my comments below are just grammar/typo fixes
for comments. The only substantives are:
* globals
* copyright/licensing comment needed in the .h file
and they're pretty minor items.
> +/**
> + * enter_phase_in_progress:
> + * Flag telling whether we are currently in an enter phase where side
> + * effects are forbidden. This flag allows us to catch if reset is called
> + * again during during this phase.
> + */
> +static bool enter_phase_in_progress;
This looks weird -- I don't think a global for this works,
because you might have several distinct subtrees of
devices, and be doing reset on them both at once.
I think that we only use this for an assert, though -- is
that right? If so, we could just drop this.
> +void resettable_assert_reset(Object *obj, ResetType type)
> +{
> + assert(!enter_phase_in_progress);
> + /* TODO: change that assert when adding support for other reset types */
I'm not sure which assert this is referring to -- the one above
the comment, or the one below ?
> + assert(type == RESET_TYPE_COLD);
> + trace_resettable_reset_assert_begin(obj, type);
> + 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)
> +{
> + assert(!enter_phase_in_progress);
> + /* TODO: change that assert when adding support for other reset types */
Ditto.
> + assert(type == RESET_TYPE_COLD);
> + trace_resettable_reset_release_begin(obj, type);
> + resettable_phase_exit(obj, NULL, type);
> + trace_resettable_reset_release_end(obj);
> +}
> +
> +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 nominally.
"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
> + * children counts are incremented too
"child counts"
> + */
> + 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_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 ensure this phase is 'atomic' */
"ensures"
> + 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);
> +}
> --- /dev/null
> +++ b/include/hw/resettable.h
> @@ -0,0 +1,199 @@
> +#ifndef HW_RESETTABLE_H
> +#define HW_RESETTABLE_H
> +
All new files, including even small header files, should have
the usual copyright-and-license comment at the top. (Can you
check also whether this needs adding for any other new files the
patchset creates, please?)
> +#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;
> + * @get_transitional_function: transitional method to handle Resettable
> objects
> + * not yet fully moved to this interface. It will be removed as soon as is
> not
"as soon as it is"
> + * needed anymore. This method is optional and may return a pointer to a
> + * function to be used instead of the phases. In case the method exists and
"If the method exists"
> + * returns a non-NULL function pointer; it is executed as a replacement of
> the
"pointer then that function is"
> + * '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.
> + */
> + /* Transitional method for legacy reset compatibility */
> + ResettableGetTrFunction get_transitional_function;
> +
> + /* Hiearchy handling method */
"Hierarchy"
> + ResettableChildForeach child_foreach;
> +} ResettableClass;
> +typedef struct ResettablePhases ResettablePhases;
> +
> +/**
> + * ResettableState:
> + * Structure holding reset related state. The fields should not be accessed
> + * directly, the definition is here to allow further inclusion into other
"directly; the definition"
> + * 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: flag telling if currently in exit phase
"@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 a object @obj of type @type. @obj must implement
"an object"
> + * 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 have to occur just after
> @resettable_assert_reset()
"It must occur"
> + * 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
thanks
-- PMM
