| Message ID | 20191018150630.31099-4-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multi-phase reset mechanism | expand |
On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@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.hedde@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
On 11/29/19 7:32 PM, Peter Maydell wrote: > On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@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.hedde@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. We say that we need to own the iothread mutex for any reset, so global should be ok. Thought, I just checked, it's only mentioned in the documentation not in the header file. I should probably add a comment there too along with the link to the documentation file. If we want to drop the iothread mutex constraint. I think we need to carefully check there is no hidden problem. In particular in hold and exit phases we allow to have external effects like setting gpios and we have no way to control what it provokes. You're right it is just for assert: to avoid any miss-use of the api which could lead to being in bad reset state. So we can indeed drop it. > >> +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 ? It refers to the assert(type == RESET_TYPE_COLD). I added theses because we cannot just add items in the enum to have working multiple reset types. A comment like this will be more clear: /* * TODO: Additional reset types need support in phases handling * functions (resettable_phase_enter/hold/exit()) before allowing more * enum entries. Remove the following assert when it is done. */ > >> + 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. > > > >> --- /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?) I'll do that and fix all the typos Thanks for the review, -- Damien
On Mon, 2 Dec 2019 at 11:07, Damien Hedde <damien.hedde@greensocs.com> wrote: > > > On 11/29/19 7:32 PM, Peter Maydell wrote: > > On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote: > >> +/** > >> + * 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. > > We say that we need to own the iothread mutex for any reset, so global > should be ok. Thought, I just checked, it's only mentioned in the > documentation not in the header file. I should probably add a comment > there too along with the link to the documentation file. Ah, right, I hadn't considered that the mutex is effectively restricting to only a single reset happening at once. If you want to keep the asserts you can, if you add a comment noting that these globals are (a) only for asserts and (b) OK because we rely on the iothread mutex to ensure that only one reset operation can be in progress at once. thanks -- PMM
On Fri, 18 Oct 2019 17:06:20 +0200 Damien Hedde <damien.hedde@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.hedde@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: > > - reset type propagation. Right now we propagate the same reset type > to the children. I don't think it will work that with multiple > types. > For example, if we add pci_bus_reset type: a pci device will > implement the reset type but not its children (they may have > nothing to do with pci). > This can be solved by changing the child_foreach method rules. > We should say that child_foreach may change the type it > propagates to its children (on a children by children basis). > For example, the pci device may just propagate cold reset type > to its children. > For this we need to pass the type as parameter to child_foreach() > method. > > - are all children concerned ? For a given reset type, some child > may not need to be reset. As above we can handle that with > child_foreach: an resettable object can propagate the reset only > to a partial set of its child. > For this we need to know the type when we release the reset, > that's why I added it to resettable_release_reset() even if it > is unused right now. > I've also added an opaque parameter to child_foreach. I think > we will need that to handle the change of parent because we > will need to test if a child is concerned by a reset type: the > opaque will allow to use a test callback and get some result. What about an optional ->filter() callback? That would be invoked if existing prior to calling the child_foreach callback and could be used to exclude children from the reset for this round for all callbacks. Or have it modify the reset type (like in your pci reset -> cold reset example above), and completely skip it if the reset type has been modified to a 'no reset' type? > > - several reset types at the same time. I don't another solution > than saying we execute *enter* and *hold* phase for every reset > type. *exit* will still be executed once for all at the end. > It will be up for each object to cope with it if it handle > multiple reset types. For *enter* is trivial, calling it twice > in a row is no problem given that it should only reset internal > state. For *hold* there may be some complication. > > - Obviously we will need to at least an interface class field to hold > the supported reset types by the class. Also the reset state will > need some modification. > --- > Makefile.objs | 1 + > hw/core/Makefile.objs | 1 + > hw/core/resettable.c | 230 ++++++++++++++++++++++++++++++++++++++++ > hw/core/trace-events | 17 +++ > include/hw/resettable.h | 199 ++++++++++++++++++++++++++++++++++ > 5 files changed, 448 insertions(+) > create mode 100644 hw/core/resettable.c > create mode 100644 include/hw/resettable.h
diff --git a/Makefile.objs b/Makefile.objs index abcbd89654..9c3fc37e29 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/hw/core/Makefile.objs b/hw/core/Makefile.objs index fd0550d1d9..18f5ddebce 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/resettable.c b/hw/core/resettable.c new file mode 100644 index 0000000000..c5e11cff4f --- /dev/null +++ b/hw/core/resettable.c @@ -0,0 +1,230 @@ +/* + * 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: + * 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; + +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) +{ + assert(!enter_phase_in_progress); + /* TODO: change that assert when adding support for other reset types */ + 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 */ + assert(type == RESET_TYPE_COLD); + trace_resettable_reset_release_begin(obj, type); + 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 nominally. + * 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 + */ + 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 ensure 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/trace-events b/hw/core/trace-events index 1a669be0ea..66081d0fb4 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, void *oldp, void *newp) "obj=%p old_parent=%p new_parent=%p" + +# 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)" diff --git a/include/hw/resettable.h b/include/hw/resettable.h new file mode 100644 index 0000000000..6b24e1633b --- /dev/null +++ b/include/hw/resettable.h @@ -0,0 +1,199 @@ +#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. + * + * 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 is not + * 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 + * returns a non-NULL function pointer; it 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 ResettableClass { + InterfaceClass parent_class; + + /* Phase methods */ + struct ResettablePhases { + ResettableEnterPhase enter; + ResettableHoldPhase hold; + ResettableExitPhase exit; + } phases; + + /* State access method */ + ResettableGetState get_state; + + /* Transitional method for legacy reset compatibility */ + ResettableGetTrFunction get_transitional_function; + + /* Hiearchy handling method */ + 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 + * 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 + */ +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 + * 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() + * 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
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> --- 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: - reset type propagation. Right now we propagate the same reset type to the children. I don't think it will work that with multiple types. For example, if we add pci_bus_reset type: a pci device will implement the reset type but not its children (they may have nothing to do with pci). This can be solved by changing the child_foreach method rules. We should say that child_foreach may change the type it propagates to its children (on a children by children basis). For example, the pci device may just propagate cold reset type to its children. For this we need to pass the type as parameter to child_foreach() method. - are all children concerned ? For a given reset type, some child may not need to be reset. As above we can handle that with child_foreach: an resettable object can propagate the reset only to a partial set of its child. For this we need to know the type when we release the reset, that's why I added it to resettable_release_reset() even if it is unused right now. I've also added an opaque parameter to child_foreach. I think we will need that to handle the change of parent because we will need to test if a child is concerned by a reset type: the opaque will allow to use a test callback and get some result. - several reset types at the same time. I don't another solution than saying we execute *enter* and *hold* phase for every reset type. *exit* will still be executed once for all at the end. It will be up for each object to cope with it if it handle multiple reset types. For *enter* is trivial, calling it twice in a row is no problem given that it should only reset internal state. For *hold* there may be some complication. - Obviously we will need to at least an interface class field to hold the supported reset types by the class. Also the reset state will need some modification. --- Makefile.objs | 1 + hw/core/Makefile.objs | 1 + hw/core/resettable.c | 230 ++++++++++++++++++++++++++++++++++++++++ hw/core/trace-events | 17 +++ include/hw/resettable.h | 199 ++++++++++++++++++++++++++++++++++ 5 files changed, 448 insertions(+) create mode 100644 hw/core/resettable.c create mode 100644 include/hw/resettable.h