| Message ID | 20190821163341.16309-3-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multi-phase reset mechanism | expand |
On Wed, Aug 21, 2019 at 06:33:33PM +0200, 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. There is 3 phases to allow a device to be held in reset > state; the ability to control this state will be added in a following > commits. > > The qdev/qbus reset in DeviceClass and BusClass will be modified in > following commits to use this interface. > No change of behavior is expected because the init phase execution order > follows the children-then-parent order inside a tree. Since this is the > actual order of qdev/qbus reset, we will be able to map current reset > handlers on init phase for example. > > In this patch only cold reset is introduced, which is pretty much the > actual semantics of the current reset handlers. The interface can be > extended to support more reset types. > > Documentation will be added in a following commit. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > --- > > I kept the non-recursive count approach (a given object counts reset > initiated on it as well as reset initiated on its parents in reset > hierarchy). I implemented the other approach, it is possible but is more > complex (an object has to know its direct parent(s) and we need to scan > the reset hierarchy to know if we are in reset) so I prefer not > to introduce it here. > Moreover I think it has drawbacks if we want to handle complex reset > use cases with more reset type. > Anyway, as long as we don't migrate the reset-related state, there is > no problem to switch between approaches. > --- So, I certainly prefer the more general "reset type" approach taken in this version. That said, I find it pretty hard to imagine what types of reset other than cold will exist that have well enough defined semantics to be meaningfully used from an external subsystem. > Makefile.objs | 1 + > hw/core/Makefile.objs | 1 + > hw/core/resettable.c | 186 ++++++++++++++++++++++++++++++++++++++++ > hw/core/trace-events | 36 ++++++++ > include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++ > 5 files changed, 383 insertions(+) > create mode 100644 hw/core/resettable.c > create mode 100644 hw/core/trace-events > create mode 100644 include/hw/resettable.h > > diff --git a/Makefile.objs b/Makefile.objs > index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4 > --- /dev/null > +++ b/hw/core/resettable.c > @@ -0,0 +1,186 @@ > +/* > + * 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" > + > +#define RESETTABLE_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE) > + > +static void resettable_foreach_child(ResettableClass *rc, > + Object *obj, > + void (*func)(Object *, ResetType type), > + ResetType type) > +{ > + if (rc->foreach_child) { > + rc->foreach_child(obj, func, type); > + } > +} > + > +static void resettable_init_reset(Object *obj, ResetType type) I wonder if "enter reset" would be better terminology so this doesn't get confused with the initial, well, initialization of the device. > +{ > + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > + ResetState *s = rc->get_state(obj); > + bool action_needed = false; > + > + /* 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; > + } > + s->count += 1; > + /* > + * 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); > + trace_resettable_phase_init(obj, object_get_typename(obj), type, > + s->count, action_needed); > + > + /* > + * handle the children even if action_needed is at false so that > + * children counts are incremented too > + */ > + resettable_foreach_child(rc, obj, resettable_init_reset, type); > + > + /* exec init phase */ > + if (action_needed) { > + s->hold_phase_needed = true; > + if (rc->phases.init) { > + rc->phases.init(obj, type); > + } > + } > + trace_resettable_phase_init_end(obj); > +} > + > +static void resettable_hold_reset_child(Object *obj, ResetType type) > +{ > + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > + ResetState *s = rc->get_state(obj); > + > + trace_resettable_phase_hold(obj, object_get_typename(obj)); > + > + /* handle children first */ > + resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0); > + > + /* exec hold phase */ > + if (s->hold_phase_needed) { > + s->hold_phase_needed = false; > + if (rc->phases.hold) { > + rc->phases.hold(obj); I was about to ask what purpose the hold phase has since it's always executed right after the init phase, before realizing that it's because it can happen after parent devices have completed their init phase. Point that out in a comment here might help to avoid confusion. > + } > + } > + trace_resettable_phase_hold_end(obj, s->hold_phase_needed); > +} > + > +static void resettable_hold_reset(Object *obj) > +{ > + /* we don't care of 2nd argument here */ > + resettable_hold_reset_child(obj, 0); > +} > + > +static void resettable_exit_reset_child(Object *obj, ResetType type) > +{ > + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > + ResetState *s = rc->get_state(obj); > + > + trace_resettable_phase_exit(obj, object_get_typename(obj)); > + > + resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0); > + > + /* > + * we could assert that count > 0 but there are some corner cases > + * where we prefer to let it go as it is probably harmless. > + * For example: if there is reset support addition between > + * hosts when doing a migration. We may do such things as > + * deassert a non-existing reset. > + */ > + if (s->count > 0) { > + s->count -= 1; > + } else { > + trace_resettable_count_underflow(obj); Should this be an assert(), IIUC this could only come about from an error within the qemu code, right? > + } > + if (s->count == 0) { > + if (rc->phases.exit) { > + rc->phases.exit(obj); Hm. It's not really clear to me whether child resets should go before or after the parent. It seems odd that it would be the same for both entering and exiting reset, though. > + } > + } > + trace_resettable_phase_exit_end(obj, s->count); > +} > + > +static void resettable_exit_reset(Object *obj) > +{ > + /* we don't care of 2nd argument here */ > + resettable_exit_reset_child(obj, 0); > +} > + > +void resettable_reset(Object *obj, ResetType type) > +{ > + /* TODO: change that when adding support for other reset types */ > + assert(type == RESET_TYPE_COLD); > + trace_resettable_reset(obj, type); > + resettable_init_reset(obj, type); > + resettable_hold_reset(obj); > + resettable_exit_reset(obj); > +} > + > +void resettable_cold_reset_fn(void *opaque) > +{ > + resettable_reset((Object *) opaque, RESET_TYPE_COLD); > +} > + > +bool resettable_is_resetting(Object *obj) > +{ > + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > + ResetState *s = rc->get_state(obj); > + > + return (s->count > 0); > +} > + > +void resettable_class_set_parent_phases(ResettableClass *rc, > + ResettableInitPhase init, > + ResettableHoldPhase hold, > + ResettableExitPhase exit, > + ResettablePhases *parent_phases) > +{ > + *parent_phases = rc->phases; > + if (init) { > + rc->phases.init = init; > + } > + 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 > new file mode 100644 > index 0000000000..ecf966c314 > --- /dev/null > +++ b/hw/core/trace-events > @@ -0,0 +1,36 @@ > +# See docs/devel/tracing.txt for syntax documentation. > +# > +# This file is processed by the tracetool script during the build. > +# > +# To add a new trace event: > +# > +# 1. Choose a name for the trace event. Declare its arguments and format > +# string. > +# > +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() -> > +# trace_multiwrite_cb(). The source file must #include "trace.h". > +# > +# Format of a trace event: > +# > +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>" > +# > +# Example: g_malloc(size_t size) "size %zu" > +# > +# The "disable" keyword will build without the trace event. > +# > +# The <name> must be a valid as a C function name. > +# > +# Types should be standard C types. Use void * for pointers because the trace > +# system may not have the necessary headers included. > +# > +# The <format-string> should be a sprintf()-compatible format string. > + > +# resettable.c > +resettable_reset(void *obj, int cold) "obj=%p cold=%d" > +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d" > +resettable_phase_init_end(void *obj) "obj=%p" > +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)" > +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d" > +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)" > +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32 > +resettable_count_underflow(void *obj) "obj=%p" > diff --git a/include/hw/resettable.h b/include/hw/resettable.h > new file mode 100644 > index 0000000000..5808c3c187 > --- /dev/null > +++ b/include/hw/resettable.h > @@ -0,0 +1,159 @@ > +#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) > + > +typedef struct ResetState ResetState; > + > +/** > + * 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, > + * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'. > + * An object will always move quickly from 'init' 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 > + * 'init' method has been called. > + * > + * Users of a resettable object should not call these methods > + * directly, but instead use the function resettable_reset(). > + * > + * @phases.init: 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.init 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 ResetState. > + * > + * @foreach_child: Executes a given function 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 > + * an additional ResetType argument which is passed to foreach_child. > + */ > +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); > +typedef void (*ResettableHoldPhase)(Object *obj); > +typedef void (*ResettableExitPhase)(Object *obj); > +typedef ResetState * (*ResettableGetState)(Object *obj); > +typedef void (*ResettableForeachChild)(Object *obj, > + void (*func)(Object *, ResetType type), > + ResetType type); > +typedef struct ResettableClass { > + InterfaceClass parent_class; > + > + struct ResettablePhases { > + ResettableInitPhase init; > + ResettableHoldPhase hold; > + ResettableExitPhase exit; > + } phases; > + > + ResettableGetState get_state; > + ResettableForeachChild foreach_child; > +} ResettableClass; > +typedef struct ResettablePhases ResettablePhases; > + > +/** > + * ResetState: > + * 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_needed: flag which indicates that we need to invoke the 'hold' > + * phase handler for this object. > + */ > +struct ResetState { > + uint32_t count; > + bool hold_phase_needed; > +}; > + > +/** > + * resettable_is_resetting: > + * Return true if @obj is under reset. > + * > + * @obj must implement Resettable interface. > + */ > +bool resettable_is_resetting(Object *obj); > + > +/** > + * 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_deassert_reset. > + */ > +void resettable_reset(Object *obj, ResetType type); > + > +/** > + * resettable_cold_reset_fn: > + * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD). > + * > + * This function is typically useful to register a reset handler with > + * qemu_register_reset. > + */ > +void resettable_cold_reset_fn(void *opaque); > + > +/** > + * resettable_class_set_parent_phases: > + * > + * Save @rc current reset phases into @parent_phases and override @rc phases > + * by the given new methods (@init, @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, > + ResettableInitPhase init, > + ResettableHoldPhase hold, > + ResettableExitPhase exit, > + ResettablePhases *parent_phases); > + > +#endif
On 9/11/19 10:06 AM, David Gibson wrote: > On Wed, Aug 21, 2019 at 06:33:33PM +0200, 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. There is 3 phases to allow a device to be held in reset >> state; the ability to control this state will be added in a following >> commits. >> >> The qdev/qbus reset in DeviceClass and BusClass will be modified in >> following commits to use this interface. >> No change of behavior is expected because the init phase execution order >> follows the children-then-parent order inside a tree. Since this is the >> actual order of qdev/qbus reset, we will be able to map current reset >> handlers on init phase for example. >> >> In this patch only cold reset is introduced, which is pretty much the >> actual semantics of the current reset handlers. The interface can be >> extended to support more reset types. >> >> Documentation will be added in a following commit. >> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> --- >> >> I kept the non-recursive count approach (a given object counts reset >> initiated on it as well as reset initiated on its parents in reset >> hierarchy). I implemented the other approach, it is possible but is more >> complex (an object has to know its direct parent(s) and we need to scan >> the reset hierarchy to know if we are in reset) so I prefer not >> to introduce it here. >> Moreover I think it has drawbacks if we want to handle complex reset >> use cases with more reset type. >> Anyway, as long as we don't migrate the reset-related state, there is >> no problem to switch between approaches. >> --- > > So, I certainly prefer the more general "reset type" approach taken in > this version. That said, I find it pretty hard to imagine what types > of reset other than cold will exist that have well enough defined > semantics to be meaningfully used from an external subsystem. Maybe I should completely remove the type then ? > >> Makefile.objs | 1 + >> hw/core/Makefile.objs | 1 + >> hw/core/resettable.c | 186 ++++++++++++++++++++++++++++++++++++++++ >> hw/core/trace-events | 36 ++++++++ >> include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++ >> 5 files changed, 383 insertions(+) >> create mode 100644 hw/core/resettable.c >> create mode 100644 hw/core/trace-events >> create mode 100644 include/hw/resettable.h >> >> diff --git a/Makefile.objs b/Makefile.objs >> index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4 >> --- /dev/null >> +++ b/hw/core/resettable.c >> @@ -0,0 +1,186 @@ >> +/* >> + * 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" >> + >> +#define RESETTABLE_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE) >> + >> +static void resettable_foreach_child(ResettableClass *rc, >> + Object *obj, >> + void (*func)(Object *, ResetType type), >> + ResetType type) >> +{ >> + if (rc->foreach_child) { >> + rc->foreach_child(obj, func, type); >> + } >> +} >> + >> +static void resettable_init_reset(Object *obj, ResetType type) > > I wonder if "enter reset" would be better terminology so this doesn't > get confused with the initial, well, initialization of the device. Do you mean for the function here or in general for the name of the phase ? > >> +{ >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); >> + ResetState *s = rc->get_state(obj); >> + bool action_needed = false; >> + >> + /* 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; >> + } >> + s->count += 1; >> + /* >> + * 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); >> + trace_resettable_phase_init(obj, object_get_typename(obj), type, >> + s->count, action_needed); >> + >> + /* >> + * handle the children even if action_needed is at false so that >> + * children counts are incremented too >> + */ >> + resettable_foreach_child(rc, obj, resettable_init_reset, type); >> + >> + /* exec init phase */ >> + if (action_needed) { >> + s->hold_phase_needed = true; >> + if (rc->phases.init) { >> + rc->phases.init(obj, type); >> + } >> + } >> + trace_resettable_phase_init_end(obj); >> +} >> + >> +static void resettable_hold_reset_child(Object *obj, ResetType type) >> +{ >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); >> + ResetState *s = rc->get_state(obj); >> + >> + trace_resettable_phase_hold(obj, object_get_typename(obj)); >> + >> + /* handle children first */ >> + resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0); >> + >> + /* exec hold phase */ >> + if (s->hold_phase_needed) { >> + s->hold_phase_needed = false; >> + if (rc->phases.hold) { >> + rc->phases.hold(obj); > > I was about to ask what purpose the hold phase has since it's always > executed right after the init phase, before realizing that it's > because it can happen after parent devices have completed their init > phase. > > Point that out in a comment here might help to avoid confusion. noted. > >> + } >> + } >> + trace_resettable_phase_hold_end(obj, s->hold_phase_needed); >> +} >> + >> +static void resettable_hold_reset(Object *obj) >> +{ >> + /* we don't care of 2nd argument here */ >> + resettable_hold_reset_child(obj, 0); >> +} >> + >> +static void resettable_exit_reset_child(Object *obj, ResetType type) >> +{ >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); >> + ResetState *s = rc->get_state(obj); >> + >> + trace_resettable_phase_exit(obj, object_get_typename(obj)); >> + >> + resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0); >> + >> + /* >> + * we could assert that count > 0 but there are some corner cases >> + * where we prefer to let it go as it is probably harmless. >> + * For example: if there is reset support addition between >> + * hosts when doing a migration. We may do such things as >> + * deassert a non-existing reset. >> + */ >> + if (s->count > 0) { >> + s->count -= 1; >> + } else { >> + trace_resettable_count_underflow(obj); > > Should this be an assert(), IIUC this could only come about from an > error within the qemu code, right? Initially I was thinking that so I put an assert. But while testing I found out that it is triggered by the raspi machine during its reset because the qbus tree is modified during it. So it depends if we consider this kind of action to be faulty. With no migration support, the only way to trigger it is to modify the qdev hierarchy during reset. But it made me think about the migration cases (with "staying in reset state"). There are some cases where migration between 2 different versions can lead to problem with the counter (typically if you migrate to a new version that have a new device on a bus held in reset). > >> + } >> + if (s->count == 0) { >> + if (rc->phases.exit) { >> + rc->phases.exit(obj); > > Hm. It's not really clear to me whether child resets should go before > or after the parent. It seems odd that it would be the same for both > entering and exiting reset, though. Since the whole point of the phases is to make the reset independent of the order in which it is "scheduled", I think we could consider it as "unspecified". If we find out that we need some hierarchical order for a yet unknown reason, we can change it. I've used the order children then parent because it is the actual qdev reset order and we do not want to modify the actual order now. > >> + } >> + } >> + trace_resettable_phase_exit_end(obj, s->count); >> +} >> + >> +static void resettable_exit_reset(Object *obj) >> +{ >> + /* we don't care of 2nd argument here */ >> + resettable_exit_reset_child(obj, 0); >> +} >> + >> +void resettable_reset(Object *obj, ResetType type) >> +{ >> + /* TODO: change that when adding support for other reset types */ >> + assert(type == RESET_TYPE_COLD); >> + trace_resettable_reset(obj, type); >> + resettable_init_reset(obj, type); >> + resettable_hold_reset(obj); >> + resettable_exit_reset(obj); >> +} >> + >> +void resettable_cold_reset_fn(void *opaque) >> +{ >> + resettable_reset((Object *) opaque, RESET_TYPE_COLD); >> +} >> + >> +bool resettable_is_resetting(Object *obj) >> +{ >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); >> + ResetState *s = rc->get_state(obj); >> + >> + return (s->count > 0); >> +} >> + >> +void resettable_class_set_parent_phases(ResettableClass *rc, >> + ResettableInitPhase init, >> + ResettableHoldPhase hold, >> + ResettableExitPhase exit, >> + ResettablePhases *parent_phases) >> +{ >> + *parent_phases = rc->phases; >> + if (init) { >> + rc->phases.init = init; >> + } >> + 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 >> new file mode 100644 >> index 0000000000..ecf966c314 >> --- /dev/null >> +++ b/hw/core/trace-events >> @@ -0,0 +1,36 @@ >> +# See docs/devel/tracing.txt for syntax documentation. >> +# >> +# This file is processed by the tracetool script during the build. >> +# >> +# To add a new trace event: >> +# >> +# 1. Choose a name for the trace event. Declare its arguments and format >> +# string. >> +# >> +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() -> >> +# trace_multiwrite_cb(). The source file must #include "trace.h". >> +# >> +# Format of a trace event: >> +# >> +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>" >> +# >> +# Example: g_malloc(size_t size) "size %zu" >> +# >> +# The "disable" keyword will build without the trace event. >> +# >> +# The <name> must be a valid as a C function name. >> +# >> +# Types should be standard C types. Use void * for pointers because the trace >> +# system may not have the necessary headers included. >> +# >> +# The <format-string> should be a sprintf()-compatible format string. >> + >> +# resettable.c >> +resettable_reset(void *obj, int cold) "obj=%p cold=%d" >> +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d" >> +resettable_phase_init_end(void *obj) "obj=%p" >> +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)" >> +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d" >> +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)" >> +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32 >> +resettable_count_underflow(void *obj) "obj=%p" >> diff --git a/include/hw/resettable.h b/include/hw/resettable.h >> new file mode 100644 >> index 0000000000..5808c3c187 >> --- /dev/null >> +++ b/include/hw/resettable.h >> @@ -0,0 +1,159 @@ >> +#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) >> + >> +typedef struct ResetState ResetState; >> + >> +/** >> + * 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, >> + * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'. >> + * An object will always move quickly from 'init' 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 >> + * 'init' method has been called. >> + * >> + * Users of a resettable object should not call these methods >> + * directly, but instead use the function resettable_reset(). >> + * >> + * @phases.init: 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.init 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 ResetState. >> + * >> + * @foreach_child: Executes a given function 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 >> + * an additional ResetType argument which is passed to foreach_child. >> + */ >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); >> +typedef void (*ResettableHoldPhase)(Object *obj); >> +typedef void (*ResettableExitPhase)(Object *obj); >> +typedef ResetState * (*ResettableGetState)(Object *obj); >> +typedef void (*ResettableForeachChild)(Object *obj, >> + void (*func)(Object *, ResetType type), >> + ResetType type); >> +typedef struct ResettableClass { >> + InterfaceClass parent_class; >> + >> + struct ResettablePhases { >> + ResettableInitPhase init; >> + ResettableHoldPhase hold; >> + ResettableExitPhase exit; >> + } phases; >> + >> + ResettableGetState get_state; >> + ResettableForeachChild foreach_child; >> +} ResettableClass; >> +typedef struct ResettablePhases ResettablePhases; >> + >> +/** >> + * ResetState: >> + * 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_needed: flag which indicates that we need to invoke the 'hold' >> + * phase handler for this object. >> + */ >> +struct ResetState { >> + uint32_t count; >> + bool hold_phase_needed; >> +}; >> + >> +/** >> + * resettable_is_resetting: >> + * Return true if @obj is under reset. >> + * >> + * @obj must implement Resettable interface. >> + */ >> +bool resettable_is_resetting(Object *obj); >> + >> +/** >> + * 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_deassert_reset. >> + */ >> +void resettable_reset(Object *obj, ResetType type); >> + >> +/** >> + * resettable_cold_reset_fn: >> + * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD). >> + * >> + * This function is typically useful to register a reset handler with >> + * qemu_register_reset. >> + */ >> +void resettable_cold_reset_fn(void *opaque); >> + >> +/** >> + * resettable_class_set_parent_phases: >> + * >> + * Save @rc current reset phases into @parent_phases and override @rc phases >> + * by the given new methods (@init, @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, >> + ResettableInitPhase init, >> + ResettableHoldPhase hold, >> + ResettableExitPhase exit, >> + ResettablePhases *parent_phases); >> + >> +#endif >
On Wed, Sep 11, 2019 at 04:56:13PM +0200, Damien Hedde wrote: > > > On 9/11/19 10:06 AM, David Gibson wrote: > > On Wed, Aug 21, 2019 at 06:33:33PM +0200, 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. There is 3 phases to allow a device to be held in reset > >> state; the ability to control this state will be added in a following > >> commits. > >> > >> The qdev/qbus reset in DeviceClass and BusClass will be modified in > >> following commits to use this interface. > >> No change of behavior is expected because the init phase execution order > >> follows the children-then-parent order inside a tree. Since this is the > >> actual order of qdev/qbus reset, we will be able to map current reset > >> handlers on init phase for example. > >> > >> In this patch only cold reset is introduced, which is pretty much the > >> actual semantics of the current reset handlers. The interface can be > >> extended to support more reset types. > >> > >> Documentation will be added in a following commit. > >> > >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > >> --- > >> > >> I kept the non-recursive count approach (a given object counts reset > >> initiated on it as well as reset initiated on its parents in reset > >> hierarchy). I implemented the other approach, it is possible but is more > >> complex (an object has to know its direct parent(s) and we need to scan > >> the reset hierarchy to know if we are in reset) so I prefer not > >> to introduce it here. > >> Moreover I think it has drawbacks if we want to handle complex reset > >> use cases with more reset type. > >> Anyway, as long as we don't migrate the reset-related state, there is > >> no problem to switch between approaches. > >> --- > > > > So, I certainly prefer the more general "reset type" approach taken in > > this version. That said, I find it pretty hard to imagine what types > > of reset other than cold will exist that have well enough defined > > semantics to be meaningfully used from an external subsystem. > > Maybe I should completely remove the type then ? That makes sense to me. I don't know if other possible users of the mechanism have different opinions though. > > > >> Makefile.objs | 1 + > >> hw/core/Makefile.objs | 1 + > >> hw/core/resettable.c | 186 ++++++++++++++++++++++++++++++++++++++++ > >> hw/core/trace-events | 36 ++++++++ > >> include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++ > >> 5 files changed, 383 insertions(+) > >> create mode 100644 hw/core/resettable.c > >> create mode 100644 hw/core/trace-events > >> create mode 100644 include/hw/resettable.h > >> > >> diff --git a/Makefile.objs b/Makefile.objs > >> index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4 > >> --- /dev/null > >> +++ b/hw/core/resettable.c > >> @@ -0,0 +1,186 @@ > >> +/* > >> + * 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" > >> + > >> +#define RESETTABLE_GET_CLASS(obj) \ > >> + OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE) > >> + > >> +static void resettable_foreach_child(ResettableClass *rc, > >> + Object *obj, > >> + void (*func)(Object *, ResetType type), > >> + ResetType type) > >> +{ > >> + if (rc->foreach_child) { > >> + rc->foreach_child(obj, func, type); > >> + } > >> +} > >> + > >> +static void resettable_init_reset(Object *obj, ResetType type) > > > > I wonder if "enter reset" would be better terminology so this doesn't > > get confused with the initial, well, initialization of the device. > > Do you mean for the function here or in general for the name of the phase ? In general. > >> +{ > >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > >> + ResetState *s = rc->get_state(obj); > >> + bool action_needed = false; > >> + > >> + /* 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; > >> + } > >> + s->count += 1; > >> + /* > >> + * 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); > >> + trace_resettable_phase_init(obj, object_get_typename(obj), type, > >> + s->count, action_needed); > >> + > >> + /* > >> + * handle the children even if action_needed is at false so that > >> + * children counts are incremented too > >> + */ > >> + resettable_foreach_child(rc, obj, resettable_init_reset, type); > >> + > >> + /* exec init phase */ > >> + if (action_needed) { > >> + s->hold_phase_needed = true; > >> + if (rc->phases.init) { > >> + rc->phases.init(obj, type); > >> + } > >> + } > >> + trace_resettable_phase_init_end(obj); > >> +} > >> + > >> +static void resettable_hold_reset_child(Object *obj, ResetType type) > >> +{ > >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > >> + ResetState *s = rc->get_state(obj); > >> + > >> + trace_resettable_phase_hold(obj, object_get_typename(obj)); > >> + > >> + /* handle children first */ > >> + resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0); > >> + > >> + /* exec hold phase */ > >> + if (s->hold_phase_needed) { > >> + s->hold_phase_needed = false; > >> + if (rc->phases.hold) { > >> + rc->phases.hold(obj); > > > > I was about to ask what purpose the hold phase has since it's always > > executed right after the init phase, before realizing that it's > > because it can happen after parent devices have completed their init > > phase. > > > > Point that out in a comment here might help to avoid confusion. > > noted. > > > > >> + } > >> + } > >> + trace_resettable_phase_hold_end(obj, s->hold_phase_needed); > >> +} > >> + > >> +static void resettable_hold_reset(Object *obj) > >> +{ > >> + /* we don't care of 2nd argument here */ > >> + resettable_hold_reset_child(obj, 0); > >> +} > >> + > >> +static void resettable_exit_reset_child(Object *obj, ResetType type) > >> +{ > >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > >> + ResetState *s = rc->get_state(obj); > >> + > >> + trace_resettable_phase_exit(obj, object_get_typename(obj)); > >> + > >> + resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0); > >> + > >> + /* > >> + * we could assert that count > 0 but there are some corner cases > >> + * where we prefer to let it go as it is probably harmless. > >> + * For example: if there is reset support addition between > >> + * hosts when doing a migration. We may do such things as > >> + * deassert a non-existing reset. > >> + */ > >> + if (s->count > 0) { > >> + s->count -= 1; > >> + } else { > >> + trace_resettable_count_underflow(obj); > > > > Should this be an assert(), IIUC this could only come about from an > > error within the qemu code, right? > > Initially I was thinking that so I put an assert. > > But while testing I found out that it is triggered by the raspi machine > during its reset because the qbus tree is modified during it. > > So it depends if we consider this kind of action to be faulty. With no > migration support, the only way to trigger it is to modify the qdev > hierarchy during reset. Hm, I see. It feels like just ignoring underflow is ignoring the error rather than really addressing it. When we add a device to the heirarchy, do we need to initialize its reset count based on its parent's current count or something. > But it made me think about the migration cases (with "staying in reset > state"). There are some cases where migration between 2 different > versions can lead to problem with the counter (typically if you migrate > to a new version that have a new device on a bus held in reset). Uh... migration is only permitted between identical machines. I don't see how a new device is possible. > > > > >> + } > >> + if (s->count == 0) { > >> + if (rc->phases.exit) { > >> + rc->phases.exit(obj); > > > > Hm. It's not really clear to me whether child resets should go before > > or after the parent. It seems odd that it would be the same for both > > entering and exiting reset, though. > > Since the whole point of the phases is to make the reset independent of > the order in which it is "scheduled", I think we could consider it as > "unspecified". If we find out that we need some hierarchical order for a > yet unknown reason, we can change it. > > I've used the order children then parent because it is the actual qdev > reset order and we do not want to modify the actual order now. Hm, ok. > > > > >> + } > >> + } > >> + trace_resettable_phase_exit_end(obj, s->count); > >> +} > >> + > >> +static void resettable_exit_reset(Object *obj) > >> +{ > >> + /* we don't care of 2nd argument here */ > >> + resettable_exit_reset_child(obj, 0); > >> +} > >> + > >> +void resettable_reset(Object *obj, ResetType type) > >> +{ > >> + /* TODO: change that when adding support for other reset types */ > >> + assert(type == RESET_TYPE_COLD); > >> + trace_resettable_reset(obj, type); > >> + resettable_init_reset(obj, type); > >> + resettable_hold_reset(obj); > >> + resettable_exit_reset(obj); > >> +} > >> + > >> +void resettable_cold_reset_fn(void *opaque) > >> +{ > >> + resettable_reset((Object *) opaque, RESET_TYPE_COLD); > >> +} > >> + > >> +bool resettable_is_resetting(Object *obj) > >> +{ > >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > >> + ResetState *s = rc->get_state(obj); > >> + > >> + return (s->count > 0); > >> +} > >> + > >> +void resettable_class_set_parent_phases(ResettableClass *rc, > >> + ResettableInitPhase init, > >> + ResettableHoldPhase hold, > >> + ResettableExitPhase exit, > >> + ResettablePhases *parent_phases) > >> +{ > >> + *parent_phases = rc->phases; > >> + if (init) { > >> + rc->phases.init = init; > >> + } > >> + 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 > >> new file mode 100644 > >> index 0000000000..ecf966c314 > >> --- /dev/null > >> +++ b/hw/core/trace-events > >> @@ -0,0 +1,36 @@ > >> +# See docs/devel/tracing.txt for syntax documentation. > >> +# > >> +# This file is processed by the tracetool script during the build. > >> +# > >> +# To add a new trace event: > >> +# > >> +# 1. Choose a name for the trace event. Declare its arguments and format > >> +# string. > >> +# > >> +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() -> > >> +# trace_multiwrite_cb(). The source file must #include "trace.h". > >> +# > >> +# Format of a trace event: > >> +# > >> +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>" > >> +# > >> +# Example: g_malloc(size_t size) "size %zu" > >> +# > >> +# The "disable" keyword will build without the trace event. > >> +# > >> +# The <name> must be a valid as a C function name. > >> +# > >> +# Types should be standard C types. Use void * for pointers because the trace > >> +# system may not have the necessary headers included. > >> +# > >> +# The <format-string> should be a sprintf()-compatible format string. > >> + > >> +# resettable.c > >> +resettable_reset(void *obj, int cold) "obj=%p cold=%d" > >> +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d" > >> +resettable_phase_init_end(void *obj) "obj=%p" > >> +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)" > >> +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d" > >> +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)" > >> +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32 > >> +resettable_count_underflow(void *obj) "obj=%p" > >> diff --git a/include/hw/resettable.h b/include/hw/resettable.h > >> new file mode 100644 > >> index 0000000000..5808c3c187 > >> --- /dev/null > >> +++ b/include/hw/resettable.h > >> @@ -0,0 +1,159 @@ > >> +#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) > >> + > >> +typedef struct ResetState ResetState; > >> + > >> +/** > >> + * 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, > >> + * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'. > >> + * An object will always move quickly from 'init' 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 > >> + * 'init' method has been called. > >> + * > >> + * Users of a resettable object should not call these methods > >> + * directly, but instead use the function resettable_reset(). > >> + * > >> + * @phases.init: 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.init 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 ResetState. > >> + * > >> + * @foreach_child: Executes a given function 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 > >> + * an additional ResetType argument which is passed to foreach_child. > >> + */ > >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); > >> +typedef void (*ResettableHoldPhase)(Object *obj); > >> +typedef void (*ResettableExitPhase)(Object *obj); > >> +typedef ResetState * (*ResettableGetState)(Object *obj); > >> +typedef void (*ResettableForeachChild)(Object *obj, > >> + void (*func)(Object *, ResetType type), > >> + ResetType type); > >> +typedef struct ResettableClass { > >> + InterfaceClass parent_class; > >> + > >> + struct ResettablePhases { > >> + ResettableInitPhase init; > >> + ResettableHoldPhase hold; > >> + ResettableExitPhase exit; > >> + } phases; > >> + > >> + ResettableGetState get_state; > >> + ResettableForeachChild foreach_child; > >> +} ResettableClass; > >> +typedef struct ResettablePhases ResettablePhases; > >> + > >> +/** > >> + * ResetState: > >> + * 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_needed: flag which indicates that we need to invoke the 'hold' > >> + * phase handler for this object. > >> + */ > >> +struct ResetState { > >> + uint32_t count; > >> + bool hold_phase_needed; > >> +}; > >> + > >> +/** > >> + * resettable_is_resetting: > >> + * Return true if @obj is under reset. > >> + * > >> + * @obj must implement Resettable interface. > >> + */ > >> +bool resettable_is_resetting(Object *obj); > >> + > >> +/** > >> + * 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_deassert_reset. > >> + */ > >> +void resettable_reset(Object *obj, ResetType type); > >> + > >> +/** > >> + * resettable_cold_reset_fn: > >> + * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD). > >> + * > >> + * This function is typically useful to register a reset handler with > >> + * qemu_register_reset. > >> + */ > >> +void resettable_cold_reset_fn(void *opaque); > >> + > >> +/** > >> + * resettable_class_set_parent_phases: > >> + * > >> + * Save @rc current reset phases into @parent_phases and override @rc phases > >> + * by the given new methods (@init, @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, > >> + ResettableInitPhase init, > >> + ResettableHoldPhase hold, > >> + ResettableExitPhase exit, > >> + ResettablePhases *parent_phases); > >> + > >> +#endif > > >
Hi All, Do you think I should respin with the sugestions made by David so far ? + reset type removal + s/init/enter/ for the phases terminology + handling of parent changes during reset On 9/18/19 11:11 AM, David Gibson wrote: > On Wed, Sep 11, 2019 at 04:56:13PM +0200, Damien Hedde wrote: >> >> >> On 9/11/19 10:06 AM, David Gibson wrote: >>> On Wed, Aug 21, 2019 at 06:33:33PM +0200, Damien Hedde wrote: >>>> This commit defines an interface allowing multi-phase reset. >>> >>> So, I certainly prefer the more general "reset type" approach taken in >>> this version. That said, I find it pretty hard to imagine what types >>> of reset other than cold will exist that have well enough defined >>> semantics to be meaningfully used from an external subsystem. >> >> Maybe I should completely remove the type then ? > > That makes sense to me. I don't know if other possible users of the > mechanism have different opinions though. > >>> >>>> +static void resettable_init_reset(Object *obj, ResetType type) >>> >>> I wonder if "enter reset" would be better terminology so this doesn't >>> get confused with the initial, well, initialization of the device. >> >> Do you mean for the function here or in general for the name of the phase ? > > In general. > >>>> + /* >>>> + * we could assert that count > 0 but there are some corner cases >>>> + * where we prefer to let it go as it is probably harmless. >>>> + * For example: if there is reset support addition between >>>> + * hosts when doing a migration. We may do such things as >>>> + * deassert a non-existing reset. >>>> + */ >>>> + if (s->count > 0) { >>>> + s->count -= 1; >>>> + } else { >>>> + trace_resettable_count_underflow(obj); >>> >>> Should this be an assert(), IIUC this could only come about from an >>> error within the qemu code, right? >> >> Initially I was thinking that so I put an assert. >> >> But while testing I found out that it is triggered by the raspi machine >> during its reset because the qbus tree is modified during it. >> >> So it depends if we consider this kind of action to be faulty. With no >> migration support, the only way to trigger it is to modify the qdev >> hierarchy during reset. > > Hm, I see. It feels like just ignoring underflow is ignoring the > error rather than really addressing it. When we add a device to the > heirarchy, do we need to initialize its reset count based on its > parent's current count or something. > I can add that. Thanks, -- Damien
On Tue, 24 Sep 2019 at 12:21, Damien Hedde <damien.hedde@greensocs.com> wrote: > > Hi All, > > Do you think I should respin with the sugestions made by David so far ? > > + reset type removal > + s/init/enter/ for the phases terminology > + handling of parent changes during reset My takes: * I think we should keep the reset type. Among other things, we probably want a reset type for "PCI bus reset" and "SCSI bus reset", when we come to conversion of those * I don't have an opinion about the phase names * I think we should look at what we're doing for dynamic changes of the reset tree. This falls into two parts, both of which have come up in this thread: - hotplug, ie what state should a hotplugged device get set up to if it's plugged into a bus that's currently in reset - the modification of the qbus tree during reset, like the raspi sd card thing These feel related to me, so maybe handling the first gives a better answer to handling the second ? thanks -- PMM
On 9/27/19 3:07 PM, Peter Maydell wrote: > On Tue, 24 Sep 2019 at 12:21, Damien Hedde <damien.hedde@greensocs.com> wrote: > > My takes: > * I think we should keep the reset type. Among other things, > we probably want a reset type for "PCI bus reset" and > "SCSI bus reset", when we come to conversion of those > * I don't have an opinion about the phase names > * I think we should look at what we're doing for dynamic > changes of the reset tree. This falls into two parts, > both of which have come up in this thread: > - hotplug, ie what state should a hotplugged device > get set up to if it's plugged into a bus that's > currently in reset > - the modification of the qbus tree during reset, > like the raspi sd card thing > These feel related to me, so maybe handling the first > gives a better answer to handling the second ? > Sorry for the delayed answer, I did not had much time to work on this last week. Regarding hotplug, right now hotplugged device are reset during the 'realize' step. So here is what I propose: + we always do the phase 1 and 2. + do the 3rd phase (to leave reset) if the device is not plugged in a bus under reset. For general case, like the raspi, it can be handled in set_parent_bus() qdev function. Damien
On Wed, 21 Aug 2019 at 17:34, 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. So, I wanted to drag up this ancient patch to ask a couple of Resettable questions, because I'm working on adding a new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD). > +/** > + * 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, > + * ResetState structure needs to be expanded. > + */ Does anybody remember what this TODO comment is about? What in particular would need to be in the ResetState struct to allow another type to be added? > +typedef enum ResetType { > + RESET_TYPE_COLD, > +} ResetType; > +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); > +typedef void (*ResettableHoldPhase)(Object *obj); > +typedef void (*ResettableExitPhase)(Object *obj); Was there a reason why we only pass the ResetType to the init phase method, and not also to the hold and exit phases ? Given that many devices don't need to implement init, it seems awkward to require them to do so just to stash the ResetType somewhere so they can look at it in the hold or exit phase, so I was thinking about adding the argument to the other two phase methods. thanks -- PMM
On 11/4/24 15:43, Peter Maydell wrote: > On Wed, 21 Aug 2019 at 17:34, 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. > > So, I wanted to drag up this ancient patch to ask a couple > of Resettable questions, because I'm working on adding a > new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD). > >> +/** >> + * 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, >> + * ResetState structure needs to be expanded. >> + */ > > Does anybody remember what this TODO comment is about? What > in particular would need to be in the ResetState struct > to allow another type to be added? IIRC this comes from this discussion: https://lore.kernel.org/qemu-devel/7c193b33-8188-2cda-cbf2-fb545254458b@greensocs.com/ Updated in this patch (see after '---' description): https://lore.kernel.org/qemu-devel/20191018150630.31099-9-damien.hedde@greensocs.com/ > >> +typedef enum ResetType { >> + RESET_TYPE_COLD, >> +} ResetType; > >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); >> +typedef void (*ResettableHoldPhase)(Object *obj); >> +typedef void (*ResettableExitPhase)(Object *obj); > > Was there a reason why we only pass the ResetType to the init > phase method, and not also to the hold and exit phases ? > Given that many devices don't need to implement init, it > seems awkward to require them to do so just to stash the > ResetType somewhere so they can look at it in the hold > or exit phase, so I was thinking about adding the argument > to the other two phase methods. You are right, the type should be propagated to to all phase handlers. > > thanks > -- PMM
On Thu, 11 Apr 2024 at 18:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 11/4/24 15:43, Peter Maydell wrote: > > On Wed, 21 Aug 2019 at 17:34, 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. > > > > So, I wanted to drag up this ancient patch to ask a couple > > of Resettable questions, because I'm working on adding a > > new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD). > > > >> +/** > >> + * 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, > >> + * ResetState structure needs to be expanded. > >> + */ > > > > Does anybody remember what this TODO comment is about? What > > in particular would need to be in the ResetState struct > > to allow another type to be added? > > IIRC this comes from this discussion: > https://lore.kernel.org/qemu-devel/7c193b33-8188-2cda-cbf2-fb545254458b@greensocs.com/ > Updated in this patch (see after '---' description): > https://lore.kernel.org/qemu-devel/20191018150630.31099-9-damien.hedde@greensocs.com/ Hmm, I can't see anything in there that mentions this TODO or what we'd need more ResetState fields to handle. I guess I'll go ahead with adding my new ResetType and ignore this TODO, because I can't see any reason why we need to do anything in particular for a new ResetType... > > > >> +typedef enum ResetType { > >> + RESET_TYPE_COLD, > >> +} ResetType; > > > >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); > >> +typedef void (*ResettableHoldPhase)(Object *obj); > >> +typedef void (*ResettableExitPhase)(Object *obj); > > > > Was there a reason why we only pass the ResetType to the init > > phase method, and not also to the hold and exit phases ? > > Given that many devices don't need to implement init, it > > seems awkward to require them to do so just to stash the > > ResetType somewhere so they can look at it in the hold > > or exit phase, so I was thinking about adding the argument > > to the other two phase methods. > > You are right, the type should be propagated to to all phase > handlers. I have some patches which do this; I'll probably send them out in a series next week once I've figured out whether they fit better in with other patches that give the motivation. thanks -- PMM
On Fri, Apr 12, 2024 at 3:05 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 11 Apr 2024 at 18:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > On 11/4/24 15:43, Peter Maydell wrote: > > > On Wed, 21 Aug 2019 at 17:34, 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. > > > > > > So, I wanted to drag up this ancient patch to ask a couple > > > of Resettable questions, because I'm working on adding a > > > new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD). > > > > > >> +/** > > >> + * 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, > > >> + * ResetState structure needs to be expanded. > > >> + */ > > > > > > Does anybody remember what this TODO comment is about? What > > > in particular would need to be in the ResetState struct > > > to allow another type to be added? > > > > IIRC this comes from this discussion: > > https://lore.kernel.org/qemu-devel/7c193b33-8188-2cda-cbf2-fb545254458b@greensocs.com/ > > Updated in this patch (see after '---' description): > > https://lore.kernel.org/qemu-devel/20191018150630.31099-9-damien.hedde@greensocs.com/ > > Hmm, I can't see anything in there that mentions this > TODO or what we'd need more ResetState fields to handle. > I guess I'll go ahead with adding my new ResetType and ignore > this TODO, because I can't see any reason why we need to > do anything in particular for a new ResetType... > > > > > > >> +typedef enum ResetType { > > >> + RESET_TYPE_COLD, > > >> +} ResetType; > > > > > >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); > > >> +typedef void (*ResettableHoldPhase)(Object *obj); > > >> +typedef void (*ResettableExitPhase)(Object *obj); > > > > > > Was there a reason why we only pass the ResetType to the init > > > phase method, and not also to the hold and exit phases ? > > > Given that many devices don't need to implement init, it > > > seems awkward to require them to do so just to stash the > > > ResetType somewhere so they can look at it in the hold > > > or exit phase, so I was thinking about adding the argument > > > to the other two phase methods. > > > > You are right, the type should be propagated to to all phase > > handlers. > > I have some patches which do this; I'll probably send them out > in a series next week once I've figured out whether they fit > better in with other patches that give the motivation. > Hi, I don't remember the details on your first questions but I also agree with adding the type to the other callbacks! Cheers, Edgar
On Fri, 12 Apr 2024 at 14:38, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Fri, Apr 12, 2024 at 3:05 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Thu, 11 Apr 2024 at 18:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > > > On 11/4/24 15:43, Peter Maydell wrote: > > > > On Wed, 21 Aug 2019 at 17:34, 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. > > > > > > > > So, I wanted to drag up this ancient patch to ask a couple > > > > of Resettable questions, because I'm working on adding a > > > > new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD). > > > > > > > >> +/** > > > >> + * 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, > > > >> + * ResetState structure needs to be expanded. > > > >> + */ > > > > > > > > Does anybody remember what this TODO comment is about? What > > > > in particular would need to be in the ResetState struct > > > > to allow another type to be added? > > > > > > IIRC this comes from this discussion: > > > https://lore.kernel.org/qemu-devel/7c193b33-8188-2cda-cbf2-fb545254458b@greensocs.com/ > > > Updated in this patch (see after '---' description): > > > https://lore.kernel.org/qemu-devel/20191018150630.31099-9-damien.hedde@greensocs.com/ > > > > Hmm, I can't see anything in there that mentions this > > TODO or what we'd need more ResetState fields to handle. > > I guess I'll go ahead with adding my new ResetType and ignore > > this TODO, because I can't see any reason why we need to > > do anything in particular for a new ResetType... > > > > > > > > > >> +typedef enum ResetType { > > > >> + RESET_TYPE_COLD, > > > >> +} ResetType; > > > > > > > >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); > > > >> +typedef void (*ResettableHoldPhase)(Object *obj); > > > >> +typedef void (*ResettableExitPhase)(Object *obj); > > > > > > > > Was there a reason why we only pass the ResetType to the init > > > > phase method, and not also to the hold and exit phases ? > > > > Given that many devices don't need to implement init, it > > > > seems awkward to require them to do so just to stash the > > > > ResetType somewhere so they can look at it in the hold > > > > or exit phase, so I was thinking about adding the argument > > > > to the other two phase methods. > > > > > > You are right, the type should be propagated to to all phase > > > handlers. > > > > I have some patches which do this; I'll probably send them out > > in a series next week once I've figured out whether they fit > > better in with other patches that give the motivation. > I don't remember the details on your first questions but I also agree > with adding the type to the other callbacks! I've now posted the series that adds the type the the hold and exit callbacks, and adds a new RESET_TYPE_SNAPSHOT_LOAD: https://patchew.org/QEMU/20240412160809.1260625-1-peter.maydell@linaro.org/ thanks -- PMM
diff --git a/Makefile.objs b/Makefile.objs index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4 --- /dev/null +++ b/hw/core/resettable.c @@ -0,0 +1,186 @@ +/* + * 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" + +#define RESETTABLE_GET_CLASS(obj) \ + OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE) + +static void resettable_foreach_child(ResettableClass *rc, + Object *obj, + void (*func)(Object *, ResetType type), + ResetType type) +{ + if (rc->foreach_child) { + rc->foreach_child(obj, func, type); + } +} + +static void resettable_init_reset(Object *obj, ResetType type) +{ + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); + ResetState *s = rc->get_state(obj); + bool action_needed = false; + + /* 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; + } + s->count += 1; + /* + * 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); + trace_resettable_phase_init(obj, object_get_typename(obj), type, + s->count, action_needed); + + /* + * handle the children even if action_needed is at false so that + * children counts are incremented too + */ + resettable_foreach_child(rc, obj, resettable_init_reset, type); + + /* exec init phase */ + if (action_needed) { + s->hold_phase_needed = true; + if (rc->phases.init) { + rc->phases.init(obj, type); + } + } + trace_resettable_phase_init_end(obj); +} + +static void resettable_hold_reset_child(Object *obj, ResetType type) +{ + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); + ResetState *s = rc->get_state(obj); + + trace_resettable_phase_hold(obj, object_get_typename(obj)); + + /* handle children first */ + resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0); + + /* exec hold phase */ + if (s->hold_phase_needed) { + s->hold_phase_needed = false; + if (rc->phases.hold) { + rc->phases.hold(obj); + } + } + trace_resettable_phase_hold_end(obj, s->hold_phase_needed); +} + +static void resettable_hold_reset(Object *obj) +{ + /* we don't care of 2nd argument here */ + resettable_hold_reset_child(obj, 0); +} + +static void resettable_exit_reset_child(Object *obj, ResetType type) +{ + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); + ResetState *s = rc->get_state(obj); + + trace_resettable_phase_exit(obj, object_get_typename(obj)); + + resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0); + + /* + * we could assert that count > 0 but there are some corner cases + * where we prefer to let it go as it is probably harmless. + * For example: if there is reset support addition between + * hosts when doing a migration. We may do such things as + * deassert a non-existing reset. + */ + if (s->count > 0) { + s->count -= 1; + } else { + trace_resettable_count_underflow(obj); + } + if (s->count == 0) { + if (rc->phases.exit) { + rc->phases.exit(obj); + } + } + trace_resettable_phase_exit_end(obj, s->count); +} + +static void resettable_exit_reset(Object *obj) +{ + /* we don't care of 2nd argument here */ + resettable_exit_reset_child(obj, 0); +} + +void resettable_reset(Object *obj, ResetType type) +{ + /* TODO: change that when adding support for other reset types */ + assert(type == RESET_TYPE_COLD); + trace_resettable_reset(obj, type); + resettable_init_reset(obj, type); + resettable_hold_reset(obj); + resettable_exit_reset(obj); +} + +void resettable_cold_reset_fn(void *opaque) +{ + resettable_reset((Object *) opaque, RESET_TYPE_COLD); +} + +bool resettable_is_resetting(Object *obj) +{ + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); + ResetState *s = rc->get_state(obj); + + return (s->count > 0); +} + +void resettable_class_set_parent_phases(ResettableClass *rc, + ResettableInitPhase init, + ResettableHoldPhase hold, + ResettableExitPhase exit, + ResettablePhases *parent_phases) +{ + *parent_phases = rc->phases; + if (init) { + rc->phases.init = init; + } + 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 new file mode 100644 index 0000000000..ecf966c314 --- /dev/null +++ b/hw/core/trace-events @@ -0,0 +1,36 @@ +# See docs/devel/tracing.txt for syntax documentation. +# +# This file is processed by the tracetool script during the build. +# +# To add a new trace event: +# +# 1. Choose a name for the trace event. Declare its arguments and format +# string. +# +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() -> +# trace_multiwrite_cb(). The source file must #include "trace.h". +# +# Format of a trace event: +# +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>" +# +# Example: g_malloc(size_t size) "size %zu" +# +# The "disable" keyword will build without the trace event. +# +# The <name> must be a valid as a C function name. +# +# Types should be standard C types. Use void * for pointers because the trace +# system may not have the necessary headers included. +# +# The <format-string> should be a sprintf()-compatible format string. + +# resettable.c +resettable_reset(void *obj, int cold) "obj=%p cold=%d" +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d" +resettable_phase_init_end(void *obj) "obj=%p" +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)" +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d" +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)" +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32 +resettable_count_underflow(void *obj) "obj=%p" diff --git a/include/hw/resettable.h b/include/hw/resettable.h new file mode 100644 index 0000000000..5808c3c187 --- /dev/null +++ b/include/hw/resettable.h @@ -0,0 +1,159 @@ +#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) + +typedef struct ResetState ResetState; + +/** + * 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, + * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'. + * An object will always move quickly from 'init' 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 + * 'init' method has been called. + * + * Users of a resettable object should not call these methods + * directly, but instead use the function resettable_reset(). + * + * @phases.init: 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.init 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 ResetState. + * + * @foreach_child: Executes a given function 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 + * an additional ResetType argument which is passed to foreach_child. + */ +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); +typedef void (*ResettableHoldPhase)(Object *obj); +typedef void (*ResettableExitPhase)(Object *obj); +typedef ResetState * (*ResettableGetState)(Object *obj); +typedef void (*ResettableForeachChild)(Object *obj, + void (*func)(Object *, ResetType type), + ResetType type); +typedef struct ResettableClass { + InterfaceClass parent_class; + + struct ResettablePhases { + ResettableInitPhase init; + ResettableHoldPhase hold; + ResettableExitPhase exit; + } phases; + + ResettableGetState get_state; + ResettableForeachChild foreach_child; +} ResettableClass; +typedef struct ResettablePhases ResettablePhases; + +/** + * ResetState: + * 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_needed: flag which indicates that we need to invoke the 'hold' + * phase handler for this object. + */ +struct ResetState { + uint32_t count; + bool hold_phase_needed; +}; + +/** + * resettable_is_resetting: + * Return true if @obj is under reset. + * + * @obj must implement Resettable interface. + */ +bool resettable_is_resetting(Object *obj); + +/** + * 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_deassert_reset. + */ +void resettable_reset(Object *obj, ResetType type); + +/** + * resettable_cold_reset_fn: + * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD). + * + * This function is typically useful to register a reset handler with + * qemu_register_reset. + */ +void resettable_cold_reset_fn(void *opaque); + +/** + * resettable_class_set_parent_phases: + * + * Save @rc current reset phases into @parent_phases and override @rc phases + * by the given new methods (@init, @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, + ResettableInitPhase init, + 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. There is 3 phases to allow a device to be held in reset state; the ability to control this state will be added in a following commits. The qdev/qbus reset in DeviceClass and BusClass will be modified in following commits to use this interface. No change of behavior is expected because the init phase execution order follows the children-then-parent order inside a tree. Since this is the actual order of qdev/qbus reset, we will be able to map current reset handlers on init phase for example. In this patch only cold reset is introduced, which is pretty much the actual semantics of the current reset handlers. The interface can be extended to support more reset types. Documentation will be added in a following commit. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- I kept the non-recursive count approach (a given object counts reset initiated on it as well as reset initiated on its parents in reset hierarchy). I implemented the other approach, it is possible but is more complex (an object has to know its direct parent(s) and we need to scan the reset hierarchy to know if we are in reset) so I prefer not to introduce it here. Moreover I think it has drawbacks if we want to handle complex reset use cases with more reset type. Anyway, as long as we don't migrate the reset-related state, there is no problem to switch between approaches. --- Makefile.objs | 1 + hw/core/Makefile.objs | 1 + hw/core/resettable.c | 186 ++++++++++++++++++++++++++++++++++++++++ hw/core/trace-events | 36 ++++++++ include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++ 5 files changed, 383 insertions(+) create mode 100644 hw/core/resettable.c create mode 100644 hw/core/trace-events create mode 100644 include/hw/resettable.h